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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 15, 2018, 17:58 UTC
Message-ID: <xmqqfu515ihj.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180315152520.23264-1-benpeart@microsoft.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.

Show 12 quoted lines

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