Page MenuHomePhabricator

treedirstate: add Store and StoreView traits
ClosedPublic

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

Details

Reviewers
jsgf
durham
Group Reviewers
Restricted Project
Commits
rFBHGX7111992684f6: treedirstate: add Store and StoreView traits
Summary

These traits represent abstract store objects than can store arbitrary data
blocks with store-generated indexes.

A NullStore implementation is provided which acts an always-empty StoreView.

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 3594.Nov 17 2017, 9:44 AM
jsgf requested changes to this revision.Nov 17 2017, 12:37 PM
jsgf added a subscriber: jsgf.
jsgf added inline comments.
rust/treedirstate/src/lib.rs
18–23

These don't seem to be being used here?

rust/treedirstate/src/store.rs
8

newtype?

#[derive(Copy, Clone, Debug, Eq, PartialEq, Hash)]
pub struct BlockId(usize);
20–24

Is the view into the store guaranteed to be immutable?

This revision now requires changes to proceed.Nov 17 2017, 12:37 PM
jsgf added inline comments.Nov 17 2017, 12:45 PM
rust/treedirstate/src/store.rs
8

Also usize isn't the right type to use for non-memory things. If the store is >4G then usize wouldn't be large enough on a 32 bit machine.

mbthomas added inline comments.Nov 20 2017, 12:13 PM
rust/treedirstate/src/lib.rs
18–23

They're used in vecmap. I couldn't really think of good properties to quickcheck for these traits.

rust/treedirstate/src/store.rs
8

I've changed BlockId to be pub struct BlockId(u64). I'm not sure if this is the right approach, though, as it means I need to use id.0 to pull out the number (e.g. to pass it to python code, or to write it out). Let me know what you think in the revised version.

20–24

Yes, the blocks of data in the store are immutable once written. I'll highlight this in the comments.

mbthomas updated this revision to Diff 3649.Nov 20 2017, 12:20 PM
mbthomas marked 5 inline comments as done.Nov 20 2017, 12:23 PM
durham accepted this revision.Nov 20 2017, 2:25 PM
durham added a subscriber: durham.
durham added inline comments.
rust/treedirstate/src/errors.rs
12

Is there any convention in rust for explaining what macro's do? For instance, if I have worked in rust, but never seen error_chain before, do I need to go read the error_chain documentation in order to understand this file? Or should we have comments that describe what these things are doing?

Seems like this type of problem will only grow as we depend on more and more macros.

rust/treedirstate/src/store.rs
12

Assumed to be thread safe? Or no? Probably something we want to be explicit about if we're planning for a future where these things run in a daemon.

14

Do we have line length lint rules for rust yet?

quark added a subscriber: quark.Nov 21 2017, 4:43 AM
quark added inline comments.
rust/treedirstate/src/errors.rs
12

There is a way to expand macros. Although sometimes it's not human-readable.

I think we won't need too many external macros. error_chain! and quickcheck! might be the only worthwhile ones for now.

rust/treedirstate/src/store.rs
12

Structs using thread-unsafe primitives won't compile when being passed across thread boundary.

14

rustfmt uses 100 characters by default.

56

Maybe Boxed<[u8]> is better than Vec<u8> here to indicate the content is immutable.

mbthomas added inline comments.Nov 22 2017, 5:10 AM
rust/treedirstate/src/store.rs
56

It's for test code only, so I'm ok with it being mutable (a future test may want to do some underhanded mutations).

jsgf accepted this revision.Nov 22 2017, 5:51 PM
jsgf added inline comments.
rust/treedirstate/src/store.rs
8

I think id.0 is OK, but you could perhaps implement From and/or Deref to pretty it up.

This revision is now accepted and ready to land.Nov 22 2017, 5:51 PM
mbthomas updated this revision to Diff 3819.Nov 24 2017, 11:52 AM
This revision was automatically updated to reflect the committed changes.