'ds/test-multi-pack-index' vs. 'ab/commit-graph-progress'

Whenever we delete pack-files from the object directory, we need to also delete the multi-pack-index that may refer to those objects. Sometimes, this connection is obvious, like during a repack. Other times, this is less obvious, like when gc calls a repack command and then does other actions on the objects, like write a commit-graph file.

The pattern we use to avoid out-of-date in-memory packed_git structs is to call close_all_packs(). This should also call close_midx(). Since we already pass an object store to close_all_packs(), this is a nicely scoped operation.

This fixes a test failure when running t6500-gc.sh with GIT_TEST_MULTI_PACK_INDEX=1.

Reported-by Szeder Gábor <[email protected]>

Signed-off-by Derrick Stolee <[email protected]>

---

Thanks for the report, Szeder! I think this is the correct fix, and more likely to be permanent across all builtins that run auto-GC. I based it on ds/test-multi-pack-index so it could easily be added on top.

(quoted diff) +0 -1

-Stolee
 
 packfile.c | 5 +++++
 1 file changed, 5 insertions(+)

packfile.c +5 -0

index 841b36182f..37fcd8f136 100644
@@ -339,6 +339,11 @@ void close_all_packs(struct raw_object_store *o)
 			BUG("want to close pack marked 'do-not-close'");
 		else
 			close_pack(p);
+
+	if (o->multi_pack_index) {
+		close_midx(o->multi_pack_index);
+		o->multi_pack_index = NULL;
+	}
 }
 
 /*

base-commit: 0465a50506023df0932fe0534fe6ac6712c0d854

Signature · 1 line

2.17.1

Quoted 19 lines · Derrick Stolee

Whenever we delete pack-files from the object directory, we need to also delete the multi-pack-index that may refer to those objects. Sometimes, this connection is obvious, like during a repack. Other times, this is less obvious, like when gc calls a repack command and then does other actions on the objects, like write a commit-graph file.

The pattern we use to avoid out-of-date in-memory packed_git structs is to call close_all_packs(). This should also call close_midx(). Since we already pass an object store to close_all_packs(), this is a nicely scoped operation.

This fixes a test failure when running t6500-gc.sh with GIT_TEST_MULTI_PACK_INDEX=1.

Reported-by Szeder Gábor <[email protected]>

Signed-off-by Derrick Stolee <[email protected]>

---

Quoted 4 lines

Thanks for the report, Szeder! I think this is the correct fix, and more likely to be permanent across all builtins that run auto-GC. I based it on ds/test-multi-pack-index so it could easily be added on top.

OK, avoiding those errors in the first place is good, of course... However, I still find it disconcerting that those errors didn't cause 'git gc' to error out, and, consequently, what other MIDX-related errors/bugs might do the same.

when branch 'ds/test-multi-pack-index' is merged with 'ab/commit-graph-progress', IOW 'master', 'next', or 'pu', 'GIT_TEST_MULTI_PACK_INDEX=1 ./t6500-gc.sh' fails with:

expecting success:

          git -c gc.writeCommitGraph=true gc --no-quiet >stdout 2>stderr &&
          test_must_be_empty stdout &&
          test_line_count = 1 stderr &&
          test_i18ngrep "Computing commit graph generation numbers" stderr

+ git -c gc.writeCommitGraph=true gc --no-quiet
+ test_must_be_empty stdout
+ test_path_is_file stdout
+ test -f stdout
+ test -s stdout
+ test_line_count = 1 stderr
+ test 3 != 3
+ wc -l
+ test 16 = 1
+ echo test_line_count: line count for stderr != 1
test_line_count: line count for stderr != 1
+ cat stderr
error: packfile .git/objects/pack/pack-c67996b982e718f8e3fa70c5ff7db3cecf688bbb.pack index unavailable
error: packfile .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index unavailable
error: packfile .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index unavailable
error: packfile .git/objects/pack/pack-c67996b982e718f8e3fa70c5ff7db3cecf688bbb.pack index unavailable
error: packfile .git/objects/pack/pack-c67996b982e718f8e3fa70c5ff7db3cecf688bbb.pack index unavailable
error: packfile .git/objects/pack/pack-c67996b982e718f8e3fa70c5ff7db3cecf688bbb.pack index unavailable
error: packfile .git/objects/pack/pack-c67996b982e718f8e3fa70c5ff7db3cecf688bbb.pack index unavailable
error: packfile .git/objects/pack/pack-c67996b982e718f8e3fa70c5ff7db3cecf688bbb.pack index unavailable
error: packfile .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index unavailable
error: packfile .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index unavailable
error: packfile .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index unavailable
error: packfile .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index unavailable
error: packfile .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index unavailable
error: packfile .git/objects/pack/pack-d4f2632c6a37149bb546b8b0cfbc56b8183cd0f8.pack index unavailable
error: packfile .git/objects/pack/pack-c67996b982e718f8e3fa70c5ff7db3cecf688bbb.pack index unavailable
Computing commit graph generation numbers: 25% (1/4) ^MComputing commit graph generation numbers: 50% (2/4) ^MComputing commit graph generation numbers: 75% (3/4) ^MComputing commit graph generation numbers: 100% (4/4) ^MComputing commit graph generation numbers: 100% (4/4), done.
+ return 1
error: last command exited with $?=1
not ok 9 - gc --no-quiet

I suspect these "packfile index unavailable" errors are a Bad Thing, but I didn't follow the MIDX development closely enough to judge. Surprisingly (to me), 'git gc' didn't exit with error despite these errors.

Anyway, this test seems to be too fragile, because that

test_line_count = 1 stderr

line will trigger, when anything else during 'git gc' prints something. And I find it quite strange that an option called '--no-quiet' only shows the commit-graph progress, but not the regular output of 'git gc'.

Quoted 3 lines

Anyway, this test seems to be too fragile, because that

test_line_count = 1 stderr

Yeah maybe it's too fragile, on the other hand it caught what seems to be a bug under GIT_TEST_MULTI_PACK_INDEX=true, and the rest of the test suite passes, so there's that.

Quoted 4 lines

line will trigger, when anything else during 'git gc' prints something. And I find it quite strange that an option called '--no-quiet' only shows the commit-graph progress, but not the regular output of 'git gc'.

It's confusing, but the reason for this seeming discrepancy is that writing the commit-graph happens in-process, whereas the rest of the work done by git-gc (and its output) comes from subprocesses. Most of that output is from "git-repack" / "git-pack-objects" which doesn't pay the same attention to --quiet and --no-quiet, instead it checks isatty(). See cmd_pack_objects().