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

From: Thomas Gummerer <t.gummerer@gmail.com>
Date: Mar 24, 2015, 17:07 UTC
Message-ID: <20150324170755.GA2006@hank>
In-Reply-To: <CACsJy8CYi+hYu8zwOy=m7zZk3-8fr+Jq9uT4kEf8fLCOcjHJzw@mail.gmail.com>

On 03/24, Duy Nguyen wrote:

Show 22 quoted lines

> On Sat, Mar 21, 2015 at 4:43 AM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > 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