This is an archive of the discontinued Mercurial Phabricator instance.

Differential D11751

rhg: only complain about poorly configured fallback when falling back
ClosedPublic

Authored by aalekseyev on Nov 11 2021, 9:27 AM.

Details

Reviewers
Alphare
Group Reviewers
hg-reviewers
Commits
rHG8960295b9246: rhg: only complain about poorly configured fallback when falling back

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

aalekseyev created this revision.Nov 11 2021, 9:27 AM
aalekseyev added a comment.EditedNov 11 2021, 9:39 AM

The motivation here is that I kept running into this error when testing things with rhg, and I thought it didn't need to be an error.
I think there is no (significant) downside, since checking that fallback-executable is set by no means guarantees that the fallback will work, so not much is lost by delaying that check.

I admit that the reason we end up with this is that our set up is a bit weird (on-unsupported=fallback is set by one component while fallback-executable is set by another), so I won't defend this change if there's any push back.

Alphare added a subscriber: Alphare.Nov 24 2021, 4:58 AM
In D11751#180329, @aalekseyev wrote:

I admit that the reason we end up with this is that our set up is a bit weird (on-unsupported=fallback is set by one component while fallback-executable is set by another), so I won't defend this change if there's any push back.

I'm not sure I understand what your use case is. The config is already fully read by the point we read it, so how does that help you?

aalekseyev added a comment.Nov 24 2021, 11:23 AM

I'm not sure I understand what your use case is. The config is already fully read by the point we read it, so how does that help you?

The static config we have only has on-unsupported=fallback, while the fallback executable is set by a command-line invocation, because that's where it's decided what the fallback binary should be.
This makes it so that when I'm making my own command line rhg invocation for debugging I have to either pass on-unsupported=abort, or an actual fallback executable, which is a bit of a chore.

Anyway, the point here is not to support this exact workflow, but simply to be a bit more forgiving to the format (so that omitting the executable is equivalent to specifying an invalid executable).

Alphare added a comment.Nov 26 2021, 10:23 AM

Sure, seems fine!

However the test works without this change, so I'm not sure what it's supposed to test.

aalekseyev updated this revision to Diff 31167.Nov 26 2021, 1:53 PM
aalekseyev added a comment.Nov 26 2021, 1:54 PM

Sorry, that test was weird indeed. I pushed a better one.

Alphare accepted this revision.Nov 29 2021, 6:06 AM
This revision is now accepted and ready to land.Nov 29 2021, 6:06 AM

Revision Contents
Changeset List

PathPackages
Mrust/rhg/src/main.rs (42 lines)
Mtests/test-rhg.t (9 lines)

Diff 31184

