This is an archive of the discontinued Mercurial Phabricator instance.

Differential D10838

rhg: add exit code to HgError::Abort()
ClosedPublic

Authored by pulkit on Jun 7 2021, 8:00 AM.

Details

Reviewers
Alphare
Group Reviewers
hg-reviewers
Commits
rHG6e49769b7f97: rhg: add exit code to HgError::Abort()
Summary

My previous attempts to have rhg end with correct exit code was more of bug
hunting. I found cases which were failing and fixed them. But as one might
expect, more tests started failing.

Let's add exit code HgError::Abort() and make it users explicitly tell what
exit code they want.

Diff Detail

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

Event Timeline

pulkit created this revision.Jun 7 2021, 8:00 AM
SimonSapin added a subscriber: SimonSapin.Jun 7 2021, 11:00 AM
SimonSapin added inline comments.
rust/hg-core/src/config/layer.rs
83

Let’s define some named const items instead of using integer literals in this and other calls to HgError::abort. Maybe in errors.rs?

rust/hg-core/src/errors.rs
33–36

Please change this variant to use named fields, in order to say what this integer represents:

Abort { message: String, detailed_exit_code: i32 }

… and change "The given string" in the doc-comment to "message"

apkreach added a subscriber: apkreach.Jun 8 2021, 6:24 AM

pixel gun 3d apk is a Android Gun game where you are experiance the 800+ guns and play with your friends.

pulkit updated this revision to Diff 28511.Jun 9 2021, 3:49 AM
Alphare requested changes to this revision.Jun 9 2021, 5:18 AM
Alphare added a subscriber: Alphare.
Alphare added inline comments.
rust/hg-core/src/exitcode.rs
1 ↗(On Diff #28511)

Nitpicking here, but exit_codes.rs would probably be a better name for this file.

rust/hg-core/src/repo.rs
154

This should use the constants defined above

This revision now requires changes to proceed.Jun 9 2021, 5:18 AM
baymax updated this revision to Diff 28537.Jun 9 2021, 1:22 PM

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

Alphare accepted this revision.Jun 10 2021, 3:32 AM
This revision is now accepted and ready to land.Jun 10 2021, 3:32 AM
pulkit added a commit: rHG6e49769b7f97: rhg: add exit code to HgError::Abort().Jun 11 2021, 3:46 AM
Closed by commit rHG6e49769b7f97: rhg: add exit code to HgError::Abort() (authored by pulkit). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 28537

rust/hg-core/src/config/layer.rs

// layer.rs // layer.rs
// //
// Copyright 2020 // Copyright 2020
// Valentin Gatien-Baron, // Valentin Gatien-Baron,
// Raphaël Gomès <rgomes@octobus.net> // Raphaël Gomès <rgomes@octobus.net>
// //
// This software may be used and distributed according to the terms of the // This software may be used and distributed according to the terms of the
// GNU General Public License version 2 or any later version. // GNU General Public License version 2 or any later version.
use crate::errors::HgError; use crate::errors::HgError;
use crate::exit_codes::CONFIG_PARSE_ERROR_ABORT;
use crate::utils::files::{get_bytes_from_path, get_path_from_bytes}; use crate::utils::files::{get_bytes_from_path, get_path_from_bytes};
use format_bytes::{format_bytes, write_bytes, DisplayBytes}; use format_bytes::{format_bytes, write_bytes, DisplayBytes};
use lazy_static::lazy_static; use lazy_static::lazy_static;
use regex::bytes::Regex; use regex::bytes::Regex;
use std::collections::HashMap; use std::collections::HashMap;
use std::path::{Path, PathBuf}; use std::path::{Path, PathBuf};
lazy_static! { lazy_static! {
} }
let mut layer = Self::new(ConfigOrigin::CommandLine); let mut layer = Self::new(ConfigOrigin::CommandLine);
for arg in cli_config_args { for arg in cli_config_args {
let arg = arg.as_ref(); let arg = arg.as_ref();
if let Some((section, item, value)) = parse_one(arg) { if let Some((section, item, value)) = parse_one(arg) {
layer.add(section, item, value, None); layer.add(section, item, value, None);
} else { } else {
Err(HgError::abort(format!( Err(HgError::abort(
format!(
"abort: malformed --config option: '{}' \ "abort: malformed --config option: '{}' \
(use --config section.name=value)", (use --config section.name=value)",
String::from_utf8_lossy(arg), String::from_utf8_lossy(arg),
)))? ),
CONFIG_PARSE_ERROR_ABORT,
SimonSapinUnsubmitted
Not Done

Let’s define some named const items instead of using integer literals in this and other calls to HgError::abort. Maybe in errors.rs?

SimonSapin: Let’s define some named `const` items instead of using integer literals in this and other calls…
))?
} }
} }
if layer.sections.is_empty() { if layer.sections.is_empty() {
Ok(None) Ok(None)
} else { } else {
Ok(Some(layer)) Ok(Some(layer))
} }
} }

