This is an archive of the discontinued Mercurial Phabricator instance.

Differential D11650

rhg: handle null changelog and manifest revisions
ClosedPublic

Authored by aalekseyev on Oct 12 2021, 2:44 PM.

Details

Reviewers
martinvonz
Group Reviewers
hg-reviewers
Commits
rHG61ce70fd420e: rhg: handle null changelog and manifest revisions

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

aalekseyev created this revision.Oct 12 2021, 2:44 PM
martinvonz added a subscriber: martinvonz.Oct 12 2021, 2:54 PM
martinvonz added inline comments.
tests/test-empty-manifest-index.t
7–11

FYI, this can be written hg commit -m "init" --config ui.allowemptycommit=true

aalekseyev added inline comments.Oct 12 2021, 3:20 PM
tests/test-empty-manifest-index.t
7–11

Thanks, I did not know that was supported in vanilla hg.
However, this is not equivalent: --amend produces an empty 00manifest.i file, while ui.allowemptycommit=true produces an absent file.
In fact the fixes in this rev are sufficient for the amend case, but are not sufficient for the emptycommit case (test is broken with rhg).
I'm working on further changes that make rhg work in an empty repo and an empty commit.

martinvonz accepted this revision.Oct 12 2021, 3:53 PM
martinvonz added inline comments.
tests/test-empty-manifest-index.t
7–11

Makes sense about the difference.

This revision is now accepted and ready to land.Oct 12 2021, 3:53 PM
aalekseyev added a commit: rHG61ce70fd420e: rhg: handle null changelog and manifest revisions.Oct 12 2021, 4:03 PM
Closed by commit rHG61ce70fd420e: rhg: handle null changelog and manifest revisions (authored by aalekseyev). · Explain Why
This revision was automatically updated to reflect the committed changes.
aalekseyev added a comment.Oct 12 2021, 4:09 PM

@martinvonz, thanks!
I pushed the follow-up changes in:
https://phab.mercurial-scm.org/D11651

SimonSapin added a subscriber: SimonSapin.Oct 13 2021, 12:12 PM

This broke a Rust unit test: https://foss.heptapod.net/mercurial/mercurial-devel/-/jobs/253010#L32

You can run these tests with (cd rust && cargo test). Could you send a follow-up patch to fix this?

aalekseyev added a comment.Oct 13 2021, 12:58 PM
In D11650#178579, @SimonSapin wrote:

This broke a Rust unit test: https://foss.heptapod.net/mercurial/mercurial-devel/-/jobs/253010#L32
You can run these tests with (cd rust && cargo test). Could you send a follow-up patch to fix this?

Sorry, I didn't even realize those tests existed. I only tested with tests/run-tests.py --rhg. Thanks.
I'll do that.

aalekseyev added a comment.Oct 13 2021, 1:15 PM
In D11650#178580, @aalekseyev wrote:

I'll do that.

Submitted: https://phab.mercurial-scm.org/D11656
Is that alright, or do you prefer me amend the original commit? (will I be able to phabsend it though?)

martinvonz added a comment.Oct 13 2021, 1:16 PM
In D11650#178587, @aalekseyev wrote:
In D11650#178580, @aalekseyev wrote:

I'll do that.

Submitted: https://phab.mercurial-scm.org/D11656
Is that alright, or do you prefer me amend the original commit? (will I be able to phabsend it though?)

I don't think you can, but I can fold that into your original commit.

Alphare added a subscriber: Alphare.Oct 18 2021, 5:00 AM

This breaks the CI: https://foss.heptapod.net/mercurial/mercurial-devel/-/jobs/254045

It seems that the previous assumption made in the code was that null revs were to be shortcut and this assumption was not changed everywhere. @aalekseyev The easiest way of making sure you're not breaking anything is to push a topic to https://foss.heptapod.net/mercurial/mercurial-devel (if you create an account I can give you the rights to do so).

Right now other tests are failing, so you might see unrelated failures, but I'm looking into those as well. I try to keep the CI is mostly green, but I was on vacation.

tests/test-empty-manifest-index.t
1

Does the test suite already have a test for empty manifest or is this specific to rhg? I'm asking because I don't think mixing rhg config options within "normal tests" is a good idea. There is test-rhg.t that is specifically done to test the differences (or lack thereof) and the fallback mechanism.

aalekseyev added a comment.Oct 18 2021, 5:21 AM
In D11650#179086, @Alphare wrote:

This breaks the CI: https://foss.heptapod.net/mercurial/mercurial-devel/-/jobs/254045
It seems that the previous assumption made in the code was that null revs were to be shortcut and this assumption was not changed everywhere. @aalekseyev The easiest way of making sure you're not breaking anything is to push a topic to https://foss.heptapod.net/mercurial/mercurial-devel (if you create an account I can give you the rights to do so).
Right now other tests are failing, so you might see unrelated failures, but I'm looking into those as well. I try to keep the CI is mostly green, but I was on vacation.

Sorry for this breakage. I've been running tests by hand, but apparently forgot to run all of them for that patch.
This should be fixed by 62630772f5f7 (https://phab.mercurial-scm.org/D11664).

aalekseyev added inline comments.Oct 18 2021, 5:27 AM
tests/test-empty-manifest-index.t
1

I couldn't find an existing test for null manifest revision.
I assumed that if one existed, it would have caught the rhg issue, therefore there is none.
It's possible that the test exists but opts-out rhg, and I had a quick look through rhg opt-outs, and found nothing relevant.

mixing rhg config options within "normal tests"

Oh, yeah, I added those temporarily to make sure I was testing what I thought I was testing.
I removed those config options in the very next commit (9d0e5629cfbf).

Alphare added inline comments.Oct 18 2021, 5:34 AM
tests/test-empty-manifest-index.t
1

I couldn't find an existing test for null manifest revision.

Sure, me neither, that's fine. :)

Oh, yeah, I added those temporarily to make sure I was testing what I thought I was testing.
I removed those config options in the very next commit

I would have preferred not to have those at all and to have waiting for the fix, but I guess the churn is not worth it anymore. Thanks for fixing the issues, I'm trying to catch up to what happened. :)

aalekseyev added a comment.Oct 18 2021, 1:38 PM
In D11650#179086, @Alphare wrote:

... push a topic to https://foss.heptapod.net/mercurial/mercurial-devel (if you create an account I can give you the rights to do so).

I made an account with username "aalekseyev". Can you please give me those rights?
Thanks.

Revision Contents
Changeset List

Diff 30765

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::node::NULL_NODE;
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};
/// 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,
self.bytes self.bytes
.split(|b| b == &b'\n') .split(|b| b == &b'\n')
.filter(|line| !line.is_empty()) .filter(|line| !line.is_empty())
} }
/// 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> {
Node::from_hex_for_repo( match self.lines().next() {
self.lines() None => Ok(NULL_NODE),
.next() Some(x) => Node::from_hex_for_repo(x),
.ok_or_else(|| HgError::corrupted("empty changelog entry"))?, }
)
} }
} }

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

