Re: [PATCH v2] t1700: make test pass with index-v4

Thomas Gummerer [off-list ref] writes:

The different index versions have different sha-1 checksums.  Those
checksums are checked in t1700, which makes it fail when the test suite
is run with TEST_GIT_INDEX_VERSION=4.  Fix it.

Signed-off-by: Thomas Gummerer <redacted>
---

quoted

An updated patch to mention "when run with TEST_GIT_INDEX_VERSION=4"
in the log message was what I was asking for ;-)

Sorry I didn't catch that.  Here it is.

Thanks.  Is this the only one that fails with v4?

On 03/20, Junio C Hamano wrote:

Thomas Gummerer [off-list ref] writes:

quoted

The different index versions have different sha-1 checksums.  Those
checksums are checked in t1700, which makes it fail when the test suite
is run with TEST_GIT_INDEX_VERSION=4.  Fix it.

Signed-off-by: Thomas Gummerer <redacted>
---

quoted

An updated patch to mention "when run with TEST_GIT_INDEX_VERSION=4"
in the log message was what I was asking for ;-)

Sorry I didn't catch that.  Here it is.

Thanks.  Is this the only one that fails with v4?

Yes, with this patch the test suite passes with TEST_GIT_INDEX_VERSION
set.

I've now tried to set the GIT_TEST_SPLIT_INDEX environment variable
and run the tests, and quite a few seem to fail for both v3 and v4.
I'm looking into that now, but that seems to be unrelated to index v4,
and I'm not sure yet if I'm testing the right thing.

quoted

 t/t1700-split-index.sh | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index 94fb473..92f7298 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh

@@ -10,9 +10,18 @@ sane_unset GIT_TEST_SPLIT_INDEX
 test_expect_success 'enable split index' '
 	git update-index --split-index &&
 	test-dump-split-index .git/index >actual &&
+	indexversion=$(test-index-version <.git/index) &&
+	if test "$indexversion" = "4"
+	then
+		own=432ef4b63f32193984f339431fd50ca796493569
+		base=508851a7f0dfa8691e9f69c7f055865389012491
+	else
+		own=8299b0bcd1ac364e5f1d7768efb62fa2da79a339
+		base=39d890139ee5356c7ef572216cebcd27aa41f9df
+	fi &&
 	cat >expect <<EOF &&
-own 8299b0bcd1ac364e5f1d7768efb62fa2da79a339
-base 39d890139ee5356c7ef572216cebcd27aa41f9df
+own $own
+base $base
 replacements:
 deletions:
 EOF

@@ -30,7 +39,7 @@ EOF

 	test-dump-split-index .git/index | sed "/^own/d" >actual &&
 	cat >expect <<EOF &&
-base 39d890139ee5356c7ef572216cebcd27aa41f9df
+base $base
 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	one
 replacements:
 deletions:

--
Thomas Gummerer

Allow adding a TEST_GIT_TEST_SPLIT_INDEX variable to config.mak to run
the test suite with split index enabled.

Signed-off-by: Thomas Gummerer <redacted>
---
 Makefile      | 6 ++++++
 t/test-lib.sh | 6 ++++++
 2 files changed, 12 insertions(+)

diff --git a/Makefile b/Makefile
index 44f1dd1..55e558a 100644
--- a/Makefile
+++ b/Makefile

@@ -339,6 +339,9 @@ all::
 # with a different indexfile format version.  If it isn't set the index
 # file format used is index-v[23].
 #
+# Define TEST_GIT_TEST_SPLIT_INDEX to 1 to run the test suite with split
+# index enabled.
+#
 # Define GMTIME_UNRELIABLE_ERRORS if your gmtime() function does not
 # return NULL when it receives a bogus time_t.
 #

@@ -2129,6 +2132,9 @@ endif
 ifdef TEST_GIT_INDEX_VERSION
 	@echo TEST_GIT_INDEX_VERSION=\''$(subst ','\'',$(subst ','\'',$(TEST_GIT_INDEX_VERSION)))'\' >>$@
 endif
+ifdef TEST_GIT_TEST_SPLIT_INDEX
+	@echo TEST_GIT_TEST_SPLIT_INDEX=\''$(subst ','\'',$(subst ','\'',$(TEST_GIT_TEST_SPLIT_INDEX)))'\' >>$@
+endif
 
 ### Detect Python interpreter path changes
 ifndef NO_PYTHON

diff --git a/t/test-lib.sh b/t/test-lib.sh
index c096778..477f253 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh

@@ -119,6 +119,12 @@ then
 	export GIT_INDEX_VERSION
 fi
 
+if test -n "${TEST_GIT_TEST_SPLIT_INDEX:+isset}"
+then
+	GIT_TEST_SPLIT_INDEX="$TEST_GIT_TEST_SPLIT_INDEX"
+	export GIT_TEST_SPLIT_INDEX
+fi
+
 # Add libc MALLOC and MALLOC_PERTURB test
 # only if we are not executing the test with valgrind
 if expr " $GIT_TEST_OPTS " : ".* --valgrind " >/dev/null ||

