This is an archive of the discontinued Mercurial Phabricator instance.

Differential D12578

rhg: use `Command::exec` instead of `Command::status`
ClosedPublic

Authored by Alphare on Apr 20 2022, 11:10 AM.

Details

Reviewers
martinvonz
Group Reviewers
hg-reviewers
Commits
rHG006688e36e12: rhg: use `Command::exec` instead of `Command::status`
rHG2b1d4e0baca6: rhg: use `Command::exec` instead of `Command::status`
Summary

rhg is supposed to be a transparent executable, using a subprocess defeats
that purpose. See inline comments for more details.

This also introduces the which crate to check if the fallback executable
actually exists to help debugging (plain execve doesn't give much
information).

The error code 253 is used to signify that the fallback is not found, but may
mean in the future that it is otherwise invalid if we start being more
specific.

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

Alphare created this revision.Apr 20 2022, 11:10 AM

Pending CI refresh

martinvonz requested changes to this revision.Apr 21 2022, 3:42 PM
martinvonz added a subscriber: martinvonz.
martinvonz added inline comments.
rust/rhg/src/main.rs
421

Since you said you used the which crate in order to make debugging easier, it surprised me that you just call .is_ok() here. How about replacing it by a if let Err(err) = ... and debug message based on that err?

441

nit: s/is ran/is run/

This revision now requires changes to proceed.Apr 21 2022, 3:42 PM
Alphare updated this revision to Diff 33308.Apr 25 2022, 5:36 AM
Alphare updated this revision to Diff 33309.
martinvonz accepted this revision.Apr 25 2022, 12:06 PM
This revision is now accepted and ready to land.Apr 25 2022, 12:06 PM
Alphare added a commit: rHG2b1d4e0baca6: rhg: use `Command::exec` instead of `Command::status`.Apr 25 2022, 4:12 PM
Closed by commit rHG2b1d4e0baca6: rhg: use `Command::exec` instead of `Command::status` (authored by Alphare). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 33312

rust/Cargo.lock

