'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 <szeder.dev@gmail.com>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> 
> 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.

> -Stolee
> 
>  packfile.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/packfile.c b/packfile.c
> index 841b36182f..37fcd8f136 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -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
> -- 
> 2.17.1
> 

> On Thu, Oct 25, 2018 at 12:54:05PM +0000, Derrick Stolee wrote:
>> 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 <szeder.dev@gmail.com>
>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>> ---
>>
>> 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

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

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