This is an archive of the discontinued Mercurial Phabricator instance.

Differential D12442

rust-revlog: add methods for getting parent revs and entries
ClosedPublic

Authored by martinvonz on Apr 5 2022, 3:08 PM.

Details

Reviewers
Alphare
Group Reviewers
hg-reviewers
Commits
rHG5d205e476057: rust-revlog: add methods for getting parent revs and entries

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, 3:08 PM
martinvonz added a subscriber: Raphael.Apr 5 2022, 3:34 PM

FYI, the functions here may seem arbitrary, but they're enough for a very simple Mercurial backend for my 20%/hobby project (see commit https://github.com/martinvonz/jj/commit/hg if you're curious). I'll add tests to this patch if you (mostly @Raphael, I suppose) think the changes look good.

Alphare added a subscriber: Alphare.Apr 6 2022, 8:48 AM
Alphare added inline comments.
rust/hg-core/src/revlog/changelog.rs
33

Docstring needs adjustment

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

Mhhh I wonder if pN_entry might be a better name, then pN_rev would just be pN. That way it's more obvious you're getting an entry?

martinvonz updated this revision to Diff 32868.Apr 7 2022, 12:24 PM
martinvonz updated this revision to Diff 32869.Apr 7 2022, 12:27 PM
martinvonz updated this revision to Diff 32989.Apr 8 2022, 6:10 PM
martinvonz retitled this revision from [RFC] rhg: implement more methods on revlogs to [RFC] rust-revlog: implement more methods on revlogs.Apr 9 2022, 1:36 AM
martinvonz added a child revision: D12486: rust-nodemap: remove unnecessary explicit lifetime.
martinvonz removed a child revision: D12424: rust: upgrade zstd from 0.5.3 to 0.11.1.
martinvonz updated this revision to Diff 32990.
Alphare accepted this revision.Apr 11 2022, 6:23 AM
This revision is now accepted and ready to land.Apr 11 2022, 6:23 AM
martinvonz retitled this revision from [RFC] rust-revlog: implement more methods on revlogs to rust-revlog: add methods for getting parent revs and entries.Apr 11 2022, 12:07 PM
martinvonz updated this revision to Diff 33004.
martinvonz added a comment.Apr 11 2022, 12:08 PM
In D12442#190320, @martinvonz wrote:

FYI, the functions here may seem arbitrary, but they're enough for a very simple Mercurial backend for my 20%/hobby project (see commit https://github.com/martinvonz/jj/commit/hg if you're curious). I'll add tests to this patch if you (mostly @Raphael, I suppose) think the changes look good.

I just removed the "[RFC]" without adding tests :) But I can add tests if you want. Where's a good place to add them in that case?

Alphare added a comment.Apr 12 2022, 4:23 AM

I think we'll want to write some basic unit tests at the end of the file. We haven't been very good about doing these in the Rust code base, but I was starting again recently with the dirstatemap (and found some bugs, of course), so that's probably a good idea.

martinvonz added a comment.Apr 12 2022, 2:32 PM
In D12442#191302, @Alphare wrote:

I think we'll want to write some basic unit tests at the end of the file. We haven't been very good about doing these in the Rust code base, but I was starting again recently with the dirstatemap (and found some bugs, of course), so that's probably a good idea.

That's actually related to another series I had not sent yet, which is about removing the &Repo input to Revlog::open(). It would be much easier to write tests if we didn't have to create a Repo instance. What do you think about taking this without tests now and then I add tests when I send that other series? Or, if you think that refactoring sounds like a good idea, I can clean that series up and insert it before this patch.

