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).

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?

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.

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.

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?

rust/treedirstate/src/dirstate.rs
44	In the test code I use a simplified `State` class, but no, it's not really necessary.

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?

rust/treedirstate/src/dirstate.rs
44	I've added D1512 which removed the type parameter on `Dirstate`.

			Path	Packages
A	M		rust/treedirstate/src/dirstate.rs (282 lines)
M			rust/treedirstate/src/lib.rs (1 line)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	3480		Nov 14 2017, 12:39 PM	★	★
Diff 2	3597		Nov 17 2017, 9:45 AM	★	★
Diff 3	3652		Nov 20 2017, 12:20 PM	★	★
Diff 4	3727		Nov 21 2017, 1:35 PM	★	★
Diff 5	3852		Nov 24 2017, 3:35 PM	★	★
Diff 6	3929	rFBHGX3d05c6c9da72cb2a0fdc2cf0a9b646b978ce762e	Nov 28 2017, 7:51 AM	★	★

Status	Author	Revision
Closed	mbthomas	D1528 treedirstate: prevent interference with other dirstate implementations
Closed	mbthomas	D1512 treedirstate: de-genericize Dirstate
Closed	mbthomas	D1510 treedirstate: extract serialization methods to separate module
Closed	mbthomas	D1490 treedirstate: add integration tests
Closed	mbthomas	D1434 treedirstate: add configuration options
Closed	mbthomas	D1412 setup: build treedirstate and rusttreedirstate packages
Closed	mbthomas	D1411 distutils_rust: add distutils extension to compile Rust extension modules
Closed	mbthomas	D1410 perftweaks: support treedirstate maps
Closed	mbthomas	D1409 treedirstate: implement casefolding maps for case insensitive filesystems
Closed	mbthomas	D1408 treedirstate: use vlqencoding for numbers
Closed	mbthomas	D1407 treedirstate: auto-repack treedirstate once it reaches 3x its original size
Closed	mbthomas	D1406 treedirstate: implement efficient case collision detection
Closed	mbthomas	D1405 treedirstate: allow absent non-normal sets
Closed	mbthomas	D1404 treedirstate: clear ambiguous times when writing the dirstate
Closed	mbthomas	D1403 treedirstate: better iteration using visitor pattern
Closed	mbthomas	D1402 treedirstate: cache dirstate data when iterating all files
Closed	mbthomas	D1401 treedirstate: add Python linkage
Closed	mbthomas	D1400 treedirstate: add Dirstate
Closed	mbthomas	D1399 treedirstate: add Tree
Closed	mbthomas	D1398 treedirstate: add FileStore
Closed	mbthomas	D1397 treedirstate: add Store and StoreView traits
Closed	mbthomas	D1396 treedirstate: add vecmap implementation
Closed	mbthomas	D1395 treedirstate: create empty Rust project

Diff 3929

rust/treedirstate/src/dirstate.rs

