This is an archive of the discontinued Mercurial Phabricator instance.

Differential D10142

rhg: `cat` command: print error messages for missing files
ClosedPublic

Authored by SimonSapin on Mar 9 2021, 4:41 AM.

Details

Reviewers
marmoute
Alphare
Group Reviewers
hg-reviewers
Commits
rHGb1f2c2b336ec: rhg: `cat` command: print error messages for missing files
Summary

And exit with an error code if no file was matched.
This matches the behavior of Python-based hg.

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

SimonSapin created this revision.Mar 9 2021, 4:41 AM
baymax updated this revision to Diff 26201.Mar 9 2021, 8:40 AM

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

marmoute accepted this revision.Mar 9 2021, 1:26 PM
Alphare requested changes to this revision.Mar 10 2021, 4:23 AM
Alphare added a subscriber: Alphare.
Alphare added inline comments.
rust/hg-core/src/operations/cat.rs
20

Could you add a simple one-line docstring to the fields?

87–98

This seems wasteful because it's always looping over all files, the happy path should be as fast as possible. Should we simply populate a Vec with files that don't match anything?

This revision now requires changes to proceed.Mar 10 2021, 4:23 AM
SimonSapin updated this revision to Diff 26241.Mar 11 2021, 5:05 AM
SimonSapin marked an inline comment as done.Mar 11 2021, 5:12 AM
SimonSapin added inline comments.
rust/hg-core/src/operations/cat.rs
20

Done.

87–98

This is looping over the path CLI arguments, which are typically much fewer than files in the manifest. Populating a Vec with files that don’t match anything is exactly what this loop does.

Note that the "main" loop above is over the manifest, not over CLI arguments. This is because when multiple files match they are concatenated in manifest order rather than in the order requested on CLI. At first I thought this was an rhg bug, but Python-based hg cat also does this so 🤷

Alphare accepted this revision.Mar 11 2021, 5:17 AM
Alphare added inline comments.
rust/hg-core/src/operations/cat.rs
87–98

Right, that makes sense, thanks!

marmoute accepted this revision.Mar 12 2021, 11:54 AM
SimonSapin marked an inline comment as done.Mar 12 2021, 5:07 PM
SimonSapin edited the summary of this revision. (Show Details)
SimonSapin updated this revision to Diff 26300.
baymax updated this revision to Diff 26348.Mar 15 2021, 9:10 AM

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

SimonSapin added a commit: rHGb1f2c2b336ec: rhg: `cat` command: print error messages for missing files.Mar 16 2021, 5:48 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHGb1f2c2b336ec: rhg: `cat` command: print error messages for missing files (authored by SimonSapin). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 26425

rust/hg-core/src/operations/cat.rs

