Re: [PATCH 0/1] fsmonitor: skip sanity check if the index is split
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 11, 2019, 02:01 UTC
- Message-ID
- <xmqqa793jhne.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <xmqqh83bjig6.fsf@gitster-ct.c.googlers.com>
Junio C Hamano <gitster@pobox.com> writes:
Show 11 quoted lines
> "Utsav Shah via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> At the very least, this patch mitigates an over-eager check for split index >> users while maintaining good invariants for the standard case. > > OK, it sounds more like this "it does not make any sense to compare > the position in the fsmonitor bitmap (which covers the entire thing) > with the position in just a split part of the index (which covers > only the delta over the base index)"? If that is the case, it means > that the "check" is even worse than being "over-eager"---it simply > is not correct.
Having said all that, I wonder if we are doing the right thing with or without 3444ec2e ("fsmonitor: don't fill bitmap with entries to be removed", 2019-10-11) in the split-index mode in the first place.
The fact that your "loosen the check and allow 'pos' that identifies a tracked path used by the fsmonitor bitmap to be larger than the size of the istate->cache[]" patch under discussion is needed is that 'pos' may sometimes be larger than isate->cache[] no? Then what happens in this hunk, for example?
diff --git a/fsmonitor.c b/fsmonitor.c index 231e83a94d..1f4aa1b150 100644 --- a/fsmonitor.c +++ b/fsmonitor.c @@ -14,8 +14,13 @@ struct trace_key trace_fsmonitor = TRACE_KEY_INIT(FSMONITOR); static void fsmonitor_ewah_callback(size_t pos, void *is) { struct index_state *istate = (struct index_state *)is; - struct cache_entry *ce = istate->cache[pos]; + struct cache_entry *ce; + if (pos >= istate->cache_nr) + BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" >= %u)", + (uintmax_t)pos, istate->cache_nr); + + ce = istate->cache[pos]; ce->ce_flags &= ~CE_FSMONITOR_VALID; The istate->cache[] is a dynamic array whose size is managed via the usual ALLOC_GROW() using istate->cache_nr and istate->cache_alloc, whether the split-index feature is in use. When your patch makes a difference, then, doesn't the access to istate->cache[] pick up a random garbage and then flip the bit? Puzzled... In any case, "check is worse than over-eager, it simply is wrong" I wrote in the message I am responding to is totally incorrect, it seems. It smells like lifting the check would just hide the underlying problem under the rug?