Re: [BUG] test suite broken with GIT_TEST_SPLIT_INDEX

From: Jeff King <peff@peff.net>
Date: Apr 21, 2017, 17:05 UTC
Message-ID: <20170421170551.jsj47btuyfwvmc62@sigill.intra.peff.net>
In-Reply-To: <CAP8UFD36dcm9EfvtWqqEON1Muyyd7_j3uRtr6C2bLzXZN5n1MQ@mail.gmail.com>

On Fri, Apr 21, 2017 at 05:23:51PM +0200, Christian Couder wrote:

Show 24 quoted lines

> > I just tried on "pu" and only the first test
> > (t7009-filter-branch-null-sha1.sh) fails there.
> 
> I bisected this test's failure (when using
> GIT_TEST_SPLIT_INDEX=YesPlease) to e6a1dd77e1 (read-cache: regenerate
> shared index if necessary, 2017-02-27).
> 
> The failing test is the following:
> 
> test_expect_success 'filter commands are still checked' '
>         test_must_fail git filter-branch \
>                 --force --prune-empty \
>                 --index-filter "git rm --cached --ignore-unmatch three.t"
> '
> 
> And if I add the following at the beginning of the test:
> 
>         git config splitIndex.maxPercentChange 100 &&
> 
> the test then passes.
> 
> So It looks like in split index mode the test doesn't expect the
> shared index to be regenerated.
> Maybe Peff, as he is the author of this test, or Duy have an idea about this?

Right. The test has a broken tree with a null sha1, and filter-branch will do:

  GIT_ALLOW_NULL_SHA1=1 git read-tree $broken

to allow it to enter the index. We expect that further commands that write out the index will not allow it (so you can run commands that remove the broken entry, but not ones that leave it).

So without split index, this command:

  git rm --cached three.t

will fail. But in split-index mode, the broken entry is in another index, and is left untouched. I'm not sure there's a way to reconcile the split-index behavior with what the test is expecting; it's inherently optimizing out the thing that the test wants to check.

Probably we should catch the broken index entry when we write out the tree, too. We usually do catch missing objects, but this one is a gitlink, so it's OK for it to be missing. I think we should catch the null sha1 specifically, though, as that was the intent of the commit that added t7009.

So the patch below _almost_ works. It fixes the failing test. But note the new test I added, with a noop index-filter. That checks that we don't retain the cache-tree extension from the bogus tree when reading. But for some reason it fails on split-index mode. Does cache-tree stuff somehow work differently there?

diff --git a/cache-tree.c b/cache-tree.c
index 345ea3596..34baa6d85 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -354,7 +354,9 @@ static int update_one(struct cache_tree *it,
 			entlen = pathlen - baselen;
 			i++;
 		}
-		if (mode != S_IFGITLINK && !missing_ok && !has_sha1_file(sha1)) {
+
+		if (is_null_sha1(sha1) ||
+		    (mode != S_IFGITLINK && !missing_ok && !has_sha1_file(sha1))) {
 			strbuf_release(&buffer);
 			if (expected_missing)
 				return -1;
diff --git a/read-cache.c b/read-cache.c
index b3d0f3c30..15a4779f2 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2197,6 +2197,7 @@ static int do_write_index(struct index_state *istate, int newfd,
 	int entries = istate->cache_nr;
 	struct stat st;
 	struct strbuf previous_name_buf = STRBUF_INIT, *previous_name;
+	int drop_cache_tree = 0;
 
 	for (i = removed = extended = 0; i < entries; i++) {
 		if (cache[i]->ce_flags & CE_REMOVE)
@@ -2247,6 +2248,8 @@ static int do_write_index(struct index_state *istate, int newfd,
 				warning(msg, ce->name);
 			else
 				return error(msg, ce->name);
+
+			drop_cache_tree = 1;
 		}
 		if (ce_write_entry(&c, newfd, ce, previous_name) < 0)
 			return -1;
@@ -2265,7 +2268,7 @@ static int do_write_index(struct index_state *istate, int newfd,
 		if (err)
 			return -1;
 	}
-	if (!strip_extensions && istate->cache_tree) {
+	if (!strip_extensions && !drop_cache_tree && istate->cache_tree) {
 		struct strbuf sb = STRBUF_INIT;
 
 		cache_tree_write(&sb, istate->cache_tree);
diff --git a/t/t7009-filter-branch-null-sha1.sh b/t/t7009-filter-branch-null-sha1.sh
index c27f90f28..a8d9ec498 100755
--- a/t/t7009-filter-branch-null-sha1.sh
+++ b/t/t7009-filter-branch-null-sha1.sh
@@ -31,6 +31,12 @@ test_expect_success 'setup: bring HEAD and index in sync' '
 	git commit -a -m "back to normal"
 '
 
+test_expect_success 'noop filter-branch complains' '
+	test_must_fail git filter-branch \
+		--force --prune-empty \
+		--index-filter "true"
+'
+
 test_expect_success 'filter commands are still checked' '
 	test_must_fail git filter-branch \
 		--force --prune-empty \