( )⚙ D11679 rhg: internally, return a structured representation from hg cat

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D11679

rhg: internally, return a structured representation from hg cat
ClosedPublic

Authored by aalekseyev on Oct 15 2021, 9:21 AM.

Details

Reviewers
Alphare
Group Reviewers
hg-reviewers
Commits
rHG027ebad952ac: rhg: internally, return a structured representation from hg cat
Summary

The purpose of this change is to make it possible to support limited templating in hg cat, so we could print separators between files etc.
The templating itself is not implemented yet, so this functionality is unused in rhg cat.
However, in our fork of hg we're implementing a slightly different command hg jscat which makes use of this.
So accepting this change will let us minimize the size of the patch we're maintaining on our side.

Diff Detail

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

Event Timeline

aalekseyev created this revision.Oct 15 2021, 9:21 AM
aalekseyev retitled this revision from rhg: simplify the type of FilelogEntry to rhg: internally, return a structured representatioon from hg cat.Oct 15 2021, 9:27 AM
aalekseyev edited the summary of this revision. (Show Details)
aalekseyev retitled this revision from rhg: internally, return a structured representatioon from hg cat to rhg: simplify the type of FilelogEntry.Oct 15 2021, 9:30 AM
aalekseyev retitled this revision from rhg: simplify the type of FilelogEntry to rhg: add FilelogEntry.into_data.
aalekseyev retitled this revision from rhg: add FilelogEntry.into_data to rhg: simplify the type of FilelogEntry.
aalekseyev edited the summary of this revision. (Show Details)
aalekseyev updated this revision to Diff 30835.
aalekseyev updated this revision to Diff 30837.
aalekseyev retitled this revision from rhg: simplify the type of FilelogEntry to rhg: internally, return a structured representatioon from hg cat.Oct 15 2021, 9:33 AM
aalekseyev edited the summary of this revision. (Show Details)
Alphare added a subscriber: Alphare.Oct 18 2021, 5:50 AM

The change looks like a good idea, as you said it'll be useful for templating later anyway.

However I wonder if the fragmented allocations have a measurable impact on performance for a case with a large output (in both number of files and number of lines). Do you have a way of testing this easily?

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

Could you possibly send a follow-up that uses more explicit names for the lifetime variables (and possibly M for manifest and F for files)?

aalekseyev added a comment.Oct 18 2021, 6:07 AM
In D11679#179111, @Alphare wrote:

However I wonder if the fragmented allocations have a measurable impact on performance for a case with a large output (in both number of files and number of lines). Do you have a way of testing this easily?

I've ran the same benchmarks I used to measure the impact of https://phab.mercurial-scm.org/D11616 and saw no noticeable difference. (the difference was on the order of 1ms, and not always in the same direction)

By the way, we only introduce extra fragmented allocations in the case when files have metadata.
If the file has no metadata, then we're just keeping around buffers created when reading the file, so I expect we're actually making one fewer copy of the data.
I don't know (and didn't measure) which is the common case.

Alphare accepted this revision.Oct 18 2021, 6:08 AM

Sounds good to me!

This revision is now accepted and ready to land.Oct 18 2021, 6:08 AM
Alphare requested changes to this revision.Oct 18 2021, 6:09 AM

This does not apply on top of default sorry, please send a rebase.

This revision now requires changes to proceed.Oct 18 2021, 6:09 AM
aalekseyev added inline comments.Oct 18 2021, 6:10 AM
rust/hg-core/src/operations/cat.rs
59

