| Message ID | f82e44f1da47890b7ef6ee2f5c1cdbbe20fa6684.1681294715.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | repack: fix geometric repacking with gitalternates | expand |
On Wed, Apr 12, 2023 at 12:22:39PM +0200, Patrick Steinhardt wrote: > When writing the multi-pack-index with geometric repacking we will add > all packfiles to the index that are part of the geometric sequence. This > can potentially also include packfiles borrowed from an alternate object > directory. But given that a multi-pack-index can only ever include packs > that are part of the main object database this does not make much sense > whatsoever. > > In the edge case where all packfiles are contained in the alternate > object database and the local repository has none itself this bug can > cause us to invoke git-multi-pack-index(1) with only non-local packfiles > that it ultimately cannot find. This causes it to return an error and > thus causes the geometric repack to fail. > > Fix the code to skip non-local packfiles. > > Co-authored-by: Taylor Blau <me@ttaylorr.com> Thanks for listing me as a co-author. Doing so probably requires my S-o-b, which you are more than free to forge here. > + # Assert that we wrote neither a new packfile nor a multi-pack-index. > + # We should not have a packfile because the single packfile in the > + # alternate object database does not invalidate the geometric sequence. > + # And we should not have a multi-pack-index because these only index > + # local packfiles, and there are none. > + find member/.git/objects/pack -type f >actual && > + test_must_be_empty actual This test looks good, though one small opportunity for cleanup might be to replace this with a single: test_dir_is_empty member/$packdir Thanks, Taylor
diff --git a/builtin/repack.c b/builtin/repack.c index 9d36dc8b84..80d4e813c8 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -569,6 +569,17 @@ static void midx_included_packs(struct string_list *include, for (i = geometry->split; i < geometry->pack_nr; i++) { struct packed_git *p = geometry->pack[i]; + /* + * The multi-pack index never refers to packfiles part + * of an alternate object database, so we skip these. + * While git-multi-pack-index(1) would silently ignore + * them anyway, this allows us to skip executing the + * command completely when we have only non-local + * packfiles. + */ + if (!p->pack_local) + continue; + strbuf_addstr(&buf, pack_basename(p)); strbuf_strip_suffix(&buf, ".pack"); strbuf_addstr(&buf, ".idx"); diff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh index 92a1aaa754..9e95350cbf 100755 --- a/t/t7703-repack-geometric.sh +++ b/t/t7703-repack-geometric.sh @@ -312,4 +312,28 @@ test_expect_success '--geometric --write-midx with packfiles in main and alterna git -C member multi-pack-index verify ' +test_expect_success '--geometric --with-midx with no local objects' ' + test_when_finished "rm -fr shared member" && + + # Create a repository with a single packfile that acts as alternate + # object database. + git init shared && + test_commit -C shared "shared-objects" && + git -C shared repack -ad && + + # Create a second repository linked to the first one and perform a + # geometric repack on it. + git clone --shared shared member && + git -C member repack --geometric 2 --write-midx 2>err && + test_must_be_empty err && + + # Assert that we wrote neither a new packfile nor a multi-pack-index. + # We should not have a packfile because the single packfile in the + # alternate object database does not invalidate the geometric sequence. + # And we should not have a multi-pack-index because these only index + # local packfiles, and there are none. + find member/.git/objects/pack -type f >actual && + test_must_be_empty actual +' + test_done
When writing the multi-pack-index with geometric repacking we will add all packfiles to the index that are part of the geometric sequence. This can potentially also include packfiles borrowed from an alternate object directory. But given that a multi-pack-index can only ever include packs that are part of the main object database this does not make much sense whatsoever. In the edge case where all packfiles are contained in the alternate object database and the local repository has none itself this bug can cause us to invoke git-multi-pack-index(1) with only non-local packfiles that it ultimately cannot find. This causes it to return an error and thus causes the geometric repack to fail. Fix the code to skip non-local packfiles. Co-authored-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/repack.c | 11 +++++++++++ t/t7703-repack-geometric.sh | 24 ++++++++++++++++++++++++ 2 files changed, 35 insertions(+)