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.
> +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.
> + &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.
> + );
> + let mut i = PARENT_SIZE * 2;
It would be nice to define the above the definition of `parents` and use it in that slice.
> + let slice = &contents[i..];
> + let state = slice as i8;
This isn't a very useful name. Maybe `entry` or `entry_bytes`.
> + 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.
To: Alphare, #hg-reviewers, kevincox
Cc: durin42, kevincox, mercurial-devel
More information about the Mercurial-devel