kevincox added inline comments. INLINE COMMENTS
> dirstate_map.rs:47 > + > +impl Default for DirstateMap { > + fn default() -> Self { Can you just `#[derive(Default)]`? > dirstate_map.rs:95 > + filename: &[u8], > + old_state: u8, > + entry: DirstateEntry, This should probably be replaced with an enum. You can implement `TryFrom<u8>` for ease of conversion from python. > dirstate_map.rs:227 > + { > + if *state != b'n' || *mtime == -1 { > + non_normal.insert(filename.to_owned()); It would probably be good to move these sential values into well-named constants. That or add comments explaining what they mean in each case. > dirstate_map.rs:237 > + } > + pub fn set_all_dirs(&mut self) -> () { > + if self.all_dirs.is_none() { Drop the `-> ()` > dirstate_map.rs:245 > + } > + pub fn set_dirs(&mut self) -> () { > + if self.dirs.is_none() { This function seems quite odd. Why isn't dirs always set? It seems like you do some initialization then set it. However does this mean that this class has a different set of allowed methods before and after dirs is set? If this is the case you should either A) Add assertions at the top of every method to ensure they are only called at allowed times or B) split out the initialization into a builder type. Otherwise why can't `dirs` be set at initialization. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D6594/new/ REVISION DETAIL https://phab.mercurial-scm.org/D6594 To: Alphare, #hg-reviewers Cc: durin42, kevincox, mjpieters, mercurial-devel _______________________________________________ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel