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

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

[Thomas Gummerer]

The different index versions have different sha-1 checksums.  Those
checksums are checked in t1700, which makes it fail when run with index
v4.  Fix it.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 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:
-- 
2.3.3.377.gdac1145

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

[Junio C Hamano]

Thomas Gummerer <t.gummerer@gmail.com> writes:

> The different index versions have different sha-1 checksums.  Those
> checksums are checked in t1700, which makes it fail when run with index
> v4.  Fix it.

I am more interested to see how you managed to use index v4 in the
tests be described next to "when run with index v4".  I thought we
were controling these things fairly tightly (e.g. we disable hooks,
move $HOME to avoid getting affected by your personal settings,
etc.).

Thanks.


> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
>  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:

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

[Thomas Gummerer]

On 03/20, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
>
> > The different index versions have different sha-1 checksums.  Those
> > checksums are checked in t1700, which makes it fail when run with index
> > v4.  Fix it.
>
> I am more interested to see how you managed to use index v4 in the
> tests be described next to "when run with index v4".  I thought we
> were controling these things fairly tightly (e.g. we disable hooks,
> move $HOME to avoid getting affected by your personal settings,
> etc.).

The tests can be run with index-v4 by setting TEST_GIT_INDEX_VERSION
in config.mak.  This configuration was introduced in 5d9fc88 test-lib:
allow setting the index format version.

> Thanks.
>
>
> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> > ---
> >  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

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

[Junio C Hamano]

Thomas Gummerer <t.gummerer@gmail.com> writes:

> On 03/20, Junio C Hamano wrote:
>> Thomas Gummerer <t.gummerer@gmail.com> writes:
>>
>> > The different index versions have different sha-1 checksums.  Those
>> > checksums are checked in t1700, which makes it fail when run with index
>> > v4.  Fix it.
>>
>> I am more interested to see how you managed to use index v4 in the
>> tests be described next to "when run with index v4".  I thought we
>> were controling these things fairly tightly (e.g. we disable hooks,
>> move $HOME to avoid getting affected by your personal settings,
>> etc.).
>
> The tests can be run with index-v4 by setting TEST_GIT_INDEX_VERSION
> in config.mak.  This configuration was introduced in 5d9fc88 test-lib:
> allow setting the index format version.

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

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

[Thomas Gummerer]

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 <t.gummerer@gmail.com>
---
> 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.
					      

 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:
-- 
2.3.3.377.gdac1145

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

[Junio C Hamano]

Thomas Gummerer <t.gummerer@gmail.com> 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 <t.gummerer@gmail.com>
> ---
>> 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?

> 					      
>
>  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:

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

[Thomas Gummerer]

On 03/20, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> 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 <t.gummerer@gmail.com>
> > ---
> >> 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.

> >
> >
> >  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

[PATCH 0/2] fix test suite with split index

[Thomas Gummerer]

> 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

[PATCH 1/2] test-lib: allow using split index in the test suite

[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 <t.gummerer@gmail.com>
---
 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

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

[Thomas Gummerer]

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 <t.gummerer@gmail.com>
---
 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

Re: [PATCH 1/2] test-lib: allow using split index in the test suite

[Junio C Hamano]

Thomas Gummerer <t.gummerer@gmail.com> writes:

> 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 <t.gummerer@gmail.com>

> ---

Hmm, it is not wrong per-se, but would it be too much trouble to do

    GIT_TEST_SPLIT_INDEX=YesPlease make test

or is this doing something a lot more than that?

>  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 ||

Re: [PATCH 1/2] test-lib: allow using split index in the test suite

[Thomas Gummerer]

On 03/20, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
>
> > 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 <t.gummerer@gmail.com>
>
> > ---
>
> Hmm, it is not wrong per-se, but would it be too much trouble to do
>
>     GIT_TEST_SPLIT_INDEX=YesPlease make test
>
> or is this doing something a lot more than that?

No that would work as well, I just thought of it as analogous to
TEST_GIT_INDEX_VERSION and it might be more convenient to set it once
and keep it in the config.mak for some people, to check that split
index doesn't break.  In any case I do not feel strongly about this.


> >  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 ||

--
Thomas Gummerer

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

[Duy Nguyen]

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
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

[PATCH] read-cache: tighten checks for do_read_index

[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 <t.gummerer@gmail.com>
---

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

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

[Thomas Gummerer]

On 03/24, Duy Nguyen wrote:
> 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

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

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> 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).

I smell a slight layering violation here, though.  As far as the
code to check the validity of the index is concerned, it is only
about the in-core index other code uses at runtime, and how that
in-core index is prepared, and most importantly, what is recorded in
the istate before it gets ready to be used by other code, is not its
concern.  The state immediately after the base index is read but
before it gets fixed up by the split-index code can have quirks
specific to how split-index code does things and it is perfectly OK
if it does not pass the check for the final shape.

The above does not change if the current split-index code happens to
promise certain properties in that intermediate state.  It is fine
if the split-index codepath wants to add its own validator to the
intermediate state for added robustness, but the rules for the
intermediate state and the rules for the final state can be
different, and from the maintainability's point of view, it is
better if we keep the validator for the final-shape oblivious to
what split-index does.

Re: [PATCH] read-cache: tighten checks for do_read_index

[Junio C Hamano]

Thomas Gummerer <t.gummerer@gmail.com> writes:

> 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 <t.gummerer@gmail.com>
> ---
>
> This is a patch on top of my previous patch, as that one has already
> been merged to next.

I am not convinced that this is a good change in the first place.

The original before your fix was wrong exactly because it was too
tightly tied to the implementation of the index file format where
there was only one file whose contents must be sorted, and that is
why it was a broken check in a new world with split-index.  And your
fix in 'next' is the right fix---it makes the verification happen
only on the result is given to the caller for its consumption.

It may be true that entries may have to be sorted in a certain order
when reading the original index file format and also reading some
steps in reading the split-index, but that merely happens to be an
imprementation detail of the two format currently supported, and as
we improve these formats (or even introduce yet another one) in the
longer term, this patch would re-introduce the same issue your
earlier fix corrected, wouldn't it?

Re: [PATCH] read-cache: tighten checks for do_read_index

[Thomas Gummerer]

On 03/24, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
>
> > 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 <t.gummerer@gmail.com>
> > ---
> >
> > This is a patch on top of my previous patch, as that one has already
> > been merged to next.
>
> I am not convinced that this is a good change in the first place.
>
> The original before your fix was wrong exactly because it was too
> tightly tied to the implementation of the index file format where
> there was only one file whose contents must be sorted, and that is
> why it was a broken check in a new world with split-index.  And your
> fix in 'next' is the right fix---it makes the verification happen
> only on the result is given to the caller for its consumption.
>
> It may be true that entries may have to be sorted in a certain order
> when reading the original index file format and also reading some
> steps in reading the split-index, but that merely happens to be an
> imprementation detail of the two format currently supported, and as
> we improve these formats (or even introduce yet another one) in the
> longer term, this patch would re-introduce the same issue your
> earlier fix corrected, wouldn't it?

Yes, after looking at it again I completely agree.  Sorry for the noise.