| Message ID | 9b278a4c91a5631d1b7b11bf56ab560c6bb58645.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:48PM +0200, Patrick Steinhardt wrote: > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c > index fdf3f440be..522eb4dd31 100644 > --- a/builtin/pack-objects.c > +++ b/builtin/pack-objects.c > @@ -3366,11 +3366,9 @@ static void read_packs_list_from_stdin(void) > for (p = get_all_packs(the_repository); p; p = p->next) { > const char *pack_name = pack_basename(p); > > - item = string_list_lookup(&include_packs, pack_name); > - if (!item) > - item = string_list_lookup(&exclude_packs, pack_name); > - > - if (item) > + if ((item = string_list_lookup(&include_packs, pack_name))) > + item->util = p; > + if ((item = string_list_lookup(&exclude_packs, pack_name))) > item->util = p; Oof. I was hoping that we could avoid having to look through both lists. But that relies on us having disjoint sets of packs in the include and exclude lists. We probably *could* just ban this combination outright, but that would also involve some work to try and detect that case. So I think that doing this (and ensuring that the resulting pack is empty, that is that the exclude set takes precedence here) is the right thing to do. This (and the elided test below) look great to me. Thanks, Taylor
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index fdf3f440be..522eb4dd31 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -3366,11 +3366,9 @@ static void read_packs_list_from_stdin(void) for (p = get_all_packs(the_repository); p; p = p->next) { const char *pack_name = pack_basename(p); - item = string_list_lookup(&include_packs, pack_name); - if (!item) - item = string_list_lookup(&exclude_packs, pack_name); - - if (item) + if ((item = string_list_lookup(&include_packs, pack_name))) + item->util = p; + if ((item = string_list_lookup(&exclude_packs, pack_name))) item->util = p; } diff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh index ab34cfc729..f389a78b38 100755 --- a/t/t5331-pack-objects-stdin.sh +++ b/t/t5331-pack-objects-stdin.sh @@ -7,6 +7,12 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh +packed_objects() { + git show-index <"$1" >tmp-object-list && + cut -d' ' -f2 tmp-object-list && + rm tmp-object-list + } + test_expect_success 'pack-objects --stdin with duplicate packfile' ' test_when_finished "rm -fr repo" && @@ -26,4 +32,24 @@ test_expect_success 'pack-objects --stdin with duplicate packfile' ' ) ' +test_expect_success 'pack-objects --stdin with same packfile excluded and included' ' + test_when_finished "rm -fr repo" && + + git init repo && + ( + cd repo && + test_commit "commit" && + git repack -ad && + + ( + basename .git/objects/pack/pack-*.pack && + printf "^%s\n" "$(basename .git/objects/pack/pack-*.pack)" + ) >packfiles && + + git pack-objects --stdin-packs generated-pack <packfiles && + packed_objects generated-pack-*.idx >packed-objects && + test_must_be_empty packed-objects + ) +' + test_done
When passing the same packfile both as included and excluded via the `--stdin-packs` option, then we will return an error because the excluded packfile cannot be found. This is because we will only set the `util` pointer for the included packfile list if it was found, so that we later die when we notice that it's in fact not set for the excluded packfile list. Fix this bug by always setting the `util` pointer for both the included and excluded list entries. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/pack-objects.c | 8 +++----- t/t5331-pack-objects-stdin.sh | 26 ++++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 5 deletions(-)