Re: [PATCH v4 01/29] fsmonitor: enhance existing comments

[Junio C Hamano <gitster@pobox.com>]

"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
>  fsmonitor.c | 37 ++++++++++++++++++++++++++++++-------
>  1 file changed, 30 insertions(+), 7 deletions(-)
>
> diff --git a/fsmonitor.c b/fsmonitor.c
> index ab9bfc60b34..ec4c46407c5 100644
> --- a/fsmonitor.c
> +++ b/fsmonitor.c
> @@ -301,9 +301,25 @@ void refresh_fsmonitor(struct index_state *istate)
>  			core_fsmonitor, query_success ? "success" : "failure");
>  	}
>  
> -	/* a fsmonitor process can return '/' to indicate all entries are invalid */
> +	/*
> +	 * The response from FSMonitor (excluding the header token) is
> +	 * either:
> +	 *
> +	 * [a] a (possibly empty) list of NUL delimited relative
> +	 *     pathnames of changed paths.  This list can contain
> +	 *     files and directories.  Directories have a trailing
> +	 *     slash.
> +	 *
> +	 * [b] a single '/' to indicate the provider had no
> +	 *     information and that we should consider everything
> +	 *     invalid.  We call this a trivial response.
> +	 */
>  	if (query_success && query_result.buf[bol] != '/') {
> -		/* Mark all entries returned by the monitor as dirty */
> +		/*
> +		 * Mark all pathnames returned by the monitor as dirty.
> +		 *
> +		 * This updates both the cache-entries and the untracked-cache.
> +		 */

Not a problem this patch introduces, but we only checked that the
query result begins with a slash, not "we did receive a trivial
response", but the "else" clause of this statement pretends as if we
did.

It is a shame that we do have fsmonitor_is_trivial_response()
function defined, but its interface is not capable of helping us
here.

Or is fsmonitor_is_trivial_response() already good to do this, and
reliance of [bol] this code has is the source of confusion?  I
notice that when we have last update token and makes a call to
query_fsmonitor() with HOOK_INTERFACE_VERSION1, nobody updates bol
(hence it stays 0), and with HOOK_INTERFACE_VERSION2, bol is at the
NUL that terminates the initial string of the query result, after
which presumably has either '/' NUL (trivial) or list of paths.

I am not sure about the VERSION1 case, but at least in the VERSION2
case, making sure that the last three bytes are NUL slash NUL like
fsmonitor_is_trivial_response() does and the half check the above is
doing (i.e. the byte after the NUL is slash, without making sure
about the length of the whole response or what follows the slash is
NUL), seems "close enough" (meaning: the check in this code is a
sloppy attempt to reinvent what _is_trivial_response() function
already does).

So, would it make sense to rewrite the condition to

	if (query_success &&
	    !fsmonitor_is_trivial_response(&query_result)) {

here?  Or perhaps

	if (query_success &&
	    !(query_result.len == bol + 3 &&
	      query_result[bol] == '/' && !query_result[bol+1])) {

which would be open coding the _is_trivial_response() function.

>  		buf = query_result.buf;
>  		for (i = bol; i < query_result.len; i++) {
>  			if (buf[i] != '\0')
> @@ -318,11 +334,15 @@ void refresh_fsmonitor(struct index_state *istate)
>  		if (istate->untracked)
>  			istate->untracked->use_fsmonitor = 1;
>  	} else {
> -
> -		/* We only want to run the post index changed hook if we've actually changed entries, so keep track
> -		 * if we actually changed entries or not */
> +		/*
> +		 * We received a trivial response, so invalidate everything.
> +		 *


> +		 * We only want to run the post index changed hook if
> +		 * we've actually changed entries, so keep track if we
> +		 * actually changed entries or not.
> +		 */
>  		int is_cache_changed = 0;
> -		/* Mark all entries invalid */
> +
>  		for (i = 0; i < istate->cache_nr; i++) {
>  			if (istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) {
>  				is_cache_changed = 1;
> @@ -330,7 +350,10 @@ void refresh_fsmonitor(struct index_state *istate)
>  			}
>  		}
>  
> -		/* If we're going to check every file, ensure we save the results */
> +		/*
> +		 * If we're going to check every file, ensure we save
> +		 * the results.
> +		 */
>  		if (is_cache_changed)
>  			istate->cache_changed |= FSMONITOR_CHANGED;