{"thread":{"id":"62867","subject":"[RFC PATCH resend] builtin/repack: Honor --keep-pack and .keep when repacking promisor objects","startedAt":"2025-01-29T10:11:52Z","lastAt":"2025-02-19T12:55:38Z","messageCount":6,"participants":["Tomáš Trnka","brian m. carlson","Han Young"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"511403","messageId":"2728513.vuYhMxLoTh@mintaka.ncbr.muni.cz","threadId":"62867","inReplyTo":null,"subject":"[RFC PATCH resend] builtin/repack: Honor --keep-pack and .keep when repacking promisor objects","fromName":"Tomáš Trnka","fromEmail":"trnka@scm.com","sentAt":"2025-01-29T10:02:06Z","receivedAt":"2025-01-29T10:11:52Z","isPatch":true,"sender":{"key":"trnka@scm.com","avatar":null},"body":"git-repack currently does not pass --keep-pack or --honor-pack-keep to\nthe git-pack-objects handling promisor packs. This means that settings\nlike gc.bigPackThreshold are completely ignored for promisor packs.\n\nThe simple fix is to just copy the keep-pack logic into\nrepack_promisor_objects(), although this could possibly be improved by\nmaking prepare_pack_objects() handle it instead.\n\nSigned-off-by: Tomáš Trnka <trnka@scm.com>\n---\n\nRFC: This probably needs a test, but where and how should it be\nimplemented? Perhaps in t7700-repack.sh, copying one of the tests using\nprepare_for_keep_packs and just touching .promisor files? Or instead in\nt/t0410-partial-clone.sh using a copy/variant of one of the basic \nrepack tests there?\n\n builtin/repack.c | 12 ++++++++++--\n 1 file changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex d6bb37e84a..fe62fe03eb 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -388,15 +388,23 @@ static int has_pack_ext(const struct generated_pack_data \n*data,\n }\n \n static void repack_promisor_objects(const struct pack_objects_args *args,\n-\t\t\t\t    struct string_list *names)\n+\t\t\t\t    struct string_list *names,\n+\t\t\t\t    struct string_list \n*keep_pack_list)\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tFILE *out;\n \tstruct strbuf line = STRBUF_INIT;\n+\tint i;\n \n \tprepare_pack_objects(&cmd, args, packtmp);\n \tcmd.in = -1;\n \n+\tif (!pack_kept_objects)\n+\t\tstrvec_push(&cmd.args, \"--honor-pack-keep\");\n+\tfor (i = 0; i < keep_pack_list->nr; i++)\n+\t\tstrvec_pushf(&cmd.args, \"--keep-pack=%s\",\n+\t\t\t     keep_pack_list->items[i].string);\n+\n \t/*\n \t * NEEDSWORK: Giving pack-objects only the OIDs without any \nordering\n \t * hints may result in suboptimal deltas in the resulting pack. See \nif\n@@ -1350,7 +1358,7 @@ int cmd_repack(int argc,\n \t\tstrvec_push(&cmd.args, \"--delta-islands\");\n \n \tif (pack_everything & ALL_INTO_ONE) {\n-\t\trepack_promisor_objects(&po_args, &names);\n+\t\trepack_promisor_objects(&po_args, &names, \n&keep_pack_list);\n \n \t\tif (has_existing_non_kept_packs(&existing) &&\n \t\t    delete_redundant &&\n\nbase-commit: 92999a42db1c5f43f330e4f2bca4026b5b81576f\n-- \n2.47.1\n\n\n\n\n"},{"id":"511450","messageId":"Z5rjSzjOXrV77_nJ@tapette.crustytoothpaste.net","threadId":"62867","inReplyTo":"2728513.vuYhMxLoTh@mintaka.ncbr.muni.cz","subject":"Re: [RFC PATCH resend] builtin/repack: Honor --keep-pack and .keep when repacking promisor objects","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2025-01-30T02:26:19Z","receivedAt":"2025-01-30T02:26:22Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2025-01-29 at 10:02:06, Tomáš Trnka wrote:\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index d6bb37e84a..fe62fe03eb 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -388,15 +388,23 @@ static int has_pack_ext(const struct generated_pack_data \n> *data,\n>  }\n>  \n>  static void repack_promisor_objects(const struct pack_objects_args *args,\n> -\t\t\t\t    struct string_list *names)\n> +\t\t\t\t    struct string_list *names,\n> +\t\t\t\t    struct string_list \n> *keep_pack_list)\n\nI don't have a strong opinion about the technical aspects of this patch\n(nor sufficient knowledge to review it)[0], but I noticed that there's a\ncouple of places, this line among them, which are unexpectedly wrapped,\nso I don't believe this patch will actually apply.  I noticed that the\nemail didn't specify an MUA header (or I missed it), so I can't make a\nsuggestion on how to fix your MUA, but you may want to use `git\nsend-email` to avoid this problem in the future.\n\n[0] In other words, no need to CC me on a resend.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"511473","messageId":"5706247.IbC2pHGDlb@electra","threadId":"62867","inReplyTo":"Z5rjSzjOXrV77_nJ@tapette.crustytoothpaste.net","subject":"Re: [RFC PATCH resend] builtin/repack: Honor --keep-pack and .keep when repacking promisor objects","fromName":"Tomáš Trnka","fromEmail":"trnka@scm.com","sentAt":"2025-01-30T08:09:32Z","receivedAt":"2025-01-30T08:09:37Z","isPatch":true,"sender":{"key":"trnka@scm.com","avatar":null},"body":"On Thursday, 30 January 2025 3:26:19, CET brian m. carlson wrote:\n> I don't have a strong opinion about the technical aspects of this patch\n> (nor sufficient knowledge to review it)[0], but I noticed that there's a\n> couple of places, this line among them, which are unexpectedly wrapped,\n> so I don't believe this patch will actually apply.\n\nOops, my sincere apologies to everyone. I'll resend once again.\n\n(I messed up by not realizing that KMail will re-enable line wrapping when re-\nsending my original message, which was formatted correctly.)\n\n2T\n\n\n"},{"id":"512174","messageId":"CAG1j3zH1xngk0NZUjHA9Akx526yfEiQ=KsdfyRjE9XAewWV=Sg@mail.gmail.com","threadId":"62867","inReplyTo":"2728513.vuYhMxLoTh@mintaka.ncbr.muni.cz","subject":"Re: [External] [RFC PATCH resend] builtin/repack: Honor --keep-pack and .keep when repacking promisor objects","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2025-02-10T11:38:17Z","receivedAt":"2025-02-10T11:38:28Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"On Wed, Jan 29, 2025 at 6:12 PM Tomáš Trnka <trnka@scm.com> wrote:\n>\n> git-repack currently does not pass --keep-pack or --honor-pack-keep to\n> the git-pack-objects handling promisor packs. This means that settings\n> like gc.bigPackThreshold are completely ignored for promisor packs.\n>\n> The simple fix is to just copy the keep-pack logic into\n> repack_promisor_objects(), although this could possibly be improved by\n> making prepare_pack_objects() handle it instead.\n>\n\nWe repack promisor packs by reading all the objects in promisor packs\n(in repack.c), and send them to pack-objects. pack-objects then write a\nsingle pack containing all the promisor objects. The actual old promisor\npack deletion happens in repack.c\n\nSo just simply copying the keep-pack logic to repack_promisor_objects()\ndoes not prevent the keep promisor packs from being repacked.\n\nOne way to achieve what you wanted would be to filter the keep packs\nin repack_promisor_objects's for_each_packed_object().\n\nThanks.\n"},{"id":"512175","messageId":"2289498.vFx2qVVIhK@electra","threadId":"62867","inReplyTo":"CAG1j3zH1xngk0NZUjHA9Akx526yfEiQ=KsdfyRjE9XAewWV=Sg@mail.gmail.com","subject":"Re: [RFC PATCH resend] builtin/repack: Honor --keep-pack and .keep when repacking promisor objects","fromName":"Tomáš Trnka","fromEmail":"trnka@scm.com","sentAt":"2025-02-10T12:52:40Z","receivedAt":"2025-02-10T13:01:50Z","isPatch":true,"sender":{"key":"trnka@scm.com","avatar":null},"body":"On Monday, 10 February 2025 12:38:17, CET Han Young wrote:\n> We repack promisor packs by reading all the objects in promisor packs\n> (in repack.c), and send them to pack-objects. pack-objects then write a\n> single pack containing all the promisor objects. The actual old promisor\n> pack deletion happens in repack.c\n> \n> So just simply copying the keep-pack logic to repack_promisor_objects()\n> does not prevent the keep promisor packs from being repacked.\n\nI don't know much about the internals so maybe I misinterpreted what I saw, \nbut the patch seems to fix the issue I observed:\n\nI have two 40 GiB promisor packs, and as soon as I accumulated 50 additional \nsmall packs (due to fetches), gc triggered a repack including the two big \npacks, despite them being above the bigPackThreshold so they could be kept. \nRepacking them is very disruptive, both because of the CPU and RAM load this \nproduces, and also because this is on btrfs with snapshots, so rewriting those \n80 GiB into a new file means consuming that much extra disk space for no good \nreason.\n\nWith my patch, gc did not touch these two big packs but still collected all \nthe small ones into one new pack as expected. Everything else also seems to \nwork fine.\n\nAccording to the man page for git-pack-objects, it seems to me that this is \nhow it's meant to work, because the description for --keep-pack says \"This \nflag causes an object already in the given pack to be ignored, even if it \nwould have otherwise been packed.\" (and something similar for --honor-pack-\nkeep). To my untrained eyes, it looks like that's also how \nwant_found_object()/add_object_entry() in pack-objects.c handle it.\n\n2T\n\n\n"},{"id":"512667","messageId":"CAG1j3zG-FcGZe-64dmZJOAitjKsnbB5KUUkmQt6edyhM7z_NTw@mail.gmail.com","threadId":"62867","inReplyTo":"2289498.vFx2qVVIhK@electra","subject":"Re: [External] Re: [RFC PATCH resend] builtin/repack: Honor --keep-pack and .keep when repacking promisor objects","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2025-02-19T12:55:25Z","receivedAt":"2025-02-19T12:55:38Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"On Mon, Feb 10, 2025 at 8:53 PM Tomáš Trnka <trnka@scm.com> wrote:\n> With my patch, gc did not touch these two big packs but still collected all\n> the small ones into one new pack as expected. Everything else also seems to\n> work fine.\n\nSorry for the long wait. I have tested the patch against a repo with only\npromisor packs. There are one big promisor pack and many small ones.\nThis patch do work as expected, I'll take back the earlier\n\n\"... does not prevent the keep promisor packs from being repacked.\"\n\n\n> According to the man page for git-pack-objects, it seems to me that this is\n> how it's meant to work, because the description for --keep-pack says \"This\n> flag causes an object already in the given pack to be ignored, even if it\n> would have otherwise been packed.\" (and something similar for --honor-pack-\n> keep). To my untrained eyes, it looks like that's also how\n> want_found_object()/add_object_entry() in pack-objects.c handle it.\n\nThis is also true. However, the for_each_packed_object macro in repack.c\ndoes not ignore the keep packs. repack still iterating objects in keep packs\nand sending them to pack-objects. pack-objects will then exclude these\nobjects. To avoid doing unnecessary work, objects in keep packs should\nnot be send over to pack-objects. Checking if the object should be ignore\ntakes some time, after all.\n\nAs for the test, t0410-partial-clone.sh is a better place imo. For the test is\nfor the partial-clone repos.\n\nThanks.\n"}]}