repack: don't remove .keep packs with `--pack-kept-objects`

`git repack` supports a `--pack-kept-objects` flag which more or less translates to whether or not we pass `--honor-pack-keep` down to `git pack-objects` when assembling a new pack. This behavior has existed since ee34a2bead (repack: add `repack.packKeptObjects` config var, 2014-03-03). In that commit, the documentation was extended to say: [...] Note that we still do not delete `.keep` packs after `pack-objects` finishes. Unfortunately, this is not the case when `--pack-kept-objects` is combined with a `--geometric` repack. When doing a geometric repack, we include `.keep` packs when enumerating available packs only when `pack_kept_objects` is set. So this all works fine when `--no-pack-kept-objects` (or similar) is given. Kept packs are excluded from the geometric roll-up, so when we go to delete redundant packs (with `-d`), no `.keep` packs appear "below the split" in our geometric progression. But when `--pack-kept-objects` is given, things can go awry. Namely, when a kept pack is included in the list of packs tracked by the `pack_geometry` struct *and* part of the pack roll-up, we will delete the `.keep` pack when we shouldn't. Note that this *doesn't* result in object corruption, since the `.keep` pack's objects are still present in the new pack. But the `.keep` pack itself is removed, which violates our promise from back in ee34a2bead. But there's more. Because `repack` computes the geometric roll-up independently from selecting which packs belong in a MIDX (with `--write-midx`), this can lead to odd behavior. Consider when a `.keep` pack appears below the geometric split (ie., its objects will be part of the new pack we generate). We'll write a MIDX containing the new pack along with the existing `.keep` pack. But because the `.keep` pack appears below the geometric split line, we'll (incorrectly) try to remove it. While this doesn't corrupt the repository, it does cause us to remove the MIDX we just wrote, since removing that pack would invalidate the new MIDX. Funny enough, this behavior became far less noticeable after e4d0c11c04 (repack: respect kept objects with '--write-midx -b', 2021-12-20), which made `pack_kept_objects` be enabled by default only when we were writing a non-MIDX bitmap. But e4d0c11c04 didn't resolve this bug, it just made it harder to notice unless callers explicitly passed `--pack-kept-objects`. The solution is to avoid trying to remove `.keep` packs during `--geometric` repacks, even when they appear below the geometric split line, which is the approach this patch implements. Co-authored-by: Victoria Dye <vdye@github.com> Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Taylor Blau committed Oct 17, 2022 at 22:26 UTC 197443e80ab03a6fc9c018a561cd1b0349bb4c05
3 files changed +34 -1
builtin/repack.c
+5
@@ -1089,6 +1089,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
1089 strbuf_addstr(&buf, pack_basename(p));
1090 strbuf_strip_suffix(&buf, ".pack");
1091
1092 + if ((p->pack_keep) ||
1093 + (string_list_has_string(&existing_kept_packs,
1094 + buf.buf)))
1095 + continue;
1096 +
1097 remove_redundant_pack(packdir, buf.buf);
1098 }
1099 strbuf_release(&buf);
t/t7700-repack.sh
+24
@@ -426,6 +426,30 @@ test_expect_success '--write-midx -b packs non-kept objects' '
426 )
427 '
428
429 +test_expect_success '--write-midx with --pack-kept-objects' '
430 + git init repo &&
431 + test_when_finished "rm -fr repo" &&
432 + (
433 + cd repo &&
434 +
435 + test_commit one &&
436 + test_commit two &&
437 +
438 + one="$(echo "one" | git pack-objects --revs $objdir/pack/pack)" &&
439 + two="$(echo "one..two" | git pack-objects --revs $objdir/pack/pack)" &&
440 +
441 + keep="$objdir/pack/pack-$one.keep" &&
442 + touch "$keep" &&
443 +
444 + git repack --write-midx --write-bitmap-index --geometric=2 -d \
445 + --pack-kept-objects &&
446 +
447 + test_path_is_file $keep &&
448 + test_path_is_file $midx &&
449 + test_path_is_file $midx-$(midx_checksum $objdir).bitmap
450 + )
451 +'
452 +
453 test_expect_success TTY '--quiet disables progress' '
454 test_terminal env GIT_PROGRESS_DELAY=0 \
455 git -C midx repack -ad --quiet --write-midx 2>stderr &&
t/t7703-repack-geometric.sh
+5 -1
@@ -176,8 +176,12 @@ test_expect_success '--geometric ignores kept packs' '
176 # be repacked, too.
177 git repack --geometric 2 -d --pack-kept-objects &&
178
179 + # After repacking, two packs remain: one new one (containing the
180 + # objects in both the .keep and non-kept pack), and the .keep
181 + # pack (since `--pack-kept-objects -d` does not actually delete
182 + # the kept pack).
183 find $objdir/pack -name "*.pack" >after &&
180 - test_line_count = 1 after
184 + test_line_count = 2 after
185 )
186 '
187