-- 
2.1.0.264.g0463184.dirty

I've now tried to set the GIT_TEST_SPLIT_INDEX environment variable
and run the tests, and quite a few seem to fail for both v3 and v4.
I'm looking into that now, but that seems to be unrelated to index v4,
and I'm not sure yet if I'm testing the right thing.

So now I've tracked it down, 15999d0 read_index_from(): catch out of
order entries when reading an index file breaks reading split index
files in some cases.

The first patch allows setting TEST_GIT_TEST_SPLIT_INDEX in config.mak
to make running the test suite with split index easier, the second
patch fixes the issue.  With the patch I previously sent, the test
suite now passes with split-index enabled for both index v3 and v4.
	    

Thomas Gummerer (2):
  test-lib: allow using split index in the test suite
  read-cache: fix reading of split index

 Makefile      |  6 ++++++
 read-cache.c  | 42 +++++++++++++++++++++++-------------------
 t/test-lib.sh |  6 ++++++
 3 files changed, 35 insertions(+), 19 deletions(-)

-- 
2.1.0.264.g0463184.dirty

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.

Signed-off-by: Thomas Gummerer <redacted>
---
 read-cache.c | 42 +++++++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 19 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 8d71860..1bf78a4 100644
--- a/read-cache.c
+++ b/read-cache.c

@@ -1486,18 +1486,25 @@ static struct cache_entry *create_from_disk(struct ondisk_cache_entry *ondisk,
 	return ce;
 }
 
-static void check_ce_order(struct cache_entry *ce, struct cache_entry *next_ce)
-{
-	int name_compare = strcmp(ce->name, next_ce->name);
-	if (0 < name_compare)
-		die("unordered stage entries in index");
-	if (!name_compare) {
-		if (!ce_stage(ce))
-			die("multiple stage entries for merged file '%s'",
-				ce->name);
-		if (ce_stage(ce) > ce_stage(next_ce))
-			die("unordered stage entries for '%s'",
-				ce->name);
+static void check_ce_order(struct index_state *istate)
+{
+	unsigned int i;
+
+	for (i = 1; i < istate->cache_nr; i++) {
+		struct cache_entry *ce = istate->cache[i - 1];
+		struct cache_entry *next_ce = istate->cache[i];
+		int name_compare = strcmp(ce->name, next_ce->name);
+
+		if (0 < name_compare)
+			die("unordered stage entries in index");
+		if (!name_compare) {
+			if (!ce_stage(ce))
+				die("multiple stage entries for merged file '%s'",
+				    ce->name);
+			if (ce_stage(ce) > ce_stage(next_ce))
+				die("unordered stage entries for '%s'",
+				    ce->name);
+		}
 	}
 }
 

@@ -1562,9 +1569,6 @@ 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 (i > 0)
-			check_ce_order(istate->cache[i - 1], ce);
-
 		src_offset += consumed;
 	}
 	strbuf_release(&previous_name_buf);

@@ -1608,11 +1612,10 @@ int read_index_from(struct index_state *istate, const char *path)
 
 	ret = do_read_index(istate, path, 0);
 	split_index = istate->split_index;
-	if (!split_index)
-		return ret;
-
-	if (is_null_sha1(split_index->base_sha1))
+	if (!split_index || is_null_sha1(split_index->base_sha1)) {
+		check_ce_order(istate);
 		return ret;
+	}
 
 	if (split_index->base)
 		discard_index(split_index->base);

@@ -1628,6 +1631,7 @@ int read_index_from(struct index_state *istate, const char *path)
 				     sha1_to_hex(split_index->base_sha1)),
 		    sha1_to_hex(split_index->base->sha1));
 	merge_base_index(istate);
+	check_ce_order(istate);
 	return ret;
 }
 

-- 
2.1.0.264.g0463184.dirty

On Sat, Mar 21, 2015 at 4:43 AM, Thomas Gummerer [off-list ref] 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
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

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

03f15a7 read-cache: fix reading of split index moved the checks for the
correct order of index entries out of do_read_index.  This loosens the
checks more than necessary.  Re-introduce the checks for the order, but
don't error out when we have multiple stage-0 entries in the index.
Return a flag for the caller instead, if we have multiple stage-0
entries and let the caller decide if we need to error out.

Signed-off-by: Thomas Gummerer <redacted>
---

This is a patch on top of my previous patch, as that one has already
been merged to next.

 cache.h                 |  2 +-
 read-cache.c            | 54 ++++++++++++++++++++++++++++++++-----------------
 test-dump-split-index.c |  2 +-
 3 files changed, 37 insertions(+), 21 deletions(-)

diff --git a/cache.h b/cache.h
index e7b24a2..3eaa258 100644
--- a/cache.h
+++ b/cache.h

