{"thread":{"id":"62872","subject":"[RFC PATCH resend] builtin/repack: Honor --keep-pack and .keep when repacking promisor objects","startedAt":"2025-01-30T08:11:34Z","lastAt":"2025-01-31T15:17:18Z","messageCount":3,"participants":["Tomáš Trnka","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"511474","messageId":"19759704.fSG56mABFh@electra","threadId":"62872","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-30T08:11:31Z","receivedAt":"2025-01-30T08:11:34Z","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 *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 *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 ordering\n \t * hints may result in suboptimal deltas in the resulting pack. See if\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, &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":"511530","messageId":"xmqq34h0aq65.fsf@gitster.g","threadId":"62872","inReplyTo":"19759704.fSG56mABFh@electra","subject":"Re: [RFC PATCH resend] builtin/repack: Honor --keep-pack and .keep when repacking promisor objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-30T22:32:34Z","receivedAt":"2025-01-30T22:32:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tomáš Trnka <trnka@scm.com> writes:\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> Signed-off-by: Tomáš Trnka <trnka@scm.com>\n> ---\n\nI don't have a strong opinion about the technical aspects of this\npatch to be a reviewer.  I have no idea how you decided whom to Cc:,\nbut I recall these two threads that worked in the same \"interaction\nbetween repack and promisor objects\" area (I am not saying and I do\nnot think they addressed the same problem as you are):\n\n * https://lore.kernel.org/git/20240925072021.77078-1-hanyang.tony@bytedance.com/\n * https://lore.kernel.org/git/cover.1730491845.git.jonathantanmy@google.com/\n\nso asking review from the authors of these topics would be more\nrelevant than sending it to me (I did that already, and that is why\nthe remainder of the original patch is not culled from this\nresponse---after the next line, there is nothing I wrote but the\noriginal patch left for others' convenience).\n\nThanks.\n\n>\n> RFC: This probably needs a test, but where and how should it be\n> implemented? Perhaps in t7700-repack.sh, copying one of the tests using\n> prepare_for_keep_packs and just touching .promisor files? Or instead in\n> t/t0410-partial-clone.sh using a copy/variant of one of the basic \n> repack tests there?\n>\n>  builtin/repack.c | 12 ++++++++++--\n>  1 file changed, 10 insertions(+), 2 deletions(-)\n>\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 *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 *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 ordering\n>  \t * hints may result in suboptimal deltas in the resulting pack. See if\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, &keep_pack_list);\n>  \n>  \t\tif (has_existing_non_kept_packs(&existing) &&\n>  \t\t    delete_redundant &&\n>\n> base-commit: 92999a42db1c5f43f330e4f2bca4026b5b81576f\n"},{"id":"511566","messageId":"7102402.jJDZkT8p0M@electra","threadId":"62872","inReplyTo":"xmqq34h0aq65.fsf@gitster.g","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-31T15:17:08Z","receivedAt":"2025-01-31T15:17:18Z","isPatch":true,"sender":{"key":"trnka@scm.com","avatar":null},"body":"On Thursday, 30 January 2025 23:32:34, CET Junio C Hamano wrote:\n> I don't have a strong opinion about the technical aspects of this\n> patch to be a reviewer.  I have no idea how you decided whom to Cc:,\n\nI'm sorry again for spamming you unnecessarily and many thanks for suggesting \nmore suitable reviewers.\n\nBeing a complete newcomer to Git development, I went by what the git-contacts \ntool suggested when I gave it my patch, since its output seemed to roughly \nagree with git log on builtin/repack.c, plus I also thought you might have a \nhigh-level opinion on where the test belongs. I didn't check the mailing list \narchives too thoroughly, so I was not aware of those discussion threads.\n\n2T\n\n\n"}]}