[Commented On] D10826: dirstate-v2: Skip readdir in status based on directory mtime

[Alphare (Raphaël Gomès)]

Alphare added a comment.


  I have mostly feeback on form, here for posterity and for your follow-up patches.

INLINE COMMENTS

> status.rs:158
> +                    if current_mtime == cached_mtime.into() {
> +                        // The mtime of that directory has not changed since
> +                        // then, which means that the

I would point to the ambiguity resolution due to insufficient granularity in this comment so that we don't get the impression that this is naive too fast.

> status.rs:372
> +    ) -> Result<(), DirstateV2ParseError> {
> +        if !directory_has_any_fs_only_entry {
> +            // All filesystem directory entries from `read_dir` have a

I find an early return (for the true case) would be easier to read since it would lead to less indentation.

> status.rs:680
> +/// date/time to be unknown.
> +fn filesystem_now(repo_root: &Path) -> Result<SystemTime, io::Error> {
> +    tempfile::tempfile_in(repo_root.join(".hg"))?

This should probably `log::debug` that it failed to write a tempfile?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D10826/new/

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

To: SimonSapin, #hg-reviewers
Cc: Alphare, mercurial-patches
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mercurial-scm.org/pipermail/mercurial-patches/attachments/20210602/ac53c9d7/attachment-0002.html>