@@ -487,7 +487,7 @@ struct lock_file;
 extern int read_index(struct index_state *);
 extern int read_index_preload(struct index_state *, const struct pathspec *pathspec);
 extern int do_read_index(struct index_state *istate, const char *path,
-			 int must_exist); /* for testting only! */
+			 int must_exist, int *multiple_stage_entries); /* for testting only! */
 extern int read_index_from(struct index_state *, const char *path);
 extern int is_index_unborn(struct index_state *);
 extern int read_index_unmerged(struct index_state *);

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

@@ -1488,30 +1488,39 @@ static struct cache_entry *create_from_disk(struct ondisk_cache_entry *ondisk,
 	return ce;
 }
 
-static void check_ce_order(struct index_state *istate)
+static int check_ce_order(struct cache_entry *ce, struct cache_entry *next_ce,
+			   int gentle_multiple_stage)
 {
-	unsigned int i;
-
-	for (i = 1; i < istate->cache_nr; i++) {
-		struct cache_entry *ce = istate->cache[i - 1];
-		struct cache_entry *next_ce = istate->cache[i];
-		int name_compare = strcmp(ce->name, next_ce->name);
+	int name_compare = strcmp(ce->name, next_ce->name);
 
-		if (0 < name_compare)
-			die("unordered stage entries in index");
-		if (!name_compare) {
-			if (!ce_stage(ce))
+	if (0 < name_compare)
+		die("unordered stage entries in index");
+	if (!name_compare) {
+		if (!ce_stage(ce)) {
+			if (gentle_multiple_stage)
+				return 1;
+			else
 				die("multiple stage entries for merged file '%s'",
 				    ce->name);
-			if (ce_stage(ce) > ce_stage(next_ce))
-				die("unordered stage entries for '%s'",
-				    ce->name);
 		}
+		if (ce_stage(ce) > ce_stage(next_ce))
+			die("unordered stage entries for '%s'",
+			    ce->name);
 	}
+	return 0;
+}
+
+static void check_istate_order(struct index_state *istate)
+{
+	unsigned int i;
+
+	for (i = 1; i < istate->cache_nr; i++)
+		check_ce_order(istate->cache[i - 1], istate->cache[i], 0);
 }
 
 /* remember to discard_cache() before reading a different cache! */
-int do_read_index(struct index_state *istate, const char *path, int must_exist)
+int do_read_index(struct index_state *istate, const char *path, int must_exist,
+		  int *multiple_stage_entries)
 {
 	int fd, i;
 	struct stat st;

@@ -1571,6 +1580,11 @@ 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 (i > 0)
+			if (check_ce_order(istate->cache[i - 1], ce, 1) > 0 &&
+			    multiple_stage_entries)
+				*multiple_stage_entries |= 1;
+
 		src_offset += consumed;
 	}
 	strbuf_release(&previous_name_buf);

@@ -1607,15 +1621,17 @@ int read_index_from(struct index_state *istate, const char *path)
 {
 	struct split_index *split_index;
 	int ret;
+	int multiple_stage_entries = 0;
 
 	/* istate->initialized covers both .git/index and .git/sharedindex.xxx */
 	if (istate->initialized)
 		return istate->cache_nr;
 
-	ret = do_read_index(istate, path, 0);
+	ret = do_read_index(istate, path, 0, &multiple_stage_entries);
 	split_index = istate->split_index;
 	if (!split_index || is_null_sha1(split_index->base_sha1)) {
-		check_ce_order(istate);
+		if (multiple_stage_entries)
+			check_istate_order(istate);
 		return ret;
 	}
 

@@ -1625,7 +1641,7 @@ int read_index_from(struct index_state *istate, const char *path)
 		split_index->base = xcalloc(1, sizeof(*split_index->base));
 	ret = do_read_index(split_index->base,
 			    git_path("sharedindex.%s",
-				     sha1_to_hex(split_index->base_sha1)), 1);
+				     sha1_to_hex(split_index->base_sha1)), 1, NULL);
 	if (hashcmp(split_index->base_sha1, split_index->base->sha1))
 		die("broken index, expect %s in %s, got %s",
 		    sha1_to_hex(split_index->base_sha1),

@@ -1633,7 +1649,7 @@ int read_index_from(struct index_state *istate, const char *path)
 				     sha1_to_hex(split_index->base_sha1)),
 		    sha1_to_hex(split_index->base->sha1));
 	merge_base_index(istate);
-	check_ce_order(istate);
+	check_istate_order(istate);
 	return ret;
 }
 

diff --git a/test-dump-split-index.c b/test-dump-split-index.c
index 9cf3112..fc9ced7 100644
--- a/test-dump-split-index.c
+++ b/test-dump-split-index.c

@@ -12,7 +12,7 @@ int main(int ac, char **av)
 	struct split_index *si;
 	int i;
 
-	do_read_index(&the_index, av[1], 1);
+	do_read_index(&the_index, av[1], 1, NULL);
 	printf("own %s\n", sha1_to_hex(the_index.sha1));
 	si = the_index.split_index;
 	if (!si) {

-- 
2.1.0.264.g0463184.dirty