Page MenuHomePhabricator

treedirstate: add Dirstate
ClosedPublic

Authored by mbthomas on Nov 14 2017, 12:39 PM.

Details

Reviewers
jsgf
durham
Group Reviewers
Restricted Project
Commits
rFBHGX3d05c6c9da72: treedirstate: add Dirstate
Summary

A Dirstate object links a Tree to an underlying Store and StoreView
implementation.

Diff Detail

Repository
rFBHGX Facebook Mercurial Extensions
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

mbthomas created this revision.Nov 14 2017, 12:39 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 14 2017, 12:39 PM
mbthomas updated this revision to Diff 3597.Nov 17 2017, 9:45 AM
simonfar added inline comments.
rust/treedirstate/src/dirstate.rs
13–15

Rust arrays have their size as part of the type. Use MAGIC.len() when you need the length, and set the type to [u8; 4] to indicate that this is a programmer-set size (don't let Rust deduce it).

mbthomas updated this revision to Diff 3652.Nov 20 2017, 12:20 PM
durham added a subscriber: durham.Nov 21 2017, 12:14 AM
durham added inline comments.
rust/treedirstate/src/dirstate.rs
90

Rust noob question: Is the extra scoping here is needed because root_data has a lifetime attached to the store, and therefore we need to unscope root_data (by unscoping root) before we can move store to the Backend::File constructor?

155

What does it mean to pass state to the remove function? Do we actually access state stored on the removed tree?

158

Why &state here but just state above in add_file?

mbthomas added inline comments.Nov 21 2017, 6:19 AM
rust/treedirstate/src/dirstate.rs
90

Correct. Hopefully non-lexical-lifetimes will remove the need for this extra scope.

155

Yes, Mercurial uses the size field on removed entries to encode some state that it needs to restore if the file gets re-added (-1 means the file was in 'm' state, -2 means the file was other-parent).

I did think about making this an explicit enum, but I think that's an API change that's outside the scope of treedirstate.

158

The & is unnecessary here as state is already a reference. I'll remove it.

mbthomas added inline comments.Nov 21 2017, 1:16 PM
rust/treedirstate/src/dirstate.rs
13–15

I need the length for line 77 where I'm creating a buffer to read from disk, and without const expressions, I can't use let buffer = [u8; MAGIC.len()]. I would use [u8; MAGIC_LEN] for the type here, but you can't initialize [u8; 4] with b"////" as the latter is considered a reference. I could use const MAGIC: [u8; 4] = { b'/', b'/', b'/', b'/' };, but that seems worse to me.

mbthomas updated this revision to Diff 3727.Nov 21 2017, 1:35 PM
jsgf added a subscriber: jsgf.Nov 22 2017, 4:49 PM
jsgf added inline comments.
rust/treedirstate/src/dirstate.rs
44

Does it make sense for this to be generic over the per-file info at the point you're committing things to storage? Does it ever get used with something other than FileState?

84–87

I think all this low-level serde stuff should be confined to types with implementations of Storable rather than just stuck in the middle of other code. It complicates the logic and it ends up being hard to maintain (it's easy to accidentally change the file format when moving code around).

120–122

Would it be better to leave the old store in place if writing the new one fails?

jsgf requested changes to this revision.Nov 22 2017, 4:50 PM

z

This revision now requires changes to proceed.Nov 22 2017, 4:50 PM
mbthomas added inline comments.Nov 23 2017, 8:00 AM
rust/treedirstate/src/dirstate.rs
44

In the test code I use a simplified State class, but no, it's not really necessary.

mbthomas added inline comments.Nov 24 2017, 6:49 AM
rust/treedirstate/src/dirstate.rs
120–122

When writing full copies of the tree out, we update the tree with IDs from the new store. If we fail to finish, then the tree will be in an inconsistent state, with potentially some nodes referring to the old store, and some referring to the new.

We should probably invalidate ourselves here, but I'm not really sure of the best way to do it? Any suggestions?

mbthomas added inline comments.Nov 24 2017, 3:33 PM
rust/treedirstate/src/dirstate.rs
44

I've added D1512 which removed the type parameter on Dirstate.

mbthomas updated this revision to Diff 3852.Nov 24 2017, 3:35 PM
durham accepted this revision.Nov 27 2017, 12:31 PM
This revision was automatically updated to reflect the committed changes.