( )⚙ D10189 rhg: Remove `rhg.fallback-executable=hg` default configuration

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D10189

rhg: Remove `rhg.fallback-executable=hg` default configuration
ClosedPublic

Authored by SimonSapin on Mar 12 2021, 5:08 PM.

Details

Reviewers
marmoute
Group Reviewers
hg-reviewers
Commits
rHGbde90e9b4507: rhg: Remove `rhg.fallback-executable=hg` default configuration
Summary

When rhg.on-unsupported is configured to fallback and an unsupported
feature is encountered, the previous default was to look for an hg
executable in $PATH.

This default was fragile since it was easy to end up accidentally using
an older version of Mercurial installed system-wide,
when a local (perhaps patched) installation was intended.

Instead, it is now an error to have rhg.on-unsupported=fallback
without also configuring an explicit path or the fallback executable.

Diff Detail

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

Event Timeline

SimonSapin created this revision.Mar 12 2021, 5:08 PM
marmoute accepted this revision.Mar 15 2021, 5:48 AM
marmoute added a subscriber: marmoute.

Looks overall good, with maybe a folow up for the message.

rust/rhg/src/main.rs
411–412

That message in a bit long, what about:

abort: 'rhg.on-unsupported=fallback' without 'rhg.fallback-executable' set.

to get < 80 char ?

baymax updated this revision to Diff 26352.Mar 15 2021, 9:10 AM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

SimonSapin added a commit: rHGbde90e9b4507: rhg: Remove `rhg.fallback-executable=hg` default configuration.Mar 16 2021, 5:48 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHGbde90e9b4507: rhg: Remove `rhg.fallback-executable=hg` default configuration (authored by SimonSapin). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mrust/rhg/src/main.rs (34 lines)
Mtests/test-rhg.t (8 lines)

Diff 26352

rust/rhg/src/main.rs

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(&non_repo_config), OnUnsupported::from_config(&ui, &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
), ),
}), }),
) )
} }
} }
let repo_path = early_args.repo.as_deref().map(get_path_from_bytes); let repo_path = early_args.repo.as_deref().map(get_path_from_bytes);
let repo_result = match Repo::find(&non_repo_config, repo_path) { let repo_result = match Repo::find(&non_repo_config, repo_path) {
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(&non_repo_config), OnUnsupported::from_config(&ui, &non_repo_config),
Err(error.into()), Err(error.into()),
), ),
}; };
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 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(&initial_current_dir, &ui, on_unsupported, result)
&initial_current_dir,
&ui,
OnUnsupported::from_config(config),
result,
)
} }
fn exit_code(result: &Result<(), CommandError>) -> i32 { fn exit_code(result: &Result<(), CommandError>) -> i32 {
match result { match result {
Ok(()) => exitcode::OK, Ok(()) => exitcode::OK,
Err(CommandError::Abort { .. }) => exitcode::ABORT, Err(CommandError::Abort { .. }) => exitcode::ABORT,
Err(CommandError::Unsuccessful) => exitcode::UNSUCCESSFUL, Err(CommandError::Unsuccessful) => exitcode::UNSUCCESSFUL,
b"tried to fall back to a '{}' sub-process but got error {}\n", b"tried to fall back to a '{}' sub-process but got error {}\n",
executable, format_bytes::Utf8(error) executable, format_bytes::Utf8(error)
)); ));
on_unsupported = OnUnsupported::Abort on_unsupported = OnUnsupported::Abort
} }
} }
} }
} }
exit_no_fallback(ui, on_unsupported, result)
}
fn exit_no_fallback(
ui: &Ui,
on_unsupported: OnUnsupported,
result: Result<(), CommandError>,
) -> ! {
match &result { match &result {
Ok(_) => {} Ok(_) => {}
Err(CommandError::Unsuccessful) => {} Err(CommandError::Unsuccessful) => {}
Err(CommandError::Abort { message }) => { Err(CommandError::Abort { message }) => {
if !message.is_empty() { if !message.is_empty() {
// Ignore errors when writing to stderr, we’re already exiting // Ignore errors when writing to stderr, we’re already exiting
// with failure code so there’s not much more we can do. // with failure code so there’s not much more we can do.
let _ = ui.write_stderr(&format_bytes!(b"{}\n", message)); let _ = ui.write_stderr(&format_bytes!(b"{}\n", message));
/// 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: Vec<u8> },
} }
impl OnUnsupported { impl OnUnsupported {
const DEFAULT: Self = OnUnsupported::Abort; const DEFAULT: Self = OnUnsupported::Abort;
const DEFAULT_FALLBACK_EXECUTABLE: &'static [u8] = b"hg";
fn from_config(config: &Config) -> Self { fn from_config(ui: &Ui, 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(Self::DEFAULT_FALLBACK_EXECUTABLE) .unwrap_or_else(|| {
exit_no_fallback(
ui,
Self::Abort,
Err(CommandError::abort(
"abort: 'rhg.on-unsupported=fallback' without \
'rhg.fallback-executable' set."
marmouteUnsubmitted
Not Done

That message in a bit long, what about:

abort: 'rhg.on-unsupported=fallback' without 'rhg.fallback-executable' set.

to get < 80 char ?

marmoute: That message in a bit long, what about: ``` abort: 'rhg.on-unsupported=fallback' without 'rhg.
)),
)
})
.to_owned(), .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

Fallback to Python Fallback to Python
$ $NO_FALLBACK rhg cat original $ $NO_FALLBACK rhg cat original
unsupported feature: `rhg cat` without `--rev` / `-r` unsupported feature: `rhg cat` without `--rev` / `-r`
[252] [252]
$ rhg cat original $ rhg cat original
original content original content
$ FALLBACK_EXE="$RHG_FALLBACK_EXECUTABLE"
$ unset RHG_FALLBACK_EXECUTABLE
$ rhg cat original
abort: 'rhg.on-unsupported=fallback' without 'rhg.fallback-executable' set.
[255]
$ RHG_FALLBACK_EXECUTABLE="$FALLBACK_EXE"
$ export RHG_FALLBACK_EXECUTABLE
$ rhg cat original --config rhg.fallback-executable=false $ rhg cat original --config rhg.fallback-executable=false
[1] [1]
$ rhg cat original --config rhg.fallback-executable=hg-non-existent $ rhg cat original --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: `rhg cat` without `--rev` / `-r` unsupported feature: `rhg cat` without `--rev` / `-r`
[252] [252]