* [PATCH v1] Fix bugs preventing adding updated cache entries to the name hash
@ 2018-03-15 15:25 Ben Peart
2018-03-15 17:58 ` Junio C Hamano
0 siblings, 1 reply; 4+ messages in thread
From: Ben Peart @ 2018-03-15 15:25 UTC (permalink / raw)
To: git; +Cc: pclouds, gitster, blees, 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()
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 <benpeart@microsoft.com>
---
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(-)
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);
@@ -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
--
2.16.2.windows.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v1] Fix bugs preventing adding updated cache entries to the name hash
2018-03-15 15:25 [PATCH v1] Fix bugs preventing adding updated cache entries to the name hash Ben Peart
@ 2018-03-15 17:58 ` Junio C Hamano
2018-03-15 18:44 ` Ben Peart
0 siblings, 1 reply; 4+ messages in thread
From: Junio C Hamano @ 2018-03-15 17:58 UTC (permalink / raw)
To: Ben Peart; +Cc: git, pclouds, blees
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v1] Fix bugs preventing adding updated cache entries to the name hash
2018-03-15 17:58 ` Junio C Hamano
@ 2018-03-15 18:44 ` Ben Peart
2018-03-15 18:58 ` Junio C Hamano
0 siblings, 1 reply; 4+ messages in thread
From: Ben Peart @ 2018-03-15 18:44 UTC (permalink / raw)
To: Junio C Hamano, Ben Peart; +Cc: git, pclouds, blees
On 3/15/2018 1:58 PM, Junio C Hamano wrote:
> 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.
>
Correct. As you note below, this one line is sufficient to fix the
actual bug.
> 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.
> 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.
> Thanks.
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v1] Fix bugs preventing adding updated cache entries to the name hash
2018-03-15 18:44 ` Ben Peart
@ 2018-03-15 18:58 ` Junio C Hamano
0 siblings, 0 replies; 4+ messages in thread
From: Junio C Hamano @ 2018-03-15 18:58 UTC (permalink / raw)
To: Ben Peart; +Cc: Ben Peart, git, pclouds, blees
Ben Peart <peartben@gmail.com> writes:
> This 2nd part of the patch was more for code cleanliness.
Thanks.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-03-15 18:58 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-15 15:25 [PATCH v1] Fix bugs preventing adding updated cache entries to the name hash Ben Peart
2018-03-15 17:58 ` Junio C Hamano
2018-03-15 18:44 ` Ben Peart
2018-03-15 18:58 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).