Hopefully the patches before this one can be queued without it (though there's no real rush for us yet).

By the way, Repo is not currently Send. I think the issue was with LazyCell. Do you think we should do something about that or does it not matter for your plans?

martinvonz updated this revision to Diff 33138.Apr 12 2022, 2:33 PM
Alphare added a comment.Apr 13 2022, 4:24 AM
In D12442#191893, @martinvonz wrote:

That's actually related to another series I had not sent yet, which is about removing the &Repo input to Revlog::open(). It would be much easier to write tests if we didn't have to create a Repo instance.

I see no issue for doing this, it's probably a good idea.

What do you think about taking this without tests now and then I add tests when I send that other series? Or, if you think that refactoring sounds like a good idea, I can clean that series up and insert it before this patch.

I prefer the option where you do the clean up first of course, but the follow-up works just as well as long as it's done.

By the way, Repo is not currently Send. I think the issue was with LazyCell. Do you think we should do something about that or does it not matter for your plans?

I'm not sure I see the value of repo being Send for now. It being Sync should probably be good enough for all of our purposes? Did you have something in mind?

martinvonz added a comment.Apr 13 2022, 9:19 AM
In D12442#191910, @Alphare wrote:
In D12442#191893, @martinvonz wrote:

That's actually related to another series I had not sent yet, which is about removing the &Repo input to Revlog::open(). It would be much easier to write tests if we didn't have to create a Repo instance.

I see no issue for doing this, it's probably a good idea.

I've inserted those patches in this series.

What do you think about taking this without tests now and then I add tests when I send that other series? Or, if you think that refactoring sounds like a good idea, I can clean that series up and insert it before this patch.

I prefer the option where you do the clean up first of course, but the follow-up works just as well as long as it's done.

While trying to add tests for that last night I realized that writing the test revlogs gets pretty verbose. I then found the IndexEntryBuilder in index.rs. Maybe we should move that to a common place? Maybe even out of test code so it can be used when we start writing revlogs from Rust?

By the way, Repo is not currently Send. I think the issue was with LazyCell. Do you think we should do something about that or does it not matter for your plans?

I'm not sure I see the value of repo being Send for now. It being Sync should probably be good enough for all of our purposes? Did you have something in mind?

Yes, what hg would want is Sync. The context was that I needed the hg backend to be Sync for my 20% project so I just wrapped the hg Repo in a mutex, which means that Repo needed to be Send. Anyway, if you agree that hg wants it to eventually be Sync, we'll need to address those LazyCells.

martinvonz added a comment.Apr 13 2022, 9:24 AM
In D12442#191985, @martinvonz wrote:
In D12442#191910, @Alphare wrote:
In D12442#191893, @martinvonz wrote:

That's actually related to another series I had not sent yet, which is about removing the &Repo input to Revlog::open(). It would be much easier to write tests if we didn't have to create a Repo instance.

I see no issue for doing this, it's probably a good idea.

I've inserted those patches in this series.

I didn't notice you had queued some patches in this series already (thanks!). I've rebased the added patches on top of @ now.

Revision Contents
Changeset List

Diff 32785

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, RevlogEntry, 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 crate::utils::hg_path::HgPath;
use std::ascii::escape_default; use std::ascii::escape_default;
use std::fmt::{Debug, Formatter}; use std::fmt::{Debug, Formatter};
use std::ops::Range; use std::ops::Range;
/// A specialized `Revlog` to work with `changelog` data format. /// A specialized `Revlog` to work with `changelog` data format.
pub fn data_for_node( pub fn data_for_node(
&self, &self,
node: NodePrefix, node: NodePrefix,
) -> Result<ChangelogRevisionData, RevlogError> { ) -> Result<ChangelogRevisionData, RevlogError> {
let rev = self.revlog.rev_from_node(node)?; let rev = self.revlog.rev_from_node(node)?;
self.data_for_rev(rev) self.data_for_rev(rev)
} }
/// Return the `ChangelogEntry` of the given revision number. /// Return the `ChangelogEntry` of the given revision number.
AlphareUnsubmitted
Not Done

Docstring needs adjustment

Alphare: Docstring needs adjustment
pub fn entry_for_node(
&self,
node: NodePrefix,
) -> Result<RevlogEntry, RevlogError> {
let rev = self.revlog.rev_from_node(node)?;
self.revlog.get_entry(rev)
}
/// Return the `ChangelogEntry` of the given revision number.
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 {

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

hash: Node, hash: Node,
} }
impl<'a> RevlogEntry<'a> { impl<'a> RevlogEntry<'a> {
pub fn revision(&self) -> Revision { pub fn revision(&self) -> Revision {
self.rev self.rev
} }
pub fn node(&self) -> &Node {
&self.hash
}
pub fn uncompressed_len(&self) -> Option<u32> { pub fn uncompressed_len(&self) -> Option<u32> {
u32::try_from(self.uncompressed_len).ok() u32::try_from(self.uncompressed_len).ok()
} }
pub fn has_p1(&self) -> bool { pub fn has_p1(&self) -> bool {
self.p1 != NULL_REVISION self.p1 != NULL_REVISION
} }
pub fn p1(&self) -> Result<Option<RevlogEntry>, RevlogError> {
AlphareUnsubmitted
Not Done

Mhhh I wonder if pN_entry might be a better name, then pN_rev would just be pN. That way it's more obvious you're getting an entry?

Alphare: Mhhh I wonder if `pN_entry` might be a better name, then `pN_rev` would just be `pN`. That way…
if self.p1 == NULL_REVISION {
Ok(None)
} else {
Ok(Some(self.revlog.get_entry(self.p1)?))
}
}
pub fn p2(&self) -> Result<Option<RevlogEntry>, RevlogError> {
if self.p2 == NULL_REVISION {
Ok(None)
} else {
Ok(Some(self.revlog.get_entry(self.p1)?))
}
}
pub fn p1_rev(&self) -> Option<Revision> {
if self.p1 == NULL_REVISION {
None
} else {
Some(self.p1)
}
}
pub fn p2_rev(&self) -> Option<Revision> {
if self.p2 == NULL_REVISION {
None
} else {
Some(self.p2)
}
}
pub fn is_cencored(&self) -> bool { pub fn is_cencored(&self) -> bool {
(self.flags & REVISION_FLAG_CENSORED) != 0 (self.flags & REVISION_FLAG_CENSORED) != 0
} }
pub fn has_length_affecting_flag_processor(&self) -> bool { pub fn has_length_affecting_flag_processor(&self) -> bool {
// Relevant Python code: revlog.size() // Relevant Python code: revlog.size()
// note: ELLIPSIS is known to not change the content // note: ELLIPSIS is known to not change the content
(self.flags & (REVIDX_KNOWN_FLAGS ^ REVISION_FLAG_ELLIPSIS)) != 0 (self.flags & (REVIDX_KNOWN_FLAGS ^ REVISION_FLAG_ELLIPSIS)) != 0