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>

diff --git a/builtin/repack.c b/builtin/repack.c
index a5bacc7797435696cd6e23e73f847fc00d39bb7f..f71909696dc5d94d02cbe9a26259745b52e11b98 100644 (file)
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -1089,6 +1089,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
                                strbuf_addstr(&buf, pack_basename(p));
                                strbuf_strip_suffix(&buf, ".pack");
 
+                               if ((p->pack_keep) ||
+                                   (string_list_has_string(&existing_kept_packs,
+                                                           buf.buf)))
+                                       continue;
+
                                remove_redundant_pack(packdir, buf.buf);
                        }
                        strbuf_release(&buf);

diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index ca45c4cd2c1a8a422e396fd4d29dee8c49c0c10e..df8e94d7a89a92f13df524d20c2890f467134509 100755 (executable)
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -426,6 +426,30 @@ test_expect_success '--write-midx -b packs non-kept objects' '
        )
 '
 
+test_expect_success '--write-midx with --pack-kept-objects' '
+       git init repo &&
+       test_when_finished "rm -fr repo" &&
+       (
+               cd repo &&
+
+               test_commit one &&
+               test_commit two &&
+
+               one="$(echo "one" | git pack-objects --revs $objdir/pack/pack)" &&
+               two="$(echo "one..two" | git pack-objects --revs $objdir/pack/pack)" &&
+
+               keep="$objdir/pack/pack-$one.keep" &&
+               touch "$keep" &&
+
+               git repack --write-midx --write-bitmap-index --geometric=2 -d \
+                       --pack-kept-objects &&
+
+               test_path_is_file $keep &&
+               test_path_is_file $midx &&
+               test_path_is_file $midx-$(midx_checksum $objdir).bitmap
+       )
+'
+
 test_expect_success TTY '--quiet disables progress' '
        test_terminal env GIT_PROGRESS_DELAY=0 \
                git -C midx repack -ad --quiet --write-midx 2>stderr &&

diff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh
index da87f8b2d8822cd5f844cfb00bcdaf5c52117f79..8821fbd2dd55844e54f55553b442ec419755546c 100755 (executable)
--- a/t/t7703-repack-geometric.sh
+++ b/t/t7703-repack-geometric.sh
@@ -176,8 +176,12 @@ test_expect_success '--geometric ignores kept packs' '
                # be repacked, too.
                git repack --geometric 2 -d --pack-kept-objects &&
 
+               # After repacking, two packs remain: one new one (containing the
+               # objects in both the .keep and non-kept pack), and the .keep
+               # pack (since `--pack-kept-objects -d` does not actually delete
+               # the kept pack).
                find $objdir/pack -name "*.pack" >after &&
-               test_line_count = 1 after
+               test_line_count = 2 after
        )
 '