Re: [PATCH 0/1] fsmonitor: skip sanity check if the index is split

Kevin Willford [off-list ref] writes:

quoted

I agree.  The only 2 places that excluding the split-index make sense are in
read_fsmonitor_extension and write_fsmonitor_extension because the
index_state that is being passing into those methods could be the delta index
in which case the number of entries for the fsmonitor bitmap would almost
always be more and cause the BUG to be hit which it should not be.

Thanks.  Here is what I came up with to tie the loose ends of this
thread.

-- >8 --
From: Junio C Hamano <redacted>
Subject: [PATCH] fsmonitor: do not compare bitmap size with size of split index

3444ec2e ("fsmonitor: don't fill bitmap with entries to be removed",
2019-10-11) added a handful of sanity checks that make sure that a
bit position in fsmonitor bitmap does not go beyond the end of the
index.  As each bit in the bitmap corresponds to a path in the
index, this is the right check most of the time.

Except for the case when we are in the split-index mode and looking
at a delta index that is to be overlayed on the base index but
before the base index has actually been merged in, namely in read_
and write_fsmonitor_extension().  In these codepaths, the entries in
the split/delta index is typically a small subset of the entire set
of paths (otherwise why would we be using split-index?), so the
bitmap used by the fsmonitor is almost always larger than the number
of entries in the partial index, and the incorrect comparison would
trigger the BUG().

Reported-by: Utsav Shah <redacted>
Helped-by: Kevin Willford [off-list ref]
Helped-by: William Baker [off-list ref]
Signed-off-by: Junio C Hamano <redacted>
---
 fsmonitor.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fsmonitor.c b/fsmonitor.c
index 1f4aa1b150..0477500b39 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c

@@ -55,7 +55,8 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,
        }
        istate->fsmonitor_dirty = fsmonitor_dirty;

-       if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
+       if (!istate->split_index &&
+           istate->fsmonitor_dirty->bit_size > istate->cache_nr)
                BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
                    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);

@@ -83,7 +84,8 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)
        uint32_t ewah_size = 0;
        int fixup = 0;

-       if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
+       if (!istate->split_index &&
+           istate->fsmonitor_dirty->bit_size > istate->cache_nr)
                BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
                    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);

--

2.24.0-346-gee0de6d492

Thanks.  Here is what I came up with to tie the loose ends of this
thread.

-- >8 --
From: Junio C Hamano <redacted>
Subject: [PATCH] fsmonitor: do not compare bitmap size with size of split index

3444ec2e ("fsmonitor: don't fill bitmap with entries to be removed",
2019-10-11) added a handful of sanity checks that make sure that a
bit position in fsmonitor bitmap does not go beyond the end of the
index.  As each bit in the bitmap corresponds to a path in the
index, this is the right check most of the time.

Except for the case when we are in the split-index mode and looking
at a delta index that is to be overlayed on the base index but
before the base index has actually been merged in, namely in read_
and write_fsmonitor_extension().  In these codepaths, the entries in
the split/delta index is typically a small subset of the entire set
of paths (otherwise why would we be using split-index?), so the
bitmap used by the fsmonitor is almost always larger than the number
of entries in the partial index, and the incorrect comparison would
trigger the BUG().

Reported-by: Utsav Shah <redacted>
Helped-by: Kevin Willford [off-list ref]
Helped-by: William Baker [off-list ref]
Signed-off-by: Junio C Hamano <redacted>
---
 fsmonitor.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fsmonitor.c b/fsmonitor.c
index 1f4aa1b150..0477500b39 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c

@@ -55,7 +55,8 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,
 	}
 	istate->fsmonitor_dirty = fsmonitor_dirty;
 
-	if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
+	if (!istate->split_index &&
+	    istate->fsmonitor_dirty->bit_size > istate->cache_nr)
 		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
 		    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
 

@@ -83,7 +84,8 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)
 	uint32_t ewah_size = 0;
 	int fixup = 0;
 
-	if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
+	if (!istate->split_index &&
+	    istate->fsmonitor_dirty->bit_size > istate->cache_nr)
 		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
 		    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);