Patchwork D5503: vfs: add support for repo names with `$` sign while using with env vars (issue5739)

login
register
mail settings
Submitter phabricator
Date Jan. 7, 2019, 8:38 a.m.
Message ID <differential-rev-PHID-DREV-bezy2n6f4lzhivyr6xrz-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/37503/
State New
Headers show

Comments

phabricator - Jan. 7, 2019, 8:38 a.m.
navaneeth.suresh created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  `$ foo=bar hg root` fails to recognise the repo `$foo`. I stopped expanding env
  vars from vfs when there exists a repo with the same name as the env var.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5503

AFFECTED FILES
  mercurial/vfs.py
  tests/test-issue5739.t

CHANGE DETAILS




To: navaneeth.suresh, #hg-reviewers
Cc: mercurial-devel
Yuya Nishihara - Jan. 7, 2019, 1:31 p.m.
>      def __init__(self, base, audit=True, cacheaudited=False, expandpath=False,
>                   realpath=False):
> +        if '$' in base and os.path.isdir(base):
> +            # when there exists a repo '$foo' and an env var foo=bar, stop
> +            # expanding path. refer issue5739.
> +            expandpath = False

This is logically incorrect. The problem is that we're doing variable
expansion at too lower layer. `vfs(expand(user_specified_path))` makes
some sense, but `vfs(expand(getcwd()))` is clearly wrong. And the vfs class
can't know where the `base` comes from.
phabricator - Jan. 7, 2019, 1:32 p.m.
yuja added a comment.


  >   def __init__(self, base, audit=True, cacheaudited=False, expandpath=False,
  >                realpath=False):
  > 
  > +        if '$' in base and os.path.isdir(base):
  >  +            # when there exists a repo '$foo' and an env var foo=bar, stop
  >  +            # expanding path. refer issue5739.
  >  +            expandpath = False
  
  This is logically incorrect. The problem is that we're doing variable
  expansion at too lower layer. `vfs(expand(user_specified_path))` makes
  some sense, but `vfs(expand(getcwd()))` is clearly wrong. And the vfs class
  can't know where the `base` comes from.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5503

To: navaneeth.suresh, #hg-reviewers
Cc: yuja, mercurial-devel
phabricator - Jan. 8, 2019, 4:08 a.m.
navaneeth.suresh added a comment.


  > This is logically incorrect. The problem is that we're doing variable
  >  expansion at too lower layer. `vfs(expand(user_specified_path))` makes
  >  some sense, but `vfs(expand(getcwd()))` is clearly wrong. And the vfs class
  >  can't know where the `base` comes from.
  
  If I add a condition for expanding env var if present in `hgrc`, can this work as a fix?
  If the only solution for this is shifting path expansion from `vfs` class, where do you
  think it can be?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5503

To: navaneeth.suresh, #hg-reviewers
Cc: yuja, mercurial-devel
Yuya Nishihara - Jan. 9, 2019, 2:09 p.m.
>   > This is logically incorrect. The problem is that we're doing variable
>   >  expansion at too lower layer. `vfs(expand(user_specified_path))` makes
>   >  some sense, but `vfs(expand(getcwd()))` is clearly wrong. And the vfs class
>   >  can't know where the `base` comes from.
>   
>   If I add a condition for expanding env var if present in `hgrc`, can this work as a fix?

Maybe no. Where do you intend to add such code? vfs doesn't know where the path
comes from. Neither does localrepo.

>   If the only solution for this is shifting path expansion from `vfs` class, where do you
>   think it can be?

Somewhere specifying repository path read from hgrc or user input.
phabricator - Jan. 9, 2019, 2:12 p.m.
yuja added a comment.


  >   > This is logically incorrect. The problem is that we're doing variable
  >   >  expansion at too lower layer. `vfs(expand(user_specified_path))` makes
  >   >  some sense, but `vfs(expand(getcwd()))` is clearly wrong. And the vfs class
  >   >  can't know where the `base` comes from.
  >   
  >   If I add a condition for expanding env var if present in `hgrc`, can this work as a fix?
  
  Maybe no. Where do you intend to add such code? vfs doesn't know where the path
  comes from. Neither does localrepo.
  
  >   If the only solution for this is shifting path expansion from `vfs` class, where do you
  >   think it can be?
  
  Somewhere specifying repository path read from hgrc or user input.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5503

To: navaneeth.suresh, #hg-reviewers
Cc: yuja, mercurial-devel

Patch

diff --git a/tests/test-issue5739.t b/tests/test-issue5739.t
new file mode 100644
--- /dev/null
+++ b/tests/test-issue5739.t
@@ -0,0 +1,8 @@ 
+-- going to create a repo with dollar sign which even
+works even when there exists an environment variable
+
+  $ mkdir '$foo'
+  $ cd '$foo'
+  $ hg init
+  $ foo=bar hg root
+  $TESTTMP/$foo
diff --git a/mercurial/vfs.py b/mercurial/vfs.py
--- a/mercurial/vfs.py
+++ b/mercurial/vfs.py
@@ -311,6 +311,10 @@ 
     '''
     def __init__(self, base, audit=True, cacheaudited=False, expandpath=False,
                  realpath=False):
+        if '$' in base and os.path.isdir(base):
+            # when there exists a repo '$foo' and an env var foo=bar, stop
+            # expanding path. refer issue5739.
+            expandpath = False
         if expandpath:
             base = util.expandpath(base)
         if realpath: