( )⚙ D1397 treedirstate: add Store and StoreView traits

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1397

treedirstate: add Store and StoreView traits
ClosedPublic

Authored by mbthomas on Nov 14 2017, 12:39 PM.
Tags
None
Subscribers
quark, durham, jsgf

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
Lint Skipped
Unit
Unit Tests Skipped

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
7

newtype?

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

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
7

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
7

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.

19–23

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
7

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
Closed by commit rFBHGX7111992684f6: treedirstate: add Store and StoreView traits (authored by mbthomas). · Explain WhyNov 28 2017, 6:00 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 3594

rust/treedirstate/Cargo.toml

[package] [package]
name = "rusttreedirstate" name = "rusttreedirstate"
version = "0.1.0" version = "0.1.0"
authors = ["Facebook Source Control Team <sourcecontrol-dev@fb.com>"] authors = ["Facebook Source Control Team <sourcecontrol-dev@fb.com>"]
[profile.release] [profile.release]
lto = true lto = true
[lib] [lib]
name = "rusttreedirstate" name = "rusttreedirstate"
crate-type = ["cdylib"] crate-type = ["cdylib"]
[dependencies]
error-chain = "*"
[dev-dependencies] [dev-dependencies]
itertools = "0.7.2" itertools = "0.7.2"
quickcheck = "*" quickcheck = "*"

rust/treedirstate/src/errors.rs

  • This file was added.
// Copyright Facebook, Inc. 2017
//! Errors.
error_chain! {
errors {
InvalidStoreId(id: usize) {
description("invalid store id"),
display("invalid store id: {}", id),
}
}
foreign_links {
Io(::std::io::Error);
durhamUnsubmitted
Not Done

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.

durham: Is there any convention in rust for explaining what macro's do? For instance, if I have worked…
quarkUnsubmitted
Not Done

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.

quark: There is [a way](https://github.com/dtolnay/cargo-expand) to expand macros. Although sometimes…
Utf8(::std::str::Utf8Error);
Utf8String(::std::string::FromUtf8Error);
}
}

rust/treedirstate/src/lib.rs

// Copyright Facebook, Inc. 2017 // Copyright Facebook, Inc. 2017
//! treedirstate - Tree-based Directory State. //! treedirstate - Tree-based Directory State.
//! //!
//! This is a Rust implementation of the dirstate concept for Mercurial, using a tree structure //! This is a Rust implementation of the dirstate concept for Mercurial, using a tree structure
//! in an append-only storage back-end. //! in an append-only storage back-end.
//! //!
//! The directory state stores information for all files in a working copy that are of interest //! The directory state stores information for all files in a working copy that are of interest
//! to Mercurial. In particular, for each file in the working copy it stores the mode flags, //! to Mercurial. In particular, for each file in the working copy it stores the mode flags,
//! size, and modification time of the file. These can be compared with current values to //! size, and modification time of the file. These can be compared with current values to
//! determine if the file has changed. //! determine if the file has changed.
//! //!
//! The directory state also stores files that are in the working copy parent manifest but have //! The directory state also stores files that are in the working copy parent manifest but have
//! been marked as removed. //! been marked as removed.
#[macro_use]
extern crate error_chain;
#[cfg(test)] #[cfg(test)]
extern crate itertools; extern crate itertools;
#[cfg(test)] #[cfg(test)]
#[macro_use] #[macro_use]
extern crate quickcheck; extern crate quickcheck;
jsgfUnsubmitted
Done

These don't seem to be being used here?

jsgf: These don't seem to be being used here?
mbthomasAuthorUnsubmitted
Done

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

mbthomas: They're used in vecmap. I couldn't really think of good properties to quickcheck for these…
pub mod errors;
pub mod store;
pub mod vecmap; pub mod vecmap;
pub use errors::*;

rust/treedirstate/src/store.rs

  • This file was added.
// Copyright Facebook, Inc. 2017
//! Trait defining an append-only storage system.
use errors::*;
use std::borrow::Cow;
pub type BlockId = usize;
jsgfUnsubmitted
Not Done

newtype?

#[derive(Copy, Clone, Debug, Eq, PartialEq, Hash)]
pub struct BlockId(usize);
jsgf: newtype? ``` #[derive(Copy, Clone, Debug, Eq, PartialEq, Hash)] pub struct BlockId(usize); ```
mbthomasAuthorUnsubmitted
Not Done

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.

mbthomas: I've changed `BlockId` to be `pub struct BlockId(u64)`. I'm not sure if this is the right…
jsgfUnsubmitted
Not Done

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

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

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.

jsgf: Also `usize` isn't the right type to use for non-memory things. If the store is >4G then…
/// Append-only storage. Blocks of data may be stored in an instance of a Store.
pub trait Store {
/// Append a new block of data to the store. Returns the ID of the block. Note that blocks
/// may be buffered until `flush` is called.
durhamUnsubmitted
Not Done

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.

durham: Assumed to be thread safe? Or no? Probably something we want to be explicit about if we're…
quarkUnsubmitted
Not Done

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

quark: Structs using thread-unsafe primitives won't compile when being passed across thread boundary.
fn append(&mut self, data: &[u8]) -> Result<BlockId>;
durhamUnsubmitted
Not Done

Do we have line length lint rules for rust yet?

durham: Do we have line length lint rules for rust yet?
quarkUnsubmitted
Not Done

rustfmt uses 100 characters by default.

quark: rustfmt uses 100 characters by default.
/// Flush all appended blocks to the backing store.
fn flush(&mut self) -> Result<()>;
}
/// Read-only view of a store.
pub trait StoreView {
/// Read a block of data from the store.
fn read<'a>(&'a self, id: BlockId) -> Result<Cow<'a, [u8]>>;
}
jsgfUnsubmitted
Done

