Re: [PATCH 3/3] Write index when populating empty untracked cache

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

"Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:

> Subject: Re: [PATCH 3/3] Write index when populating empty untracked cache

Common to all three patches.  

Look at "git shortlog --no-merges -200 master" and observe the
pattern, i.e. our commit title are <area> prefix, a colon and then a
summary.  Try to make your commits blend in.

> From: Tao Klerks <tao@klerks.biz>
>
> It is expected that an empty/unpopulated untracked
> cache structure can be written to the index - by update-
> index, or by a "git status" call that sees the untracked cache
> should be enabled, but is running with options that make
> it non-applicable in that run.

Would an example be helpful?  What requests the untracked cache to
be created, and what options (or perhaps the lack of what options)
prevent the untracked cache to be "non-applicable"?

> Currently, if that happens, then subsequent "git status"
> calls end up populating the untracked cache, but not
> writing the index (not saving their work) - so the
> performance outcome is almost identical to the cache
> being altogether disabled.
>
> This continues until the index gets written with the cache
> populated, for some *other* reason.

Here (and only here), the word "cache" is used instead of "untracked
cache"---but I suspect that "the cache" here is the same "untracked
cache".  If that is the case, being consistent and spelling it out
would reduce the risk of confusing readers.

> In this change, we detect the "existing cache is empty
> and it looks like we are using it" condition, and queues
> an index write when this happens.

"we detect ... and queues" sounds like a grammo.

In this project, we describe the change being proposed as if we are
giving an order to the codebase to "become like so".  So, perhaps

    Detect the condition where an empty untracked cache exists in
    the index and we collect the list of untracked paths, and queue
    an index write under that condition, so that the collected
    untracked paths can be written out to the untracked cache
    extension in the index.

or something along the line.

> This change depends on previous fixes to t7519 for the
> "ignore .git changes when invalidating UNTR" test case to
> pass - before this fix, the test never actually did anything
> as it was not set up correctly.
>
> Signed-off-by: Tao Klerks <tao@klerks.biz>
> ---
>  dir.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/dir.c b/dir.c
> index ebe5ec046e0..a326e40e1c1 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -2703,7 +2703,8 @@ void remove_untracked_cache(struct index_state *istate)
>  
>  static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *dir,
>  						      int base_len,
> -						      const struct pathspec *pathspec)
> +						      const struct pathspec *pathspec,
> +						      struct index_state *istate)
>  {
>  	struct untracked_cache_dir *root;
>  	static int untracked_cache_disabled = -1;
> @@ -2767,8 +2768,15 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d
>  		return NULL;
>  	}
>  
> -	if (!dir->untracked->root)
> +	if (!dir->untracked->root) {
>  		FLEX_ALLOC_STR(dir->untracked->root, name, "");
> +		/*
> +		 * If we've had to initialize the root, then what we had was an
> +		 * empty uninitialized untracked cache structure. We will be
> +		 * populating it now, so we should trigger an index write.
> +		 */
> +		istate->cache_changed |= UNTRACKED_CHANGED;
> +	}

The logic sounds fairly straight-forward.  The fact that we came
this far in the helper function means that we know we want to use
(i.e. untracked cache is not disabled, and various other "we
shouldn't be contaminating the cache with the one-shot information"
cases did not return from the helper) untracked cache, and root
being NULL originally means the untracked cache wasn't populated.

>  	/* Validate $GIT_DIR/info/exclude and core.excludesfile */
>  	root = dir->untracked->root;
> @@ -2838,7 +2846,7 @@ int read_directory(struct dir_struct *dir, struct index_state *istate,
>  		return dir->nr;
>  	}
>  
> -	untracked = validate_untracked_cache(dir, len, pathspec);
> +	untracked = validate_untracked_cache(dir, len, pathspec, istate);
>  	if (!untracked)
>  		/*
>  		 * make sure untracked cache code path is disabled,