This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9012

hg-core: add `Revlog.get_node_rev`
ClosedPublic

Authored by acezar on Sep 11 2020, 3:53 AM.

Details

Reviewers
Alphare
Group Reviewers
hg-reviewers
Commits
rHG4f11a67a12fb: hg-core: add `Revlog.get_node_rev`
Summary

Find the revision of a node given its full hash.

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

acezar created this revision.Sep 11 2020, 3:53 AM
Alphare added subscribers: gracinet, Alphare.Sep 17 2020, 6:11 AM
Alphare requested changes to this revision.
Alphare added inline comments.
rust/hg-core/src/revlog/revlog.rs
82

I know Clippy will complain about the lack of is_empty method when you have a len method, if you want to add it quickly.

91

Wouldn't it make more sense to start from the end as we're more likely to be looking for recent nodes?

Also, it's not clear by looking at this that this bruteforce method is not the one we'll use in the future, using @gracinet's nodemap. I think a small comment would be enough.

This revision now requires changes to proceed.Sep 17 2020, 6:11 AM
acezar added inline comments.Sep 18 2020, 8:46 AM
rust/hg-core/src/revlog/revlog.rs
82

I've seen the clippy warning, but did not added it because it wasn't needed and since it's not enforced by an interface, I was not sure about what to do about it. Maybe having 0 warning on easy things should be a goal.

91

Like you said we're more likely to be looking for recent nodes thus searching from the start when the revolg is huge is probably not the way to go.

acezar updated this revision to Diff 22723.Sep 18 2020, 10:49 AM
acezar marked 2 inline comments as done.Sep 18 2020, 10:52 AM
acezar added inline comments.
rust/hg-core/src/revlog/revlog.rs
82

It has been added in another commit. I moved the code here

acezar added a commit: rHG4f11a67a12fb: hg-core: add `Revlog.get_node_rev`.Sep 23 2020, 10:57 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG4f11a67a12fb: hg-core: add `Revlog.get_node_rev` (authored by acezar). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

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

Diff 22793

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

}; };
Ok(Revlog { Ok(Revlog {
index_bytes, index_bytes,
data_bytes, data_bytes,
}) })
} }
/// Return number of entries of the `Revlog`.
AlphareUnsubmitted
Done

I know Clippy will complain about the lack of is_empty method when you have a len method, if you want to add it quickly.

Alphare: I know Clippy will complain about the lack of `is_empty` method when you have a `len` method…
acezarAuthorUnsubmitted
Done

I've seen the clippy warning, but did not added it because it wasn't needed and since it's not enforced by an interface, I was not sure about what to do about it. Maybe having 0 warning on easy things should be a goal.

acezar: I've seen the clippy warning, but did not added it because it wasn't needed and since it's not…
acezarAuthorUnsubmitted
Done

It has been added in another commit. I moved the code here

acezar: It has been added in another commit. I moved the code here
pub fn len(&self) -> usize {
self.index().len()
}
/// Returns `true` if the `Revlog` has zero `entries`.
pub fn is_empty(&self) -> bool {
self.index().is_empty()
}
AlphareUnsubmitted
Done

Wouldn't it make more sense to start from the end as we're more likely to be looking for recent nodes?

Also, it's not clear by looking at this that this bruteforce method is not the one we'll use in the future, using @gracinet's nodemap. I think a small comment would be enough.

Alphare: Wouldn't it make more sense to start from the end as we're more likely to be looking for recent…
acezarAuthorUnsubmitted
Done

Like you said we're more likely to be looking for recent nodes thus searching from the start when the revolg is huge is probably not the way to go.

acezar: Like you said we're more likely to be looking for recent nodes thus searching from the start…
/// Return the full data associated to a node.
#[timed]
pub fn get_node_rev(&self, node: &[u8]) -> Result<Revision, RevlogError> {
let index = self.index();
// This is brute force. But it is fast enough for now.
// Optimization will come later.
for rev in (0..self.len() as Revision).rev() {
let index_entry =
index.get_entry(rev).ok_or(RevlogError::Corrupted)?;
if node == index_entry.hash() {
return Ok(rev);
}
}
Err(RevlogError::InvalidRevision)
}
/// 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> {
// Todo return -> Cow // Todo return -> Cow