D6594: RFC dirstatemap

kevincox (Kevin Cox) phabricator at mercurial-scm.org
Thu Jul 4 11:19:28 EDT 2019


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


More information about the Mercurial-devel mailing list