This is an archive of the discontinued Mercurial Phabricator instance.

Differential D12439

rust-changelog: start parsing changeset data
ClosedPublic

Authored by martinvonz on Apr 5 2022, 2:11 PM.

Details

Reviewers
Alphare
Group Reviewers
hg-reviewers
Commits
rHG95da3e99cbd8: rust-changelog: start parsing changeset data
Summary

This patch makes ChangelogRevisionData do some coarse, line-level
splitting of the changeset data into manifest node, user, timestamp,
files list, and description. There are no (in-tree) users of these
functions yet, but I've added tests to prevent regressions. We'll
surely add callers at some point.

Diff Detail

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

Event Timeline

martinvonz created this revision.Apr 5 2022, 2:11 PM
Alphare requested changes to this revision.Apr 6 2022, 8:39 AM
Alphare added a subscriber: Alphare.
Alphare added inline comments.
rust/hg-core/src/revlog/changelog.rs
61

I see you've already proposed an implementation for your question in the earlier patch. :)

I am not a fan of using ranges in the struct. They are not Copy, which requires cloning all over the place. As suggested in the previous patch, we should probably use an array of offsets and use them in the different methods for each field.

73–112

This should return a Result<Self, HgError> instead of an Option.

75

Please return an HgError::corrupted(...) instead of using unwrap()

186

Please use String::from_utf8_lossy instead of panicking in the presence of non-UTF8 bytes

This revision now requires changes to proceed.Apr 6 2022, 8:39 AM
martinvonz added inline comments.Apr 6 2022, 11:49 AM
rust/hg-core/src/revlog/changelog.rs
61

Sure, I'll switch to offsets.

75

As mentioned in an earlier patch, I don't think the iterator can be empty.

186

Sure, will do (FYI, that would indicate a bug in escape_default() or in my understanding of what it does anyway).

martinvonz marked 3 inline comments as done.Apr 7 2022, 12:24 PM
martinvonz retitled this revision from [RFC] rhg: start parsing changeset data to rhg: start parsing changeset data.
martinvonz edited the summary of this revision. (Show Details)
martinvonz updated this revision to Diff 32867.
martinvonz added inline comments.Apr 7 2022, 12:25 PM
rust/hg-core/src/revlog/changelog.rs
61

That's now done. It turned out to be simpler overall, so that's nice :)

73–112

Hmm, I thought I I had already asked this but my question got lost somehow (maybe I clicked Cancel by mistake?). Anyway, what I wondering was why you prefer that. Is it that you want a specialized error message for each case (rather than the generic "Invalid changelog data for revision" on line 44)?

martinvonz updated this revision to Diff 32988.Apr 8 2022, 6:10 PM
martinvonz retitled this revision from rhg: start parsing changeset data to rust-changelog: start parsing changeset data.Apr 9 2022, 1:35 AM
Alphare added inline comments.Apr 11 2022, 6:16 AM
rust/hg-core/src/revlog/changelog.rs
73–112

Having a specific error message could be a nice bonus, but the main thing is about the semantics of Option vs Result. The former describes the presence or absence of a value, while the latter gives the information about the result of a fallible operation. I know that this is not always true (even the documentation says that you may use an Option for a simple case of error), but I find the distinction clearer.

martinvonz marked 2 inline comments as done.Apr 11 2022, 12:07 PM
Alphare requested changes to this revision.Apr 12 2022, 4:19 AM
Alphare added inline comments.
rust/hg-core/src/revlog/changelog.rs
60–69

Shouldn't this be &'changelog [u8]? We will want to avoid copies most of the time since the changelog will be somewhere in memory (most likely mmapped) and immutable. Much like RevlogEntry.

89

This comment is now outdated. Sorry for the churn. :)

143

Agreed, this should probably become a struct down the line, but this is out of scope for now.

This revision now requires changes to proceed.Apr 12 2022, 4:19 AM
martinvonz added inline comments.Apr 12 2022, 8:39 AM
rust/hg-core/src/revlog/changelog.rs
60–69

Yes, that would make sense, but I didn't change it in this series. I can add a patch to fix it.

Alphare added inline comments.Apr 12 2022, 10:33 AM
rust/hg-core/src/revlog/changelog.rs
60–69

Ah indeed, sorry. If you're up for it, that'd certainly be nice.

martinvonz marked an inline comment as done.Apr 12 2022, 2:33 PM
martinvonz updated this revision to Diff 33137.
Alphare accepted this revision.Apr 13 2022, 4:25 AM
This revision is now accepted and ready to land.Apr 13 2022, 4:25 AM
martinvonz added a commit: rHG95da3e99cbd8: rust-changelog: start parsing changeset data.Apr 13 2022, 5:18 AM
Closed by commit rHG95da3e99cbd8: rust-changelog: start parsing changeset data (authored by martinvonz). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mrust/hg-core/src/revlog/changelog.rs (189 lines)

Diff 33137

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::Revision; use crate::revlog::Revision;
use crate::revlog::{Node, NodePrefix}; use crate::revlog::{Node, NodePrefix};
use crate::utils::hg_path::HgPath;
use itertools::Itertools;
use std::ascii::escape_default;
use std::fmt::{Debug, Formatter};
/// 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 {
pub fn data_for_rev( pub fn data_for_rev(
&self, &self,
rev: Revision, rev: Revision,
) -> Result<ChangelogRevisionData, RevlogError> { ) -> Result<ChangelogRevisionData, RevlogError> {
let bytes = self.revlog.get_rev_data(rev)?.into_owned(); let bytes = self.revlog.get_rev_data(rev)?.into_owned();
if bytes.is_empty() { if bytes.is_empty() {
Ok(ChangelogRevisionData::null()) Ok(ChangelogRevisionData::null())
} else { } else {
Ok(ChangelogRevisionData::new(bytes)) Ok(ChangelogRevisionData::new(bytes).map_err(|err| {
RevlogError::Other(HgError::CorruptedRepository(format!(
"Invalid changelog data for revision {}: {:?}",
rev, err
)))
})?)
} }
} }
pub fn node_from_rev(&self, rev: Revision) -> Option<&Node> { pub fn node_from_rev(&self, rev: Revision) -> Option<&Node> {
self.revlog.node_from_rev(rev) self.revlog.node_from_rev(rev)
} }
} }
/// `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(PartialEq)]
pub struct ChangelogRevisionData { pub struct ChangelogRevisionData {
/// The data bytes of the `changelog` entry. /// The data bytes of the `changelog` entry.
bytes: Vec<u8>, bytes: Vec<u8>,
/// The end offset for the hex manifest (not including the newline)
AlphareUnsubmitted
Done

I see you've already proposed an implementation for your question in the earlier patch. :)

I am not a fan of using ranges in the struct. They are not Copy, which requires cloning all over the place. As suggested in the previous patch, we should probably use an array of offsets and use them in the different methods for each field.

Alphare: I see you've already proposed an implementation for your question in the earlier patch. :) I…
martinvonzAuthorUnsubmitted
Done

Sure, I'll switch to offsets.

martinvonz: Sure, I'll switch to offsets.
martinvonzAuthorUnsubmitted
Done

That's now done. It turned out to be simpler overall, so that's nice :)

martinvonz: That's now done. It turned out to be simpler overall, so that's nice :)
manifest_end: usize,
/// The end offset for the user+email (not including the newline)
user_end: usize,
/// The end offset for the timestamp+timezone+extras (not including the
/// newline)
timestamp_end: usize,
/// The end offset for the file list (not including the newline)
files_end: usize,
AlphareUnsubmitted
Not Done

Shouldn't this be &'changelog [u8]? We will want to avoid copies most of the time since the changelog will be somewhere in memory (most likely mmapped) and immutable. Much like RevlogEntry.

