Re: [PATCH v12 13/13] fsmonitor: fix split-index bitmap bounds in tweak_fsmonitor()

From: Paul Tarjan <paul@paultarjan.com>
Date: Apr 1, 2026, 04:19 UTC
Message-ID: <20260401041945.86738-1-github@paulisageek.com>
In-Reply-To: <xmqqfr5f289u.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 5 quoted lines

> While this may avoid this "assert" stopping the process, what is the
> implication of returning from here?  Who are the callers, what did
> the callers want us to do to the cache entry at the "pos", and what
> happens to their expectations if we simply returned to them without
> doing anything they asked us to do?

The callback's job is to clear CE_FSMONITOR_VALID on the entry at pos (marking it dirty). The only caller is tweak_fsmonitor() via ewah_each_bit().

In practice this guard shouldn't fire during normal operation. By the time tweak_fsmonitor() runs, merge_base_index() has already merged the split-index entries, so cache_nr is the full merged size and the bitmap positions are all in range. The call order in read_index_from() is:

  do_read_index()           -- reads split index, sets fsmonitor_dirty
  do_read_index(base)       -- reads shared index
  merge_base_index()        -- merges base into istate, cache_nr grows
  post_read_index_from()    -- calls tweak_split_index, then tweak_fsmonitor

The guard only fires if the bitmap references positions that no longer exist (e.g., entries were removed between write and read). In that case returning is correct -- there's no entry to mark dirty.

And even if we somehow missed marking something, refresh_fsmonitor() runs right after (line 813) and re-queries the daemon by pathname, so it catches any changes independently of the bitmap.

That said, if you'd rather keep the BUG() assertion as a canary and handle this on the test side like Derrick did, I'm fine with dropping the callback change and just keeping the !istate->split_index guard in tweak_fsmonitor().

> This part does make sense; the minimum may be relevant only when
> split-index is in effect, so we limit the check to non split case.

Right.

> I do have to wonder if there are some invariants we can/want to
> enforce here in the split case, but I do not mind punting it to a
> fix that is more focused on the split-index theme, as Derrick hinted
> in the earlier commit you found.

Agreed. The bitmap is written against the full merged index and read back before the merge, so during the read phase the positions can legitimately exceed the split cache_nr. After the merge they should all be in range. The read and write paths already punt on this with the !istate->split_index guard; enforcing anything tighter probably belongs in a split-index focused series.

I've also moved this commit to 1/13 per your earlier feedback about bisectability.

Paul