Re: [PATCH v2 3/6] fsmonitor: teach git to optionally utilize a file system monitor to speed up detecting new or changed files.

From: Christian Couder <christian.couder@gmail.com>
Date: May 24, 2017, 12:30 UTC
Message-ID: <CAP8UFD0Q6kpsOBWWtt=2OV0MZvKayBQxVk9H1EfTCOuUaB-onw@mail.gmail.com>
In-Reply-To: <20170518201333.13088-4-benpeart@microsoft.com>

On Thu, May 18, 2017 at 10:13 PM, Ben Peart <peartben@gmail.com> wrote:

> When the index is read from disk, the query-fsmonitor index extension is
> used to flag the last known potentially dirty index and untracked cach

s/cach/cache/

> entries.

[...]

Show 9 quoted lines

> diff --git a/cache.h b/cache.h
> index 188811920c..36423c77cc 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -201,6 +201,7 @@ struct cache_entry {
>  #define CE_ADDED             (1 << 19)
>
>  #define CE_HASHED            (1 << 20)
> +#define CE_FSMONITOR_DIRTY   (1 << 21)

It looks like the (1 << 21) value was used before (as CE_UNHASHED) and was removed in:

419a597f64 (name-hash.c: remove cache entries instead of marking them CE_UNHASHED, 2013-11-14)

I wondered if using this value again could make old and new versions of git interact badly, but it looks like these are in memory only flags, so it should be ok.

Show 11 quoted lines

>  #define CE_WT_REMOVE         (1 << 22) /* remove in work directory */
>  #define CE_CONFLICTED        (1 << 23)
>
>  struct split_index;
>  struct untracked_cache;
> @@ -342,6 +344,8 @@ struct index_state {
>         struct hashmap dir_hash;
>         unsigned char sha1[20];
>         struct untracked_cache *untracked;
> +       time_t fsmonitor_last_update;
> +       struct ewah_bitmap *fsmonitor_dirty_bitmap;

Maybe you could remove "_bitmap" at the end of the name.

>  };

Show 16 quoted lines

> diff --git a/dir.c b/dir.c
> index 1b5558fdf9..da428489e2 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1652,6 +1652,18 @@ static int valid_cached_dir(struct dir_struct *dir,
>         if (!untracked)
>                 return 0;
>
> +       refresh_by_fsmonitor(&the_index);
> +       if (dir->untracked->use_fsmonitor) {
> +               /*
> +                * With fsmonitor, we can trust the untracked cache's
> +                * valid field.
> +                */
> +               if (untracked->valid)
> +                       goto skip_stat;

Maybe you could avoid this goto using a valid_cached_dir_after_stat() function that would do what is after the "skip_stat:" label below?

Show 15 quoted lines

> +               else
> +                       invalidate_directory(dir->untracked, untracked);
> +       }
> +
>         if (stat(path->len ? path->buf : ".", &st)) {
>                 invalidate_directory(dir->untracked, untracked);
>                 memset(&untracked->stat_data, 0, sizeof(untracked->stat_data));
> @@ -1665,6 +1677,7 @@ static int valid_cached_dir(struct dir_struct *dir,
>                 return 0;
>         }
>
> +skip_stat:
>         if (untracked->check_only != !!check_only) {
>                 invalidate_directory(dir->untracked, untracked);
>                 return 0;

[...]

Show 24 quoted lines

> +void refresh_by_fsmonitor(struct index_state *istate)
> +{
> +       static has_run_once = FALSE;
> +       struct strbuf query_result = STRBUF_INIT;
> +       int query_success = 0;
> +       size_t bol = 0; /* beginning of line */
> +       time_t last_update;
> +       char *buf, *entry;
> +       int i;
> +
> +       if (!core_fsmonitor || has_run_once)
> +               return;
> +       has_run_once = TRUE;
> +
> +       /*
> +        * This could be racy so save the date/time now and the hook
> +        * should be inclusive to ensure we don't miss potential changes.
> +        */
> +       last_update = time(NULL);
> +
> +       /* If we have a last update time, call query-monitor for the set of changes since that time */
> +       if (istate->fsmonitor_last_update) {
> +               query_success = !query_fsmonitor(istate->fsmonitor_last_update, &query_result);
> +       }

Braces can be removed.

Show 28 quoted lines

> +       if (query_success) {
> +               /* Mark all entries returned by the monitor as dirty */
> +               buf = entry = query_result.buf;
> +               for (i = 0; i < query_result.len; i++) {
> +                       if (buf[i] != '\0')
> +                               continue;
> +                       mark_file_dirty(istate, buf + bol);
> +                       bol = i + 1;
> +               }
> +               if (bol < query_result.len)
> +                       mark_file_dirty(istate, buf + bol);
> +
> +               /* Mark all clean entries up-to-date */
> +               for (i = 0; i < istate->cache_nr; i++) {
> +                       struct cache_entry *ce = istate->cache[i];
> +                       if (ce_stage(ce) || (ce->ce_flags & CE_FSMONITOR_DIRTY))
> +                               continue;
> +                       ce_mark_uptodate(ce);
> +               }
> +
> +               /*
> +                * Now that we've marked the invalid entries in the
> +                * untracked-cache itself, we can mark the untracked cache for
> +                * fsmonitor usage.
> +                */
> +               if (istate->untracked) {
> +                       istate->untracked->use_fsmonitor = 1;
> +               }

Braces can be removed.

Show 16 quoted lines

> +       }
> +       else {
> +               /* if we can't update the cache, fall back to checking them all */
> +               for (i = 0; i < istate->cache_nr; i++)
> +                       istate->cache[i]->ce_flags |= CE_FSMONITOR_DIRTY;
> +
> +               /* mark the untracked cache as unusable for fsmonitor */
> +               if (istate->untracked)
> +                       istate->untracked->use_fsmonitor = 0;
> +       }
> +       strbuf_release(&query_result);
> +
> +       /* Now that we've updated istate, save the last_update time */
> +       istate->fsmonitor_last_update = last_update;
> +       istate->cache_changed |= FSMONITOR_CHANGED;
> +}