Alphare: Shouldn't this be `&'changelog [u8]`? We will want to avoid copies most of the time since the…
martinvonzAuthorUnsubmitted
Done

Yes, that would make sense, but I didn't change it in this series. I can add a patch to fix it.

martinvonz: Yes, that would make sense, but I didn't change it in this series. I can add a patch to fix it.
AlphareUnsubmitted
Not Done

Ah indeed, sorry. If you're up for it, that'd certainly be nice.

Alphare: Ah indeed, sorry. If you're up for it, that'd certainly be nice.
} }
impl ChangelogRevisionData { impl ChangelogRevisionData {
fn new(bytes: Vec<u8>) -> Self { fn new(bytes: Vec<u8>) -> Result<Self, HgError> {
Self { bytes } let mut line_iter = bytes.split(|b| b == &b'\n');
let manifest_end = line_iter
AlphareUnsubmitted
Done

Please return an HgError::corrupted(...) instead of using unwrap()

Alphare: Please return an `HgError::corrupted(...)` instead of using `unwrap()`
martinvonzAuthorUnsubmitted
Done

As mentioned in an earlier patch, I don't think the iterator can be empty.

martinvonz: As mentioned in an earlier patch, I don't think the iterator can be empty.
.next()
.expect("Empty iterator from split()?")
.len();
let user_slice = line_iter.next().ok_or_else(|| {
HgError::corrupted("Changeset data truncated after manifest line")
})?;
let user_end = manifest_end + 1 + user_slice.len();
let timestamp_slice = line_iter.next().ok_or_else(|| {
HgError::corrupted("Changeset data truncated after user line")
})?;
let timestamp_end = user_end + 1 + timestamp_slice.len();
let mut files_end = timestamp_end + 1;
loop {
let line = line_iter.next().ok_or_else(|| {
AlphareUnsubmitted
Done

This comment is now outdated. Sorry for the churn. :)

Alphare: This comment is now outdated. Sorry for the churn. :)
HgError::corrupted("Changeset data truncated in files list")
})?;
if line.is_empty() {
if files_end == bytes.len() {
// The list of files ended with a single newline (there
// should be two)
return Err(HgError::corrupted(
"Changeset data truncated after files list",
));
}
files_end -= 1;
break;
}
files_end += line.len() + 1;
}
Ok(Self {
bytes,
manifest_end,
user_end,
timestamp_end,
files_end,
})
AlphareUnsubmitted
Done

This should return a Result<Self, HgError> instead of an Option.

Alphare: This should return a `Result<Self, HgError>` instead of an `Option`.
martinvonzAuthorUnsubmitted
Done

Hmm, I thought I I had already asked this but my question got lost somehow (maybe I clicked Cancel by mistake?). Anyway, what I wondering was why you prefer that. Is it that you want a specialized error message for each case (rather than the generic "Invalid changelog data for revision" on line 44)?

martinvonz: Hmm, I thought I I had already asked this but my question got lost somehow (maybe I clicked…
AlphareUnsubmitted
Done

Having a specific error message could be a nice bonus, but the main thing is about the semantics of Option vs Result. The former describes the presence or absence of a value, while the latter gives the information about the result of a fallible operation. I know that this is not always true (even the documentation says that you may use an Option for a simple case of error), but I find the distinction clearer.

Alphare: Having a specific error message could be a nice bonus, but the main thing is about the…
} }
fn null() -> Self { fn null() -> Self {
Self::new( Self::new(
b"0000000000000000000000000000000000000000\n\n0 0\n\n".to_vec(), b"0000000000000000000000000000000000000000\n\n0 0\n\n".to_vec(),
) )
.unwrap()
} }
/// Return an iterator over the lines of the entry. /// Return an iterator over the lines of the entry.
pub fn lines(&self) -> impl Iterator<Item = &[u8]> { pub fn lines(&self) -> impl Iterator<Item = &[u8]> {
self.bytes.split(|b| b == &b'\n') self.bytes.split(|b| b == &b'\n')
} }
/// Return the node id of the `manifest` referenced by this `changelog` /// Return the node id of the `manifest` referenced by this `changelog`
/// entry. /// entry.
pub fn manifest_node(&self) -> Result<Node, HgError> { pub fn manifest_node(&self) -> Result<Node, HgError> {
let manifest_node_hex = let manifest_node_hex = &self.bytes[..self.manifest_end];
self.lines().next().expect("Empty iterator from split()?");
Node::from_hex_for_repo(manifest_node_hex) Node::from_hex_for_repo(manifest_node_hex)
} }
/// The full user string (usually a name followed by an email enclosed in
/// angle brackets)
pub fn user(&self) -> &[u8] {
&self.bytes[self.manifest_end + 1..self.user_end]
}
/// The full timestamp line (timestamp in seconds, offset in seconds, and
/// possibly extras)
// TODO: We should expose this in a more useful way
pub fn timestamp_line(&self) -> &[u8] {
AlphareUnsubmitted
Not Done

Agreed, this should probably become a struct down the line, but this is out of scope for now.

Alphare: Agreed, this should probably become a struct down the line, but this is out of scope for now.
&self.bytes[self.user_end + 1..self.timestamp_end]
}
/// The files changed in this revision.
pub fn files(&self) -> impl Iterator<Item = &HgPath> {
self.bytes[self.timestamp_end + 1..self.files_end]
.split(|b| b == &b'\n')
.map(|path| HgPath::new(path))
}
/// The change description.
pub fn description(&self) -> &[u8] {
&self.bytes[self.files_end + 2..]
}
}
impl Debug for ChangelogRevisionData {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
f.debug_struct("ChangelogRevisionData")
.field("bytes", &debug_bytes(&self.bytes))
.field("manifest", &debug_bytes(&self.bytes[..self.manifest_end]))
.field(
"user",
&debug_bytes(
&self.bytes[self.manifest_end + 1..self.user_end],
),
)
.field(
"timestamp",
&debug_bytes(
&self.bytes[self.user_end + 1..self.timestamp_end],
),
)
.field(
"files",
&debug_bytes(
&self.bytes[self.timestamp_end + 1..self.files_end],
),
)
.field(
"description",
&debug_bytes(&self.bytes[self.files_end + 2..]),
)
AlphareUnsubmitted
Done

Please use String::from_utf8_lossy instead of panicking in the presence of non-UTF8 bytes

Alphare: Please use `String::from_utf8_lossy` instead of panicking in the presence of non-UTF8 bytes
martinvonzAuthorUnsubmitted
Done

Sure, will do (FYI, that would indicate a bug in escape_default() or in my understanding of what it does anyway).

martinvonz: Sure, will do (FYI, that would indicate a bug in `escape_default()` or in my understanding of…
.finish()
}
}
fn debug_bytes(bytes: &[u8]) -> String {
String::from_utf8_lossy(
&bytes.iter().flat_map(|b| escape_default(*b)).collect_vec(),
)
.to_string()
}
#[cfg(test)]
mod tests {
use super::*;
use itertools::Itertools;
use pretty_assertions::assert_eq;
#[test]
fn test_create_changelogrevisiondata_invalid() {
// Completely empty
assert!(ChangelogRevisionData::new(b"abcd".to_vec()).is_err());
// No newline after manifest
assert!(ChangelogRevisionData::new(b"abcd".to_vec()).is_err());
// No newline after user
assert!(ChangelogRevisionData::new(b"abcd\n".to_vec()).is_err());
// No newline after timestamp
assert!(ChangelogRevisionData::new(b"abcd\n\n0 0".to_vec()).is_err());
// Missing newline after files
assert!(ChangelogRevisionData::new(
b"abcd\n\n0 0\nfile1\nfile2".to_vec()
)
.is_err(),);
// Only one newline after files
assert!(ChangelogRevisionData::new(
b"abcd\n\n0 0\nfile1\nfile2\n".to_vec()
)
.is_err(),);
}
#[test]
fn test_create_changelogrevisiondata() {
let data = ChangelogRevisionData::new(
b"0123456789abcdef0123456789abcdef01234567
Some One <someone@example.com>
0 0
file1
file2
some
commit
message"
.to_vec(),
)
.unwrap();
assert_eq!(
data.manifest_node().unwrap(),
Node::from_hex("0123456789abcdef0123456789abcdef01234567")
.unwrap()
);
assert_eq!(data.user(), b"Some One <someone@example.com>");
assert_eq!(data.timestamp_line(), b"0 0");
assert_eq!(
data.files().collect_vec(),
vec![HgPath::new("file1"), HgPath::new("file2")]
);
assert_eq!(data.description(), b"some\ncommit\nmessage");
}
} }