[[package]] [[package]]
name = "lazy_static" name = "lazy_static"
version = "1.4.0" version = "1.4.0"
source = "registry+https://github.com/rust-lang/crates.io-index" source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "e2abad23fbc42b3700f2f279844dc832adb2b2eb069b2df918f455c4e18cc646" checksum = "e2abad23fbc42b3700f2f279844dc832adb2b2eb069b2df918f455c4e18cc646"
[[package]] [[package]]
name = "libc" name = "libc"
version = "0.2.81" version = "0.2.124"
source = "registry+https://github.com/rust-lang/crates.io-index" source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "1482821306169ec4d07f6aca392a4681f66c75c9918aa49641a2595db64053cb" checksum = "21a41fed9d98f27ab1c6d161da622a4fa35e8a54a8adc24bbf3ddd0ef70b0e50"
[[package]] [[package]]
name = "libm" name = "libm"
version = "0.2.1" version = "0.2.1"
source = "registry+https://github.com/rust-lang/crates.io-index" source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "c7d73b3f436185384286bd8098d17ec07c9a7d2388a6599f824d8502b529702a" checksum = "c7d73b3f436185384286bd8098d17ec07c9a7d2388a6599f824d8502b529702a"
[[package]] [[package]]
"format-bytes", "format-bytes",
"hg-core", "hg-core",
"home", "home",
"lazy_static", "lazy_static",
"log", "log",
"micro-timer", "micro-timer",
"regex", "regex",
"users", "users",
"which",
] ]
[[package]] [[package]]
name = "same-file" name = "same-file"
version = "1.0.6" version = "1.0.6"
source = "registry+https://github.com/rust-lang/crates.io-index" source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "93fc1dc3aaa9bfed95e02e6eadabb4baf7e3078b0bd1b4d7b6b0b68378900502" checksum = "93fc1dc3aaa9bfed95e02e6eadabb4baf7e3078b0bd1b4d7b6b0b68378900502"
dependencies = [ dependencies = [
[[package]] [[package]]
name = "wasi" name = "wasi"
version = "0.10.0+wasi-snapshot-preview1" version = "0.10.0+wasi-snapshot-preview1"
source = "registry+https://github.com/rust-lang/crates.io-index" source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "1a143597ca7c7793eff794def352d41792a93c481eb1042423ff7ff72ba2c31f" checksum = "1a143597ca7c7793eff794def352d41792a93c481eb1042423ff7ff72ba2c31f"
[[package]] [[package]]
name = "which"
version = "4.2.5"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "5c4fb54e6113b6a8772ee41c3404fb0301ac79604489467e0a9ce1f3e97c24ae"
dependencies = [
"either",
"lazy_static",
"libc",
]
[[package]]
name = "winapi" name = "winapi"
version = "0.3.9" version = "0.3.9"
source = "registry+https://github.com/rust-lang/crates.io-index" source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "5c839a674fcd7a98952e593242ea400abe93992746761e38641405d28b00f419" checksum = "5c839a674fcd7a98952e593242ea400abe93992746761e38641405d28b00f419"
dependencies = [ dependencies = [
"winapi-i686-pc-windows-gnu", "winapi-i686-pc-windows-gnu",
"winapi-x86_64-pc-windows-gnu", "winapi-x86_64-pc-windows-gnu",
] ]

rust/hg-core/src/exit_codes.rs

// Abort when there is an error while parsing config // Abort when there is an error while parsing config
pub const CONFIG_PARSE_ERROR_ABORT: ExitCode = 10; pub const CONFIG_PARSE_ERROR_ABORT: ExitCode = 10;
/// Generic something completed but did not succeed /// Generic something completed but did not succeed
pub const UNSUCCESSFUL: ExitCode = 1; pub const UNSUCCESSFUL: ExitCode = 1;
/// Command or feature not implemented by rhg /// Command or feature not implemented by rhg
pub const UNIMPLEMENTED: ExitCode = 252; pub const UNIMPLEMENTED: ExitCode = 252;
/// The fallback path is not valid
pub const INVALID_FALLBACK: ExitCode = 253;

rust/rhg/Cargo.toml

home = "0.5.3" home = "0.5.3"
lazy_static = "1.4.0" lazy_static = "1.4.0"
log = "0.4.11" log = "0.4.11"
micro-timer = "0.3.1" micro-timer = "0.3.1"
regex = "1.3.9" regex = "1.3.9"
env_logger = "0.7.1" env_logger = "0.7.1"
format-bytes = "0.3.0" format-bytes = "0.3.0"
users = "0.11.0" users = "0.11.0"
which = "4.2.5"

rust/rhg/src/error.rs

/// Exit with a failure exit code but no message. /// Exit with a failure exit code but no message.
Unsuccessful, Unsuccessful,
/// Encountered something (such as a CLI argument, repository layout, …) /// Encountered something (such as a CLI argument, repository layout, …)
/// not supported by this version of `rhg`. Depending on configuration /// not supported by this version of `rhg`. Depending on configuration
/// `rhg` may attempt to silently fall back to Python-based `hg`, which /// `rhg` may attempt to silently fall back to Python-based `hg`, which
/// may or may not support this feature. /// may or may not support this feature.
UnsupportedFeature { message: Vec<u8> }, UnsupportedFeature { message: Vec<u8> },
/// The fallback executable does not exist (or has some other problem if
/// we end up being more precise about broken fallbacks).
InvalidFallback { path: Vec<u8>, err: String },
} }
impl CommandError { impl CommandError {
pub fn abort(message: impl AsRef<str>) -> Self { pub fn abort(message: impl AsRef<str>) -> Self {
CommandError::abort_with_exit_code(message, exit_codes::ABORT) CommandError::abort_with_exit_code(message, exit_codes::ABORT)
} }
pub fn abort_with_exit_code( pub fn abort_with_exit_code(

rust/rhg/src/main.rs

extern crate log; extern crate log;
use crate::error::CommandError; use crate::error::CommandError;
use crate::ui::{local_to_utf8, Ui}; use crate::ui::{local_to_utf8, Ui};
use clap::App; use clap::App;
use clap::AppSettings; use clap::AppSettings;
use clap::Arg; use clap::Arg;
use clap::ArgMatches; use clap::ArgMatches;
use format_bytes::{format_bytes, join}; use format_bytes::{format_bytes, join};
use hg::config::{Config, ConfigSource}; use hg::config::{Config, ConfigSource};
use hg::exit_codes; use hg::exit_codes;
use hg::repo::{Repo, RepoError}; use hg::repo::{Repo, RepoError};
use hg::utils::files::{get_bytes_from_os_str, get_path_from_bytes}; use hg::utils::files::{get_bytes_from_os_str, get_path_from_bytes};
use hg::utils::SliceExt; use hg::utils::SliceExt;
use std::collections::HashSet; use std::collections::HashSet;
use std::ffi::OsString; use std::ffi::OsString;
use std::os::unix::prelude::CommandExt;
use std::path::PathBuf; use std::path::PathBuf;
use std::process::Command; use std::process::Command;
mod blackbox; mod blackbox;
mod color; mod color;
mod error; mod error;
mod ui; mod ui;
pub mod utils { pub mod utils {
}) => { }) => {
if use_detailed_exit_code { if use_detailed_exit_code {
*detailed_exit_code *detailed_exit_code
} else { } else {
exit_codes::ABORT exit_codes::ABORT
} }
} }
Err(CommandError::Unsuccessful) => exit_codes::UNSUCCESSFUL, Err(CommandError::Unsuccessful) => exit_codes::UNSUCCESSFUL,
// Exit with a specific code and no error message to let a potential // Exit with a specific code and no error message to let a potential
// wrapper script fallback to Python-based Mercurial. // wrapper script fallback to Python-based Mercurial.
Err(CommandError::UnsupportedFeature { .. }) => { Err(CommandError::UnsupportedFeature { .. }) => {
exit_codes::UNIMPLEMENTED exit_codes::UNIMPLEMENTED
} }
Err(CommandError::InvalidFallback { .. }) => {
exit_codes::INVALID_FALLBACK
}
} }
} }
fn exit( fn exit(
initial_current_dir: &Option<PathBuf>, initial_current_dir: &Option<PathBuf>,
ui: &Ui, ui: &Ui,
mut on_unsupported: OnUnsupported, mut on_unsupported: OnUnsupported,
result: Result<(), CommandError>, result: Result<(), CommandError>,
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 {
log::debug!("falling back (see trace-level log)"); log::debug!("falling back (see trace-level log)");
log::trace!("{}", local_to_utf8(message)); log::trace!("{}", local_to_utf8(message));
if let Err(err) = which::which(executable_path) {
martinvonzUnsubmitted
Not Done

Since you said you used the which crate in order to make debugging easier, it surprised me that you just call .is_ok() here. How about replacing it by a if let Err(err) = ... and debug message based on that err?

martinvonz: Since you said you used the `which` crate in order to make debugging easier, it surprised me…
exit_no_fallback(
ui,
OnUnsupported::Abort,
Err(CommandError::InvalidFallback {
path: executable.to_owned(),
err: err.to_string(),
}),
use_detailed_exit_code,
)
}
// `args` is now `argv[1..]` since we’ve already consumed // `args` is now `argv[1..]` since we’ve already consumed
// `argv[0]` // `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(); // We don't use subprocess because proper signal handling is harder
match result { // and we don't want to keep `rhg` around after a fallback anyway.
Ok(status) => std::process::exit( // For example, if `rhg` is run in the background and falls back to
martinvonzUnsubmitted
Not Done

nit: s/is ran/is run/

martinvonz: nit: s/is ran/is run/
status.code().unwrap_or(exit_codes::ABORT), // `hg` which, in turn, waits for a signal, we'll get stuck if
), // we're doing plain subprocess.
Err(error) => { //
let _ = ui.write_stderr(&format_bytes!( // If `exec` returns, we can only assume our process is very broken
b"tried to fall back to a '{}' sub-process but got error {}\n", // (see its documentation), so only try to forward the error code
executable, format_bytes::Utf8(error) // when exiting.
)); let err = command.exec();
on_unsupported = OnUnsupported::Abort std::process::exit(
} err.raw_os_error().unwrap_or(exit_codes::ABORT),
} );
} }
} }
exit_no_fallback(ui, on_unsupported, result, use_detailed_exit_code) exit_no_fallback(ui, on_unsupported, result, use_detailed_exit_code)
} }
fn exit_no_fallback( fn exit_no_fallback(
ui: &Ui, ui: &Ui,
on_unsupported: OnUnsupported, on_unsupported: OnUnsupported,
b"unsupported feature: {}\n", b"unsupported feature: {}\n",
message message
)); ));
} }
OnUnsupported::AbortSilent => {} OnUnsupported::AbortSilent => {}
OnUnsupported::Fallback { .. } => unreachable!(), OnUnsupported::Fallback { .. } => unreachable!(),
} }
} }
Err(CommandError::InvalidFallback { path, err }) => {
let _ = ui.write_stderr(&format_bytes!(
b"abort: invalid fallback '{}': {}\n",
path,
err.as_bytes(),
));
}
} }
std::process::exit(exit_code(&result, use_detailed_exit_code)) std::process::exit(exit_code(&result, use_detailed_exit_code))
} }
macro_rules! subcommands { macro_rules! subcommands {
($( $command: ident )+) => { ($( $command: ident )+) => {
mod commands { mod commands {
$( $(

tests/test-rhg.t

$ (unset RHG_FALLBACK_EXECUTABLE; rhg cat original) $ (unset RHG_FALLBACK_EXECUTABLE; rhg cat original)
original content 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$ abort: invalid fallback 'hg-non-existent': cannot find binary path
unsupported feature: error: Found argument '--exclude' which wasn't expected, or isn't valid in this context [253]
USAGE:
rhg cat [OPTIONS] <FILE>...
For more information try --help
[252]
$ rhg cat original --exclude="*.rs" --config rhg.fallback-executable=rhg $ rhg cat original --exclude="*.rs" --config rhg.fallback-executable=rhg
Blocking recursive fallback. The 'rhg.fallback-executable = rhg' config points to `rhg` itself. Blocking recursive fallback. The 'rhg.fallback-executable = rhg' config points to `rhg` itself.
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
USAGE: USAGE:
rhg cat [OPTIONS] <FILE>... rhg cat [OPTIONS] <FILE>...