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

[Ben Peart <peartben@gmail.com>]

On 6/27/2017 11:43 AM, Christian Couder wrote:
> On Sat, Jun 10, 2017 at 3:40 PM, Ben Peart <peartben@gmail.com> wrote:
> 
>> +int read_fsmonitor_extension(struct index_state *istate, const void *data,
>> +       unsigned long sz)
>> +{
>> +       const char *index = data;
>> +       uint32_t hdr_version;
>> +       uint32_t ewah_size;
>> +       int ret;
>> +
>> +       if (sz < sizeof(uint32_t) + sizeof(uint64_t) + sizeof(uint32_t))
>> +               return error("corrupt fsmonitor extension (too short)");
>> +
>> +       hdr_version = get_be32(index);
> 
> Here we use get_be32(), ...
> 
>> +       index += sizeof(uint32_t);
>> +       if (hdr_version != INDEX_EXTENSION_VERSION)
>> +               return error("bad fsmonitor version %d", hdr_version);
>> +
>> +       istate->fsmonitor_last_update = get_be64(index);
> 
> ...get_be64(), ...
> 
>> +       index += sizeof(uint64_t);
>> +
>> +       ewah_size = get_be32(index);
> 
> ... and get_be32 again, ...
> 
>> +       index += sizeof(uint32_t);
>> +
>> +       istate->fsmonitor_dirty = ewah_new();
>> +       ret = ewah_read_mmap(istate->fsmonitor_dirty, index, ewah_size);
>> +       if (ret != ewah_size) {
>> +               ewah_free(istate->fsmonitor_dirty);
>> +               istate->fsmonitor_dirty = NULL;
>> +               return error("failed to parse ewah bitmap reading fsmonitor index extension");
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)
>> +{
>> +       uint32_t hdr_version;
>> +       uint64_t tm;
>> +       struct ewah_bitmap *bitmap;
>> +       int i;
>> +       uint32_t ewah_start;
>> +       uint32_t ewah_size = 0;
>> +       int fixup = 0;
>> +
>> +       hdr_version = htonl(INDEX_EXTENSION_VERSION);
> 
> ... but here we use htonl() instead of put_be32(), ...
> 
>> +       strbuf_add(sb, &hdr_version, sizeof(uint32_t));
>> +
>> +       tm = htonll((uint64_t)istate->fsmonitor_last_update);
> 
> ... htonll(), ...
> 
>> +       strbuf_add(sb, &tm, sizeof(uint64_t));
>> +       fixup = sb->len;
>> +       strbuf_add(sb, &ewah_size, sizeof(uint32_t)); /* we'll fix this up later */
>> +
>> +       ewah_start = sb->len;
>> +       bitmap = ewah_new();
>> +       for (i = 0; i < istate->cache_nr; i++)
>> +               if (istate->cache[i]->ce_flags & CE_FSMONITOR_DIRTY)
>> +                       ewah_set(bitmap, i);
>> +       ewah_serialize_strbuf(bitmap, sb);
>> +       ewah_free(bitmap);
>> +
>> +       /* fix up size field */
>> +       ewah_size = htonl(sb->len - ewah_start);
> 
> ... and htonl() again.
> 
> It would be more consistent (and perhaps more correct) to use
> put_beXX() functions, instead of the htonl() and htonll() functions.

I agree that these are asymmetric.  I followed the pattern used in the 
untracked cache in which write_untracked_extension uses htonl and 
read_untracked_extension uses get_be32. I can change this to be more 
symmetric.

> 
>> +       memcpy(sb->buf + fixup, &ewah_size, sizeof(uint32_t));
>> +}
> 
>> +/*
>> + * Call the query-fsmonitor hook passing the time of the last saved results.
>> + */
>> +static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *query_result)
>> +{
>> +       struct child_process cp = CHILD_PROCESS_INIT;
>> +       char ver[64];
>> +       char date[64];
>> +       const char *argv[4];
>> +
>> +       if (!(argv[0] = find_hook("query-fsmonitor")))
>> +               return -1;
>> +
>> +       snprintf(ver, sizeof(version), "%d", version);
>> +       snprintf(date, sizeof(date), "%" PRIuMAX, (uintmax_t)last_update);
>> +       argv[1] = ver;
>> +       argv[2] = date;
>> +       argv[3] = NULL;
>> +       cp.argv = argv;
> 
> Maybe it would be nicer using the argv_array_pushX() functions.

When the number of arguments is fixed and known, I prefer avoiding the 
dynamic allocations that come along with the argv_array_pushX() functions.

> 
>> +       cp.out = -1;
>> +
>> +       return capture_command(&cp, query_result, 1024);
>> +}
>> +
>> +static void mark_file_dirty(struct index_state *istate, const char *name)
>> +{
>> +       struct untracked_cache_dir *dir;
>> +       int pos;
>> +
>> +       /* find it in the index and mark that entry as dirty */
>> +       pos = index_name_pos(istate, name, strlen(name));
>> +       if (pos >= 0) {
>> +               if (!(istate->cache[pos]->ce_flags & CE_FSMONITOR_DIRTY)) {
>> +                       istate->cache[pos]->ce_flags |= CE_FSMONITOR_DIRTY;
>> +                       istate->cache_changed |= FSMONITOR_CHANGED;
>> +               }
>> +       }
>> +
>> +       /*
>> +        * Find the corresponding directory in the untracked cache
>> +        * and mark it as invalid
>> +        */
>> +       if (!istate->untracked || !istate->untracked->root)
>> +               return;
>> +
>> +       dir = find_untracked_cache_dir(istate->untracked, istate->untracked->root, name);
>> +       if (dir) {
>> +               if (dir->valid) {
>> +                       dir->valid = 0;
>> +                       istate->cache_changed |= FSMONITOR_CHANGED;
>> +               }
>> +       }
> 
> The code above is quite similar as what is in mark_fsmonitor_dirty(),
> so I wonder if a refactoring is possible.

I've felt the same way and looked at how to refactor it better a number 
of times but never came up with a way that made it simpler, clearer and 
easier to maintain.  I'm happy to review a patch if you can figure out 
something better. :)

> 
>> +}
>> +
>> +void refresh_by_fsmonitor(struct index_state *istate)
>> +{
>> +       static int has_run_once = 0;
>> +       struct strbuf query_result = STRBUF_INIT;
>> +       int query_success = 0;
>> +       size_t bol = 0; /* beginning of line */
>> +       uint64_t last_update;
>> +       char *buf, *entry;
>> +       int i;
>> +
>> +       if (!core_fsmonitor || has_run_once)
>> +               return;
>> +       has_run_once = 1;
>> +
>> +       /*
>> +        * 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 = getnanotime();
>> +
>> +       /*
>> +        * 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(HOOK_INTERFACE_VERSION,
>> +                       istate->fsmonitor_last_update, &query_result);
>> +               trace_performance_since(last_update, "query-fsmonitor");
>> +       }
>> +
>> +       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);
> 
> It looks like bol is always equal to i here...
> 
>> +                       bol = i + 1;
>> +               }
>> +               if (bol < query_result.len)
>> +                       mark_file_dirty(istate, buf + bol);
> 
> ... and here too. As it is not used below, I wonder if you really need
> the bol variable.

bol saves the position of the beginning of the current line while i 
iterates through the remainder of the string looking for the NULL.

However, the entry variable is no longer used so I will remove it.

> 
>> +               /* 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);
>> +               }