This is an archive of the discontinued Mercurial Phabricator instance.

rust-dirstate: add "dirs" Rust implementation
ClosedPublic

Authored by Alphare on May 17 2019, 6:10 AM.

Details

Summary

Following the work done in d1786c1d34fa and working towards the goal of a
complete Rust implementation of the dirstate, this rewrites the dirs class.

There is already a C implementation, which relies heavily on CPython hacks and
protocol violations for performance, so I don't expect this to perform as well
for now, as this is very straight-forward code.
The immediate benefits are new high-level documentation and some unit tests.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

Alphare created this revision.May 17 2019, 6:10 AM

Does this need updating after D6403? Based on a cursory look at the patch, it has not been updated yet.

Alphare updated this revision to Diff 15341.Jun 5 2019, 12:22 PM

Does this need updating after D6403? Based on a cursory look at the patch, it has not been updated yet.

Indeed it did. I've updated this changeset and rebased.

Alphare updated this revision to Diff 15508.Jun 14 2019, 6:13 AM

@kevincox, do you have any comments about this change?

kevincox accepted this revision.Jun 24 2019, 9:46 AM

Sorry. I was busy. In general don't worry about blocking on me, I can't promise any sort of reasonable response time. Worst case I can review the changes after submission and changes can be made afterwards.

rust/hg-core/src/dirstate/dirs_multiset.rs
43

You can replate the nested if with:

if skip_state == Some(state) {
63

I would remove the pos argument. IIUC following two are currently identical.

find_dir(path, n);
find_dir(path[..n], n);

If you remove the second argument then you can just always remove the last component of the path. This will also allow you to have a more clear doc comment. Right now I find it a little confusing.

Alphare marked an inline comment as done.Jun 27 2019, 9:16 AM
Alphare updated this revision to Diff 15670.

Sorry. I was busy. In general don't worry about blocking on me, I can't promise any sort of reasonable response time. Worst case I can review the changes after submission and changes can be made afterwards.

Sure, no problem, thanks.
I've also sneaked some &[u8] instead of Vec<u8> in my last update.

rust/hg-core/src/dirstate/dirs_multiset.rs
43

Unless I'm missing something, this would render the if statement useless.

martinvonz added inline comments.Jun 27 2019, 12:50 PM
rust/hg-core/src/dirstate/dirs_multiset.rs
43

Maybe Kevin meant something like this?

if skip_state == None | skip_state == Some(state) {
    multiset.add_path(filename);
}
kevincox added inline comments.Jun 28 2019, 5:01 AM
rust/hg-core/src/dirstate/dirs_multiset.rs
43

No, you are right. I misread the condition.

This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.
yuja added a subscriber: yuja.Jun 29 2019, 11:40 PM

+impl DirsMultiset {
+ / Initializes the multiset from a dirstate or a manifest.
+
/
+ / If skip_state is provided, skips dirstate entries with equal state.
+ pub fn new(iterable: DirsIterable, skip_state: Option<i8>) -> Self {
+ let mut multiset = DirsMultiset {
+ inner: HashMap::new(),
+ };
+
+ match iterable {
+ DirsIterable::Dirstate(vec) => {
+ for (ref filename, DirstateEntry { state, .. }) in vec {
+
This if is optimized out of the loop
+ if let Some(skip) = skip_state {
+ if skip != state {
+ multiset.add_path(filename);
+ }
+ } else {
+ multiset.add_path(filename);
+ }
+ }
+ }
+ DirsIterable::Manifest(vec) => {
+ for ref filename in vec {
+ multiset.add_path(filename);
+ }
+ }
+ }
+
+ multiset
+ }

Could be from_dirstate(vec, skip_state) and from_vec(vec) since the
skip_state argument only applies to the Dirstate variant.

+ / Returns the slice up to the next directory name from right to left,
+
/ without trailing slash
+ fn find_dir(path: &[u8]) -> &[u8] {
+ let mut path = path;
+ loop {
+ if let Some(new_pos) = path.len().checked_sub(1) {
+ if path[new_pos] == b'/' {
+ break &path[..new_pos];
+ }
+ path = &path[..new_pos];
+ } else {
+ break &[];
+ }
+ }
+ }

Maybe use Iterator::rposition()?

let p = path.iter().rposition(|&c| c == b'/').unwrap_or(0);
&path[..p]

Anyway, we'll probably want an iterator which yields parent directories.