D6348: rust-dirstate: add rust implementation of `parse_dirstate` and `pack_dirstate`

kevincox (Kevin Cox) phabricator at mercurial-scm.org
Fri May 10 11:04:41 EDT 2019


kevincox requested changes to this revision.
kevincox added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> dirstate.rs:16
> +pub type DirstateVec = Vec<(Vec<u8>, DirstateTuple)>;
> +pub type CopyMap<'a> = Vec<(&'a [u8], &'a [u8])>;
> +

You have a lot of tuples here an it is unclear what they represent. I would much prefer making structs for these, then you can give the members names which will really help the reader.

> dirstate.rs:34
> +        &contents[..PARENT_SIZE],
> +        &contents[PARENT_SIZE..PARENT_SIZE * 2],
> +    );

You check for PARENT_SIZE then index by PERENT_SIZE*2. This will panic if the size is between the two.

> dirstate.rs:36
> +    );
> +    let mut i = PARENT_SIZE * 2;
> +

It would be nice to define the above the definition of `parents` and use it in that slice.

> dirstate.rs:43
> +
> +        let slice = &contents[i..];
> +        let state = slice[0] as i8;

This isn't a very useful name. Maybe `entry` or `entry_bytes`.

> dirstate.rs:48
> +        let mtime = BigEndian::read_i32(&slice[9..]);
> +        let path_len = BigEndian::read_i32(&slice[13..]);
> +

This is a lot of manual index numbers. It would probably me more clear to wrap it in a Cursor and use https://docs.rs/byteorder/1.3.1/byteorder/trait.ReadBytesExt.html to read the values. This way the cursor is automatically moved to match the data you have read.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6348

To: Alphare, #hg-reviewers, kevincox
Cc: durin42, kevincox, mercurial-devel


More information about the Mercurial-devel mailing list