This file was added.

				// Copyright Facebook, Inc. 2017
				//! Directory State.

				use byteorder::{BigEndian, ReadBytesExt, WriteBytesExt};
				use errors::*;
				use filestore::FileStore;
				use std::io::{Cursor, Read, Write};
				use std::path::Path;
				use store::{BlockId, NullStore, Store, StoreView};
				use tree::{Key, KeyRef, Storable, Tree};

				/// Marker indicating that a block is probably a root node.
				const MAGIC: &[u8] = b"////";
				const MAGIC_LEN: usize = 4;

				simonfarUnsubmitted Not Done 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). simonfar: Rust arrays have their size as part of the type. Use `MAGIC.len()` when you need the length…
				mbthomasAuthorUnsubmitted Not Done 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: I need the length for line 77 where I'm creating a buffer to read from disk, and without const…
				/// Selected backend implementation for the dirstate.
				enum Backend {
				/// The dirstate is not currently backed by a file.
				Empty(NullStore),

				/// The dirstate is backed by a file on disk.
				File(FileStore),
				}

				impl Backend {
				pub fn store<'a>(&'a mut self) -> &'a mut Store {
				match *self {
				Backend::Empty(ref mut _null) => panic!("attempt to write to uninitialized dirstate"),
				Backend::File(ref mut file) => file,
				}
				}

				pub fn store_view<'a>(&'a self) -> &'a StoreView {
				match *self {
				Backend::Empty(ref null) => null,
				Backend::File(ref file) => file,
				}
				}
				}

				/// A dirstate object. This contains the state of all files in the dirstate, stored in tree
				/// structures, and backed by an append-only store on disk.
				pub struct Dirstate<T> {
				/// The store currently in use by the Dirstate.
				jsgfUnsubmitted Not Done 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`? jsgf: Does it make sense for this to be generic over the per-file info at the point you're committing…
				mbthomasAuthorUnsubmitted Not Done In the test code I use a simplified `State` class, but no, it's not really necessary. mbthomas: In the test code I use a simplified `State` class, but no, it's not really necessary.
				mbthomasAuthorUnsubmitted Not Done I've added D1512 which removed the type parameter on `Dirstate`. mbthomas: I've added D1512 which removed the type parameter on `Dirstate`.
				store: Backend,

				/// The tree of tracked files.
				tracked: Tree<T>,

				/// The tree of removed files.
				removed: Tree<T>,

				/// The ID of the root block.
				root_id: Option<BlockId>,
				}

				impl<T: Storable + Clone> Dirstate<T> {
				/// Create a new, empty dirstate, with no backend store.
				pub fn new() -> Dirstate<T> {
				Dirstate {
				store: Backend::Empty(NullStore::new()),
				tracked: Tree::new(),
				removed: Tree::new(),
				root_id: None,
				}
				}

				/// Open an existing dirstate file. The entries in the file will not be materialized from
				/// the disk until they are accessed.
				pub fn open<P: AsRef<Path>>(&mut self, filename: P, root_id: BlockId) -> Result<()> {
				let store = FileStore::open(filename)?;
				{
				let root_data = store.read(root_id)?;
				let mut root = Cursor::new(root_data);

				// Sanity check that this is a root
				let mut buffer = [0; MAGIC_LEN];
				root.read_exact(&mut buffer)?;
				if buffer != MAGIC {
				bail!(ErrorKind::InvalidStoreId(root_id.0));
				}

				let tracked_root_id = BlockId(root.read_u64::<BigEndian>()?);
				let tracked_file_count = root.read_u32::<BigEndian>()?;
				let removed_root_id = BlockId(root.read_u64::<BigEndian>()?);
				let removed_file_count = root.read_u32::<BigEndian>()?;
				self.tracked = Tree::open(tracked_root_id, tracked_file_count);
				jsgfUnsubmitted Not Done 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). jsgf: I think all this low-level serde stuff should be confined to types with implementations of…
				self.removed = Tree::open(removed_root_id, removed_file_count);
				}
				self.store = Backend::File(store);
				durhamUnsubmitted Not Done 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? durham: Rust noob question: Is the extra scoping here is needed because root_data has a lifetime…
				mbthomasAuthorUnsubmitted Not Done Correct. Hopefully non-lexical-lifetimes will remove the need for this extra scope. mbthomas: Correct. Hopefully non-lexical-lifetimes will remove the need for this extra scope.
				self.root_id = Some(root_id);
				Ok(())
				}

				/// Write a new root block to the store. This contains the identities of the tree roots
				/// and the tree sizes.
				fn write_root(&mut self) -> Result<()> {
				let store = self.store.store();
				let mut data = Vec::new();
				data.write(MAGIC)?;
				data.write_u64::<BigEndian>(self.tracked.root_id().unwrap().0)?;
				data.write_u32::<BigEndian>(self.tracked.file_count())?;
				data.write_u64::<BigEndian>(self.removed.root_id().unwrap().0)?;
				data.write_u32::<BigEndian>(self.removed.file_count())?;
				self.root_id = Some(store.append(&data)?);
				store.flush()?;
				Ok(())
				}

				/// Write a full copy of the dirstate out to a new file.
				pub fn write_full<P: AsRef<Path>>(&mut self, filename: P) -> Result<()> {
				{
				let mut store = FileStore::create(filename)?;
				{
				let old_store = self.store.store_view();
				self.tracked.write_full(&mut store, old_store)?;
				self.removed.write_full(&mut store, old_store)?;
				}
				self.store = Backend::File(store);
				}
				self.write_root()
				}
				jsgfUnsubmitted Not Done Would it be better to leave the old store in place if writing the new one fails? jsgf: Would it be better to leave the old store in place if writing the new one fails?
				mbthomasAuthorUnsubmitted Not Done 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: When writing full copies of the tree out, we update the tree with IDs from the new store. If…

				/// Write updated entries in the dirstate to the store.
				pub fn write_delta(&mut self) -> Result<()> {
				{
				let store = self.store.store();
				self.tracked.write_delta(store)?;
				self.removed.write_delta(store)?;
				}
				self.write_root()
				}

				/// Clears all entries from the dirstate.
				pub fn clear(&mut self) {
				self.tracked.clear();
				self.removed.clear();
				}

				/// Returns the ID of the root block.
				pub fn root_id(&self) -> Option<BlockId> {
				self.root_id
				}

				/// Add or update a file entry in the dirstate.
				pub fn add_file(&mut self, name: KeyRef, state: &T) -> Result<()> {
				let store = self.store.store_view();
				self.removed.remove(store, name)?;
				self.tracked.add(store, name, state)?;
				Ok(())
				}

				/// Mark a file as removed in the dirstate.
				pub fn remove_file(&mut self, name: KeyRef, state: &T) -> Result<()> {
				let store = self.store.store_view();
				durhamUnsubmitted Not Done What does it mean to pass state to the remove function? Do we actually access state stored on the removed tree? durham: What does it mean to pass state to the remove function? Do we actually access state stored on…
				mbthomasAuthorUnsubmitted Not Done 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. mbthomas: Yes, Mercurial uses the `size` field on removed entries to encode some state that it needs to…
				self.tracked.remove(store, name)?;
				self.removed.add(store, name, state)?;
				Ok(())
				durhamUnsubmitted Not Done Why &state here but just state above in add_file? durham: Why &state here but just state above in add_file?
				mbthomasAuthorUnsubmitted Not Done The `&` is unnecessary here as `state` is already a reference. I'll remove it. mbthomas: The `&` is unnecessary here as `state` is already a reference. I'll remove it.
				}

				/// Drop a file from the dirstate.
				pub fn drop_file(&mut self, name: KeyRef) -> Result<bool> {
				let store = self.store.store_view();
				let tracked = self.tracked.remove(store, name)?;
				let removed = self.removed.remove(store, name)?;
				Ok(tracked \|\| removed)
				}

				pub fn tracked_count(&self) -> u32 {
				self.tracked.file_count()
				}

				pub fn removed_count(&self) -> u32 {
				self.removed.file_count()
				}

				/// Get an entry from the tracked tree.
				pub fn get_tracked<'a>(&'a mut self, name: KeyRef) -> Result<Option<&'a T>> {
				self.tracked.get(self.store.store_view(), name)
				}

				/// Get the name and state of the first file in the tracked tree.
				pub fn get_first_tracked<'a>(&'a mut self) -> Result<Option<(Key, &'a T)>> {
				self.tracked.get_first(self.store.store_view())
				}

				/// Get the name and state of the next file in the tracked tree after the named file.
				pub fn get_next_tracked<'a>(&'a mut self, name: KeyRef) -> Result<Option<(Key, &'a T)>> {
				self.tracked.get_next(self.store.store_view(), name)
				}

				pub fn has_tracked_dir(&mut self, name: KeyRef) -> Result<bool> {
				self.tracked.has_dir(self.store.store_view(), name)
				}

				/// Get an entry from the removed tree.
				pub fn get_removed<'a>(&'a mut self, name: KeyRef) -> Result<Option<&'a T>> {
				self.removed.get(self.store.store_view(), name)
				}

				/// Get the name and state of the first file in the removed tree.
				pub fn get_first_removed<'a>(&'a mut self) -> Result<Option<(Key, &'a T)>> {
				self.removed.get_first(self.store.store_view())
				}

				/// Get the name and state of the next file in the removed tree after the named file.
				pub fn get_next_removed<'a>(&'a mut self, name: KeyRef) -> Result<Option<(Key, &'a T)>> {
				self.removed.get_next(self.store.store_view(), name)
				}

				pub fn has_removed_dir(&mut self, name: KeyRef) -> Result<bool> {
				self.removed.has_dir(self.store.store_view(), name)
				}
				}

				#[cfg(test)]
				mod tests {
				use dirstate::Dirstate;
				use tempdir::TempDir;
				use tree::Storable;
				use std::io::{Read, Write};
				use byteorder::{ReadBytesExt, WriteBytesExt};
				use errors::*;

				#[derive(PartialEq, Clone, Debug)]
				struct State(char);

				impl Storable for State {
				fn write(&self, w: &mut Write) -> Result<()> {
				w.write_u8(self.0 as u8)?;
				Ok(())
				}

				fn read(r: &mut Read) -> Result<State> {
				Ok(State(r.read_u8()? as char))
				}
				}

				#[test]
				fn goodpath() {
				let dir = TempDir::new("dirstate_test").expect("create temp dir");
				let p = dir.path().join("store");
				let mut ds = Dirstate::<State>::new();
				ds.write_full(&p).expect("can write full empty dirstate");
				ds.add_file(b"dirA/file1", &State('n')).expect("can add");
				ds.remove_file(b"dirA/file2", &State('r'))
				.expect("can remove");
				ds.write_delta().expect("can write delta");
				ds.add_file(b"dirA/file2", &State('n')).expect("can add");
				ds.remove_file(b"dirA/file1", &State('r'))
				.expect("can remove");
				ds.write_delta().expect("can write delta");
				let ds_root = ds.root_id().unwrap();
				drop(ds);
				let mut ds2 = Dirstate::<State>::new();
				ds2.open(&p, ds_root).expect("can re-open");
				ds2.add_file(b"dirB/file3", &State('m')).expect("can add");
				ds2.remove_file(b"dirC/file4", &State('r'))
				.expect("can remove");
				assert_eq!(ds2.get_tracked(b"dirA/file1").expect("can get"), None);
				assert_eq!(
				ds2.get_tracked(b"dirA/file2").expect("can get"),
				Some(&State('n'))
				);
				assert_eq!(
				ds2.get_removed(b"dirA/file1").expect("can get"),
				Some(&State('r'))
				);
				assert_eq!(ds2.get_removed(b"dirA/file2").expect("can get"), None);
				assert_eq!(ds2.tracked_count(), 2);
				assert_eq!(ds2.removed_count(), 2);
				ds2.drop_file(b"dirA/file1").expect("can drop");
				ds2.drop_file(b"dirA/file2").expect("can drop");
				ds2.write_delta().expect("can write delta");
				assert_eq!(ds2.tracked_count(), 1);
				assert_eq!(ds2.removed_count(), 1);
				ds2.clear();
				ds2.write_delta().expect("can write delta");
				assert_eq!(ds2.tracked_count(), 0);
				assert_eq!(ds2.removed_count(), 0);
				}
				}

rust/treedirstate/src/lib.rs


	#[cfg(test)]			#[cfg(test)]
	#[macro_use]			#[macro_use]
	extern crate quickcheck;			extern crate quickcheck;

	#[cfg(test)]			#[cfg(test)]
	extern crate tempdir;			extern crate tempdir;

				pub mod dirstate;
	pub mod errors;			pub mod errors;
	pub mod filestate;			pub mod filestate;
	pub mod filestore;			pub mod filestore;
	pub mod store;			pub mod store;
	pub mod tree;			pub mod tree;
	pub mod vecmap;			pub mod vecmap;

	pub use errors::*;			pub use errors::*;

This is an archive of the discontinued Mercurial Phabricator instance.

treedirstate: add Dirstate
ClosedPublic

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 3929

rust/treedirstate/src/dirstate.rs

rust/treedirstate/src/lib.rs

This is an archive of the discontinued Mercurial Phabricator instance.

treedirstate: add DirstateClosedPublic

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 3929

rust/treedirstate/src/dirstate.rs

rust/treedirstate/src/lib.rs

treedirstate: add Dirstate
ClosedPublic

Revision Contents
Changeset List