Fix bugs preventing adding updated cache entries to the name hash

PATCH In review Ben Peart 3 participants 4 messages Mar 15, 2018

Share & tools

Ben Peart

author

2018-03-15 15:25 UTC #0 lore raw

PATCH 1/1 Fix bugs preventing adding updated cache entries to the name hash

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()

Fix refresh_cache_ent() to use the copy_cache_entry() macro instead of memcpy() so that it doesn't incorrectly copy the hash state from the old entry.

Signed-off-by Ben Peart <[email protected]>

---

Notes:

    Base Ref: master
    Web-Diff: https://github.com/benpeart/git/commit/079be4ac07
    Checkout: git fetch https://github.com/benpeart/git ce_hashed-v1 && git checkout 079be4ac07

 read-cache.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

read-cache.c +3 -1

index 977921d90c..bdfa552861 100644
@@ -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);

@@ -1324,7 +1325,8 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,
 
 	size = ce_size(ce);
 	updated = xmalloc(size);
-	memcpy(updated, ce, size);
+	copy_cache_entry(updated, ce);
+	memcpy(updated->name, ce->name, ce->ce_namelen + 1);
 	fill_stat_cache_info(updated, &st);
 	/*
 	 * If ignore_valid is not set, we should leave CE_VALID bit

base-commit: d0db9edba0050ada6f6eac68061599690d2a4333

Signature · 1 line

2.16.2.windows.1

Junio C Hamano

2018-03-15 17:58 UTC #1 lore raw

Quoted 2 lines · Ben Peart

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.

read-cache.c static void replace_index_entry(struct index_state *istate, int nr, struct cache · line 68

Quoted 12 lines

read-cache.c +1 -0

index 977921d90c..bdfa552861 100644
@@ -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.

Ben Peart

2018-03-15 18:44 UTC #2 lore raw

Quoted 17 lines · Junio C Hamano

Quoted 2 lines · Ben Peart
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.
Quoted 10 lines
read-cache.c +1 -0
index 977921d90c..bdfa552861 100644
@@ -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);

Quoted 9 lines

read-cache.c static void replace_index_entry(struct index_state *istate, int nr, struct cache · line 68
Quoted 2 lines
read-cache.c +0 -0
  	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.

Correct. As you note below, this one line is sufficient to fix the actual bug.

Quoted 11 lines

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?)

This 2nd part of the patch was more for code cleanliness. When I was investigating why the hashed bit was set, it was caused by this memcpy(). When I examined the rest of the code base, I only found 1 other instance (in dup_entry()) that did a straight memcpy(), the rest used the copy_cache_entry() macro. I updated this code to match that pattern as it would have prevented the bug as well though as you correctly point out, it is not necessary with the other fix.

Quoted 2 lines

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

Unfortunately I was unable to find a way to reliably demonstrate this bug and fix with a new test. I only ran across it while working on another patch series that ends up triggering it more reliably.

The symptom was that occasionally in very specific circumstances a git status call would incorrectly show some directories as having untracked files when there were actually no untracked files in that directory. A subsequent call to status would correctly show no untracked files as would git status -uall.

I do have a test for this fix as part of the other patch series but thought I'd submit this bug fix separately as it may be a while before the other patch series is ready.

Quoted 2 lines

Thanks.

Junio C Hamano

2018-03-15 18:58 UTC #3 lore raw

Quoted 1 line · Ben Peart

This 2nd part of the patch was more for code cleanliness.

Thanks.