Re: [PATCH 2/2] read-cache: fix reading of split index

On 03/24, Duy Nguyen wrote:

On Sat, Mar 21, 2015 at 4:43 AM, Thomas Gummerer [off-list ref] wrote:

quoted

The split index extension uses ewah bitmaps to mark index entries as
deleted, instead of removing them from the index directly.  This can
result in an on-disk index, in which entries of stage #0 and higher
stages appear, which are removed later when the index bases are merged.

15999d0 read_index_from(): catch out of order entries when reading an
index file introduces a check which checks if the entries are in order
after each index entry is read in do_read_index.  This check may however
fail when a split index is read.

Fix this by moving checking the index after we know there is no split
index or after the split index bases are successfully merged instead.

Thank you for catching this. I was about to write "would be nice to
point out what tests fail so the reviewer has easier time trying
themselves", but whoa.. quite a few of them!

May I suggest a slight modification. Even though stage info is messed
up before the index is merged, I think we should still check that both
front and base indexes have all the names sorted correctly (and even
stronger, the base index should never contain staged entries). If

Hmm I just tried adding another check for that, but the base index
does seem to include staged entries sometimes.

I've tried with this, but there are quite a few test failures.  For
example in t3600-rm.sh test #52 fails, and test-dump-split-index shows
the submodule with stages 1, 2 and 3 in the index.

own 74cd8e14a8fcc5df52e5c47a3ba0c30b29e5075a
base 0ff6ae43b1caa039c2a6262f07678b88314a5b4f
160000 6daff6d0fc4a9299deb0a51881e14cdbda16b88d 1	submod
160000 ee8321115a919c0607236124af886df2c9f16e2f 2	submod
160000 f3abce3ddcc2d68a8c113bd16767deeb376276f9 3	submod
replacements:
deletions: 3

diff --git a/read-cache.c b/read-cache.c
index 2ba67ce..b502290 100644
--- a/read-cache.c
+++ b/read-cache.c

@@ -1528,6 +1528,7 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist,
        struct cache_header *hdr;
        void *mmap;
        size_t mmap_size;
+       int fully_merged = 1;
        struct strbuf previous_name_buf = STRBUF_INIT, *previous_name;

        if (istate->initialized)

@@ -1580,6 +1581,10 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist,
                ce = create_from_disk(disk_ce, &consumed, previous_name);
                set_index_entry(istate, i, ce);

+               if (ce_stage(ce)) {
+                       fully_merged = 0;
+               }
+
                if (i > 0)
                        if (check_ce_order(istate->cache[i - 1], ce, 1) > 0 &&
                            multiple_stage_entries)

@@ -1610,6 +1615,8 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist,
                src_offset += extsize;
        }
        munmap(mmap, mmap_size);
+       if (!fully_merged && istate->split_index)
+               die("base index cannot contain staged entries");
        return istate->cache_nr;

 unmap:

sorting order is messed up, it could lead to other problems. So
instead of removing the test from do_read_index(), perhaps add a flag
in check_ce_order() to optionally detect the stage problem, but
print/do nothing, only set a flag so the caller know there may be a
problem. In the two new call sites you added, we still call the new
check_ce_order() again to make sure everything is in order. In the
call site where split index is not active, if the previous
check_ce_order() call from inside do_read_index() says "everything is
ok", we could even skip the check.
--
Duy

--
Thomas Gummerer