/// are used. /// are used.
pub fn hash(&self) -> &'a Node { pub fn hash(&self) -> &'a Node {
(&self.bytes[32..52]).try_into().unwrap() (&self.bytes[32..52]).try_into().unwrap()
} }
} }
/// Value of the inline flag. /// Value of the inline flag.
pub fn is_inline(index_bytes: &[u8]) -> bool { pub fn is_inline(index_bytes: &[u8]) -> bool {
if index_bytes.len() < 4 {
return true;
}
match &index_bytes[0..=1] { match &index_bytes[0..=1] {
[0, 0] | [0, 2] => false, [0, 0] | [0, 2] => false,
_ => true, _ => true,
} }
} }
#[cfg(test)] #[cfg(test)]
mod tests { mod tests {

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

pub fn open( pub fn open(
repo: &Repo, repo: &Repo,
index_path: impl AsRef<Path>, index_path: impl AsRef<Path>,
data_path: Option<&Path>, data_path: Option<&Path>,
) -> Result<Self, HgError> { ) -> Result<Self, HgError> {
let index_path = index_path.as_ref(); let index_path = index_path.as_ref();
let index_mmap = repo.store_vfs().mmap_open(&index_path)?; let index_mmap = repo.store_vfs().mmap_open(&index_path)?;
let version = get_version(&index_mmap); let version = get_version(&index_mmap)?;
if version != 1 { if version != 1 {
// A proper new version should have had a repo/store requirement. // A proper new version should have had a repo/store requirement.
return Err(HgError::corrupted("corrupted revlog")); return Err(HgError::corrupted("corrupted revlog"));
} }
let index = Index::new(Box::new(index_mmap))?; let index = Index::new(Box::new(index_mmap))?;
let default_data_path = index_path.with_extension("d"); let default_data_path = index_path.with_extension("d");
/// Return the full data associated to a revision. /// Return the full data associated to a revision.
/// ///
/// All entries required to build the final data out of deltas will be /// All entries required to build the final data out of deltas will be
/// retrieved as needed, and the deltas will be applied to the inital /// retrieved as needed, and the deltas will be applied to the inital
/// snapshot to rebuild the final data. /// snapshot to rebuild the final data.
#[timed] #[timed]
pub fn get_rev_data(&self, rev: Revision) -> Result<Vec<u8>, RevlogError> { pub fn get_rev_data(&self, rev: Revision) -> Result<Vec<u8>, RevlogError> {
if rev == NULL_REVISION {
return Ok(vec![]);
};
// Todo return -> Cow // Todo return -> Cow
let mut entry = self.get_entry(rev)?; let mut entry = self.get_entry(rev)?;
let mut delta_chain = vec![]; let mut delta_chain = vec![];
while let Some(base_rev) = entry.base_rev { while let Some(base_rev) = entry.base_rev {
delta_chain.push(entry); delta_chain.push(entry);
entry = self entry = self
.get_entry(base_rev) .get_entry(base_rev)
.map_err(|_| RevlogError::corrupted())?; .map_err(|_| RevlogError::corrupted())?;
/// Tell if the entry is a snapshot or a delta /// Tell if the entry is a snapshot or a delta
/// (influences on decompression). /// (influences on decompression).
fn is_delta(&self) -> bool { fn is_delta(&self) -> bool {
self.base_rev.is_some() self.base_rev.is_some()
} }
} }
/// Format version of the revlog. /// Format version of the revlog.
pub fn get_version(index_bytes: &[u8]) -> u16 { pub fn get_version(index_bytes: &[u8]) -> Result<u16, HgError> {
BigEndian::read_u16(&index_bytes[2..=3]) if index_bytes.len() == 0 {
return Ok(1);
};
if index_bytes.len() < 4 {
return Err(HgError::corrupted(
"corrupted revlog: can't read the index format header",
));
};
Ok(BigEndian::read_u16(&index_bytes[2..=3]))
} }
/// Calculate the hash of a revision given its data and its parents. /// Calculate the hash of a revision given its data and its parents.
fn hash( fn hash(
data: &[u8], data: &[u8],
p1_hash: &[u8], p1_hash: &[u8],
p2_hash: &[u8], p2_hash: &[u8],
) -> [u8; NODE_BYTES_LENGTH] { ) -> [u8; NODE_BYTES_LENGTH] {

tests/test-empty-manifest-index.t

  • This file was added.
Create a repo such that the changelog entry refers to a null manifest node:
AlphareUnsubmitted
Not Done

Does the test suite already have a test for empty manifest or is this specific to rhg? I'm asking because I don't think mixing rhg config options within "normal tests" is a good idea. There is test-rhg.t that is specifically done to test the differences (or lack thereof) and the fallback mechanism.

Alphare: Does the test suite already have a test for empty manifest or is this specific to `rhg`? I'm…
aalekseyevAuthorUnsubmitted
Done

I couldn't find an existing test for null manifest revision.
I assumed that if one existed, it would have caught the rhg issue, therefore there is none.
It's possible that the test exists but opts-out rhg, and I had a quick look through rhg opt-outs, and found nothing relevant.

mixing rhg config options within "normal tests"

Oh, yeah, I added those temporarily to make sure I was testing what I thought I was testing.
I removed those config options in the very next commit (9d0e5629cfbf).

aalekseyev: I couldn't find an existing test for null manifest revision. I assumed that if one existed, it…
AlphareUnsubmitted
Not Done

I couldn't find an existing test for null manifest revision.

Sure, me neither, that's fine. :)

Oh, yeah, I added those temporarily to make sure I was testing what I thought I was testing.
I removed those config options in the very next commit

I would have preferred not to have those at all and to have waiting for the fix, but I guess the churn is not worth it anymore. Thanks for fixing the issues, I'm trying to catch up to what happened. :)

Alphare: > I couldn't find an existing test for null manifest revision. Sure, me neither, that's fine.
$ hg init a
$ cd a
$ hg log
$ touch x
$ hg add x
$ hg commit -m "init"
$ hg rm x
$ hg commit -q --amend
martinvonzUnsubmitted
Not Done

FYI, this can be written hg commit -m "init" --config ui.allowemptycommit=true

martinvonz: FYI, this can be written `hg commit -m "init" --config ui.allowemptycommit=true`
aalekseyevAuthorUnsubmitted
Done

Thanks, I did not know that was supported in vanilla hg.
However, this is not equivalent: --amend produces an empty 00manifest.i file, while ui.allowemptycommit=true produces an absent file.
In fact the fixes in this rev are sufficient for the amend case, but are not sufficient for the emptycommit case (test is broken with rhg).
I'm working on further changes that make rhg work in an empty repo and an empty commit.

aalekseyev: Thanks, I did not know that was supported in vanilla hg. However, this is not equivalent: `…
martinvonzUnsubmitted
Not Done

Makes sense about the difference.

martinvonz: Makes sense about the difference.
$ wc -c < .hg/store/00manifest.i
0
Make sure that the manifest can be read (and is empty):
$ hg --config rhg.on-unsupported=abort files -r .
[1]
Test a null changelog rev, too:
$ hg --config rhg.on-unsupported=abort files -r 0000000000000000000000000000000000000000
[1]