Re: [PATCH v2 1/4] fsmonitor: change last update timestamp on the index_state to opaque token

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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
>
>