This is an archive of the discontinued Mercurial Phabricator instance.

vfs: add support for repo names with `$` when using with env vars (issue5739)
Needs RevisionPublic

Authored by navaneeth.suresh on Jan 7 2019, 3:38 AM.

Details

Reviewers
baymax
Group Reviewers
hg-reviewers
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.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

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).Jan 7 2019, 7:08 AM
navaneeth.suresh updated this revision to Diff 13046.
yuja added a subscriber: yuja.Jan 7 2019, 8:32 AM
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.

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?

yuja added a comment.Jan 9 2019, 9:12 AM
> 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.

baymax requested changes to this revision.Jan 24 2020, 12:32 PM

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:

This revision now requires changes to proceed.Jan 24 2020, 12:32 PM