[v4,01/29] fsmonitor: enhance existing comments

Commit Message

Jeff Hostetler Oct. 21, 2021, 2:24 p.m. UTC

From: Jeff Hostetler <[e-mail]>

Signed-off-by: Jeff Hostetler <[e-mail]>
---
 fsmonitor.c | 37 ++++++++++++++++++++++++++++++-------
 1 file changed, 30 insertions(+), 7 deletions(-)

Comments

Junio C Hamano Oct. 21, 2021, 8:40 p.m. UTC | #1

"Jeff Hostetler via GitGitGadget" <[e-mail]> writes:

> From: Jeff Hostetler <[e-mail]>
>
> Signed-off-by: Jeff Hostetler <[e-mail]>
> ---
>  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;

Jeff Hostetler Oct. 27, 2021, 6:46 p.m. UTC | #2

On 10/21/21 4:40 PM, Junio C Hamano wrote:
> "Jeff Hostetler via GitGitGadget" <[e-mail]> writes:
> 
>> From: Jeff Hostetler <[e-mail]>
>>
>> Signed-off-by: Jeff Hostetler <[e-mail]>
>> ---
>>   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.
> 

Yes, there is an opportunity here to clean this up.

The original code was a little sloppy -- just testing buf[bol] for
slash and assuming that that was sufficient.  Technically, no response
should begin with slash (since the set of reported modified files are
relative to the root directory), so my _is_trivial_ function could be
simplified a little.

I'll refactor this.

Thanks
Jeff

Patch

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