use crate::revlog::manifest::Manifest; use crate::revlog::manifest::Manifest;
use crate::revlog::path_encode::path_encode; use crate::revlog::path_encode::path_encode;
use crate::revlog::revlog::Revlog; use crate::revlog::revlog::Revlog;
use crate::revlog::revlog::RevlogError; use crate::revlog::revlog::RevlogError;
use crate::revlog::Node; use crate::revlog::Node;
use crate::utils::files::get_path_from_bytes; use crate::utils::files::get_path_from_bytes;
use crate::utils::hg_path::{HgPath, HgPathBuf}; use crate::utils::hg_path::{HgPath, HgPathBuf};
pub struct CatOutput {
AlphareUnsubmitted
Done

Could you add a simple one-line docstring to the fields?

Alphare: Could you add a simple one-line docstring to the fields?
SimonSapinAuthorUnsubmitted
Done

Done.

SimonSapin: Done.
/// Whether any file in the manifest matched the paths given as CLI
/// arguments
pub found_any: bool,
/// The contents of matching files, in manifest order
pub concatenated: Vec<u8>,
/// Which of the CLI arguments did not match any manifest file
pub missing: Vec<HgPathBuf>,
/// The node ID that the given revset was resolved to
pub node: Node,
}
const METADATA_DELIMITER: [u8; 2] = [b'\x01', b'\n']; const METADATA_DELIMITER: [u8; 2] = [b'\x01', b'\n'];
/// List files under Mercurial control at a given revision. /// Output the given revision of files
/// ///
/// * `root`: Repository root /// * `root`: Repository root
/// * `rev`: The revision to cat the files from. /// * `rev`: The revision to cat the files from.
/// * `files`: The files to output. /// * `files`: The files to output.
pub fn cat( pub fn cat<'a>(
repo: &Repo, repo: &Repo,
revset: &str, revset: &str,
files: &[HgPathBuf], files: &'a [HgPathBuf],
) -> Result<Vec<u8>, RevlogError> { ) -> Result<CatOutput, RevlogError> {
let rev = crate::revset::resolve_single(revset, repo)?; let rev = crate::revset::resolve_single(revset, repo)?;
let changelog = Changelog::open(repo)?; let changelog = Changelog::open(repo)?;
let manifest = Manifest::open(repo)?; let manifest = Manifest::open(repo)?;
let changelog_entry = changelog.get_rev(rev)?; let changelog_entry = changelog.get_rev(rev)?;
let node = *changelog
.node_from_rev(rev)
.expect("should succeed when changelog.get_rev did");
let manifest_node = let manifest_node =
Node::from_hex_for_repo(&changelog_entry.manifest_node()?)?; Node::from_hex_for_repo(&changelog_entry.manifest_node()?)?;
let manifest_entry = manifest.get_node(manifest_node.into())?; let manifest_entry = manifest.get_node(manifest_node.into())?;
let mut bytes = vec![]; let mut bytes = vec![];
let mut matched = vec![false; files.len()];
let mut found_any = false;
for (manifest_file, node_bytes) in manifest_entry.files_with_nodes() { for (manifest_file, node_bytes) in manifest_entry.files_with_nodes() {
for cat_file in files.iter() { for (cat_file, is_matched) in files.iter().zip(&mut matched) {
if cat_file.as_bytes() == manifest_file.as_bytes() { if cat_file.as_bytes() == manifest_file.as_bytes() {
*is_matched = true;
found_any = true;
let index_path = store_path(manifest_file, b".i"); let index_path = store_path(manifest_file, b".i");
let data_path = store_path(manifest_file, b".d"); let data_path = store_path(manifest_file, b".d");
let file_log = let file_log =
Revlog::open(repo, &index_path, Some(&data_path))?; Revlog::open(repo, &index_path, Some(&data_path))?;
let file_node = Node::from_hex_for_repo(node_bytes)?; let file_node = Node::from_hex_for_repo(node_bytes)?;
let file_rev = file_log.get_node_rev(file_node.into())?; let file_rev = file_log.get_node_rev(file_node.into())?;
let data = file_log.get_rev_data(file_rev)?; let data = file_log.get_rev_data(file_rev)?;
if data.starts_with(&METADATA_DELIMITER) { if data.starts_with(&METADATA_DELIMITER) {
let end_delimiter_position = data let end_delimiter_position = data
[METADATA_DELIMITER.len()..] [METADATA_DELIMITER.len()..]
.windows(METADATA_DELIMITER.len()) .windows(METADATA_DELIMITER.len())
.position(|bytes| bytes == METADATA_DELIMITER); .position(|bytes| bytes == METADATA_DELIMITER);
if let Some(position) = end_delimiter_position { if let Some(position) = end_delimiter_position {
let offset = METADATA_DELIMITER.len() * 2; let offset = METADATA_DELIMITER.len() * 2;
bytes.extend(data[position + offset..].iter()); bytes.extend(data[position + offset..].iter());
} }
} else { } else {
bytes.extend(data); bytes.extend(data);
} }
} }
} }
} }
Ok(bytes) let missing: Vec<_> = files
.iter()
.zip(&matched)
.filter(|pair| !*pair.1)
.map(|pair| pair.0.clone())
.collect();
Ok(CatOutput {
found_any,
concatenated: bytes,
missing,
node,
})
AlphareUnsubmitted
Not Done

This seems wasteful because it's always looping over all files, the happy path should be as fast as possible. Should we simply populate a Vec with files that don't match anything?

Alphare: This seems wasteful because it's always looping over all files, the happy path should be as…
SimonSapinAuthorUnsubmitted
Done

This is looping over the path CLI arguments, which are typically much fewer than files in the manifest. Populating a Vec with files that don’t match anything is exactly what this loop does.

Note that the "main" loop above is over the manifest, not over CLI arguments. This is because when multiple files match they are concatenated in manifest order rather than in the order requested on CLI. At first I thought this was an rhg bug, but Python-based hg cat also does this so 🤷

SimonSapin: This is looping over the path CLI arguments, which are typically much fewer than files in the…
AlphareUnsubmitted
Not Done

Right, that makes sense, thanks!

Alphare: Right, that makes sense, thanks!
} }
fn store_path(hg_path: &HgPath, suffix: &[u8]) -> PathBuf { fn store_path(hg_path: &HgPath, suffix: &[u8]) -> PathBuf {
let encoded_bytes = let encoded_bytes =
path_encode(&[b"data/", hg_path.as_bytes(), suffix].concat()); path_encode(&[b"data/", hg_path.as_bytes(), suffix].concat());
get_path_from_bytes(&encoded_bytes).into() get_path_from_bytes(&encoded_bytes).into()
} }

