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

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

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

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



>
> 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);
>  }
>  
>  /*