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

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