rust/hg-core/src/errors.rs

use crate::config::ConfigValueParseError; use crate::config::ConfigValueParseError;
use crate::exit_codes;
use std::fmt; use std::fmt;
/// Common error cases that can happen in many different APIs /// Common error cases that can happen in many different APIs
#[derive(Debug, derive_more::From)] #[derive(Debug, derive_more::From)]
pub enum HgError { pub enum HgError {
IoError { IoError {
error: std::io::Error, error: std::io::Error,
context: IoErrorContext, context: IoErrorContext,
/// implementation may or may not work. /// implementation may or may not work.
/// ///
/// The given string is a short explanation for users, not intended to be /// The given string is a short explanation for users, not intended to be
/// machine-readable. /// machine-readable.
UnsupportedFeature(String), UnsupportedFeature(String),
/// Operation cannot proceed for some other reason. /// Operation cannot proceed for some other reason.
/// ///
/// The given string is a short explanation for users, not intended to be /// The message is a short explanation for users, not intended to be
/// machine-readable. /// machine-readable.
Abort(String), Abort {
message: String,
detailed_exit_code: exit_codes::ExitCode,
},
SimonSapinUnsubmitted
Not Done

Please change this variant to use named fields, in order to say what this integer represents:

Abort { message: String, detailed_exit_code: i32 }

… and change "The given string" in the doc-comment to "message"

SimonSapin: Please change this variant to use named fields, in order to say what this integer represents…
/// A configuration value is not in the expected syntax. /// A configuration value is not in the expected syntax.
/// ///
/// These errors can happen in many places in the code because values are /// These errors can happen in many places in the code because values are
/// parsed lazily as the file-level parser does not know the expected type /// parsed lazily as the file-level parser does not know the expected type
/// and syntax of each value. /// and syntax of each value.
#[from] #[from]
ConfigValueParseError(ConfigValueParseError), ConfigValueParseError(ConfigValueParseError),
// to aid debugging? // to aid debugging?
// https://doc.rust-lang.org/std/backtrace/struct.Backtrace.html // https://doc.rust-lang.org/std/backtrace/struct.Backtrace.html
HgError::CorruptedRepository(explanation.into()) HgError::CorruptedRepository(explanation.into())
} }
pub fn unsupported(explanation: impl Into<String>) -> Self { pub fn unsupported(explanation: impl Into<String>) -> Self {
HgError::UnsupportedFeature(explanation.into()) HgError::UnsupportedFeature(explanation.into())
} }
pub fn abort(explanation: impl Into<String>) -> Self {
HgError::Abort(explanation.into()) pub fn abort(
explanation: impl Into<String>,
exit_code: exit_codes::ExitCode,
) -> Self {
HgError::Abort {
message: explanation.into(),
detailed_exit_code: exit_code,
}
} }
} }
// TODO: use `DisplayBytes` instead to show non-Unicode filenames losslessly? // TODO: use `DisplayBytes` instead to show non-Unicode filenames losslessly?
impl fmt::Display for HgError { impl fmt::Display for HgError {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match self { match self {
HgError::Abort(explanation) => write!(f, "{}", explanation), HgError::Abort { message, .. } => write!(f, "{}", message),
HgError::IoError { error, context } => { HgError::IoError { error, context } => {
write!(f, "abort: {}: {}", context, error) write!(f, "abort: {}: {}", context, error)
} }
HgError::CorruptedRepository(explanation) => { HgError::CorruptedRepository(explanation) => {
write!(f, "abort: {}", explanation) write!(f, "abort: {}", explanation)
} }
HgError::UnsupportedFeature(explanation) => { HgError::UnsupportedFeature(explanation) => {
write!(f, "unsupported feature: {}", explanation) write!(f, "unsupported feature: {}", explanation)

rust/hg-core/src/exit_codes.rs

  • This file was added.
pub type ExitCode = i32;
/// Successful exit
pub const OK: ExitCode = 0;
/// Generic abort
pub const ABORT: ExitCode = 255;
// Abort when there is a config related error
pub const CONFIG_ERROR_ABORT: ExitCode = 30;
// Abort when there is an error while parsing config
pub const CONFIG_PARSE_ERROR_ABORT: ExitCode = 10;
/// Generic something completed but did not succeed
pub const UNSUCCESSFUL: ExitCode = 1;
/// Command or feature not implemented by rhg
pub const UNIMPLEMENTED: ExitCode = 252;

rust/hg-core/src/lib.rs

// Copyright 2018-2020 Georges Racinet <georges.racinet@octobus.net> // Copyright 2018-2020 Georges Racinet <georges.racinet@octobus.net>
// and Mercurial contributors // and Mercurial contributors
// //
// This software may be used and distributed according to the terms of the // This software may be used and distributed according to the terms of the
// GNU General Public License version 2 or any later version. // GNU General Public License version 2 or any later version.
mod ancestors; mod ancestors;
pub mod dagops; pub mod dagops;
pub mod errors; pub mod errors;
pub use ancestors::{AncestorsIterator, LazyAncestors, MissingAncestors}; pub use ancestors::{AncestorsIterator, LazyAncestors, MissingAncestors};
pub mod dirstate; pub mod dirstate;
pub mod dirstate_tree; pub mod dirstate_tree;
pub mod discovery; pub mod discovery;
pub mod exit_codes;
pub mod requirements; pub mod requirements;
pub mod testing; // unconditionally built, for use from integration tests pub mod testing; // unconditionally built, for use from integration tests
pub use dirstate::{ pub use dirstate::{
dirs_multiset::{DirsMultiset, DirsMultisetIter}, dirs_multiset::{DirsMultiset, DirsMultisetIter},
dirstate_map::DirstateMap, dirstate_map::DirstateMap,
parsers::{pack_dirstate, parse_dirstate, PARENT_SIZE}, parsers::{pack_dirstate, parse_dirstate, PARENT_SIZE},
status::{ status::{
status, BadMatch, BadType, DirstateStatus, HgPathCow, StatusError, status, BadMatch, BadType, DirstateStatus, HgPathCow, StatusError,

rust/hg-core/src/repo.rs

use crate::config::{Config, ConfigError, ConfigParseError}; use crate::config::{Config, ConfigError, ConfigParseError};
use crate::errors::{HgError, IoErrorContext, IoResultExt}; use crate::errors::{HgError, IoErrorContext, IoResultExt};
use crate::exit_codes;
use crate::requirements; use crate::requirements;
use crate::utils::files::get_path_from_bytes; use crate::utils::files::get_path_from_bytes;
use crate::utils::SliceExt; use crate::utils::SliceExt;
use memmap::{Mmap, MmapOptions}; use memmap::{Mmap, MmapOptions};
use std::collections::HashSet; use std::collections::HashSet;
use std::path::{Path, PathBuf}; use std::path::{Path, PathBuf};
/// A repository on disk /// A repository on disk
if share_safe && !source_is_share_safe { if share_safe && !source_is_share_safe {
return Err(match config return Err(match config
.get(b"share", b"safe-mismatch.source-not-safe") .get(b"share", b"safe-mismatch.source-not-safe")
{ {
Some(b"abort") | None => HgError::abort( Some(b"abort") | None => HgError::abort(
"abort: share source does not support share-safe requirement\n\ "abort: share source does not support share-safe requirement\n\
(see `hg help config.format.use-share-safe` for more information)", (see `hg help config.format.use-share-safe` for more information)",
exit_codes::ABORT,
AlphareUnsubmitted
Not Done

This should use the constants defined above

Alphare: This should use the constants defined above
), ),
_ => HgError::unsupported("share-safe downgrade"), _ => HgError::unsupported("share-safe downgrade"),
} }
.into()); .into());
} else if source_is_share_safe && !share_safe { } else if source_is_share_safe && !share_safe {
return Err( return Err(
match config.get(b"share", b"safe-mismatch.source-safe") { match config.get(b"share", b"safe-mismatch.source-safe") {
Some(b"abort") | None => HgError::abort( Some(b"abort") | None => HgError::abort(
"abort: version mismatch: source uses share-safe \ "abort: version mismatch: source uses share-safe \
functionality while the current share does not\n\ functionality while the current share does not\n\
(see `hg help config.format.use-share-safe` for more information)", (see `hg help config.format.use-share-safe` for more information)",
exit_codes::ABORT,
), ),
_ => HgError::unsupported("share-safe upgrade"), _ => HgError::unsupported("share-safe upgrade"),
} }
.into(), .into(),
); );
} }
if share_safe { if share_safe {

rust/rhg/src/error.rs

use crate::exitcode;
use crate::ui::utf8_to_local; use crate::ui::utf8_to_local;
use crate::ui::UiError; use crate::ui::UiError;
use crate::NoRepoInCwdError; use crate::NoRepoInCwdError;
use format_bytes::format_bytes; use format_bytes::format_bytes;
use hg::config::{ConfigError, ConfigParseError, ConfigValueParseError}; use hg::config::{ConfigError, ConfigParseError, ConfigValueParseError};
use hg::errors::HgError; use hg::errors::HgError;
use hg::exit_codes;
use hg::repo::RepoError; use hg::repo::RepoError;
use hg::revlog::revlog::RevlogError; use hg::revlog::revlog::RevlogError;
use hg::utils::files::get_bytes_from_path; use hg::utils::files::get_bytes_from_path;
use hg::{DirstateError, DirstateMapError, StatusError}; use hg::{DirstateError, DirstateMapError, StatusError};
use std::convert::From; use std::convert::From;
/// The kind of command error /// The kind of command error
#[derive(Debug)] #[derive(Debug)]
pub enum CommandError { pub enum CommandError {
/// Exit with an error message and "standard" failure exit code. /// Exit with an error message and "standard" failure exit code.
Abort { Abort {
message: Vec<u8>, message: Vec<u8>,
detailed_exit_code: exitcode::ExitCode, detailed_exit_code: exit_codes::ExitCode,
}, },
/// 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> },
} }
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, exitcode::ABORT) CommandError::abort_with_exit_code(message, exit_codes::ABORT)
} }
pub fn abort_with_exit_code( pub fn abort_with_exit_code(
message: impl AsRef<str>, message: impl AsRef<str>,
detailed_exit_code: exitcode::ExitCode, detailed_exit_code: exit_codes::ExitCode,
) -> Self { ) -> Self {
CommandError::Abort { CommandError::Abort {
// TODO: bytes-based (instead of Unicode-based) formatting // TODO: bytes-based (instead of Unicode-based) formatting
// of error messages to handle non-UTF-8 filenames etc: // of error messages to handle non-UTF-8 filenames etc:
// https://www.mercurial-scm.org/wiki/EncodingStrategy#Mixing_output // https://www.mercurial-scm.org/wiki/EncodingStrategy#Mixing_output
message: utf8_to_local(message.as_ref()).into(), message: utf8_to_local(message.as_ref()).into(),
detailed_exit_code: detailed_exit_code, detailed_exit_code: detailed_exit_code,
} }
} }
} }
} }
impl From<ConfigValueParseError> for CommandError { impl From<ConfigValueParseError> for CommandError {
fn from(error: ConfigValueParseError) -> Self { fn from(error: ConfigValueParseError) -> Self {
CommandError::abort_with_exit_code( CommandError::abort_with_exit_code(
error.to_string(), error.to_string(),
exitcode::CONFIG_ERROR_ABORT, exit_codes::CONFIG_ERROR_ABORT,
) )
} }
} }
impl From<UiError> for CommandError { impl From<UiError> for CommandError {
fn from(_error: UiError) -> Self { fn from(_error: UiError) -> Self {
// If we already failed writing to stdout or stderr, // If we already failed writing to stdout or stderr,
// writing an error message to stderr about it would be likely to fail // writing an error message to stderr about it would be likely to fail
// too. // too.
CommandError::abort("") CommandError::abort("")
} }
} }
impl From<RepoError> for CommandError { impl From<RepoError> for CommandError {
fn from(error: RepoError) -> Self { fn from(error: RepoError) -> Self {
match error { match error {
RepoError::NotFound { at } => CommandError::Abort { RepoError::NotFound { at } => CommandError::Abort {
message: format_bytes!( message: format_bytes!(
b"abort: repository {} not found", b"abort: repository {} not found",
get_bytes_from_path(at) get_bytes_from_path(at)
), ),
detailed_exit_code: exitcode::ABORT, detailed_exit_code: exit_codes::ABORT,
}, },
RepoError::ConfigParseError(error) => error.into(), RepoError::ConfigParseError(error) => error.into(),
RepoError::Other(error) => error.into(), RepoError::Other(error) => error.into(),
} }
} }
} }
impl<'a> From<&'a NoRepoInCwdError> for CommandError { impl<'a> From<&'a NoRepoInCwdError> for CommandError {
fn from(error: &'a NoRepoInCwdError) -> Self { fn from(error: &'a NoRepoInCwdError) -> Self {
let NoRepoInCwdError { cwd } = error; let NoRepoInCwdError { cwd } = error;
CommandError::Abort { CommandError::Abort {
message: format_bytes!( message: format_bytes!(
b"abort: no repository found in '{}' (.hg not found)!", b"abort: no repository found in '{}' (.hg not found)!",
get_bytes_from_path(cwd) get_bytes_from_path(cwd)
), ),
detailed_exit_code: exitcode::ABORT, detailed_exit_code: exit_codes::ABORT,
} }
} }
} }
impl From<ConfigError> for CommandError { impl From<ConfigError> for CommandError {
fn from(error: ConfigError) -> Self { fn from(error: ConfigError) -> Self {
match error { match error {
ConfigError::Parse(error) => error.into(), ConfigError::Parse(error) => error.into(),
}; };
CommandError::Abort { CommandError::Abort {
message: format_bytes!( message: format_bytes!(
b"config error at {}{}: {}", b"config error at {}{}: {}",
origin, origin,
line_message, line_message,
message message
), ),
detailed_exit_code: exitcode::CONFIG_ERROR_ABORT, detailed_exit_code: exit_codes::CONFIG_ERROR_ABORT,
} }
} }
} }
impl From<(RevlogError, &str)> for CommandError { impl From<(RevlogError, &str)> for CommandError {
fn from((err, rev): (RevlogError, &str)) -> CommandError { fn from((err, rev): (RevlogError, &str)) -> CommandError {
match err { match err {
RevlogError::WDirUnsupported => CommandError::abort( RevlogError::WDirUnsupported => CommandError::abort(

rust/rhg/src/exitcode.rs

  • This file was deleted.
pub type ExitCode = i32;
/// Successful exit
pub const OK: ExitCode = 0;
/// Generic abort
pub const ABORT: ExitCode = 255;
// Abort when there is a config related error
pub const CONFIG_ERROR_ABORT: ExitCode = 30;
/// Generic something completed but did not succeed
pub const UNSUCCESSFUL: ExitCode = 1;
/// Command or feature not implemented by rhg
pub const UNIMPLEMENTED: ExitCode = 252;

rust/rhg/src/main.rs

extern crate log; extern crate log;
use crate::ui::Ui; use crate::ui::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::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::ffi::OsString; use std::ffi::OsString;
use std::path::PathBuf; use std::path::PathBuf;
use std::process::Command; use std::process::Command;
mod blackbox; mod blackbox;
mod error; mod error;
mod exitcode;
mod ui; mod ui;
use error::CommandError; use error::CommandError;
fn main_with_result( fn main_with_result(
process_start_time: &blackbox::ProcessStartTime, process_start_time: &blackbox::ProcessStartTime,
ui: &ui::Ui, ui: &ui::Ui,
repo: Result<&Repo, &NoRepoInCwdError>, repo: Result<&Repo, &NoRepoInCwdError>,
config: &Config, config: &Config,
) )
} }
fn exit_code( fn exit_code(
result: &Result<(), CommandError>, result: &Result<(), CommandError>,
use_detailed_exit_code: bool, use_detailed_exit_code: bool,
) -> i32 { ) -> i32 {
match result { match result {
Ok(()) => exitcode::OK, Ok(()) => exit_codes::OK,
Err(CommandError::Abort { Err(CommandError::Abort {
message: _, message: _,
detailed_exit_code, detailed_exit_code,
}) => { }) => {
if use_detailed_exit_code { if use_detailed_exit_code {
*detailed_exit_code *detailed_exit_code
} else { } else {
exitcode::ABORT exit_codes::ABORT
} }
} }
Err(CommandError::Unsuccessful) => exitcode::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 { .. }) => {
exitcode::UNIMPLEMENTED exit_codes::UNIMPLEMENTED
} }
} }
} }
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,
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(
status.code().unwrap_or(exitcode::ABORT), status.code().unwrap_or(exit_codes::ABORT),
), ),
Err(error) => { Err(error) => {
let _ = ui.write_stderr(&format_bytes!( let _ = ui.write_stderr(&format_bytes!(
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
} }
rust/rhg/src/error.rs