* 'ds/test-multi-pack-index' vs. 'ab/commit-graph-progress'
@ 2018-10-25 11:15 SZEDER Gábor
2018-10-25 12:54 ` [PATCH] packfile: close multi-pack-index in close_all_packs Derrick Stolee
2018-10-25 21:11 ` 'ds/test-multi-pack-index' vs. 'ab/commit-graph-progress' Ævar Arnfjörð Bjarmason
0 siblings, 2 replies; 6+ messages in thread
From: SZEDER Gábor @ 2018-10-25 11:15 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, Derrick Stolee; +Cc: git
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'.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] packfile: close multi-pack-index in close_all_packs
2018-10-25 11:15 'ds/test-multi-pack-index' vs. 'ab/commit-graph-progress' SZEDER Gábor
@ 2018-10-25 12:54 ` Derrick Stolee
2018-10-29 11:10 ` SZEDER Gábor
2018-10-25 21:11 ` 'ds/test-multi-pack-index' vs. 'ab/commit-graph-progress' Ævar Arnfjörð Bjarmason
1 sibling, 1 reply; 6+ messages in thread
From: Derrick Stolee @ 2018-10-25 12:54 UTC (permalink / raw)
To: git; +Cc: szeder.dev, avarab, dstolee
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: 'ds/test-multi-pack-index' vs. 'ab/commit-graph-progress'
2018-10-25 11:15 'ds/test-multi-pack-index' vs. 'ab/commit-graph-progress' SZEDER Gábor
2018-10-25 12:54 ` [PATCH] packfile: close multi-pack-index in close_all_packs Derrick Stolee
@ 2018-10-25 21:11 ` Ævar Arnfjörð Bjarmason
2018-10-29 11:16 ` SZEDER Gábor
1 sibling, 1 reply; 6+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-10-25 21:11 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Derrick Stolee, git
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().
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] packfile: close multi-pack-index in close_all_packs
2018-10-25 12:54 ` [PATCH] packfile: close multi-pack-index in close_all_packs Derrick Stolee
@ 2018-10-29 11:10 ` SZEDER Gábor
2018-10-29 13:15 ` Derrick Stolee
0 siblings, 1 reply; 6+ messages in thread
From: SZEDER Gábor @ 2018-10-29 11:10 UTC (permalink / raw)
To: Derrick Stolee; +Cc: git, avarab, dstolee
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: 'ds/test-multi-pack-index' vs. 'ab/commit-graph-progress'
2018-10-25 21:11 ` 'ds/test-multi-pack-index' vs. 'ab/commit-graph-progress' Ævar Arnfjörð Bjarmason
@ 2018-10-29 11:16 ` SZEDER Gábor
0 siblings, 0 replies; 6+ messages in thread
From: SZEDER Gábor @ 2018-10-29 11:16 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: Derrick Stolee, git
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] packfile: close multi-pack-index in close_all_packs
2018-10-29 11:10 ` SZEDER Gábor
@ 2018-10-29 13:15 ` Derrick Stolee
0 siblings, 0 replies; 6+ messages in thread
From: Derrick Stolee @ 2018-10-29 13:15 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: git, avarab, dstolee
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-10-29 13:15 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-25 11:15 'ds/test-multi-pack-index' vs. 'ab/commit-graph-progress' SZEDER Gábor
2018-10-25 12:54 ` [PATCH] packfile: close multi-pack-index in close_all_packs Derrick Stolee
2018-10-29 11:10 ` SZEDER Gábor
2018-10-29 13:15 ` Derrick Stolee
2018-10-25 21:11 ` 'ds/test-multi-pack-index' vs. 'ab/commit-graph-progress' Ævar Arnfjörð Bjarmason
2018-10-29 11:16 ` SZEDER Gábor
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).