| Message ID | dd8145beade440e5444130d3a3189b2c5d15a911.1681384405.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ceb96a160b05c3ec45c416851bac00b711418839 |
| Headers | show |
| Series | repack: fix geometric repacking with gitalternates | expand |
On 4/13/2023 7:16 AM, Patrick Steinhardt wrote: > Fix this bug by resetting the preferred packfile index to `-1` before > searching for the preferred pack. This fixes the segfault as we already > check for whether the index is `> - 1`. If it is not, we simply don't > pick a preferred packfile at all. > if (preferred_pack_name) { > - int found = 0; > + ctx.preferred_pack_idx = -1; > + This patch looks good, but I did need to think about it for a bit, so here are my thoughts (feel free to ignore): I briefly considered recommending that we set this as the default in an initializer macro, something like #define WRITE_MIDX_CONTEXT_INIT { .preferred_pack_idx = -1 } but this struct is internal to the file and only constructed once (at the start of write_midx_internal). Further, outside the context of this patch we have this code: } else if (ctx.nr && (flags & (MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP))) { struct packed_git *oldest = ctx.info[ctx.preferred_pack_idx].p; ctx.preferred_pack_idx = 0; I don't see a way that ctx.preferred_pack_idx can be anything but zero here, but it's also not going to give a segfault because of the 'ctx.nr' condition. Thanks, -Stolee
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 && diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh index 499d5d4c78..0883c7c6bd 100755 --- a/t/t5319-multi-pack-index.sh +++ b/t/t5319-multi-pack-index.sh @@ -183,6 +183,18 @@ 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 && + warning: unknown preferred pack: ${SQ}does-not-exist${SQ} + 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 resetting the preferred packfile index to `-1` before searching for the preferred pack. This fixes the segfault as we already check for whether the index is `> - 1`. If it is not, we simply don't pick a preferred packfile at all. Helped-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Patrick Steinhardt <ps@pks.im> --- midx.c | 6 +++--- t/t5319-multi-pack-index.sh | 12 ++++++++++++ 2 files changed, 15 insertions(+), 3 deletions(-)