| Message ID | 5ecad306b42441fa7d4f50bdfb9c09ccce22b6c9.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:31PM +0200, Patrick Steinhardt wrote: > Fix this bug by exiting early in case we have determined that the MIDX > wouldn't have any packfiles to index. While the request itself does not > make much sense anyway, it is still preferable to exit gracefully than > to abort. Interesting. This reminded me quite a bit of eb57277ba3 (midx: prevent writing a .bitmap without any objects, 2022-02-09) which tackled a similar problem of trying to write a MIDX bitmap without any objects. We may want to consider moving that conditional further up, since this makes the conditional added in eb57277ba3 dead code AFAICT. Here's a patch on top of this one that I think would do the trick. It has the added benefit of sticking a: warning: unknown preferred pack: 'does-not-exist' in the output before dying, which might be nice (though I doubt anybody will ever see it ;-)). The main difference is that we unset the bitmap related bits from `flags`, which avoids us trying to compute a preferred pack in the first place. For it to work, though, we need to make sure that ctx.preferred_pack_idx is set to -1, and not zero-initialized, since we'll segfault otherwise when trying to read into an empty array. --- 8< --- diff --git a/midx.c b/midx.c index 22ea7ffb75..dc4821eab8 100644 --- a/midx.c +++ b/midx.c @@ -1263,6 +1263,7 @@ static int write_midx_internal(const char *object_dir, ctx.nr = 0; ctx.alloc = ctx.m ? ctx.m->num_packs : 16; ctx.info = NULL; + ctx.preferred_pack_idx = -1; ALLOC_ARRAY(ctx.info, ctx.alloc); if (ctx.m) { @@ -1307,10 +1308,10 @@ static int write_midx_internal(const char *object_dir, for_each_file_in_pack_dir(object_dir, add_pack_to_midx, &ctx); stop_progress(&ctx.progress); - if (!ctx.nr) { - error(_("no pack files to index.")); - result = 1; - goto cleanup; + if (!ctx.entries_nr) { + if (flags & MIDX_WRITE_BITMAP) + warning(_("refusing to write multi-pack .bitmap without any objects")); + flags &= ~(MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP); } if ((ctx.m && ctx.nr == ctx.m->num_packs) && @@ -1488,12 +1489,6 @@ static int write_midx_internal(const char *object_dir, goto cleanup; } - if (!ctx.entries_nr) { - if (flags & MIDX_WRITE_BITMAP) - warning(_("refusing to write multi-pack .bitmap without any objects")); - flags &= ~(MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP); - } - cf = init_chunkfile(f); add_chunk(cf, MIDX_CHUNKID_PACKNAMES, pack_name_concat_len, diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh index be7f3c1e1f..8a17361272 100755 --- a/t/t5319-multi-pack-index.sh +++ b/t/t5319-multi-pack-index.sh @@ -188,10 +188,9 @@ test_expect_success 'write with no objects and preferred pack' ' git init empty && test_must_fail git -C empty multi-pack-index write \ --stdin-packs --preferred-pack=does-not-exist </dev/null 2>err && - cat >expect <<-EOF && - error: no pack files to index. - EOF - test_cmp expect err + cat err && + grep "warning: unknown preferred pack: .does-not-exist." err && + grep "error: no pack files to index." err ' test_expect_success 'write progress off for redirected stderr' ' --- >8 --- Thanks, Taylor
On Wed, Apr 12, 2023 at 01:56:58PM -0400, Taylor Blau wrote: > On Wed, Apr 12, 2023 at 12:22:31PM +0200, Patrick Steinhardt wrote: > > Fix this bug by exiting early in case we have determined that the MIDX > > wouldn't have any packfiles to index. While the request itself does not > > make much sense anyway, it is still preferable to exit gracefully than > > to abort. > > Interesting. This reminded me quite a bit of eb57277ba3 (midx: prevent > writing a .bitmap without any objects, 2022-02-09) which tackled a > similar problem of trying to write a MIDX bitmap without any objects. > > We may want to consider moving that conditional further up, since this > makes the conditional added in eb57277ba3 dead code AFAICT. Here's a > patch on top of this one that I think would do the trick. > > It has the added benefit of sticking a: > > warning: unknown preferred pack: 'does-not-exist' > > in the output before dying, which might be nice (though I doubt anybody > will ever see it ;-)). The main difference is that we unset the bitmap > related bits from `flags`, which avoids us trying to compute a preferred > pack in the first place. > > For it to work, though, we need to make sure that ctx.preferred_pack_idx > is set to -1, and not zero-initialized, since we'll segfault otherwise > when trying to read into an empty array. Indeed, that is a good point. I think we can simplify your patch even further in that case: diff --git a/midx.c b/midx.c index 47989f7ea7..67eb617591 100644 --- a/midx.c +++ b/midx.c @@ -1328,17 +1328,17 @@ static int write_midx_internal(const char *object_dir, } if (preferred_pack_name) { - int found = 0; + ctx.preferred_pack_idx = -1; + for (i = 0; i < ctx.nr; i++) { if (!cmp_idx_or_pack_name(preferred_pack_name, ctx.info[i].pack_name)) { ctx.preferred_pack_idx = i; - found = 1; break; } } - if (!found) + if (ctx.preferred_pack_idx == -1) warning(_("unknown preferred pack: '%s'"), preferred_pack_name); } else if (ctx.nr && The other cases already set `preferred_pack_idx = -1`, so this is really all we need to do to fix the segfault. Patrick
diff --git a/midx.c b/midx.c index 9af3e5de88..22ea7ffb75 100644 --- a/midx.c +++ b/midx.c @@ -1307,6 +1307,12 @@ static int write_midx_internal(const char *object_dir, for_each_file_in_pack_dir(object_dir, add_pack_to_midx, &ctx); stop_progress(&ctx.progress); + if (!ctx.nr) { + error(_("no pack files to index.")); + result = 1; + goto cleanup; + } + if ((ctx.m && ctx.nr == ctx.m->num_packs) && !(packs_to_include || packs_to_drop)) { struct bitmap_index *bitmap_git; diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh index 499d5d4c78..be7f3c1e1f 100755 --- a/t/t5319-multi-pack-index.sh +++ b/t/t5319-multi-pack-index.sh @@ -183,6 +183,17 @@ test_expect_success 'write midx with --stdin-packs' ' compare_results_with_midx "mixed mode (one pack + extra)" +test_expect_success 'write with no objects and preferred pack' ' + test_when_finished "rm -rf empty" && + git init empty && + test_must_fail git -C empty multi-pack-index write \ + --stdin-packs --preferred-pack=does-not-exist </dev/null 2>err && + cat >expect <<-EOF && + error: no pack files to index. + EOF + test_cmp expect err +' + test_expect_success 'write progress off for redirected stderr' ' git multi-pack-index --object-dir=$objdir write 2>err && test_line_count = 0 err
When asked to write a multi-pack-index the user can specify a preferred pack that is used as a tie breaker when multiple packs contain the same objects. When this packfile cannot be found, we just pick the first pack that is getting tracked by the newly written multi-pack-index as a fallback. Picking the fallback can fail in the case where we're asked to write a multi-pack-index with no packfiles at all: picking the fallback value will cause a segfault as we blindly index into the array of packfiles, which would be empty. Fix this bug by exiting early in case we have determined that the MIDX wouldn't have any packfiles to index. While the request itself does not make much sense anyway, it is still preferable to exit gracefully than to abort. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- midx.c | 6 ++++++ t/t5319-multi-pack-index.sh | 11 +++++++++++ 2 files changed, 17 insertions(+)