rust/hg-core/src/operations/mod.rs

//! A distinction is made between operations and commands. //! A distinction is made between operations and commands.
//! An operation is what can be done whereas a command is what is exposed by //! An operation is what can be done whereas a command is what is exposed by
//! the cli. A single command can use several operations to achieve its goal. //! the cli. A single command can use several operations to achieve its goal.
mod cat; mod cat;
mod debugdata; mod debugdata;
mod dirstate_status; mod dirstate_status;
mod list_tracked_files; mod list_tracked_files;
pub use cat::cat; pub use cat::{cat, CatOutput};
pub use debugdata::{debug_data, DebugDataKind}; pub use debugdata::{debug_data, DebugDataKind};
pub use list_tracked_files::Dirstate; pub use list_tracked_files::Dirstate;
pub use list_tracked_files::{list_rev_tracked_files, FilesForRev}; pub use list_tracked_files::{list_rev_tracked_files, FilesForRev};

rust/hg-core/src/revlog/changelog.rs

use crate::errors::HgError; use crate::errors::HgError;
use crate::repo::Repo; use crate::repo::Repo;
use crate::revlog::revlog::{Revlog, RevlogError}; use crate::revlog::revlog::{Revlog, RevlogError};
use crate::revlog::NodePrefix;
use crate::revlog::Revision; use crate::revlog::Revision;
use crate::revlog::{Node, NodePrefix};
/// A specialized `Revlog` to work with `changelog` data format. /// A specialized `Revlog` to work with `changelog` data format.
pub struct Changelog { pub struct Changelog {
/// The generic `revlog` format. /// The generic `revlog` format.
pub(crate) revlog: Revlog, pub(crate) revlog: Revlog,
} }
impl Changelog { impl Changelog {
/// Return the `ChangelogEntry` of a given node revision. /// Return the `ChangelogEntry` of a given node revision.
pub fn get_rev( pub fn get_rev(
&self, &self,
rev: Revision, rev: Revision,
) -> Result<ChangelogEntry, RevlogError> { ) -> Result<ChangelogEntry, RevlogError> {
let bytes = self.revlog.get_rev_data(rev)?; let bytes = self.revlog.get_rev_data(rev)?;
Ok(ChangelogEntry { bytes }) Ok(ChangelogEntry { bytes })
} }
pub fn node_from_rev(&self, rev: Revision) -> Option<&Node> {
Some(self.revlog.index.get_entry(rev)?.hash())
}
} }
/// `Changelog` entry which knows how to interpret the `changelog` data bytes. /// `Changelog` entry which knows how to interpret the `changelog` data bytes.
#[derive(Debug)] #[derive(Debug)]
pub struct ChangelogEntry { pub struct ChangelogEntry {
/// The data bytes of the `changelog` entry. /// The data bytes of the `changelog` entry.
bytes: Vec<u8>, bytes: Vec<u8>,
} }

rust/hg-core/src/revlog/node.rs

/// Used to indicate the absence of node. /// Used to indicate the absence of node.
pub const NULL_NODE_ID: [u8; NODE_BYTES_LENGTH] = [0u8; NODE_BYTES_LENGTH]; pub const NULL_NODE_ID: [u8; NODE_BYTES_LENGTH] = [0u8; NODE_BYTES_LENGTH];
/// The length in bytes of a `Node` /// The length in bytes of a `Node`
/// ///
/// see also `NODES_BYTES_LENGTH` about it being private. /// see also `NODES_BYTES_LENGTH` about it being private.
const NODE_NYBBLES_LENGTH: usize = 2 * NODE_BYTES_LENGTH; const NODE_NYBBLES_LENGTH: usize = 2 * NODE_BYTES_LENGTH;
/// Default for UI presentation
const SHORT_PREFIX_DEFAULT_NYBBLES_LENGTH: u8 = 12;
/// Private alias for readability and to ease future change /// Private alias for readability and to ease future change
type NodeData = [u8; NODE_BYTES_LENGTH]; type NodeData = [u8; NODE_BYTES_LENGTH];
/// Binary revision SHA /// Binary revision SHA
/// ///
/// ## Future changes of hash size /// ## Future changes of hash size
/// ///
/// To accomodate future changes of hash size, Rust callers /// To accomodate future changes of hash size, Rust callers
/// Provide access to binary data /// Provide access to binary data
/// ///
/// This is needed by FFI layers, for instance to return expected /// This is needed by FFI layers, for instance to return expected
/// binary values to Python. /// binary values to Python.
pub fn as_bytes(&self) -> &[u8] { pub fn as_bytes(&self) -> &[u8] {
&self.data &self.data
} }
pub fn short(&self) -> NodePrefix {
NodePrefix {
nybbles_len: SHORT_PREFIX_DEFAULT_NYBBLES_LENGTH,
data: self.data,
}
}
} }
/// The beginning of a binary revision SHA. /// The beginning of a binary revision SHA.
/// ///
/// Since it can potentially come from an hexadecimal representation with /// Since it can potentially come from an hexadecimal representation with
/// odd length, it needs to carry around whether the last 4 bits are relevant /// odd length, it needs to carry around whether the last 4 bits are relevant
/// or not. /// or not.
#[derive(Debug, PartialEq, Copy, Clone)] #[derive(Debug, PartialEq, Copy, Clone)]