Is the view into the store guaranteed to be immutable?

jsgf: Is the view into the store guaranteed to be immutable?
mbthomasAuthorUnsubmitted
Done

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

mbthomas: Yes, the blocks of data in the store are immutable once written. I'll highlight this in the…
/// Null implementation of a store. This cannot be used to store new blocks of data, and returns
/// an error if any attempts to read are made.
pub struct NullStore;
impl NullStore {
pub fn new() -> NullStore {
NullStore
}
}
impl StoreView for NullStore {
fn read<'a>(&'a self, id: BlockId) -> Result<Cow<'a, [u8]>> {
bail!(ErrorKind::InvalidStoreId(id))
}
}
#[cfg(test)]
pub mod tests {
use std::collections::HashMap;
use errors::*;
use store::{BlockId, Store, StoreView};
use std::borrow::Cow;
/// Define a Store to be used in tests. This doesn't store the data on disk, but rather
/// keeps it in memory in a hash map.
pub struct MapStore {
next_id: BlockId,
data: HashMap<BlockId, Vec<u8>>,
}
impl MapStore {
pub fn new() -> MapStore {
quarkUnsubmitted
Not Done

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

quark: Maybe `Boxed<[u8]>` is better than `Vec<u8>` here to indicate the content is immutable.
mbthomasAuthorUnsubmitted
Not Done

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

mbthomas: It's for test code only, so I'm ok with it being mutable (a future test may want to do some…
// Initial ID is set to 24 to simulate a header.
MapStore {
next_id: 24,
data: HashMap::new(),
}
}
}
impl Store for MapStore {
fn append(&mut self, data: &[u8]) -> Result<BlockId> {
let id = self.next_id;
self.data.insert(id, data.to_vec());
self.next_id += data.len();
Ok(id)
}
fn flush(&mut self) -> Result<()> {
Ok(())
}
}
impl StoreView for MapStore {
fn read<'a>(&'a self, id: BlockId) -> Result<Cow<'a, [u8]>> {
match self.data.get(&id) {
Some(data) => Ok(Cow::from(data.as_slice())),
None => bail!(ErrorKind::InvalidStoreId(id)),
}
}
}
#[test]
fn basic_test() {
let mut ms = MapStore::new();
let key1 = ms.append("12345".as_bytes()).expect("append key1");
let key2 = ms.append("67890".as_bytes()).expect("append key2");
ms.flush().expect("flush");
assert_eq!(ms.read(key2).unwrap(), "67890".as_bytes());
assert_eq!(ms.read(key1).unwrap(), "12345".as_bytes());
assert_eq!(
ms.read(999).unwrap_err().to_string(),
"invalid store id: 999"
);
}
}