Re: [PATCH v1] Fix bugs preventing adding updated cache entries to the name hash

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

Ben Peart <benpeart@microsoft.com> writes:

> Update replace_index_entry() to clear the CE_HASHED flag from the new cache
> entry so that it can add it to the name hash in set_index_entry()

OK.  

> diff --git a/read-cache.c b/read-cache.c
> index 977921d90c..bdfa552861 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -62,6 +62,7 @@ static void replace_index_entry(struct index_state *istate, int nr, struct cache
>  	replace_index_entry_in_base(istate, old, ce);
>  	remove_name_hash(istate, old);
>  	free(old);
> +	ce->ce_flags &= ~CE_HASHED;
>  	set_index_entry(istate, nr, ce);
>  	ce->ce_flags |= CE_UPDATE_IN_BASE;
>  	mark_fsmonitor_invalid(istate, ce);

As we are removing "old" that is not "ce", an earlier call to
remove_name_hash() that clears the CE_HASHED bit from the cache
entry does not help us at all.  We need to clear the bit from "ce"
ourselves before calling set_index_entry() on it, otherwise the call
would become a no-op wrt the name hash.  Makes sense.

Makes me wonder why "ce" which is a replacement for what is in the
index already has the hashed bit, though.  Is that the failure to
use copy_cache_entry() in the caller the other part of this patch
fixes?  To me it looks like copy_cache_entry() is designed for
copying an entry's data to another one that has a different name,
but in the refresh codepath, we _know_ we are replacing an old entry
with an entry with the same name, so it somehow feels a bit strange
to use copy_cache_entry(), instead of doing memcpy() (and possibly
dropping the HASHED bit from the new copy--but wouldn't that become
unnecessary with the fix to replace_index_entry() we saw above?)

Is this fix something we can demonstrate in a new test, by the way?

Thanks.