{"thread":{"id":"58641","subject":"[PATCH] repack: don't remove .keep packs with `--pack-kept-objects`","startedAt":"2022-10-18T02:26:14Z","lastAt":"2022-10-21T03:14:19Z","messageCount":3,"participants":["Taylor Blau","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"465164","messageId":"6a012cd625c1d197ede91c85299cbfb37adf356b.1666059872.git.me@ttaylorr.com","threadId":"58641","inReplyTo":null,"subject":"[PATCH] repack: don't remove .keep packs with `--pack-kept-objects`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-18T02:26:06Z","receivedAt":"2022-10-18T02:26:14Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"`git repack` supports a `--pack-kept-objects` flag which more or less\ntranslates to whether or not we pass `--honor-pack-keep` down to `git\npack-objects` when assembling a new pack.\n\nThis behavior has existed since ee34a2bead (repack: add\n`repack.packKeptObjects` config var, 2014-03-03). In that commit, the\ndocumentation was extended to say:\n\n    [...] Note that we still do not delete `.keep` packs after\n    `pack-objects` finishes.\n\nUnfortunately, this is not the case when `--pack-kept-objects` is\ncombined with a `--geometric` repack. When doing a geometric repack, we\ninclude `.keep` packs when enumerating available packs only when\n`pack_kept_objects` is set.\n\nSo this all works fine when `--no-pack-kept-objects` (or similar) is\ngiven. Kept packs are excluded from the geometric roll-up, so when we go\nto delete redundant packs (with `-d`), no `.keep` packs appear \"below\nthe split\" in our geometric progression.\n\nBut when `--pack-kept-objects` is given, things can go awry. Namely,\nwhen a kept pack is included in the list of packs tracked by the\n`pack_geometry` struct *and* part of the pack roll-up, we will delete\nthe `.keep` pack when we shouldn't.\n\nNote that this *doesn't* result in object corruption, since the `.keep`\npack's objects are still present in the new pack. But the `.keep` pack\nitself is removed, which violates our promise from back in ee34a2bead.\n\nBut there's more. Because `repack` computes the geometric roll-up\nindependently from selecting which packs belong in a MIDX (with\n`--write-midx`), this can lead to odd behavior. Consider when a `.keep`\npack appears below the geometric split (ie., its objects will be part of\nthe new pack we generate).\n\nWe'll write a MIDX containing the new pack along with the existing\n`.keep` pack. But because the `.keep` pack appears below the geometric\nsplit line, we'll (incorrectly) try to remove it. While this doesn't\ncorrupt the repository, it does cause us to remove the MIDX we just\nwrote, since removing that pack would invalidate the new MIDX.\n\nFunny enough, this behavior became far less noticeable after e4d0c11c04\n(repack: respect kept objects with '--write-midx -b', 2021-12-20), which\nmade `pack_kept_objects` be enabled by default only when we were writing\na non-MIDX bitmap.\n\nBut e4d0c11c04 didn't resolve this bug, it just made it harder to notice\nunless callers explicitly passed `--pack-kept-objects`.\n\nThe solution is to avoid trying to remove `.keep` packs during\n`--geometric` repacks, even when they appear below the geometric split\nline, which is the approach this patch implements.\n\nCo-authored-by: Victoria Dye <vdye@github.com>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\nWe at GitHub had a slow stream of repacks that removed the just-written\nMIDX.\n\nThis had mostly been a mystery that didn't occur at high enough volume\nto justify looking into. But it went away entirely after merging in\nv2.36.x, which contains e4d0c11c04.\n\nSome investigating with Victoria today led to the patch above, which is\nstill relevant since e4d0c11c04 papers over an existing bug.\n\n builtin/repack.c            |  5 +++++\n t/t7700-repack.sh           | 24 ++++++++++++++++++++++++\n t/t7703-repack-geometric.sh |  6 +++++-\n 3 files changed, 34 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex a5bacc7797..f71909696d 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -1089,6 +1089,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tstrbuf_addstr(&buf, pack_basename(p));\n \t\t\t\tstrbuf_strip_suffix(&buf, \".pack\");\n\n+\t\t\t\tif ((p->pack_keep) ||\n+\t\t\t\t    (string_list_has_string(&existing_kept_packs,\n+\t\t\t\t\t\t\t    buf.buf)))\n+\t\t\t\t\tcontinue;\n+\n \t\t\t\tremove_redundant_pack(packdir, buf.buf);\n \t\t\t}\n \t\t\tstrbuf_release(&buf);\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex ca45c4cd2c..df8e94d7a8 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -426,6 +426,30 @@ test_expect_success '--write-midx -b packs non-kept objects' '\n \t)\n '\n\n+test_expect_success '--write-midx with --pack-kept-objects' '\n+\tgit init repo &&\n+\ttest_when_finished \"rm -fr repo\" &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\ttest_commit one &&\n+\t\ttest_commit two &&\n+\n+\t\tone=\"$(echo \"one\" | git pack-objects --revs $objdir/pack/pack)\" &&\n+\t\ttwo=\"$(echo \"one..two\" | git pack-objects --revs $objdir/pack/pack)\" &&\n+\n+\t\tkeep=\"$objdir/pack/pack-$one.keep\" &&\n+\t\ttouch \"$keep\" &&\n+\n+\t\tgit repack --write-midx --write-bitmap-index --geometric=2 -d \\\n+\t\t\t--pack-kept-objects &&\n+\n+\t\ttest_path_is_file $keep &&\n+\t\ttest_path_is_file $midx &&\n+\t\ttest_path_is_file $midx-$(midx_checksum $objdir).bitmap\n+\t)\n+'\n+\n test_expect_success TTY '--quiet disables progress' '\n \ttest_terminal env GIT_PROGRESS_DELAY=0 \\\n \t\tgit -C midx repack -ad --quiet --write-midx 2>stderr &&\ndiff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\nindex da87f8b2d8..8821fbd2dd 100755\n--- a/t/t7703-repack-geometric.sh\n+++ b/t/t7703-repack-geometric.sh\n@@ -176,8 +176,12 @@ test_expect_success '--geometric ignores kept packs' '\n \t\t# be repacked, too.\n \t\tgit repack --geometric 2 -d --pack-kept-objects &&\n\n+\t\t# After repacking, two packs remain: one new one (containing the\n+\t\t# objects in both the .keep and non-kept pack), and the .keep\n+\t\t# pack (since `--pack-kept-objects -d` does not actually delete\n+\t\t# the kept pack).\n \t\tfind $objdir/pack -name \"*.pack\" >after &&\n-\t\ttest_line_count = 1 after\n+\t\ttest_line_count = 2 after\n \t)\n '\n\n--\n2.38.0.16.g393fd4c6db\n"},{"id":"465176","messageId":"Y05NwIfY/906b2Vd@coredump.intra.peff.net","threadId":"58641","inReplyTo":"6a012cd625c1d197ede91c85299cbfb37adf356b.1666059872.git.me@ttaylorr.com","subject":"Re: [PATCH] repack: don't remove .keep packs with `--pack-kept-objects`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-18T06:54:56Z","receivedAt":"2022-10-18T06:55:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 17, 2022 at 10:26:06PM -0400, Taylor Blau wrote:\n\n> But when `--pack-kept-objects` is given, things can go awry. Namely,\n> when a kept pack is included in the list of packs tracked by the\n> `pack_geometry` struct *and* part of the pack roll-up, we will delete\n> the `.keep` pack when we shouldn't.\n\nOops. Your explanation makes sense, and this is an easy case to miss.\n\nAfter reading the message but before seeing the patch, I'd guess we just\nneed to add \"if (!this_pack_is_kept)\" to the removal loop.\n\nAnd indeed, it's close to that:\n\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index a5bacc7797..f71909696d 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -1089,6 +1089,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \t\t\t\tstrbuf_addstr(&buf, pack_basename(p));\n>  \t\t\t\tstrbuf_strip_suffix(&buf, \".pack\");\n> \n> +\t\t\t\tif ((p->pack_keep) ||\n> +\t\t\t\t    (string_list_has_string(&existing_kept_packs,\n> +\t\t\t\t\t\t\t    buf.buf)))\n> +\t\t\t\t\tcontinue;\n> +\n>  \t\t\t\tremove_redundant_pack(packdir, buf.buf);\n>  \t\t\t}\n>  \t\t\tstrbuf_release(&buf);\n\nbut what is the difference between p->pack_keep, and being in the\nexisting_kept_packs list? From the similar logic in\ninit_pack_geometry():\n\n         /*\n          * Any pack that has its pack_keep bit set will appear\n          * in existing_kept_packs below, but this saves us from\n          * doing a more expensive check.\n          */\n         if (p->pack_keep)\n                 continue;\n\n         /*\n          * The pack may be kept via the --keep-pack option;\n          * check 'existing_kept_packs' to determine whether to\n          * ignore it.\n          */\n         strbuf_reset(&buf);\n         strbuf_addstr(&buf, pack_basename(p));\n         strbuf_strip_suffix(&buf, \".pack\");\n\n         if (string_list_has_string(existing_kept_packs, buf.buf))\n                  continue;\n\nit looks like one is a superset of the other, but checking p->pack_keep\nis just slightly cheaper, so we do it first. And checking just\np->pack_keep isn't sufficient; the list has extra packs from the\n--keep-pack command-line. So your new code checking both is correct.\n\nBut one thing to note. In that existing logic, the two checks are split\napart, since in the fast path (i.e., when p->pack_keep is set) we can\nskip the extra work to form the pack string. We could do that here, but\nI doubt it matters much:\n\n  1. It's really not that expensive. It's a little extra string\n     manipulation and a binary-search lookup for each pack.\n\n  2. Normally most packs aren't kept, so we'd end up in the \"slow\" path\n     most of the time anyway.\n\nSo I suspect that existing code is premature optimization, and you\nreally could just look in existing_kept_packs (both there and in this\npatch).\n\nBut I don't mind this patch copying the existing logic in the meantime.\nIt's exactly this kind of \"while we are in the area\" cleanup where you\nend up surprised that the p->pack_keep check really was covering some\nsubtle corner case. ;)\n\nSo the patch looks good to me as-is (and sorry for the verbose review;\nwe've just had enough tricky corner cases in this repack code that I\nwanted to make sure I understood all the implications).\n\n> This had mostly been a mystery that didn't occur at high enough volume\n> to justify looking into. But it went away entirely after merging in\n> v2.36.x, which contains e4d0c11c04.\n> \n> Some investigating with Victoria today led to the patch above, which is\n> still relevant since e4d0c11c04 papers over an existing bug.\n\nThank you both for digging into this. Because of the scale, GitHub has a\nunique opportunity to trigger these kind of weird corner cases and races\nthat would otherwise go unreported and just cause occasional\nhead-scratching among people running sane-sized Git servers.\n\n-Peff\n"},{"id":"465408","messageId":"Y1IOhH2jAwHHrLY+@nand.local","threadId":"58641","inReplyTo":"Y05NwIfY/906b2Vd@coredump.intra.peff.net","subject":"Re: [PATCH] repack: don't remove .keep packs with `--pack-kept-objects`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-21T03:14:12Z","receivedAt":"2022-10-21T03:14:19Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Oct 18, 2022 at 02:54:56AM -0400, Jeff King wrote:\n> So the patch looks good to me as-is (and sorry for the verbose review;\n> we've just had enough tricky corner cases in this repack code that I\n> wanted to make sure I understood all the implications).\n\nYeah, I agree that this is probably premature optimization, but in\npractice it doesn't hurt much. Copying it was definitely intentional,\nand I share your general unease about changing it in this patch.\n\nThanks,\nTaylor\n"}]}