{"thread":{"id":"62578","subject":"[PATCH] builtin/repack.c: prune unreachable objects with `--expire-to`","startedAt":"2024-12-01T00:01:06Z","lastAt":"2025-01-15T08:09:07Z","messageCount":4,"participants":["Taylor Blau","Jeff King","ZheNing Hu"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"508381","messageId":"48438876fb42a889110e100a6c42ca84e93aac49.1733011259.git.me@ttaylorr.com","threadId":"62578","inReplyTo":null,"subject":"[PATCH] builtin/repack.c: prune unreachable objects with `--expire-to`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-12-01T00:01:03Z","receivedAt":"2024-12-01T00:01:06Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When invoked with '--expire-to', 'git repack' will move unreachable\nobjects beyond the grace period to a separate repository outside of the\nmain object store.\n\nLater on, 'git repack' will remove any existing packs which were made\nredundant by the 'repack' operation, before then pruning loose objects\nwhich were packed. Ordinarily, unreachable objects which have expired\nwere already packed via some earlier 'repack' operation, and so are\nremoved from the main repository in the first step.\n\nBut if a repository has unreachable objects which:\n\n - have an mtime earlier than the --cruft-expiration period,\n - are loose, and\n - have never been packed\n\nThen we'll create a pack containing those objects to store in the\nrepository specified by the '--expire-to' option, but never prune the\nloose copies of those objects from the main repository. That's because\nwe don't have a pack in the main repository which contains those\nobjects, so prune_packed_objects() skips over them.\n\n(As an aside, for repositories that have a large number of unreachable\nobjects which were never packed, and are old enough to be expired, this\ncan be quite painful. That's because even though we expect the repack to\nprune those objects which were GC'd, we don't per the above).\n\nTeach repack to add the repository specified by '--expire-to' as an\nalternate of the main object store so that 'prune_packed_objects()' can\n\"see\" the packed copy of those objects, and remove them appropriately.\n\nSigned-off-by: Taylor Blau \n---\n builtin/repack.c | 15 +++++++++++++++\n t/t7704-repack-cruft.sh | 12 ++++++++++++\n 2 files changed, 27 insertions(+)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex d6bb37e84ae..57cab72dcf5 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -1553,6 +1553,21 @@ int cmd_repack(int argc,\n \t\t\t\t\t\t\t&existing);\n \t\tif (show_progress)\n \t\t\topts |= PRUNE_PACKED_VERBOSE;\n+\n+\t\tif (expire_to && *expire_to) {\n+\t\t\tchar *alt = dirname(xstrdup(expire_to));\n+\t\t\tsize_t len = strlen(alt);\n+\n+\t\t\tif (strip_suffix(alt, \"pack\", &len) &&\n+\t\t\t is_dir_sep(alt[len - 1])) {\n+\t\t\t\talt[len - 1] = '\\0';\n+\n+\t\t\t\tadd_to_alternates_memory(alt);\n+\t\t\t\treprepare_packed_git(the_repository);\n+\t\t\t}\n+\n+\t\t\tfree(alt);\n+\t\t}\n \t\tprune_packed_objects(opts);\n \n \t\tif (!keep_unreachable &&\ndiff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh\nindex 5db9f4e10f7..ee1ffcdae3c 100755\n--- a/t/t7704-repack-cruft.sh\n+++ b/t/t7704-repack-cruft.sh\n@@ -30,6 +30,12 @@ test_expect_success '--expire-to stores pruned objects (now)' '\n \t\tgit branch -D cruft &&\n \t\tgit reflog expire --all --expire=all &&\n \n+\t\tfor obj in $(cat moved.want)\n+\t\tdo\n+\t\t\tpath=\"$objdir/$(test_oid_to_path $obj)\" &&\n+\t\t\ttest_path_is_file \"$path\" || return 1\n+\t\tdone &&\n+\n \t\tgit init --bare expired.git &&\n \t\tgit repack -d \\\n \t\t\t--cruft --cruft-expiration=\"now\" \\\n@@ -38,6 +44,12 @@ test_expect_success '--expire-to stores pruned objects (now)' '\n \t\texpired=\"$(ls expired.git/objects/pack/pack-*.idx)\" &&\n \t\ttest_path_is_file \"${expired%.idx}.mtimes\" &&\n \n+\t\tfor obj in $(cat moved.want)\n+\t\tdo\n+\t\t\tpath=\"$objdir/$(test_oid_to_path $obj)\" &&\n+\t\t\ttest_path_is_missing \"$path\" || return 1\n+\t\tdone &&\n+\n \t\t# Since the `--cruft-expiration` is \"now\", the effective\n \t\t# behavior is to move _all_ unreachable objects out to\n \t\t# the location in `--expire-to`.\n\nbase-commit: cc01bad4a9f566cf4453c7edd6b433851b0835e2\n-- \n2.47.1.314.g48438876fb4.dirty\n"},{"id":"508385","messageId":"Z0vlD7/vOhot0qwc@nand.local","threadId":"62578","inReplyTo":"48438876fb42a889110e100a6c42ca84e93aac49.1733011259.git.me@ttaylorr.com","subject":"Re: [PATCH] builtin/repack.c: prune unreachable objects with `--expire-to`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-12-01T04:24:47Z","receivedAt":"2024-12-01T04:24:54Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Nov 30, 2024 at 07:01:03PM -0500, Taylor Blau wrote:\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index d6bb37e84ae..57cab72dcf5 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -1553,6 +1553,21 @@ int cmd_repack(int argc,\n> \t\t\t\t\t\t\t&existing);\n> \t\tif (show_progress)\n> \t\t\topts |= PRUNE_PACKED_VERBOSE;\n> +\n> +\t\tif (expire_to && *expire_to) {\n> +\t\t\tchar *alt = dirname(xstrdup(expire_to));\n> +\t\t\tsize_t len = strlen(alt);\n> +\n> +\t\t\tif (strip_suffix(alt, \"pack\", &len) &&\n> +\t\t\t is_dir_sep(alt[len - 1])) {\n\nArgh. This clearly needs a bounds check to ensure that 'len >= 1'.\n\nI suspect that passing \"--expire-to=pack/xyz\" would segfault.\n\nThanks,\nTaylor\n"},{"id":"508390","messageId":"20241201213439.GA145938@coredump.intra.peff.net","threadId":"62578","inReplyTo":"48438876fb42a889110e100a6c42ca84e93aac49.1733011259.git.me@ttaylorr.com","subject":"Re: [PATCH] builtin/repack.c: prune unreachable objects with `--expire-to`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-12-01T21:34:39Z","receivedAt":"2024-12-01T21:34:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 30, 2024 at 07:01:03PM -0500, Taylor Blau wrote:\n\n> When invoked with '--expire-to', 'git repack' will move unreachable\n> objects beyond the grace period to a separate repository outside of the\n> main object store.\n> \n> Later on, 'git repack' will remove any existing packs which were made\n> redundant by the 'repack' operation, before then pruning loose objects\n> which were packed. Ordinarily, unreachable objects which have expired\n> were already packed via some earlier 'repack' operation, and so are\n> removed from the main repository in the first step.\n> \n> But if a repository has unreachable objects which:\n> \n> - have an mtime earlier than the --cruft-expiration period,\n> - are loose, and\n> - have never been packed\n> \n> Then we'll create a pack containing those objects to store in the\n> repository specified by the '--expire-to' option, but never prune the\n> loose copies of those objects from the main repository. That's because\n> we don't have a pack in the main repository which contains those\n> objects, so prune_packed_objects() skips over them.\n> \n> (As an aside, for repositories that have a large number of unreachable\n> objects which were never packed, and are old enough to be expired, this\n> can be quite painful. That's because even though we expect the repack to\n> prune those objects which were GC'd, we don't per the above).\n> \n> Teach repack to add the repository specified by '--expire-to' as an\n> alternate of the main object store so that 'prune_packed_objects()' can\n> \"see\" the packed copy of those objects, and remove them appropriately.\n\nI think the solution you came up with makes sense in the confines of\nwhat \"repack --cruft\" and \"--expire-to\" do. But I can't help but feel\nthat we may have taken a wrong (or at least somewhat confusing) turn\nbefore this.\n\nThat is, why are we expecting \"repack\" to prune those loose objects at\nall? In a traditional \"repack -ad\" they would not be removed, because\nthey are not packed and not reachable. It is the responsibility of a\nfollow-up \"git prune\" to get rid of them, and that is run automatically\nby \"git gc\".\n\nWhen we added in cruft packs, now those objects are \"packed\" in the\nsense that we are storing them in a cruft pack instead of loose. But\nthey are not really \"packed\" in the sense that the original \"repack\"\nwould do. They're not in a pack we intend to keep, and the storage in a\ncruft pack is mostly an incidental optimization over the loose format.\nBut \"repack\" started deleting them (because it knew they were moved into\na cruft pack), rather than waiting for \"git prune\" to do so. Which is a\nlittle weird, but OK.\n\nBut then --expire-to adds extra confusion because we are _really_ not\npacking them in the repository now. They are going to some magical\nout-of-repo spot. Which yes, happens to be a pack. But I think one could\nargue that they are not packed in the repo at that point, and it the\nresponsibility of \"git prune\" to get rid of them.\n\n\nNow all of that is fairly philosophical. Is there a real-world benefit\nto trying to retain this more purist view-point? I don't know. Certainly\nI can think of some downsides, one of which is that running a follow-up\ngit-prune requires computing the full reachability again. That's both\nexpensive and racy with respect to what we saved in the cruft pack.\n\nSo I think there's some argument there that the pruning _has_ to be part\nof the repack process for atomicity, and therefore --expire-to has to do\nthe same.\n\n> @@ -1553,6 +1553,21 @@ int cmd_repack(int argc,\n> \t\t\t\t\t\t\t&existing);\n> \t\tif (show_progress)\n> \t\t\topts |= PRUNE_PACKED_VERBOSE;\n> +\n> +\t\tif (expire_to && *expire_to) {\n> +\t\t\tchar *alt = dirname(xstrdup(expire_to));\n> +\t\t\tsize_t len = strlen(alt);\n> +\n> +\t\t\tif (strip_suffix(alt, \"pack\", &len) &&\n> +\t\t\t is_dir_sep(alt[len - 1])) {\n> +\t\t\t\talt[len - 1] = '\\0';\n> +\n> +\t\t\t\tadd_to_alternates_memory(alt);\n> +\t\t\t\treprepare_packed_git(the_repository);\n> +\t\t\t}\n> +\n> +\t\t\tfree(alt);\n> +\t\t}\n\nOK, so we are adding the containing directory as an alternate. That\ngives me two concerns:\n\n 1. The expire-to string is something like \"path/to/objects/pack/pack\",\n and we'll have created \"path/to/objects/pack/pack-.pack\"\n Using dirname() strips that down to \"path/to/objects/pack\". OK. And\n then we manually strip \"pack/\" off the end, which we have to do to\n get the \"base\" objects/ directory.\n\n But what if the path given by the user via --expire-to doesn't look\n like an object directory? I.e., does not end in \"pack/\"? Then this\n feature would not work at all.\n\n Should we be mentioning this in the git-repack docs?\n\n As an aside, I think the current --expire-to docs are misleading.\n They say:\n\n --expire-to=\n\t Write a cruft pack containing pruned objects (if any) to the\n\t directory . [...]\n\n But that isn't right. It is not a but a similar\n to the one that pack-objects takes. If you do --expire-to=some/dir,\n then you'll get some/dir-.pack.\n\n 2. Since we're adding the whole directory, that reprepare_packed_git()\n will also find any existing packs that were sitting in the\n --expire-to directory. If you are repeatedly stuffing cruft packs\n into the same directory every time you repack, then you'll see\n those older packs, and we'll prune objects that they mention. I'm\n not sure it's too bad from a correctness perspective to allow that\n (and I think they'd have ended up in the most recent cruft pack\n anyway). But I wonder if the expense of checking those packs\n eventually adds up.\n\nOne thing that could perhaps deal with both issues is to add the single\ncruft pack itself, rather than the surrounding directory. I.e., would it\nwork to add_packed_git() and install_packed_git() each cruft pack\n(should just be one, but I think in theory there may be multiple if\nmaxPackSize is enabled)?\n\nIt's maybe a little weird to have an entry in packed_git that doesn't\ncome from an object database that we're using. But I don't think there\nare any problems with that (I'd probably pass \"0\" for the \"local\" flag\n;) ).\n\nThe only other gotcha I see is that you probably need to\nreprepare_packed_git() afterwards to make sure the packed_git_mru list\nis refreshed.\n\n-Peff\n"},{"id":"510558","messageId":"CAOLTT8R3UULA9xrv8FZcTsTE1qvsToU=WgOEWMfuO0vq5ztUAw@mail.gmail.com","threadId":"62578","inReplyTo":"20241201213439.GA145938@coredump.intra.peff.net","subject":"Re: [PATCH] builtin/repack.c: prune unreachable objects with `--expire-to`","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2025-01-15T08:08:54Z","receivedAt":"2025-01-15T08:09:07Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"> OK, so we are adding the containing directory as an alternate. That\n> gives me two concerns:\n>\n> 1. The expire-to string is something like \"path/to/objects/pack/pack\",\n> and we'll have created \"path/to/objects/pack/pack-.pack\"\n> Using dirname() strips that down to \"path/to/objects/pack\". OK. And\n> then we manually strip \"pack/\" off the end, which we have to do to\n> get the \"base\" objects/ directory.\n>\n> But what if the path given by the user via --expire-to doesn't look\n> like an object directory? I.e., does not end in \"pack/\"? Then this\n> feature would not work at all.\n>\n> Should we be mentioning this in the git-repack docs?\n>\n> As an aside, I think the current --expire-to docs are misleading.\n> They say:\n>\n> --expire-to=\n> Write a cruft pack containing pruned objects (if any) to the\n> directory . [...]\n>\n> But that isn't right. It is not a but a similar\n> to the one that pack-objects takes. If you do --expire-to=some/dir,\n> then you'll get some/dir-.pack.\n>\n\nI agree. The `--expire-to=` option can easily cause confusion for users.\nFor example, using `--expire-to=xxx.git/objects/pack` will actually generate\nfiles like xxx.git/objects/pack-*.{mtimes,idx,pack} instead of placing them\nin xxx.git/objects/pack/pack-*.{mtimes,idx,pack}.\n\n--\nZheNing Hu\n"}]}