I can't think of very meaningful names for the lifetime variables. Is there a convention I should follow? (simply 'm and 'f, perhaps?)
(possibly the name 'a can simply be omitted here, actually)

Alphare added inline comments.Oct 18 2021, 6:12 AM
rust/hg-core/src/operations/cat.rs
59

How about 'manifest and 'files?

One-letter variables are fine for trivial cases, but for more complex cases they should be more explicit. We tend to be more lazy with lifetime naming that regular variables for some reason, myself included.

Alphare added a comment.Oct 18 2021, 6:15 AM

Oh also, (sorry I forgot to say anything) the commit message has a typo in "representation".

aalekseyev retitled this revision from rhg: internally, return a structured representatioon from hg cat to rhg: simplify the type of FilelogEntry.Oct 18 2021, 6:19 AM
aalekseyev edited the summary of this revision. (Show Details)
aalekseyev updated this revision to Diff 30860.
aalekseyev edited the summary of this revision. (Show Details)Oct 18 2021, 6:24 AM
aalekseyev updated this revision to Diff 30866.
aalekseyev updated this revision to Diff 30867.Oct 18 2021, 6:28 AM
aalekseyev added a comment.Oct 18 2021, 6:28 AM
In D11679#179143, @Alphare wrote:

Oh also, (sorry I forgot to say anything) the commit message has a typo in "representation".

Thanks, amended.

aalekseyev added inline comments.Oct 18 2021, 6:30 AM
rust/hg-core/src/operations/cat.rs
59

I renamed all one-letter variables here. What do you think? (apparently the 'a aka 'm' aka 'manifest` can't be omitted)
I also renamed "files" into "query" because I think that makes its role clearer.

aalekseyev added a comment.Oct 18 2021, 6:31 AM
In D11679#179138, @Alphare wrote:

This does not apply on top of default sorry, please send a rebase.

Rebased.

aalekseyev retitled this revision from rhg: simplify the type of FilelogEntry to rhg: internally, return a structured representation from hg cat.Oct 18 2021, 6:35 AM
aalekseyev edited the summary of this revision. (Show Details)
Alphare accepted this revision.Oct 18 2021, 7:40 AM
Alphare added inline comments.
rust/hg-core/src/operations/cat.rs
59

Yep, that is because there are two inputs and two outputs, so no way of automatically knowing how the lifetimes are supposed to be bound.

That looks good, query is better, thanks.

This revision is now accepted and ready to land.Oct 18 2021, 7:40 AM
spectral added a subscriber: spectral.Oct 18 2021, 7:06 PM
spectral added inline comments.
rust/hg-core/src/operations/cat.rs
22

Does this still need to specify/require manifest ordering if we have the path as well?

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

I'm not very familiar with Rust, can you explain why this separate function was necessary/useful? Wouldn't it have been possible to only add into_data without modifying split like this:

/// Consume the entry, and convert it into data, discarding any metadata,
/// if present.
pub fn into_data(self) -> Result<Vec<u8>, HgError> {
    let (metadata, data) = self.split()?;
    Ok(match metadata {
        Some(_) => data.to_owned(),
        None => self.0
    })
}
Alphare added a comment.Oct 19 2021, 3:52 AM

@aalekseyev could you follow-up on the new comments?

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

I guess not, there shouldn't be any code making an assumption on ordering.

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

I was expecting @aalekseyev to have a plan with this private function in following patches, but I agree with you that this is more code than it needs to be.

aalekseyev added inline comments.Oct 19 2021, 5:38 AM
rust/hg-core/src/operations/cat.rs
22

The order matters in so far as it shows up in the output of the hg cat. No other reason. (as there wasn't before this change)

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

@spectral, thanks, that's simpler indeed. I just didn't think of that.
It did not occur to me to add a post-condition to split.
I'll make a follow-up change.

Alphare added inline comments.Oct 19 2021, 5:39 AM
rust/hg-core/src/operations/cat.rs
22

Let's keep the comment since it gives more information.

Revision Contents
Changeset List

Diff 30833

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

// list_tracked_files.rs // list_tracked_files.rs
// //
// Copyright 2020 Antoine Cezar <antoine.cezar@octobus.net> // Copyright 2020 Antoine Cezar <antoine.cezar@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::repo::Repo; use crate::repo::Repo;
use crate::revlog::revlog::RevlogError; use crate::revlog::revlog::RevlogError;
use crate::revlog::Node; use crate::revlog::Node;
use crate::utils::hg_path::HgPath; use crate::utils::hg_path::HgPath;
use crate::utils::hg_path::HgPathBuf;
use itertools::put_back; use itertools::put_back;
use itertools::PutBack; use itertools::PutBack;
use std::cmp::Ordering; use std::cmp::Ordering;
pub struct CatOutput { pub struct CatOutput<'a> {
/// Whether any file in the manifest matched the paths given as CLI /// Whether any file in the manifest matched the paths given as CLI
/// arguments /// arguments
pub found_any: bool, pub found_any: bool,
/// The contents of matching files, in manifest order /// The contents of matching files, in manifest order
spectralUnsubmitted
Not Done

Does this still need to specify/require manifest ordering if we have the path as well?

spectral: Does this still need to specify/require manifest ordering if we have the path as well?
AlphareUnsubmitted
Not Done

I guess not, there shouldn't be any code making an assumption on ordering.

Alphare: I guess not, there shouldn't be any code making an assumption on ordering.
aalekseyevAuthorUnsubmitted
Done

The order matters in so far as it shows up in the output of the hg cat. No other reason. (as there wasn't before this change)

aalekseyev: The order matters in so far as it shows up in the output of the `hg cat`. No other reason. (as…
AlphareUnsubmitted
Not Done

Let's keep the comment since it gives more information.

Alphare: Let's keep the comment since it gives more information.
pub concatenated: Vec<u8>, pub results: Vec<(&'a HgPath, Vec<u8>)>,
/// Which of the CLI arguments did not match any manifest file /// Which of the CLI arguments did not match any manifest file
pub missing: Vec<HgPathBuf>, pub missing: Vec<&'a HgPath>,
/// The node ID that the given revset was resolved to /// The node ID that the given revset was resolved to
pub node: Node, pub node: Node,
} }
// Find an item in an iterator over a sorted collection. // Find an item in an iterator over a sorted collection.
fn find_item<'a, 'b, 'c, D, I: Iterator<Item = (&'a HgPath, D)>>( fn find_item<'a, 'b, 'c, D, I: Iterator<Item = (&'a HgPath, D)>>(
i: &mut PutBack<I>, i: &mut PutBack<I>,
needle: &'b HgPath, needle: &'b HgPath,
) -> Option<I::Item> { ) -> Option<D> {
loop { loop {
match i.next() { match i.next() {
None => return None, None => return None,
Some(val) => match needle.as_bytes().cmp(val.0.as_bytes()) { Some(val) => match needle.as_bytes().cmp(val.0.as_bytes()) {
Ordering::Less => { Ordering::Less => {
i.put_back(val); i.put_back(val);
return None; return None;
} }
Ordering::Greater => continue, Ordering::Greater => continue,
Ordering::Equal => return Some(val), Ordering::Equal => return Some(val.1),
}, },
} }
} }
} }
fn find_files_in_manifest< fn find_files_in_manifest<
'a, 'a,
'b, 'b,
D, D,
I: Iterator<Item = (&'a HgPath, D)>, I: Iterator<Item = (&'a HgPath, D)>,
J: Iterator<Item = &'b HgPath>, J: Iterator<Item = &'b HgPath>,
>( >(
manifest: I, manifest: I,
files: J, files: J,
) -> (Vec<(&'a HgPath, D)>, Vec<&'b HgPath>) { ) -> (Vec<(&'b HgPath, D)>, Vec<&'b HgPath>) {
AlphareUnsubmitted
Not Done

Could you possibly send a follow-up that uses more explicit names for the lifetime variables (and possibly M for manifest and F for files)?

Alphare: Could you possibly send a follow-up that uses more explicit names for the lifetime variables…
aalekseyevAuthorUnsubmitted
Done

I can't think of very meaningful names for the lifetime variables. Is there a convention I should follow? (simply 'm and 'f, perhaps?)
(possibly the name 'a can simply be omitted here, actually)

aalekseyev: I can't think of very meaningful names for the lifetime variables. Is there a convention I…
AlphareUnsubmitted
Not Done

How about 'manifest and 'files?

One-letter variables are fine for trivial cases, but for more complex cases they should be more explicit. We tend to be more lazy with lifetime naming that regular variables for some reason, myself included.

Alphare: How about `'manifest` and `'files`? One-letter variables are fine for trivial cases, but for…
aalekseyevAuthorUnsubmitted
Done

I renamed all one-letter variables here. What do you think? (apparently the 'a aka 'm' aka 'manifest` can't be omitted)
I also renamed "files" into "query" because I think that makes its role clearer.

aalekseyev: I renamed all one-letter variables here. What do you think? (apparently the `'a` aka `'m' aka…
AlphareUnsubmitted
Not Done

Yep, that is because there are two inputs and two outputs, so no way of automatically knowing how the lifetimes are supposed to be bound.

That looks good, query is better, thanks.

Alphare: Yep, that is because there are two inputs and two outputs, so no way of automatically knowing…
let mut manifest = put_back(manifest); let mut manifest = put_back(manifest);
let mut res = vec![]; let mut res = vec![];
let mut missing = vec![]; let mut missing = vec![];
for file in files { for file in files {
match find_item(&mut manifest, file) { match find_item(&mut manifest, file) {
None => missing.push(file), None => missing.push(file),
Some(item) => res.push(item), Some(item) => res.push((file, item)),
} }
} }
return (res, missing); return (res, missing);
} }
/// Output the given revision of files /// 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<'a>( pub fn cat<'a>(
repo: &Repo, repo: &Repo,
revset: &str, revset: &str,
mut files: Vec<HgPathBuf>, mut files: Vec<&'a HgPath>,
) -> Result<CatOutput, RevlogError> { ) -> Result<CatOutput<'a>, RevlogError> {
let rev = crate::revset::resolve_single(revset, repo)?; let rev = crate::revset::resolve_single(revset, repo)?;
let manifest = repo.manifest_for_rev(rev)?; let manifest = repo.manifest_for_rev(rev)?;
let node = *repo let node = *repo
.changelog()? .changelog()?
.node_from_rev(rev) .node_from_rev(rev)
.expect("should succeed when repo.manifest did"); .expect("should succeed when repo.manifest did");
let mut bytes: Vec<u8> = vec![]; let mut results: Vec<(&'a HgPath, Vec<u8>)> = vec![];
let mut found_any = false; let mut found_any = false;
files.sort_unstable(); files.sort_unstable();
let (found, missing) = find_files_in_manifest( let (found, missing) = find_files_in_manifest(
manifest.files_with_nodes(), manifest.files_with_nodes(),
files.iter().map(|f| f.as_ref()), files.into_iter().map(|f| f.as_ref()),
); );
for (manifest_file, node_bytes) in found { for (file_path, node_bytes) in found {
found_any = true; found_any = true;
let file_log = repo.filelog(manifest_file)?; let file_log = repo.filelog(file_path)?;
let file_node = Node::from_hex_for_repo(node_bytes)?; let file_node = Node::from_hex_for_repo(node_bytes)?;
bytes.extend(file_log.data_for_node(file_node)?.data()?); results.push((
file_path,
file_log.data_for_node(file_node)?.into_data()?,
));
} }
let missing: Vec<HgPathBuf> = missing
.iter()
.map(|file| (*file).to_owned())
.collect();
Ok(CatOutput { Ok(CatOutput {
found_any, found_any,
concatenated: bytes, results,
missing, missing,
node, node,
}) })
} }

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

use crate::errors::HgError; use crate::errors::HgError;
use crate::repo::Repo; use crate::repo::Repo;
use crate::revlog::path_encode::path_encode; use crate::revlog::path_encode::path_encode;
use crate::revlog::revlog::{Revlog, RevlogError}; use crate::revlog::revlog::{Revlog, RevlogError};
use crate::revlog::NodePrefix; use crate::revlog::NodePrefix;
use crate::revlog::Revision; use crate::revlog::Revision;
use crate::utils::files::get_path_from_bytes; use crate::utils::files::get_path_from_bytes;
use crate::utils::hg_path::HgPath; use crate::utils::hg_path::HgPath;
use crate::utils::SliceExt; use crate::utils::SliceExt;
use std::borrow::Cow;
use std::path::PathBuf; use std::path::PathBuf;
/// A specialized `Revlog` to work with file data logs. /// A specialized `Revlog` to work with file data logs.
pub struct Filelog { pub struct Filelog {
/// The generic `revlog` format. /// The generic `revlog` format.
revlog: Revlog, revlog: Revlog,
} }
} }
/// The given revision is that of the file as found in a manifest, not of a /// The given revision is that of the file as found in a manifest, not of a
/// changeset. /// changeset.
pub fn data_for_rev( pub fn data_for_rev(
&self, &self,
file_rev: Revision, file_rev: Revision,
) -> Result<FilelogEntry, RevlogError> { ) -> Result<FilelogEntry, RevlogError> {
let data = self.revlog.get_rev_data(file_rev)?; let data: Vec<u8> = self.revlog.get_rev_data(file_rev)?;
Ok(FilelogEntry(data.into())) Ok(FilelogEntry(data.into()))
} }
} }
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()
} }
pub struct FilelogEntry<'filelog>(Cow<'filelog, [u8]>); pub struct FilelogEntry(Vec<u8>);
impl<'filelog> FilelogEntry<'filelog> { impl FilelogEntry {
/// Split into metadata and data /// Split into metadata and data
pub fn split(&self) -> Result<(Option<&[u8]>, &[u8]), HgError> { /// Returns None if there is no metadata, so the entire entry is data.
fn split_metadata(&self) -> Result<Option<(&[u8], &[u8])>, HgError> {
spectralUnsubmitted
Not Done

I'm not very familiar with Rust, can you explain why this separate function was necessary/useful? Wouldn't it have been possible to only add into_data without modifying split like this:

/// Consume the entry, and convert it into data, discarding any metadata,
/// if present.
pub fn into_data(self) -> Result<Vec<u8>, HgError> {
    let (metadata, data) = self.split()?;
    Ok(match metadata {
        Some(_) => data.to_owned(),
        None => self.0
    })
}
spectral: I'm not very familiar with Rust, can you explain why this separate function was…
AlphareUnsubmitted
Not Done

I was expecting @aalekseyev to have a plan with this private function in following patches, but I agree with you that this is more code than it needs to be.

Alphare: I was expecting @aalekseyev to have a plan with this private function in following patches, but…
aalekseyevAuthorUnsubmitted
Done

@spectral, thanks, that's simpler indeed. I just didn't think of that.
It did not occur to me to add a post-condition to split.
I'll make a follow-up change.

aalekseyev: @spectral, thanks, that's simpler indeed. I just didn't think of that. It did not occur to me…
const DELIMITER: &[u8; 2] = &[b'\x01', b'\n']; const DELIMITER: &[u8; 2] = &[b'\x01', b'\n'];
if let Some(rest) = self.0.drop_prefix(DELIMITER) { if let Some(rest) = self.0.drop_prefix(DELIMITER) {
if let Some((metadata, data)) = rest.split_2_by_slice(DELIMITER) { if let Some((metadata, data)) = rest.split_2_by_slice(DELIMITER) {
Ok((Some(metadata), data)) Ok(Some((metadata, data)))
} else { } else {
Err(HgError::corrupted( Err(HgError::corrupted(
"Missing metadata end delimiter in filelog entry", "Missing metadata end delimiter in filelog entry",
)) ))
} }
} else { } else {
Ok(None)
}
}
/// Split into metadata and data
pub fn split(&self) -> Result<(Option<&[u8]>, &[u8]), HgError> {
if let Some((metadata, data)) = self.split_metadata()? {
Ok((Some(metadata), data))
} else {
Ok((None, &self.0)) Ok((None, &self.0))
} }
} }
/// Returns the file contents at this revision, stripped of any metadata /// Returns the file contents at this revision, stripped of any metadata
pub fn data(&self) -> Result<&[u8], HgError> { pub fn data(&self) -> Result<&[u8], HgError> {
let (_metadata, data) = self.split()?; let (_metadata, data) = self.split()?;
Ok(data) Ok(data)
} }
pub fn into_data(self) -> Result<Vec<u8>, HgError> {
if let Some((_metadata, data)) = self.split_metadata()? {
Ok(data.to_owned())
} else {
Ok(self.0)
}
}
} }

rust/rhg/src/commands/cat.rs

let stripped = normalized let stripped = normalized
.strip_prefix(&working_directory) .strip_prefix(&working_directory)
// TODO: error message for path arguments outside of the repo // TODO: error message for path arguments outside of the repo
.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);
} }
let files = files.iter().map(|file| file.as_ref()).collect();
// TODO probably move this to a util function like `repo.default_rev` or // TODO probably move this to a util function like `repo.default_rev` or
// something when it's used somewhere else // something when it's used somewhere else
let rev = match rev { let rev = match rev {
Some(r) => r.to_string(), Some(r) => r.to_string(),
None => format!("{:x}", repo.dirstate_parents()?.p1), None => format!("{:x}", repo.dirstate_parents()?.p1),
}; };
let output = cat(&repo, &rev, files).map_err(|e| (e, rev.as_str()))?; let output = cat(&repo, &rev, files).map_err(|e| (e, rev.as_str()))?;
invocation.ui.write_stdout(&output.concatenated)?; for (_file, contents) in output.results {
invocation.ui.write_stdout(&contents)?;
}
if !output.missing.is_empty() { if !output.missing.is_empty() {
let short = format!("{:x}", output.node.short()).into_bytes(); let short = format!("{:x}", output.node.short()).into_bytes();
for path in &output.missing { for path in &output.missing {
invocation.ui.write_stderr(&format_bytes!( invocation.ui.write_stderr(&format_bytes!(
b"{}: no such file in rev {}\n", b"{}: no such file in rev {}\n",
path.as_bytes(), path.as_bytes(),
short short
))?; ))?;
} }
} }
if output.found_any { if output.found_any {
Ok(()) Ok(())
} else { } else {
Err(CommandError::Unsuccessful) Err(CommandError::Unsuccessful)
} }
} }