Hi Kevin,
On Thu, 23 Jan 2020, Kevin Willford via GitGitGadget wrote:
> diff --git a/fsmonitor.c b/fsmonitor.c
> index 868cca01e2..9860587225 100644
> --- a/fsmonitor.c
> +++ b/fsmonitor.c
> @@ -89,11 +98,12 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)
> BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
> (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
>
> - put_be32(&hdr_version, INDEX_EXTENSION_VERSION);
> + put_be32(&hdr_version, INDEX_EXTENSION_VERSION2);
> strbuf_add(sb, &hdr_version, sizeof(uint32_t));
>
> - put_be64(&tm, istate->fsmonitor_last_update);
> - strbuf_add(sb, &tm, sizeof(uint64_t));
> + strbuf_addstr(sb, istate->fsmonitor_last_update);
> + strbuf_addch(sb, 0); /* Want to keep a NUL */
I have a slight preference to use '\0' here, which my brain somehow reads
as `NUL`.
> +
> fixup = sb->len;
> strbuf_add(sb, &ewah_size, sizeof(uint32_t)); /* we'll fix this up later */
>
> @@ -110,9 +120,9 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)
> }
>
> /*
> - * Call the query-fsmonitor hook passing the time of the last saved results.
> + * Call the query-fsmonitor hook passing the last update token of the saved results.
> */
> -static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *query_result)
> +static int query_fsmonitor(int version, const char *last_update, struct strbuf *query_result)
> {
> struct child_process cp = CHILD_PROCESS_INIT;
>
> @@ -121,7 +131,7 @@ static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *que
>
> argv_array_push(&cp.args, core_fsmonitor);
> argv_array_pushf(&cp.args, "%d", version);
> - argv_array_pushf(&cp.args, "%" PRIuMAX, (uintmax_t)last_update);
> + argv_array_pushf(&cp.args, "%s", last_update);
Maybe `argv_array_push(&cp.args, last_update)`?
> cp.use_shell = 1;
> cp.dir = get_git_work_tree();
>
> @@ -151,6 +161,7 @@ void refresh_fsmonitor(struct index_state *istate)
> int query_success = 0;
> size_t bol; /* beginning of line */
> uint64_t last_update;
> + struct strbuf last_update_token = STRBUF_INIT;
> char *buf;
> unsigned int i;
>
> @@ -164,6 +175,7 @@ void refresh_fsmonitor(struct index_state *istate)
> * should be inclusive to ensure we don't miss potential changes.
> */
> last_update = getnanotime();
> + strbuf_addf(&last_update_token, "%"PRIu64"", last_update);
>
> /*
> * If we have a last update time, call query_fsmonitor for the set of
> @@ -217,18 +229,21 @@ void refresh_fsmonitor(struct index_state *istate)
> }
> strbuf_release(&query_result);
>
> - /* Now that we've updated istate, save the last_update time */
> - istate->fsmonitor_last_update = last_update;
> + /* Now that we've updated istate, save the last_update_token */
> + FREE_AND_NULL(istate->fsmonitor_last_update);
> + istate->fsmonitor_last_update = strbuf_detach(&last_update_token, NULL);
I see quite a few `strbuf_detach()` calls in this patch, and could imagine
that this is a good indicator that the `fsmonitor_last_update` attribute
of `struct index_state` could be a `struct strbuf` instead, that is
`strbuf_reset()`ed and `strbuf_addf()`ed to, rather than having the
strbufs as local variables.
Other than that, this patch looks very good to me.
Thanks!
Dscho
> }
>
> void add_fsmonitor(struct index_state *istate)
> {
> unsigned int i;
> + struct strbuf last_update = STRBUF_INIT;
>
> if (!istate->fsmonitor_last_update) {
> trace_printf_key(&trace_fsmonitor, "add fsmonitor");
> istate->cache_changed |= FSMONITOR_CHANGED;
> - istate->fsmonitor_last_update = getnanotime();
> + strbuf_addf(&last_update, "%"PRIu64"", getnanotime());
> + istate->fsmonitor_last_update = strbuf_detach(&last_update, NULL);
>
> /* reset the fsmonitor state */
> for (i = 0; i < istate->cache_nr; i++)
> @@ -250,7 +265,7 @@ void remove_fsmonitor(struct index_state *istate)
> if (istate->fsmonitor_last_update) {
> trace_printf_key(&trace_fsmonitor, "remove fsmonitor");
> istate->cache_changed |= FSMONITOR_CHANGED;
> - istate->fsmonitor_last_update = 0;
> + FREE_AND_NULL(istate->fsmonitor_last_update);
> }
> }
>
> diff --git a/t/helper/test-dump-fsmonitor.c b/t/helper/test-dump-fsmonitor.c
> index 2786f47088..975f0ac890 100644
> --- a/t/helper/test-dump-fsmonitor.c
> +++ b/t/helper/test-dump-fsmonitor.c
> @@ -13,7 +13,7 @@ int cmd__dump_fsmonitor(int ac, const char **av)
> printf("no fsmonitor\n");
> return 0;
> }
> - printf("fsmonitor last update %"PRIuMAX"\n", (uintmax_t)istate->fsmonitor_last_update);
> + printf("fsmonitor last update %s\n", istate->fsmonitor_last_update);
>
> for (i = 0; i < istate->cache_nr; i++)
> printf((istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) ? "+" : "-");
> --
> gitgitgadget
>
>