rust/hg-core/src/revlog/revlog.rs

} }
} }
/// Read only implementation of revlog. /// Read only implementation of revlog.
pub struct Revlog { pub struct Revlog {
/// When index and data are not interleaved: bytes of the revlog index. /// When index and data are not interleaved: bytes of the revlog index.
/// When index and data are interleaved: bytes of the revlog index and /// When index and data are interleaved: bytes of the revlog index and
/// data. /// data.
index: Index, pub(crate) index: Index,
/// When index and data are not interleaved: bytes of the revlog data /// When index and data are not interleaved: bytes of the revlog data
data_bytes: Option<Box<dyn Deref<Target = [u8]> + Send>>, data_bytes: Option<Box<dyn Deref<Target = [u8]> + Send>>,
/// When present on disk: the persistent nodemap for this revlog /// When present on disk: the persistent nodemap for this revlog
nodemap: Option<nodemap::NodeTree>, nodemap: Option<nodemap::NodeTree>,
} }
impl Revlog { impl Revlog {
/// Open a revlog index file. /// Open a revlog index file.

rust/rhg/src/commands/cat.rs

use crate::error::CommandError; use crate::error::CommandError;
use clap::Arg; use clap::Arg;
use format_bytes::format_bytes;
use hg::operations::cat; use hg::operations::cat;
use hg::utils::hg_path::HgPathBuf; use hg::utils::hg_path::HgPathBuf;
use micro_timer::timed; use micro_timer::timed;
use std::convert::TryFrom; use std::convert::TryFrom;
pub const HELP_TEXT: &str = " pub const HELP_TEXT: &str = "
Output the current or given revision of files Output the current or given revision of files
"; ";
.map_err(|_| CommandError::abort(""))?; .map_err(|_| CommandError::abort(""))?;
let hg_file = HgPathBuf::try_from(stripped.to_path_buf()) let hg_file = HgPathBuf::try_from(stripped.to_path_buf())
.map_err(|e| CommandError::abort(e.to_string()))?; .map_err(|e| CommandError::abort(e.to_string()))?;
files.push(hg_file); files.push(hg_file);
} }
match rev { match rev {
Some(rev) => { Some(rev) => {
let data = cat(&repo, rev, &files).map_err(|e| (e, rev))?; let output = cat(&repo, rev, &files).map_err(|e| (e, rev))?;
invocation.ui.write_stdout(&data)?; invocation.ui.write_stdout(&output.concatenated)?;
if !output.missing.is_empty() {
let short = format!("{:x}", output.node.short()).into_bytes();
for path in &output.missing {
invocation.ui.write_stderr(&format_bytes!(
b"{}: no such file in rev {}\n",
path.as_bytes(),
short
))?;
}
}
if output.found_any {
Ok(()) Ok(())
} else {
Err(CommandError::Unsuccessful)
}
} }
None => Err(CommandError::unsupported( None => Err(CommandError::unsupported(
"`rhg cat` without `--rev` / `-r`", "`rhg cat` without `--rev` / `-r`",
)), )),
} }
} }

rust/rhg/src/error.rs

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 { message: Vec<u8> }, Abort { message: Vec<u8> },
/// Exit with a failure exit code but no message.
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 {

rust/rhg/src/exitcode.rs

pub type ExitCode = i32; pub type ExitCode = i32;
/// Successful exit /// Successful exit
pub const OK: ExitCode = 0; pub const OK: ExitCode = 0;
/// Generic abort /// Generic abort
pub const ABORT: ExitCode = 255; pub const ABORT: ExitCode = 255;
/// Generic something completed but did not succeed
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;

rust/rhg/src/main.rs

result, 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,
// 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 exitcode::UNIMPLEMENTED
} }
} }
} }
)); ));
on_unsupported = OnUnsupported::Abort on_unsupported = OnUnsupported::Abort
} }
} }
} }
} }
match &result { match &result {
Ok(_) => {} Ok(_) => {}
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));
} }
} }
Err(CommandError::UnsupportedFeature { message }) => { Err(CommandError::UnsupportedFeature { message }) => {