Page MenuHomePhabricator

rhg: read [paths] for `--repository` value
Needs ReviewPublic

Authored by pulkit on Fri, Apr 2, 6:46 AM.

Details

Reviewers
Alphare
SimonSapin
Group Reviewers
hg-reviewers
Summary

hg parses -R and --repository CLI arguments "early" in order to know which
local repository to load config from. (Config can then affect whether or how to
fall back.)

The value of of those arguments can be not only a filesystem path, but also an
alias configured in the [paths] section. This part was missing in rhg and this
patch implements that.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

pulkit created this revision.Fri, Apr 2, 6:46 AM
Alphare requested changes to this revision.Sat, Apr 3, 12:02 PM
Alphare added a subscriber: Alphare.
Alphare added inline comments.
rust/rhg/src/main.rs
175

The nested ifs can be replaced by:

match config_val {
    Ok(Some(val)) if val.len() > 0 => Some(get_path_from_bytes(val.as_bytes())),
    _ => Some(get_path_from_bytes(&repo_arg)),
}

Also, you're using get_str, when you should probably just be using get, which already returns bytes (and does not unnecessarily force UTF8). That would remove the need for as_bytes() in the above suggested code.

This revision now requires changes to proceed.Sat, Apr 3, 12:02 PM
pulkit updated this revision to Diff 26659.Mon, Apr 5, 4:33 PM
pulkit marked an inline comment as done.Mon, Apr 5, 4:34 PM
Alphare accepted this revision.Tue, Apr 6, 4:59 AM

Looks good, but there seems to be yet another edge case within the edge case… *sigh*. See inline comment.

tests/test-globalopts.t
73–76

This tests suggests that relative paths in [paths] are resolved relative to the user’s $HOME directory, not relative to the the config file nor the processes current directory. To be honest I find this really weird and unexpected, but it does appear to be the case when I change HOME=/somethingelse in the test and I do see root = os.path.expanduser(b'~') in mercurial/ui.py before _applyconfig calls fixconfig which looks at [paths] items and makes them absolute.

When run with --rhg this test ends up passing but not quite for the right reason. rhg resolves path a relative to its current directory which is the repository c. That repository happens to contains a file named a, so Repo::find returns HgError::unsupported("bundle repository") which causes fallback to Python. Fallback is what we want, but only because the identify command is not implemented (yet).

It looks like os.path.expanduser returns its input unchanged when getting the home directory fails, so the code above should be something like Some(home::home_dir().unwrap_or_else(|| PathBuf::new("~")).join(get_path_from_bytes(val)))

pulkit updated this revision to Diff 26679.Wed, Apr 7, 6:21 PM
pulkit added inline comments.Wed, Apr 7, 6:25 PM
tests/test-globalopts.t
73–76

Ugh!

Btw, how did you figured out that we are falling back with HgError:unsupported("bundle ...")? I mean is there some config or something which shows these debug prints or is there any other way?

I updated the patch after fighting with borrow checker a bit, does seems bit ugly to me, kindly have a look.

SimonSapin accepted this revision.Thu, Apr 8, 4:06 PM

This version of the patch looks good, thanks!

Re debugging, there isn’t an easy way unfortunately. --config rhg.on-unsupported=abort prints the string passed to HgError::unsupported, but also disables fallback. We could add a fourth mode that keeps fallback but also has a debug log line printed to stderr.

What I did was add a use of the dbg! macro in main.rs to print the error value: https://doc.rust-lang.org/std/macro.dbg.html Since the value contains a byte string, the derived implementation of the Debug trait prints an array of integers. I copy-pasted that into a Python REPL and passed it to bytes() decode it as ASCII. Really not a good debugging experience.

Re debugging, there isn’t an easy way unfortunately. --config rhg.on-unsupported=abort prints the string passed to HgError::unsupported, but also disables fallback. We could add a fourth mode that keeps fallback but also has a debug log line printed to stderr.

Why don't we add a simple log::debug (with a possible rhg target in case we have a log consumer that allows for filtering by targets)? I've done this (albeit with a log::trace in rust/hg-cpython/src/dirstate/status.rs:89, and it's handy enough to use RUST_LOG=debug to get the debug entries printed.

pulkit retitled this revision from rhg: try read [paths] for `--repository` value to rhg: read [paths] for `--repository` value.Sat, Apr 10, 5:15 PM
pulkit edited the summary of this revision. (Show Details)
pulkit updated this revision to Diff 26787.

@SimonSapin I updated this patch to also include reading .hg/hgrc and .hg/hgrc-not-shared of current dir. Kindly have a re-look. Thanks!

This version of the patch looks good, thanks!
Re debugging, there isn’t an easy way unfortunately. --config rhg.on-unsupported=abort prints the string passed to HgError::unsupported, but also disables fallback. We could add a fourth mode that keeps fallback but also has a debug log line printed to stderr.

+1 for fourth mode. Will help a lot in case of testing new code.