rust/rhg/src/main.rs

}); });
non_repo_config non_repo_config
.load_cli_args_config(early_args.config) .load_cli_args_config(early_args.config)
.unwrap_or_else(|error| { .unwrap_or_else(|error| {
exit( exit(
&initial_current_dir, &initial_current_dir,
&ui, &ui,
OnUnsupported::from_config(&ui, &non_repo_config), OnUnsupported::from_config(&non_repo_config),
Err(error.into()), Err(error.into()),
non_repo_config non_repo_config
.get_bool(b"ui", b"detailed-exit-code") .get_bool(b"ui", b"detailed-exit-code")
.unwrap_or(false), .unwrap_or(false),
) )
}); });
if let Some(repo_path_bytes) = &early_args.repo { if let Some(repo_path_bytes) = &early_args.repo {
lazy_static::lazy_static! { lazy_static::lazy_static! {
static ref SCHEME_RE: regex::bytes::Regex = static ref SCHEME_RE: regex::bytes::Regex =
// Same as `_matchscheme` in `mercurial/util.py` // Same as `_matchscheme` in `mercurial/util.py`
regex::bytes::Regex::new("^[a-zA-Z0-9+.\\-]+:").unwrap(); regex::bytes::Regex::new("^[a-zA-Z0-9+.\\-]+:").unwrap();
} }
if SCHEME_RE.is_match(&repo_path_bytes) { if SCHEME_RE.is_match(&repo_path_bytes) {
exit( exit(
&initial_current_dir, &initial_current_dir,
&ui, &ui,
OnUnsupported::from_config(&ui, &non_repo_config), OnUnsupported::from_config(&non_repo_config),
Err(CommandError::UnsupportedFeature { Err(CommandError::UnsupportedFeature {
message: format_bytes!( message: format_bytes!(
b"URL-like --repository {}", b"URL-like --repository {}",
repo_path_bytes repo_path_bytes
), ),
}), }),
// TODO: show a warning or combine with original error if // TODO: show a warning or combine with original error if
// `get_bool` returns an error // `get_bool` returns an error
Ok(repo) => Ok(repo), Ok(repo) => Ok(repo),
Err(RepoError::NotFound { at }) if repo_path.is_none() => { Err(RepoError::NotFound { at }) if repo_path.is_none() => {
// Not finding a repo is not fatal yet, if `-R` was not given // Not finding a repo is not fatal yet, if `-R` was not given
Err(NoRepoInCwdError { cwd: at }) Err(NoRepoInCwdError { cwd: at })
} }
Err(error) => exit( Err(error) => exit(
&initial_current_dir, &initial_current_dir,
&ui, &ui,
OnUnsupported::from_config(&ui, &non_repo_config), OnUnsupported::from_config(&non_repo_config),
Err(error.into()), Err(error.into()),
// TODO: show a warning or combine with original error if // TODO: show a warning or combine with original error if
// `get_bool` returns an error // `get_bool` returns an error
non_repo_config non_repo_config
.get_bool(b"ui", b"detailed-exit-code") .get_bool(b"ui", b"detailed-exit-code")
.unwrap_or(false), .unwrap_or(false),
), ),
}; };
let config = if let Ok(repo) = &repo_result { let config = if let Ok(repo) = &repo_result {
repo.config() repo.config()
} else { } else {
&non_repo_config &non_repo_config
}; };
let on_unsupported = OnUnsupported::from_config(&ui, config); let on_unsupported = OnUnsupported::from_config(config);
let result = main_with_result( let result = main_with_result(
&process_start_time, &process_start_time,
&ui, &ui,
repo_result.as_ref(), repo_result.as_ref(),
config, config,
); );
exit( exit(
use_detailed_exit_code: bool, use_detailed_exit_code: bool,
) -> ! { ) -> ! {
if let ( if let (
OnUnsupported::Fallback { executable }, OnUnsupported::Fallback { executable },
Err(CommandError::UnsupportedFeature { .. }), Err(CommandError::UnsupportedFeature { .. }),
) = (&on_unsupported, &result) ) = (&on_unsupported, &result)
{ {
let mut args = std::env::args_os(); let mut args = std::env::args_os();
let executable = match executable {
None => {
exit_no_fallback(
ui,
OnUnsupported::Abort,
Err(CommandError::abort(
"abort: 'rhg.on-unsupported=fallback' without \
'rhg.fallback-executable' set.",
)),
false,
);
}
Some(executable) => executable,
};
let executable_path = get_path_from_bytes(&executable); let executable_path = get_path_from_bytes(&executable);
let this_executable = args.next().expect("exepcted argv[0] to exist"); let this_executable = args.next().expect("exepcted argv[0] to exist");
if executable_path == &PathBuf::from(this_executable) { if executable_path == &PathBuf::from(this_executable) {
// Avoid spawning infinitely many processes until resource // Avoid spawning infinitely many processes until resource
// exhaustion. // exhaustion.
let _ = ui.write_stderr(&format_bytes!( let _ = ui.write_stderr(&format_bytes!(
b"Blocking recursive fallback. The 'rhg.fallback-executable = {}' config \ b"Blocking recursive fallback. The 'rhg.fallback-executable = {}' config \
points to `rhg` itself.\n", points to `rhg` itself.\n",
executable executable
)); ));
on_unsupported = OnUnsupported::Abort on_unsupported = OnUnsupported::Abort
} else { } else {
// `args` is now `argv[1..]` since we’ve already consumed `argv[0]` // `args` is now `argv[1..]` since we’ve already consumed
// `argv[0]`
let mut command = Command::new(executable_path); let mut command = Command::new(executable_path);
command.args(args); command.args(args);
if let Some(initial) = initial_current_dir { if let Some(initial) = initial_current_dir {
command.current_dir(initial); command.current_dir(initial);
} }
let result = command.status(); let result = command.status();
match result { match result {
Ok(status) => std::process::exit( Ok(status) => std::process::exit(
/// See `HgError::UnsupportedFeature` and `CommandError::UnsupportedFeature`. /// See `HgError::UnsupportedFeature` and `CommandError::UnsupportedFeature`.
enum OnUnsupported { enum OnUnsupported {
/// Print an error message describing what feature is not supported, /// Print an error message describing what feature is not supported,
/// and exit with code 252. /// and exit with code 252.
Abort, Abort,
/// Silently exit with code 252. /// Silently exit with code 252.
AbortSilent, AbortSilent,
/// Try running a Python implementation /// Try running a Python implementation
Fallback { executable: Vec<u8> }, Fallback { executable: Option<Vec<u8>> },
} }
impl OnUnsupported { impl OnUnsupported {
const DEFAULT: Self = OnUnsupported::Abort; const DEFAULT: Self = OnUnsupported::Abort;
fn from_config(ui: &Ui, config: &Config) -> Self { fn from_config(config: &Config) -> Self {
match config match config
.get(b"rhg", b"on-unsupported") .get(b"rhg", b"on-unsupported")
.map(|value| value.to_ascii_lowercase()) .map(|value| value.to_ascii_lowercase())
.as_deref() .as_deref()
{ {
Some(b"abort") => OnUnsupported::Abort, Some(b"abort") => OnUnsupported::Abort,
Some(b"abort-silent") => OnUnsupported::AbortSilent, Some(b"abort-silent") => OnUnsupported::AbortSilent,
Some(b"fallback") => OnUnsupported::Fallback { Some(b"fallback") => OnUnsupported::Fallback {
executable: config executable: config
.get(b"rhg", b"fallback-executable") .get(b"rhg", b"fallback-executable")
.unwrap_or_else(|| { .map(|x| x.to_owned()),
exit_no_fallback(
ui,
Self::Abort,
Err(CommandError::abort(
"abort: 'rhg.on-unsupported=fallback' without \
'rhg.fallback-executable' set."
)),
false,
)
})
.to_owned(),
}, },
None => Self::DEFAULT, None => Self::DEFAULT,
Some(_) => { Some(_) => {
// TODO: warn about unknown config value // TODO: warn about unknown config value
Self::DEFAULT Self::DEFAULT
} }
} }
} }

tests/test-rhg.t

rhg cat [OPTIONS] <FILE>... rhg cat [OPTIONS] <FILE>...
For more information try --help For more information try --help
[252] [252]
$ rhg cat original --exclude="*.rs" $ rhg cat original --exclude="*.rs"
original content original content
$ FALLBACK_EXE="$RHG_FALLBACK_EXECUTABLE" $ (unset RHG_FALLBACK_EXECUTABLE; rhg cat original --exclude="*.rs")
$ unset RHG_FALLBACK_EXECUTABLE
$ rhg cat original --exclude="*.rs"
abort: 'rhg.on-unsupported=fallback' without 'rhg.fallback-executable' set. abort: 'rhg.on-unsupported=fallback' without 'rhg.fallback-executable' set.
[255] [255]
$ RHG_FALLBACK_EXECUTABLE="$FALLBACK_EXE"
$ export RHG_FALLBACK_EXECUTABLE $ (unset RHG_FALLBACK_EXECUTABLE; rhg cat original)
original content
$ rhg cat original --exclude="*.rs" --config rhg.fallback-executable=false $ rhg cat original --exclude="*.rs" --config rhg.fallback-executable=false
[1] [1]
$ rhg cat original --exclude="*.rs" --config rhg.fallback-executable=hg-non-existent $ rhg cat original --exclude="*.rs" --config rhg.fallback-executable=hg-non-existent
tried to fall back to a 'hg-non-existent' sub-process but got error $ENOENT$ tried to fall back to a 'hg-non-existent' sub-process but got error $ENOENT$
unsupported feature: error: Found argument '--exclude' which wasn't expected, or isn't valid in this context unsupported feature: error: Found argument '--exclude' which wasn't expected, or isn't valid in this context