{"thread":{"id":"48048","subject":"[PATCH v1] Fix bugs preventing adding updated cache entries to the name hash","startedAt":"2018-03-15T15:25:44Z","lastAt":"2018-03-15T18:58:19Z","messageCount":4,"participants":["Ben Peart","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"341750","messageId":"20180315152520.23264-1-benpeart@microsoft.com","threadId":"48048","inReplyTo":null,"subject":"[PATCH v1] Fix bugs preventing adding updated cache entries to the name hash","fromName":"Ben Peart","fromEmail":"benpeart@microsoft.com","sentAt":"2018-03-15T15:25:20Z","receivedAt":"2018-03-15T15:25:44Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"Update replace_index_entry() to clear the CE_HASHED flag from the new cache\nentry so that it can add it to the name hash in set_index_entry()\n\nFix refresh_cache_ent() to use the copy_cache_entry() macro instead of memcpy()\nso that it doesn't incorrectly copy the hash state from the old entry.\n\nSigned-off-by: Ben Peart <benpeart@microsoft.com>\n---\n\nNotes:\n    Base Ref: master\n    Web-Diff: https://github.com/benpeart/git/commit/079be4ac07\n    Checkout: git fetch https://github.com/benpeart/git ce_hashed-v1 && git checkout 079be4ac07\n\n read-cache.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 977921d90c..bdfa552861 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -62,6 +62,7 @@ static void replace_index_entry(struct index_state *istate, int nr, struct cache\n \treplace_index_entry_in_base(istate, old, ce);\n \tremove_name_hash(istate, old);\n \tfree(old);\n+\tce->ce_flags &= ~CE_HASHED;\n \tset_index_entry(istate, nr, ce);\n \tce->ce_flags |= CE_UPDATE_IN_BASE;\n \tmark_fsmonitor_invalid(istate, ce);\n@@ -1324,7 +1325,8 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,\n \n \tsize = ce_size(ce);\n \tupdated = xmalloc(size);\n-\tmemcpy(updated, ce, size);\n+\tcopy_cache_entry(updated, ce);\n+\tmemcpy(updated->name, ce->name, ce->ce_namelen + 1);\n \tfill_stat_cache_info(updated, &st);\n \t/*\n \t * If ignore_valid is not set, we should leave CE_VALID bit\n\nbase-commit: d0db9edba0050ada6f6eac68061599690d2a4333\n-- \n2.16.2.windows.1\n\n"},{"id":"341815","messageId":"xmqqfu515ihj.fsf@gitster-ct.c.googlers.com","threadId":"48048","inReplyTo":"20180315152520.23264-1-benpeart@microsoft.com","subject":"Re: [PATCH v1] Fix bugs preventing adding updated cache entries to the name hash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-15T17:58:00Z","receivedAt":"2018-03-15T17:58:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Peart <benpeart@microsoft.com> writes:\n\n> Update replace_index_entry() to clear the CE_HASHED flag from the new cache\n> entry so that it can add it to the name hash in set_index_entry()\n\nOK.  \n\n> diff --git a/read-cache.c b/read-cache.c\n> index 977921d90c..bdfa552861 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -62,6 +62,7 @@ static void replace_index_entry(struct index_state *istate, int nr, struct cache\n>  \treplace_index_entry_in_base(istate, old, ce);\n>  \tremove_name_hash(istate, old);\n>  \tfree(old);\n> +\tce->ce_flags &= ~CE_HASHED;\n>  \tset_index_entry(istate, nr, ce);\n>  \tce->ce_flags |= CE_UPDATE_IN_BASE;\n>  \tmark_fsmonitor_invalid(istate, ce);\n\nAs we are removing \"old\" that is not \"ce\", an earlier call to\nremove_name_hash() that clears the CE_HASHED bit from the cache\nentry does not help us at all.  We need to clear the bit from \"ce\"\nourselves before calling set_index_entry() on it, otherwise the call\nwould become a no-op wrt the name hash.  Makes sense.\n\nMakes me wonder why \"ce\" which is a replacement for what is in the\nindex already has the hashed bit, though.  Is that the failure to\nuse copy_cache_entry() in the caller the other part of this patch\nfixes?  To me it looks like copy_cache_entry() is designed for\ncopying an entry's data to another one that has a different name,\nbut in the refresh codepath, we _know_ we are replacing an old entry\nwith an entry with the same name, so it somehow feels a bit strange\nto use copy_cache_entry(), instead of doing memcpy() (and possibly\ndropping the HASHED bit from the new copy--but wouldn't that become\nunnecessary with the fix to replace_index_entry() we saw above?)\n\nIs this fix something we can demonstrate in a new test, by the way?\n\nThanks.\n"},{"id":"341821","messageId":"339cd7f7-9ca0-62cf-ea46-588be119eedd@gmail.com","threadId":"48048","inReplyTo":"xmqqfu515ihj.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v1] Fix bugs preventing adding updated cache entries to the name hash","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-03-15T18:44:06Z","receivedAt":"2018-03-15T18:44:13Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 3/15/2018 1:58 PM, Junio C Hamano wrote:\n> Ben Peart <benpeart@microsoft.com> writes:\n> \n>> Update replace_index_entry() to clear the CE_HASHED flag from the new cache\n>> entry so that it can add it to the name hash in set_index_entry()\n> \n> OK.\n> \n>> diff --git a/read-cache.c b/read-cache.c\n>> index 977921d90c..bdfa552861 100644\n>> --- a/read-cache.c\n>> +++ b/read-cache.c\n>> @@ -62,6 +62,7 @@ static void replace_index_entry(struct index_state *istate, int nr, struct cache\n>>   \treplace_index_entry_in_base(istate, old, ce);\n>>   \tremove_name_hash(istate, old);\n>>   \tfree(old);\n>> +\tce->ce_flags &= ~CE_HASHED;\n>>   \tset_index_entry(istate, nr, ce);\n>>   \tce->ce_flags |= CE_UPDATE_IN_BASE;\n>>   \tmark_fsmonitor_invalid(istate, ce);\n> \n> As we are removing \"old\" that is not \"ce\", an earlier call to\n> remove_name_hash() that clears the CE_HASHED bit from the cache\n> entry does not help us at all.  We need to clear the bit from \"ce\"\n> ourselves before calling set_index_entry() on it, otherwise the call\n> would become a no-op wrt the name hash.  Makes sense.\n> \n\nCorrect.  As you note below, this one line is sufficient to fix the \nactual bug.\n\n> Makes me wonder why \"ce\" which is a replacement for what is in the\n> index already has the hashed bit, though.  Is that the failure to\n> use copy_cache_entry() in the caller the other part of this patch\n> fixes?  To me it looks like copy_cache_entry() is designed for\n> copying an entry's data to another one that has a different name,\n> but in the refresh codepath, we _know_ we are replacing an old entry\n> with an entry with the same name, so it somehow feels a bit strange\n> to use copy_cache_entry(), instead of doing memcpy() (and possibly\n> dropping the HASHED bit from the new copy--but wouldn't that become\n> unnecessary with the fix to replace_index_entry() we saw above?)\n> \n\nThis 2nd part of the patch was more for code cleanliness.  When I was \ninvestigating why the hashed bit was set, it was caused by this \nmemcpy().  When I examined the rest of the code base, I only found 1 \nother instance (in dup_entry()) that did a straight memcpy(), the rest \nused the copy_cache_entry() macro.  I updated this code to match that \npattern as it would have prevented the bug as well though as you \ncorrectly point out, it is not necessary with the other fix.\n\n> Is this fix something we can demonstrate in a new test, by the way?\n> \n\nUnfortunately I was unable to find a way to reliably demonstrate this \nbug and fix with a new test.  I only ran across it while working on \nanother patch series that ends up triggering it more reliably.\n\nThe symptom was that occasionally in very specific circumstances a git \nstatus call would incorrectly show some directories as having untracked \nfiles when there were actually no untracked files in that directory.  A \nsubsequent call to status would correctly show no untracked files as \nwould git status -uall.\n\nI do have a test for this fix as part of the other patch series but \nthought I'd submit this bug fix separately as it may be a while before \nthe other patch series is ready.\n\n> Thanks.\n> \n"},{"id":"341822","messageId":"xmqqy3it414s.fsf@gitster-ct.c.googlers.com","threadId":"48048","inReplyTo":"339cd7f7-9ca0-62cf-ea46-588be119eedd@gmail.com","subject":"Re: [PATCH v1] Fix bugs preventing adding updated cache entries to the name hash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-15T18:58:11Z","receivedAt":"2018-03-15T18:58:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Peart <peartben@gmail.com> writes:\n\n> This 2nd part of the patch was more for code cleanliness.\n\nThanks.\n"}]}