{"thread":{"id":"62895","subject":"[PATCH 0/2] builtin/repack: fix `--keep-unreachable` when there are no packs","startedAt":"2025-02-03T13:07:11Z","lastAt":"2025-02-05T05:36:10Z","messageCount":15,"participants":["Patrick Steinhardt","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"511729","messageId":"20250203-b4-pks-repack-unreachable-objects-wo-packfiles-v1-0-7c4d69c5072c@pks.im","threadId":"62895","inReplyTo":null,"subject":"[PATCH 0/2] builtin/repack: fix `--keep-unreachable` when there are no packs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-03T13:06:53Z","receivedAt":"2025-02-03T13:07:11Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis small patch series fixes `git repack -ad --keep-unreachable` when\nthere aren't any preexisting packfiles.\n\nThanks!\n\nPatrick\n\n---\nPatrick Steinhardt (2):\n      t7700: add tests for `--keep-unreachable`\n      builtin/repack: fix `--keep-unreachable` when there are no packs\n\n builtin/repack.c  |  5 +++-\n t/t7700-repack.sh | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 77 insertions(+), 1 deletion(-)\n\n\n---\nbase-commit: 3b0d05c4a79d0e441283680a864529b02dca5f08\nchange-id: 20250203-b4-pks-repack-unreachable-objects-wo-packfiles-33c26066e5c3\n\n"},{"id":"511730","messageId":"20250203-b4-pks-repack-unreachable-objects-wo-packfiles-v1-2-7c4d69c5072c@pks.im","threadId":"62895","inReplyTo":"20250203-b4-pks-repack-unreachable-objects-wo-packfiles-v1-0-7c4d69c5072c@pks.im","subject":"[PATCH 2/2] builtin/repack: fix `--keep-unreachable` when there are no packs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-03T13:06:55Z","receivedAt":"2025-02-03T13:07:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The \"--keep-unreachable\" flag is supposed to append any unreachable\nobjects to the newly written pack. This flag is explicitly documented as\nappending both packed and loose unreachable objects to the new packfile.\nAnd while this works alright when repacking with preexisting packfiles,\nit stops working when the repository does not have any packfiles at all.\n\nThe root cause are the conditions used to decide whether or not we want\nto append \"--pack-loose-unreachable\" to git-pack-objects(1). There are\na couple of conditions here:\n\n  - `has_existing_non_kept_packs()` checks whether there are existing\n    packfiles. This condition makes sense to guard \"--keep-pack=\",\n    \"--unpack-unreachable\" and \"--keep-unreachable\", because all of\n    these flags only make sense in combination with existing packfiles.\n    But it does not make sense to disable `--pack-loose-unreachable`\n    when there aren't any preexisting packfiles, as loose objects can be\n    packed into the new packfile regardless of that.\n\n  - `delete_redundant` checks whether we want to delete any objects or\n    packs that are about to become redundant. The documentation of\n    `--keep-unreachable` explicitly says that `git repack -ad` needs to\n    be executed for the flag to have an effect.\n\n    It is not immediately obvious why such redundant objects need to be\n    deleted in order for \"--pack-unreachable-objects\" to be effective.\n    But as things are working as documented this is nothing we'll change\n    for now.\n\n  - `pack_everything & PACK_CRUFT` checks that we're not creating a\n    cruft pack. This condition makes sense in the context of\n    \"--pack-loose-unreachable\", as unreachable objects would end up in\n    the cruft pack anyway.\n\nSo while the second and third condition are sensible, it does not make\nany sense to condition `--pack-loose-unreachable` on the existence of\npackfiles.\n\nFix the bug by splitting out the \"--pack-loose-unreachable\" and only\nmaking it depend on the second and third condition. Like this, loose\nunreachable objects will be packed regardless of any preexisting\npackfiles.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/repack.c  | 5 ++++-\n t/t7700-repack.sh | 2 +-\n 2 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 81d13630ea..8194344b04 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -1370,9 +1370,12 @@ int cmd_repack(int argc,\n \t\t\t\t\t    \"--unpack-unreachable\");\n \t\t\t} else if (keep_unreachable) {\n \t\t\t\tstrvec_push(&cmd.args, \"--keep-unreachable\");\n-\t\t\t\tstrvec_push(&cmd.args, \"--pack-loose-unreachable\");\n \t\t\t}\n \t\t}\n+\n+\t\tif (keep_unreachable && delete_redundant &&\n+\t\t    !(pack_everything & PACK_CRUFT))\n+\t\t\tstrvec_push(&cmd.args, \"--pack-loose-unreachable\");\n \t} else if (geometry.split_factor) {\n \t\tstrvec_push(&cmd.args, \"--stdin-packs\");\n \t\tstrvec_push(&cmd.args, \"--unpacked\");\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex b26566473f..57523db696 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -880,7 +880,7 @@ test_expect_success '--keep-unreachable packs unreachable loose object with exis\n \t)\n '\n \n-test_expect_failure '--keep-unreachable packs unreachable loose object without existing packs' '\n+test_expect_success '--keep-unreachable packs unreachable loose object without existing packs' '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n \t(\n\n-- \n2.48.1.502.g6dc24dfdaf.dirty\n\n"},{"id":"511731","messageId":"20250203-b4-pks-repack-unreachable-objects-wo-packfiles-v1-1-7c4d69c5072c@pks.im","threadId":"62895","inReplyTo":"20250203-b4-pks-repack-unreachable-objects-wo-packfiles-v1-0-7c4d69c5072c@pks.im","subject":"[PATCH 1/2] t7700: add tests for `--keep-unreachable`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-03T13:06:54Z","receivedAt":"2025-02-03T13:07:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We don't have any tests for `git repack --keep-unreachable`. Add three\ntests that exercise its behaviour with different packed states for the\nunreachable object.\n\nNote that the last test is failing. This will be fixed in the next\ncommit.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n t/t7700-repack.sh | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 73 insertions(+)\n\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex be1188e736..b26566473f 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -826,4 +826,77 @@ test_expect_success '-n overrides repack.updateServerInfo=true' '\n \ttest_server_info_missing\n '\n \n+expect_object_count () {\n+\tfind .git/objects \\( -type d \\( -name pack -o -name info \\) -prune \\) -o -type f -print >objects &&\n+\ttest_line_count = \"$1\" objects\n+}\n+\n+expect_object_in_idx () {\n+\tgit verify-pack -v \"$1\" >objects &&\n+\ttest_grep \"^$2\" objects\n+}\n+\n+test_expect_success '--keep-unreachable appends unreachable packed objects to new pack' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit config set core.logAllRefUpdates false &&\n+\n+\t\t# Set up the repo so that all objects, including the\n+\t\t# unreachable one, are packed.\n+\t\ttest_commit --no-tag unreachable &&\n+\t\tgit repack -ad &&\n+\t\texpect_object_count 0 &&\n+\t\tunreachable_oid=$(git rev-parse --verify HEAD) &&\n+\t\tgit commit --amend --message rewritten &&\n+\n+\t\tgit repack -ad --keep-unreachable &&\n+\t\texpect_object_count 0 &&\n+\t\texpect_object_in_idx .git/objects/pack/*.idx \"$unreachable_oid\"\n+\t)\n+'\n+\n+test_expect_success '--keep-unreachable packs unreachable loose object with existing packs' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit config set core.logAllRefUpdates false &&\n+\n+\t\t# Set up the repo so that we have an existing packfile with\n+\t\t# reachable objects, only. The unreachable object as well as\n+\t\t# the rewritten commit are both loose.\n+\t\ttest_commit --no-tag initial &&\n+\t\tgit repack -ad &&\n+\t\tgit commit --amend --message unreachable &&\n+\t\tunreachable_oid=$(git rev-parse --verify HEAD) &&\n+\t\tgit commit --amend --message rewritten &&\n+\t\texpect_object_count 2 &&\n+\n+\t\tgit repack -ad --keep-unreachable &&\n+\t\texpect_object_count 0 &&\n+\t\texpect_object_in_idx .git/objects/pack/*.idx \"$unreachable_oid\"\n+\t)\n+'\n+\n+test_expect_failure '--keep-unreachable packs unreachable loose object without existing packs' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit config set core.logAllRefUpdates false &&\n+\n+\t\t# Set up the repo so that all objects are unpacked.\n+\t\ttest_commit --no-tag unreachable &&\n+\t\tunreachable_oid=$(git rev-parse --verify HEAD) &&\n+\t\tgit commit --amend --message rewritten &&\n+\t\texpect_object_count 4 &&\n+\n+\t\tgit repack -ad --keep-unreachable &&\n+\t\texpect_object_count 0 &&\n+\t\texpect_object_in_idx .git/objects/pack/*.idx \"$unreachable_oid\"\n+\t)\n+'\n+\n test_done\n\n-- \n2.48.1.502.g6dc24dfdaf.dirty\n\n"},{"id":"511756","messageId":"xmqqy0yn0x4p.fsf@gitster.g","threadId":"62895","inReplyTo":"20250203-b4-pks-repack-unreachable-objects-wo-packfiles-v1-1-7c4d69c5072c@pks.im","subject":"Re: [PATCH 1/2] t7700: add tests for `--keep-unreachable`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-02-03T17:13:42Z","receivedAt":"2025-02-03T17:13:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> +expect_object_count () {\n> +\tfind .git/objects \\( -type d \\( -name pack -o -name info \\) -prune \\) -o -type f -print >objects &&\n> +\ttest_line_count = \"$1\" objects\n> +}\n\nSo this is counting \"loose\" objects.  Do we want to have \"loose\"\nsomewhere in its name?\n\n> +expect_object_in_idx () {\n> +\tgit verify-pack -v \"$1\" >objects &&\n> +\ttest_grep \"^$2\" objects\n> +}\n\nAnd this one checks if an object exists in a given pack in somewhat\nan expensive way.  It can take either .idx or .pack but the name\nmakes the caller assume it should be called with .idx, which is OK.\n\nWould it achieve the same effect and be faster to do something like\n\n\tgit show-index <\"$1\" >objects &&\n\ttest_grep \"^[0-9]* $2 (\" objects\n\ninstead?\n\nThanks.\n"},{"id":"511757","messageId":"20250203183224.GA4183065@coredump.intra.peff.net","threadId":"62895","inReplyTo":"20250203-b4-pks-repack-unreachable-objects-wo-packfiles-v1-1-7c4d69c5072c@pks.im","subject":"Re: [PATCH 1/2] t7700: add tests for `--keep-unreachable`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-02-03T18:32:24Z","receivedAt":"2025-02-03T18:32:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 03, 2025 at 02:06:54PM +0100, Patrick Steinhardt wrote:\n\n> We don't have any tests for `git repack --keep-unreachable`. Add three\n> tests that exercise its behaviour with different packed states for the\n> unreachable object.\n\nThere are a few in t7701. It's spelled \"-k\" there, so a grep for\n\"--keep-unreachable\" would not find them.\n\n> +test_expect_success '--keep-unreachable appends unreachable packed objects to new pack' '\n> [...]\n> +test_expect_success '--keep-unreachable packs unreachable loose object with existing packs' '\n\nI think these match the two that are in t7701.\n\n-Peff\n"},{"id":"511767","messageId":"xmqqcyfyzit1.fsf@gitster.g","threadId":"62895","inReplyTo":"20250203183224.GA4183065@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] t7700: add tests for `--keep-unreachable`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-02-03T23:53:46Z","receivedAt":"2025-02-03T23:53:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Mon, Feb 03, 2025 at 02:06:54PM +0100, Patrick Steinhardt wrote:\n>\n>> We don't have any tests for `git repack --keep-unreachable`. Add three\n>> tests that exercise its behaviour with different packed states for the\n>> unreachable object.\n>\n> There are a few in t7701. It's spelled \"-k\" there, so a grep for\n> \"--keep-unreachable\" would not find them.\n\nAhh, good eyes.  Thanks.\n\n>\n>> +test_expect_success '--keep-unreachable appends unreachable packed objects to new pack' '\n>> [...]\n>> +test_expect_success '--keep-unreachable packs unreachable loose object with existing packs' '\n>\n> I think these match the two that are in t7701.\n\nDoubly thanks.\n"},{"id":"511778","messageId":"20250204023538.GB23954@coredump.intra.peff.net","threadId":"62895","inReplyTo":"xmqqcyfyzit1.fsf@gitster.g","subject":"Re: [PATCH 1/2] t7700: add tests for `--keep-unreachable`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-02-04T02:35:38Z","receivedAt":"2025-02-04T02:35:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 03, 2025 at 03:53:46PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > On Mon, Feb 03, 2025 at 02:06:54PM +0100, Patrick Steinhardt wrote:\n> >\n> >> We don't have any tests for `git repack --keep-unreachable`. Add three\n> >> tests that exercise its behaviour with different packed states for the\n> >> unreachable object.\n> >\n> > There are a few in t7701. It's spelled \"-k\" there, so a grep for\n> > \"--keep-unreachable\" would not find them.\n> \n> Ahh, good eyes.  Thanks.\n\nI got to cheat a little as the original author of the flag. ;)\n\n-Peff\n"},{"id":"511780","messageId":"20250204030157.GC23954@coredump.intra.peff.net","threadId":"62895","inReplyTo":"20250203-b4-pks-repack-unreachable-objects-wo-packfiles-v1-2-7c4d69c5072c@pks.im","subject":"Re: [PATCH 2/2] builtin/repack: fix `--keep-unreachable` when there are no packs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-02-04T03:01:57Z","receivedAt":"2025-02-04T03:01:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 03, 2025 at 02:06:55PM +0100, Patrick Steinhardt wrote:\n\n> The \"--keep-unreachable\" flag is supposed to append any unreachable\n> objects to the newly written pack. This flag is explicitly documented as\n> appending both packed and loose unreachable objects to the new packfile.\n> And while this works alright when repacking with preexisting packfiles,\n> it stops working when the repository does not have any packfiles at all.\n\nOK. I thought from the subject / cover letter this was going to be about\nthe fact that \"git repack -adk\" may sometimes say \"Nothing new to pack\".\nAnd the issue there is that if there are no reachable objects, we don't\nactually pack at al.\n\nBut this is a separate issue, where we actually do repack, but don't\ncorrectly pass the options. Let's read on...\n\n> The root cause are the conditions used to decide whether or not we want\n> to append \"--pack-loose-unreachable\" to git-pack-objects(1). There are\n> a couple of conditions here:\n> \n>   - `has_existing_non_kept_packs()` checks whether there are existing\n>     packfiles. This condition makes sense to guard \"--keep-pack=\",\n>     \"--unpack-unreachable\" and \"--keep-unreachable\", because all of\n>     these flags only make sense in combination with existing packfiles.\n>     But it does not make sense to disable `--pack-loose-unreachable`\n>     when there aren't any preexisting packfiles, as loose objects can be\n>     packed into the new packfile regardless of that.\n\nYeah, this analysis makes sense, and is the root of the problem.\n\n>   - `delete_redundant` checks whether we want to delete any objects or\n>     packs that are about to become redundant. The documentation of\n>     `--keep-unreachable` explicitly says that `git repack -ad` needs to\n>     be executed for the flag to have an effect.\n> \n>     It is not immediately obvious why such redundant objects need to be\n>     deleted in order for \"--pack-unreachable-objects\" to be effective.\n>     But as things are working as documented this is nothing we'll change\n>     for now.\n\nI don't think it's strictly necessary to require \"-d\" here. The original\nconcept of \"-k\" was to modify \"-d\" to keep objects instead of loosening\n(so an alternative to --unpack-unreachable/-A). And then it expanded to\ncollecting the loose objects, too, for the reasons given in e26a8c4721\n(repack: extend --keep-unreachable to loose objects, 2016-06-13).\n\nI think you could conceive of \"-k\" as an alternative to \"-d\", rather\nthan an alternative to \"-A\". I.e., so that \"repack -ak\" did the same as\n\"repack -adk\" does now.\n\nAnd it would probably not even be a big code change, but it's possible\nthere would be some unexpected fallout (the logic in repack is quite\ntortured and intricate from my recollection).\n\nSo I don't know that it's really worth it to change now. Especially\nbecause I think \"-k\" has mostly outlived its usefulness. Cruft packs\nsolve the same problem but keep the extra objects in their own pack,\nwhere they're less likely to interfere with normal operations. I'd\nrecommend anybody considering \"-k\" now to look into cruft packs instead.\n\n>   - `pack_everything & PACK_CRUFT` checks that we're not creating a\n>     cruft pack. This condition makes sense in the context of\n>     \"--pack-loose-unreachable\", as unreachable objects would end up in\n>     the cruft pack anyway.\n> \n> So while the second and third condition are sensible, it does not make\n> any sense to condition `--pack-loose-unreachable` on the existence of\n> packfiles.\n\nYup, agreed.\n\n> Fix the bug by splitting out the \"--pack-loose-unreachable\" and only\n> making it depend on the second and third condition. Like this, loose\n> unreachable objects will be packed regardless of any preexisting\n> packfiles.\n\nMakes sense. My only question would be whether there are any gotchas\ninside pack-objects about using --pack-loose-unreachable without\n--keep-unreachable (since the two were up until now always used\ntogether).\n\nIt was added by e26a8c4721. And looking over that patch, I don't see\nanything that would let the options be used independently. So this seems\nlike a good solution.\n\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index 81d13630ea..8194344b04 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -1370,9 +1370,12 @@ int cmd_repack(int argc,\n>  \t\t\t\t\t    \"--unpack-unreachable\");\n>  \t\t\t} else if (keep_unreachable) {\n>  \t\t\t\tstrvec_push(&cmd.args, \"--keep-unreachable\");\n> -\t\t\t\tstrvec_push(&cmd.args, \"--pack-loose-unreachable\");\n>  \t\t\t}\n>  \t\t}\n> +\n> +\t\tif (keep_unreachable && delete_redundant &&\n> +\t\t    !(pack_everything & PACK_CRUFT))\n> +\t\t\tstrvec_push(&cmd.args, \"--pack-loose-unreachable\");\n\nOne funny thing here is that previously unpack_unreachable took\nprecedence over keep_unreachable in the if-else chain. I wondered if we\ncould end up invoking pack-objects with both --unpack-unreachable and\n--pack-loose-unreachable, which is nonsense.\n\nBut I think the answer is no, because we forbid --unpack-unreachable/-A\nand --keep-unreachable from both being passed up front.\n\n> -test_expect_failure '--keep-unreachable packs unreachable loose object without existing packs' '\n> +test_expect_success '--keep-unreachable packs unreachable loose object without existing packs' '\n>  \ttest_when_finished \"rm -rf repo\" &&\n>  \tgit init repo &&\n>  \t(\n\nYour test from patch 1 looked reasonable to me. If you fold patch 1 into\nthe existing tests in t7701, you might want to adjust it to match the\ntechniques those tests use for checking the object (rather than the new\nhelpers you added).\n\n-Peff\n"},{"id":"511793","messageId":"20250204-b4-pks-repack-unreachable-objects-wo-packfiles-v2-1-1eae23366711@pks.im","threadId":"62895","inReplyTo":"20250203-b4-pks-repack-unreachable-objects-wo-packfiles-v1-0-7c4d69c5072c@pks.im","subject":"[PATCH v2] builtin/repack: fix `--keep-unreachable` when there are no packs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-04T07:00:41Z","receivedAt":"2025-02-04T07:00:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The \"--keep-unreachable\" flag is supposed to append any unreachable\nobjects to the newly written pack. This flag is explicitly documented as\nappending both packed and loose unreachable objects to the new packfile.\nAnd while this works alright when repacking with preexisting packfiles,\nit stops working when the repository does not have any packfiles at all.\n\nThe root cause are the conditions used to decide whether or not we want\nto append \"--pack-loose-unreachable\" to git-pack-objects(1). There are\na couple of conditions here:\n\n  - `has_existing_non_kept_packs()` checks whether there are existing\n    packfiles. This condition makes sense to guard \"--keep-pack=\",\n    \"--unpack-unreachable\" and \"--keep-unreachable\", because all of\n    these flags only make sense in combination with existing packfiles.\n    But it does not make sense to disable `--pack-loose-unreachable`\n    when there aren't any preexisting packfiles, as loose objects can be\n    packed into the new packfile regardless of that.\n\n  - `delete_redundant` checks whether we want to delete any objects or\n    packs that are about to become redundant. The documentation of\n    `--keep-unreachable` explicitly says that `git repack -ad` needs to\n    be executed for the flag to have an effect.\n\n    It is not immediately obvious why such redundant objects need to be\n    deleted in order for \"--pack-unreachable-objects\" to be effective.\n    But as things are working as documented this is nothing we'll change\n    for now.\n\n  - `pack_everything & PACK_CRUFT` checks that we're not creating a\n    cruft pack. This condition makes sense in the context of\n    \"--pack-loose-unreachable\", as unreachable objects would end up in\n    the cruft pack anyway.\n\nSo while the second and third condition are sensible, it does not make\nany sense to condition `--pack-loose-unreachable` on the existence of\npackfiles.\n\nFix the bug by splitting out the \"--pack-loose-unreachable\" and only\nmaking it depend on the second and third condition. Like this, loose\nunreachable objects will be packed regardless of any preexisting\npackfiles.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\nHi,\n\nthis small patch series fixes `git repack -ad --keep-unreachable` when\nthere aren't any preexisting packfiles.\n\nChanges in v2:\n  - Merge tests into t7701.\n  - Link to v1: https://lore.kernel.org/r/20250203-b4-pks-repack-unreachable-objects-wo-packfiles-v1-0-7c4d69c5072c@pks.im\n\nThanks!\n\nPatrick\n---\n builtin/repack.c                     |  5 ++++-\n t/t7701-repack-unpack-unreachable.sh | 16 ++++++++++++++++\n 2 files changed, 20 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 81d13630ea..8194344b04 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -1370,9 +1370,12 @@ int cmd_repack(int argc,\n \t\t\t\t\t    \"--unpack-unreachable\");\n \t\t\t} else if (keep_unreachable) {\n \t\t\t\tstrvec_push(&cmd.args, \"--keep-unreachable\");\n-\t\t\t\tstrvec_push(&cmd.args, \"--pack-loose-unreachable\");\n \t\t\t}\n \t\t}\n+\n+\t\tif (keep_unreachable && delete_redundant &&\n+\t\t    !(pack_everything & PACK_CRUFT))\n+\t\t\tstrvec_push(&cmd.args, \"--pack-loose-unreachable\");\n \t} else if (geometry.split_factor) {\n \t\tstrvec_push(&cmd.args, \"--stdin-packs\");\n \t\tstrvec_push(&cmd.args, \"--unpacked\");\ndiff --git a/t/t7701-repack-unpack-unreachable.sh b/t/t7701-repack-unpack-unreachable.sh\nindex 5715f4d69a..5559d4ccb4 100755\n--- a/t/t7701-repack-unpack-unreachable.sh\n+++ b/t/t7701-repack-unpack-unreachable.sh\n@@ -195,4 +195,20 @@ test_expect_success 'repack -k packs unreachable loose objects' '\n \tgit cat-file -p $sha1\n '\n \n+test_expect_success 'repack -k packs unreachable loose objects without existing packfiles' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\toid=$(echo would-be-deleted-loose | git hash-object -w --stdin) &&\n+\t\tobjpath=.git/objects/$(echo $sha1 | sed \"s,..,&/,\") &&\n+\t\ttest_path_is_file $objpath &&\n+\n+\t\tgit repack -ad --keep-unreachable &&\n+\t\ttest_path_is_missing $objpath &&\n+\t\tgit cat-file -p $oid\n+\t)\n+'\n+\n test_done\n\n---\nbase-commit: 3b0d05c4a79d0e441283680a864529b02dca5f08\nchange-id: 20250203-b4-pks-repack-unreachable-objects-wo-packfiles-33c26066e5c3\n\n"},{"id":"511794","messageId":"Z6G7KSCAGKeHP060@pks.im","threadId":"62895","inReplyTo":"20250204023538.GB23954@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] t7700: add tests for `--keep-unreachable`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-04T07:00:57Z","receivedAt":"2025-02-04T07:01:00Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Feb 03, 2025 at 09:35:38PM -0500, Jeff King wrote:\n> On Mon, Feb 03, 2025 at 03:53:46PM -0800, Junio C Hamano wrote:\n> \n> > Jeff King <peff@peff.net> writes:\n> > \n> > > On Mon, Feb 03, 2025 at 02:06:54PM +0100, Patrick Steinhardt wrote:\n> > >\n> > >> We don't have any tests for `git repack --keep-unreachable`. Add three\n> > >> tests that exercise its behaviour with different packed states for the\n> > >> unreachable object.\n> > >\n> > > There are a few in t7701. It's spelled \"-k\" there, so a grep for\n> > > \"--keep-unreachable\" would not find them.\n> > \n> > Ahh, good eyes.  Thanks.\n> \n> I got to cheat a little as the original author of the flag. ;)\n\nThe file is even *named* \"unpack-unreachable\". I didn't figure to grep\nfor \"-k\" though. Thanks for the pointer!\n\nPatrick\n"},{"id":"511795","messageId":"Z6G7LTVqw3l-b7lN@pks.im","threadId":"62895","inReplyTo":"20250204030157.GC23954@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] builtin/repack: fix `--keep-unreachable` when there are no packs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-04T07:01:01Z","receivedAt":"2025-02-04T07:01:04Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Feb 03, 2025 at 10:01:57PM -0500, Jeff King wrote:\n> On Mon, Feb 03, 2025 at 02:06:55PM +0100, Patrick Steinhardt wrote:\n> > Fix the bug by splitting out the \"--pack-loose-unreachable\" and only\n> > making it depend on the second and third condition. Like this, loose\n> > unreachable objects will be packed regardless of any preexisting\n> > packfiles.\n> \n> Makes sense. My only question would be whether there are any gotchas\n> inside pack-objects about using --pack-loose-unreachable without\n> --keep-unreachable (since the two were up until now always used\n> together).\n> \n> It was added by e26a8c4721. And looking over that patch, I don't see\n> anything that would let the options be used independently. So this seems\n> like a good solution.\n\nYou probably meant \"I don't see anything that would *not* let the\noptions be used independently.\" But yeah, they don't seem to require one\nanother.\n\n> > diff --git a/builtin/repack.c b/builtin/repack.c\n> > index 81d13630ea..8194344b04 100644\n> > --- a/builtin/repack.c\n> > +++ b/builtin/repack.c\n> > @@ -1370,9 +1370,12 @@ int cmd_repack(int argc,\n> >  \t\t\t\t\t    \"--unpack-unreachable\");\n> >  \t\t\t} else if (keep_unreachable) {\n> >  \t\t\t\tstrvec_push(&cmd.args, \"--keep-unreachable\");\n> > -\t\t\t\tstrvec_push(&cmd.args, \"--pack-loose-unreachable\");\n> >  \t\t\t}\n> >  \t\t}\n> > +\n> > +\t\tif (keep_unreachable && delete_redundant &&\n> > +\t\t    !(pack_everything & PACK_CRUFT))\n> > +\t\t\tstrvec_push(&cmd.args, \"--pack-loose-unreachable\");\n> \n> One funny thing here is that previously unpack_unreachable took\n> precedence over keep_unreachable in the if-else chain. I wondered if we\n> could end up invoking pack-objects with both --unpack-unreachable and\n> --pack-loose-unreachable, which is nonsense.\n> \n> But I think the answer is no, because we forbid --unpack-unreachable/-A\n> and --keep-unreachable from both being passed up front.\n\nYup.\n\n> > -test_expect_failure '--keep-unreachable packs unreachable loose object without existing packs' '\n> > +test_expect_success '--keep-unreachable packs unreachable loose object without existing packs' '\n> >  \ttest_when_finished \"rm -rf repo\" &&\n> >  \tgit init repo &&\n> >  \t(\n> \n> Your test from patch 1 looked reasonable to me. If you fold patch 1 into\n> the existing tests in t7701, you might want to adjust it to match the\n> techniques those tests use for checking the object (rather than the new\n> helpers you added).\n\nWill do.\n\nThanks!\n\nPatrick\n"},{"id":"511805","messageId":"xmqq8qqlx2ja.fsf@gitster.g","threadId":"62895","inReplyTo":"20250203-b4-pks-repack-unreachable-objects-wo-packfiles-v1-2-7c4d69c5072c@pks.im","subject":"Re: [PATCH 2/2] builtin/repack: fix `--keep-unreachable` when there are no packs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-02-04T13:28:09Z","receivedAt":"2025-02-04T13:28:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> The \"--keep-unreachable\" flag is supposed to append any unreachable\n> objects to the newly written pack. This flag is explicitly documented as\n> appending both packed and loose unreachable objects to the new packfile.\n> And while this works alright when repacking with preexisting packfiles,\n> it stops working when the repository does not have any packfiles at all.\n\nChuckle.  That's a cute corner case the developers never considered,\nit seems ;-).\n\n> The root cause are the conditions used to decide whether or not we want\n> to append \"--pack-loose-unreachable\" to git-pack-objects(1). There are\n> a couple of conditions here:\n>\n>   - `has_existing_non_kept_packs()` checks whether there are existing\n>     packfiles. This condition makes sense to guard \"--keep-pack=\",\n>     \"--unpack-unreachable\" and \"--keep-unreachable\", because all of\n>     these flags only make sense in combination with existing packfiles.\n>     But it does not make sense to disable `--pack-loose-unreachable`\n>     when there aren't any preexisting packfiles, as loose objects can be\n>     packed into the new packfile regardless of that.\n>\n>   - `delete_redundant` checks whether we want to delete any objects or\n>     packs that are about to become redundant. The documentation of\n>     `--keep-unreachable` explicitly says that `git repack -ad` needs to\n>     be executed for the flag to have an effect.\n>\n>     It is not immediately obvious why such redundant objects need to be\n>     deleted in order for \"--pack-unreachable-objects\" to be effective.\n>     But as things are working as documented this is nothing we'll change\n>     for now.\n>\n>   - `pack_everything & PACK_CRUFT` checks that we're not creating a\n>     cruft pack. This condition makes sense in the context of\n>     \"--pack-loose-unreachable\", as unreachable objects would end up in\n>     the cruft pack anyway.\n>\n> So while the second and third condition are sensible, it does not make\n> any sense to condition `--pack-loose-unreachable` on the existence of\n> packfiles.\n>\n> Fix the bug by splitting out the \"--pack-loose-unreachable\" and only\n> making it depend on the second and third condition. Like this, loose\n> unreachable objects will be packed regardless of any preexisting\n> packfiles.\n>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  builtin/repack.c  | 5 ++++-\n>  t/t7700-repack.sh | 2 +-\n>  2 files changed, 5 insertions(+), 2 deletions(-)\n\nNicely analized and described.  Thanks.\n"},{"id":"511814","messageId":"20250204145843.GA620055@coredump.intra.peff.net","threadId":"62895","inReplyTo":"Z6G7LTVqw3l-b7lN@pks.im","subject":"Re: [PATCH 2/2] builtin/repack: fix `--keep-unreachable` when there are no packs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-02-04T14:58:43Z","receivedAt":"2025-02-04T14:58:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 04, 2025 at 08:01:01AM +0100, Patrick Steinhardt wrote:\n\n> On Mon, Feb 03, 2025 at 10:01:57PM -0500, Jeff King wrote:\n> > On Mon, Feb 03, 2025 at 02:06:55PM +0100, Patrick Steinhardt wrote:\n> > > Fix the bug by splitting out the \"--pack-loose-unreachable\" and only\n> > > making it depend on the second and third condition. Like this, loose\n> > > unreachable objects will be packed regardless of any preexisting\n> > > packfiles.\n> > \n> > Makes sense. My only question would be whether there are any gotchas\n> > inside pack-objects about using --pack-loose-unreachable without\n> > --keep-unreachable (since the two were up until now always used\n> > together).\n> > \n> > It was added by e26a8c4721. And looking over that patch, I don't see\n> > anything that would let the options be used independently. So this seems\n> > like a good solution.\n> \n> You probably meant \"I don't see anything that would *not* let the\n> options be used independently.\" But yeah, they don't seem to require one\n> another.\n\nWhoops, yes. Last minute rephrasing strikes again. :)\n\n-Peff\n"},{"id":"511815","messageId":"20250204152236.GB620055@coredump.intra.peff.net","threadId":"62895","inReplyTo":"20250204-b4-pks-repack-unreachable-objects-wo-packfiles-v2-1-1eae23366711@pks.im","subject":"Re: [PATCH v2] builtin/repack: fix `--keep-unreachable` when there are no packs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-02-04T15:22:36Z","receivedAt":"2025-02-04T15:22:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 04, 2025 at 08:00:41AM +0100, Patrick Steinhardt wrote:\n\n> this small patch series fixes `git repack -ad --keep-unreachable` when\n> there aren't any preexisting packfiles.\n> \n> Changes in v2:\n>   - Merge tests into t7701.\n>   - Link to v1: https://lore.kernel.org/r/20250203-b4-pks-repack-unreachable-objects-wo-packfiles-v1-0-7c4d69c5072c@pks.im\n\nThis looks good to me.\n\nOne interesting thing I did notice:\n\n> +test_expect_success 'repack -k packs unreachable loose objects without existing packfiles' '\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\n> +\t\toid=$(echo would-be-deleted-loose | git hash-object -w --stdin) &&\n> +\t\tobjpath=.git/objects/$(echo $sha1 | sed \"s,..,&/,\") &&\n> +\t\ttest_path_is_file $objpath &&\n> +\n> +\t\tgit repack -ad --keep-unreachable &&\n> +\t\ttest_path_is_missing $objpath &&\n> +\t\tgit cat-file -p $oid\n> +\t)\n> +'\n\nIn the test in v1, we had reachable commits to pack. And here we don't.\nSo before your patch, the behavior in the v1 test was that we'd create a\nnew pack, but it wouldn't pick up the loose object. But the behavior of\nthis test is that we say \"Nothing new to pack\".\n\nI originally thought that output meant that we were not running\npack-objects at all. But looking at builtin/repack.c, we do run it, and\nit simply chooses not to make a pack (which makes sense; how would\nrepack even realize if there was stuff to pack, since pack-objects is\nwhat does the traversal).\n\nSo the two outcomes are both the result of the same bug. In both cases\nwe do not correctly pack the loose objects, so whether we make a pack is\njust a question of whether there was other reachable stuff to pack. And\nsince your patch is fixing the bug at its root, both outcomes are fixed.\n\nAnd when I suggested in my response to v1 that \"Nothing new to pack\" in\nan empty repo was a separate bug, I was just wrong. ;) There is nothing\nelse to fix after your patch.\n\nThanks for finding and fixing.\n\n-Peff\n"},{"id":"511848","messageId":"Z6L4xDMqc6aXEA8A@pks.im","threadId":"62895","inReplyTo":"20250204152236.GB620055@coredump.intra.peff.net","subject":"Re: [PATCH v2] builtin/repack: fix `--keep-unreachable` when there are no packs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-05T05:36:04Z","receivedAt":"2025-02-05T05:36:10Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Feb 04, 2025 at 10:22:36AM -0500, Jeff King wrote:\n> On Tue, Feb 04, 2025 at 08:00:41AM +0100, Patrick Steinhardt wrote:\n> \n> > this small patch series fixes `git repack -ad --keep-unreachable` when\n> > there aren't any preexisting packfiles.\n> > \n> > Changes in v2:\n> >   - Merge tests into t7701.\n> >   - Link to v1: https://lore.kernel.org/r/20250203-b4-pks-repack-unreachable-objects-wo-packfiles-v1-0-7c4d69c5072c@pks.im\n> \n> This looks good to me.\n> \n> One interesting thing I did notice:\n> \n> > +test_expect_success 'repack -k packs unreachable loose objects without existing packfiles' '\n> > +\ttest_when_finished \"rm -rf repo\" &&\n> > +\tgit init repo &&\n> > +\t(\n> > +\t\tcd repo &&\n> > +\n> > +\t\toid=$(echo would-be-deleted-loose | git hash-object -w --stdin) &&\n> > +\t\tobjpath=.git/objects/$(echo $sha1 | sed \"s,..,&/,\") &&\n> > +\t\ttest_path_is_file $objpath &&\n> > +\n> > +\t\tgit repack -ad --keep-unreachable &&\n> > +\t\ttest_path_is_missing $objpath &&\n> > +\t\tgit cat-file -p $oid\n> > +\t)\n> > +'\n> \n> In the test in v1, we had reachable commits to pack. And here we don't.\n> So before your patch, the behavior in the v1 test was that we'd create a\n> new pack, but it wouldn't pick up the loose object. But the behavior of\n> this test is that we say \"Nothing new to pack\".\n> \n> I originally thought that output meant that we were not running\n> pack-objects at all. But looking at builtin/repack.c, we do run it, and\n> it simply chooses not to make a pack (which makes sense; how would\n> repack even realize if there was stuff to pack, since pack-objects is\n> what does the traversal).\n> \n> So the two outcomes are both the result of the same bug. In both cases\n> we do not correctly pack the loose objects, so whether we make a pack is\n> just a question of whether there was other reachable stuff to pack. And\n> since your patch is fixing the bug at its root, both outcomes are fixed.\n> \n> And when I suggested in my response to v1 that \"Nothing new to pack\" in\n> an empty repo was a separate bug, I was just wrong. ;) There is nothing\n> else to fix after your patch.\n> \n> Thanks for finding and fixing.\n\nThanks for your thorough review!\n\nPatrick\n"}]}