This is an archive of the discontinued Mercurial Phabricator instance.

rust-dirstate: create dirstate submodule
ClosedPublic

Authored by Alphare on May 16 2019, 11:49 AM.

Details

Summary

This change is here to facilitate a future patch that is written in a
different file. I expect this module to grow a few different files.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

Alphare created this revision.May 16 2019, 11:49 AM
Alphare updated this revision to Diff 15159.May 17 2019, 5:17 AM
kevincox added inline comments.May 17 2019, 9:37 AM
rust/hg-core/src/dirstate/mod.rs
6

If 1 and 2 are the best names why not just make it pub struct DirstateParetens([&[u8]; 2])?

7

It seems odd that this struct is public seeing as it is just a bag of bytes. Would it make sense to keep this internal to the parser/serializer?

rust/hg-core/src/dirstate/parsers.rs
40

It seems to me that this code would be simpler if you constructed a single cursor at the top of the function and just used it to track your state instead of having a couple of other variables to track it.

Alphare added inline comments.May 24 2019, 4:59 AM
rust/hg-core/src/dirstate/mod.rs
6

I feel that accessing parents.p1 is more semantic and less noisy than parents.0[0]. Also doing

DirstateParents([
    b"12345678910111213141",
    b"00000000000000000000",
]);

is not miles clearer than

DirstateParents {
    p1: b"12345678910111213141",
    p2: b"00000000000000000000",
};
7

This is public so that hg-cpython can use it. I think it helps with readability too, a "bag of bytes" is sometimes not really helpful when stumbling on code.

kevincox added inline comments.May 24 2019, 5:07 AM
rust/hg-core/src/dirstate/mod.rs
7

I guess the point I am trying to make is why do we pass around this "half-parsed" object. Why can't the public API go straight from bytes -> fully parsed.

Alphare added inline comments.May 24 2019, 5:34 AM
rust/hg-core/src/dirstate/mod.rs
7

The current Python public API expects parents as return value and does mutate-on-parse for other in-memory objects. Sorry if I'm missing something obvious.

kevincox accepted this revision.May 24 2019, 5:53 AM
kevincox added inline comments.
rust/hg-core/src/dirstate/mod.rs
7

Ok, I'm still not convinced this is the best API to expose but if it is a public API I guess it can't be changed.

Alphare added inline comments.May 24 2019, 6:04 AM
rust/hg-core/src/dirstate/mod.rs
7

Yup. I've rebased this change.

@kevincox I've rebase my changes as well as the next series (D6393, D6394, D6395).

martinvonz added inline comments.
rust/hg-core/src/dirstate/mod.rs
7

Ok, I'm still not convinced this is the best API to expose but if it is a public API I guess it can't be changed.

@kevincox, I haven't followed the discussion here and I don't know much about rust. I'll queue this patch based on your review. If you had any more comments, those can be addressed in a follow-up patch.

This revision was automatically updated to reflect the committed changes.
yuja added a subscriber: yuja.Jun 6 2019, 7:26 PM

Amended this to reproduce the copy history. No content change.