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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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 28546

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