Junio C Hamano <gitster@xxxxxxxxx> writes:
> "Utsav Shah via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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?
