D5503: vfs: add support for repo names with `$` when using with env vars (issue5739)
This revision now requires changes to proceed. baymax added a comment. baymax requested changes to this revision. There seems to have been no activities on this Diff for the past 3 Months. By policy, we are automatically moving it out of the `need-review` state. Please, move it back to `need-review` without hesitation if this diff should still be discussed. :baymax:need-review-idle: REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D5503/new/ REVISION DETAIL https://phab.mercurial-scm.org/D5503 To: navaneeth.suresh, #hg-reviewers, baymax Cc: yuja, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D5503: vfs: add support for repo names with `$` when using with env vars (issue5739)
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 ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: D5503: vfs: add support for repo names with `$` when using with env vars (issue5739)
> > 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. ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D5503: vfs: add support for repo names with `$` when using with env vars (issue5739)
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 ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D5503: vfs: add support for repo names with `$` when using with env vars (issue5739)
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 ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D5503: vfs: add support for repo names with `$` when using with env vars (issue5739)
navaneeth.suresh updated this revision to Diff 13046. navaneeth.suresh retitled this revision from "vfs: add support for repo names with `$` sign while using with env vars (issue5739)" to "vfs: add support for repo names with `$` when using with env vars (issue5739)". REPOSITORY rHG Mercurial CHANGES SINCE LAST UPDATE https://phab.mercurial-scm.org/D5503?vs=13025&id=13046 REVISION DETAIL https://phab.mercurial-scm.org/D5503 AFFECTED FILES mercurial/vfs.py tests/test-issue5739.t CHANGE DETAILS 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: To: navaneeth.suresh, #hg-reviewers Cc: mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
