Re: [PATCH 07/12] fsmonitor: refactor untracked-cache invalidation

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 14, 2024, 16:46 UTC
Message-ID: <xmqqo7cjxad3.fsf@gitster.g>
In-Reply-To: <1df4019931c29824b174defb75e09823d604219e.1707857541.git.gitgitgadget@gmail.com>

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

Show 6 quoted lines

> From: Jeff Hostetler <jeffhostetler@github.com>
>
> Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
> ---
>  fsmonitor.c | 38 ++++++++++++++++++++++++++------------
>  1 file changed, 26 insertions(+), 12 deletions(-)

Sorry, but the proposed commit log is way lacking for this particular step. Readers have already understood, after reading steps like [04/12] and [05/12], that you use the verb "refactor" in its usual sense, i.e. reorganize the code around without changing behaviour in order to enhance readability and to make it easier for code reuse in future steps, and these two steps did exactly that: helper functions are split out of larger functions, presumably either to allow adding new callers to the helpers, or to make the result of adding more code to the caller easier to follow [*].

However, the changes in this step look vastly different, and it is not even clear if this change intends to keep the behaviour before and after it the same, or if it does, how they are the same.

I can sort-of see that the original code made a call to untracked_cache_invalidate_path() at the very end of the fsmonitor_refresh_callback(), but the updated code no longer does so. Why? Is it because it is the root cause of an unstated bug that we don't do so until the end in the current code? Is it because the order does not matter (how and why?) and the resulting code becomes better (how? simpler to follow? more performant? avoids duplicated work? something else)?

It does not help to call a new helper function with a cryptic "my_" name, either.

Please try again?  Thanks.

[Footnote]

 * These two are vastly different goals, and there may be other
   reasons why you are doing such refactoring.  It would have been
   nicer if such a preliminary refactoring steps had explained what
   the intended course of evolution for the code involved in the
   refactoring is.

Show 72 quoted lines

>
> diff --git a/fsmonitor.c b/fsmonitor.c
> index 754fe20cfd0..14585b6c516 100644
> --- a/fsmonitor.c
> +++ b/fsmonitor.c
> @@ -183,11 +183,35 @@ static int query_fsmonitor_hook(struct repository *r,
>  	return result;
>  }
>  
> +/*
> + * Invalidate the untracked cache for the given pathname.  Copy the
> + * buffer to a proper null-terminated string (since the untracked
> + * cache code does not use (buf, len) style argument).  Also strip any
> + * trailing slash.
> + */
> +static void my_invalidate_untracked_cache(
> +	struct index_state *istate, const char *name, int len)
> +{
> +	struct strbuf work_path = STRBUF_INIT;
> +
> +	if (!len)
> +		return;
> +
> +	if (name[len-1] == '/')
> +		len--;
> +
> +	strbuf_add(&work_path, name, len);
> +	untracked_cache_invalidate_path(istate, work_path.buf, 0);
> +	strbuf_release(&work_path);
> +}
> +
>  static void fsmonitor_refresh_callback_unqualified(
>  	struct index_state *istate, const char *name, int len, int pos)
>  {
>  	int i;
>  
> +	my_invalidate_untracked_cache(istate, name, len);
> +
>  	if (pos >= 0) {
>  		/*
>  		 * We have an exact match for this path and can just
> @@ -253,6 +277,8 @@ static int fsmonitor_refresh_callback_slash(
>  	int i;
>  	int nr_in_cone = 0;
>  
> +	my_invalidate_untracked_cache(istate, name, len);
> +
>  	if (pos < 0)
>  		pos = -pos - 1;
>  
> @@ -278,21 +304,9 @@ static void fsmonitor_refresh_callback(struct index_state *istate, char *name)
>  
>  	if (name[len - 1] == '/') {
>  		fsmonitor_refresh_callback_slash(istate, name, len, pos);
> -
> -		/*
> -		 * We need to remove the traling "/" from the path
> -		 * for the untracked cache.
> -		 */
> -		name[len - 1] = '\0';
>  	} else {
>  		fsmonitor_refresh_callback_unqualified(istate, name, len, pos);
>  	}
> -
> -	/*
> -	 * Mark the untracked cache dirty even if it wasn't found in the index
> -	 * as it could be a new untracked file.
> -	 */
> -	untracked_cache_invalidate_path(istate, name, 0);
>  }
>  
>  /*