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

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

[SZEDER Gábor]

Hi,

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

[PATCH] packfile: close multi-pack-index in close_all_packs

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

Re: [PATCH] packfile: close multi-pack-index in close_all_packs

[SZEDER Gábor]

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.

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

Re: [PATCH] packfile: close multi-pack-index in close_all_packs

[Derrick Stolee]

On 10/29/2018 7:10 AM, SZEDER Gábor wrote:
> 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 I added GIT_TEST_MULTI_PACK_INDEX, one of the important pieces was 
to delete the multi-pack-index file whenever deleting the pack-files it 
contains. This only happens during a 'git repack'.

The thing I had missed (and is covered by this patch) is when we run a 
subcommand that can remove pack-files while we have a multi-pack-index open.

The reason this wasn't caught is that the test suite did not include any 
cases where the following things happened in order:

1. Open pack-files and multi-pack-index.
2. Delete pack-files, but keep multi-pack-index open.
3. Read objects (from the multi-pack-index).

This step 3 was added to 'git gc' by the commit-graph write, hence the 
break. The pack-file deletion happens in the repack subcommand.

Since close_all_packs() is the way we guarded against this problem in 
the traditional pack-file environment, this is the right place to insert 
a close_midx() call, and will avoid cases like this in the future (at 
least, the multi-pack-index will not be the reason on its own).

Thanks,
-Stolee

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

[Ævar Arnfjörð Bjarmason]

On Thu, Oct 25 2018, SZEDER Gábor wrote:

> 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

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

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

[SZEDER Gábor]

On Thu, Oct 25, 2018 at 11:11:55PM +0200, Ævar Arnfjörð Bjarmason wrote:
> > 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.

I can image more prudent approaches that would have done the same,
e.g. '! grep ^error 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'.
> 
> 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().

This explains what implementation details led to the current
behaviour, but does not justify the actual inconsistency.