{"thread":{"id":"60123","subject":"[PATCH 0/1] Fix the order of consuming unpackLimit config settings.","startedAt":"2023-08-17T21:54:47Z","lastAt":"2023-08-23T19:04:10Z","messageCount":8,"participants":["Taylor Santiago","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"480769","messageId":"20230817215325.2550998-1-taylorsantiago@google.com","threadId":"60123","inReplyTo":null,"subject":"[PATCH 0/1] Fix the order of consuming unpackLimit config settings.","fromName":"Taylor Santiago","fromEmail":"taylorsantiago@google.com","sentAt":"2023-08-17T21:53:24Z","receivedAt":"2023-08-17T21:54:47Z","isPatch":true,"sender":{"key":"taylorsantiago@google.com","avatar":null},"body":"The documentation for fetch.unpackLimit states that fetch.unpackLimit\nshould be treated as higher priority than transfer.unpackLimit, but the\nintended order is currently inverted.\n\nTaylor Santiago (1):\n  Fix the order of consuming unpackLimit config settings.\n\n fetch-pack.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\n\nbase-commit: f1ed9d7dc0e49dc1a044941d821c9d2342313c26\n-- \n2.41.0.694.ge786442a9b-goog\n\n"},{"id":"480770","messageId":"20230817215325.2550998-2-taylorsantiago@google.com","threadId":"60123","inReplyTo":"20230817215325.2550998-1-taylorsantiago@google.com","subject":"[PATCH 1/1] Fix the order of consuming unpackLimit config settings.","fromName":"Taylor Santiago","fromEmail":"taylorsantiago@google.com","sentAt":"2023-08-17T21:53:25Z","receivedAt":"2023-08-17T21:55:20Z","isPatch":true,"sender":{"key":"taylorsantiago@google.com","avatar":null},"body":"The documentation for fetch.unpackLimit states that fetch.unpackLimit\nshould be treated as higher priority than transfer.unpackLimit, but the\nintended order is currently inverted.\n\nSigned-off-by: Taylor Santiago <taylorsantiago@google.com>\n---\n fetch-pack.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 65c1ff4bb4..26999e3b65 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1911,10 +1911,10 @@ static void fetch_pack_setup(void)\n \tif (did_setup)\n \t\treturn;\n \tfetch_pack_config();\n-\tif (0 <= transfer_unpack_limit)\n-\t\tunpack_limit = transfer_unpack_limit;\n-\telse if (0 <= fetch_unpack_limit)\n+\tif (0 <= fetch_unpack_limit)\n \t\tunpack_limit = fetch_unpack_limit;\n+\telse if (0 <= transfer_unpack_limit)\n+\t\tunpack_limit = transfer_unpack_limit;\n \tdid_setup = 1;\n }\n \n-- \n2.41.0.694.ge786442a9b-goog\n\n"},{"id":"480872","messageId":"20230821203051.GA1800481@coredump.intra.peff.net","threadId":"60123","inReplyTo":"20230817215325.2550998-2-taylorsantiago@google.com","subject":"Re: [PATCH 1/1] Fix the order of consuming unpackLimit config settings.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-21T20:30:51Z","receivedAt":"2023-08-21T20:30:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 17, 2023 at 02:53:25PM -0700, Taylor Santiago wrote:\n\n> The documentation for fetch.unpackLimit states that fetch.unpackLimit\n> should be treated as higher priority than transfer.unpackLimit, but the\n> intended order is currently inverted.\n\nLooks like this has been broken since it was introduced in e28714c527\n(Consolidate {receive,fetch}.unpackLimit, 2007-01-24).\n\nSometimes when documentation and code have diverged for so long, we\nprefer to change the documentation instead, to avoid disrupting users.\nBut that would make these weirdly unlike most other \"specific overrides\ngeneral\" config options. And the fact that the bug has existed for so\nlong without anyone noticing implies to me that nobody really tries to\nmix and match them much.\n\nSo I'm in favor of fixing them[1]. Doesn't receive-pack have the same\nbug, though? And we'd probably want to have tests for both.\n\n-Peff\n\n[1] Commit e28714c527 does mention deprecating the operation-specific\n    ones. In my experience (and I did use transfer.unpackLimit quite a\n    bit for server-side code at GitHub) there is no real need for have\n    operation-specific ones. OTOH it is work to deprecate them, and they\n    are not causing a big maintenance burden. So it is probably simplest\n    to keep them.\n"},{"id":"480912","messageId":"xmqqttsrn9l2.fsf@gitster.g","threadId":"60123","inReplyTo":"20230821203051.GA1800481@coredump.intra.peff.net","subject":"Re: [PATCH 1/1] Fix the order of consuming unpackLimit config settings.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-22T01:34:49Z","receivedAt":"2023-08-22T01:34:57Z","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> So I'm in favor of fixing them[1]. Doesn't receive-pack have the same\n> bug, though? And we'd probably want to have tests for both.\n\nThanks, both.  The receive-pack side (changes to the code and\nadditional test) should look like this squashable patch.\n\n\n\n builtin/receive-pack.c    |  6 +++---\n t/t5546-receive-limits.sh | 35 ++++++++++++++++++++++++++++++++---\n 2 files changed, 35 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 1a31a58367..03ac7b01d2 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -2524,10 +2524,10 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \tif (cert_nonce_seed)\n \t\tpush_cert_nonce = prepare_push_cert_nonce(service_dir, time(NULL));\n \n-\tif (0 <= transfer_unpack_limit)\n-\t\tunpack_limit = transfer_unpack_limit;\n-\telse if (0 <= receive_unpack_limit)\n+\tif (0 <= receive_unpack_limit)\n \t\tunpack_limit = receive_unpack_limit;\n+\telse if (0 <= transfer_unpack_limit)\n+\t\tunpack_limit = transfer_unpack_limit;\n \n \tswitch (determine_protocol_version_server()) {\n \tcase protocol_v2:\ndiff --git a/t/t5546-receive-limits.sh b/t/t5546-receive-limits.sh\nindex eed3c9d81a..9fc9ba552f 100755\n--- a/t/t5546-receive-limits.sh\n+++ b/t/t5546-receive-limits.sh\n@@ -9,10 +9,26 @@ TEST_PASSES_SANITIZE_LEAK=true\n # When the limit is 1, `git receive-pack` will call `git index-pack`.\n # When the limit is 10000, `git receive-pack` will call `git unpack-objects`.\n \n+validate_store_type () {\n+\tgit -C dest count-objects -v >actual &&\n+\tcase \"$store_type\" in\n+\tindex)\n+\t\tgrep \"^count: 0$\" actual ;;\n+\tunpack)\n+\t\tgrep \"^packs: 0$\" actual ;;\n+\tesac || {\n+\t\techo \"store_type is $store_type\"\n+\t\tcat actual\n+\t\tfalse;\n+\t}\n+}\n+\n test_pack_input_limit () {\n-\tcase \"$1\" in\n-\tindex) unpack_limit=1 ;;\n-\tunpack) unpack_limit=10000 ;;\n+\tstore_type=$1\n+\n+\tcase \"$store_type\" in\n+\tindex) unpack_limit=1 other_limit=10000 ;;\n+\tunpack) unpack_limit=10000 other_limit=1 ;;\n \tesac\n \n \ttest_expect_success 'prepare destination repository' '\n@@ -43,6 +59,19 @@ test_pack_input_limit () {\n \t\tgit --git-dir=dest config receive.maxInputSize 0 &&\n \t\tgit push dest HEAD\n \t'\n+\n+\ttest_expect_success 'prepare destination repository (once more)' '\n+\t\trm -fr dest &&\n+\t\tgit --bare init dest\n+\t'\n+\n+\ttest_expect_success 'receive trumps transfer' '\n+\t\tgit --git-dir=dest config receive.unpacklimit \"$unpack_limit\" &&\n+\t\tgit --git-dir=dest config transfer.unpacklimit \"$other_limit\" &&\n+\t\tgit push dest HEAD &&\n+\t\tvalidate_store_type\n+\t'\n+\n }\n \n test_expect_success \"create known-size (1024 bytes) commit\" '\n-- \n2.42.0\n\n"},{"id":"480941","messageId":"xmqqpm3eh7f6.fsf@gitster.g","threadId":"60123","inReplyTo":"xmqqttsrn9l2.fsf@gitster.g","subject":"Re: [PATCH 1/1] Fix the order of consuming unpackLimit config settings.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-23T01:30:21Z","receivedAt":"2023-08-23T01:30:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jeff King <peff@peff.net> writes:\n>\n>> So I'm in favor of fixing them[1]. Doesn't receive-pack have the same\n>> bug, though? And we'd probably want to have tests for both.\n>\n> Thanks, both.  The receive-pack side (changes to the code and\n> additional test) should look like this squashable patch.\n>\n>\n>\n>  builtin/receive-pack.c    |  6 +++---\n>  t/t5546-receive-limits.sh | 35 ++++++++++++++++++++++++++++++++---\n>  2 files changed, 35 insertions(+), 6 deletions(-)\n\nYesterday I was a bit too busy (well it was a release day wasn't\nit?) and did not bother writing the tests for fetch/transfer, but it\nseems nobody took the bait to finish it so far, so here it is again.\n\nThis time, what I am sending is not a squashable patch, but the\nwhole thing as a single patch.\n\n------- >8 ------------- >8 ------------- >8 -------\nSubject: transfer.unpackLimit: fetch/receive.unpackLimit takes precedence\n\nThe transfer.unpackLimit configuration variable is documented to be\nused only as a fallback value when the more operation-specific\nfetch.unpackLimit and receive.unpackLimit variables are not set, but\nthe implementation had the precedence reversed.  Apparently this was\nbroken since the transfer.unpackLimit was introduced in e28714c5\n(Consolidate {receive,fetch}.unpackLimit, 2007-01-24).\n\nOften when documentation and code have diverged for so long, we\nprefer to change the documentation instead, to avoid disrupting\nusers.  But doing so would make these weirdly unlike most other\n\"specific overrides general\" config options. And the fact that the\nbug has existed for so long without anyone noticing implies to me\nthat nobody really tries to mix and match them much.\n\nSigned-off-by: Taylor Santiago <taylorsantiago@google.com>\n[jc: rewrote the log message, added tests, covered receive-pack as well]\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/receive-pack.c    |  6 +++---\n fetch-pack.c              |  6 +++---\n t/t5510-fetch.sh          | 46 ++++++++++++++++++++++++++++++++++++++++++++++\n t/t5546-receive-limits.sh | 35 ++++++++++++++++++++++++++++++++---\n 4 files changed, 84 insertions(+), 9 deletions(-)\n\ndiff --git c/builtin/receive-pack.c w/builtin/receive-pack.c\nindex 1a31a58367..03ac7b01d2 100644\n--- c/builtin/receive-pack.c\n+++ w/builtin/receive-pack.c\n@@ -2524,10 +2524,10 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \tif (cert_nonce_seed)\n \t\tpush_cert_nonce = prepare_push_cert_nonce(service_dir, time(NULL));\n \n-\tif (0 <= transfer_unpack_limit)\n-\t\tunpack_limit = transfer_unpack_limit;\n-\telse if (0 <= receive_unpack_limit)\n+\tif (0 <= receive_unpack_limit)\n \t\tunpack_limit = receive_unpack_limit;\n+\telse if (0 <= transfer_unpack_limit)\n+\t\tunpack_limit = transfer_unpack_limit;\n \n \tswitch (determine_protocol_version_server()) {\n \tcase protocol_v2:\ndiff --git c/fetch-pack.c w/fetch-pack.c\nindex 0f71054fba..8b300545d5 100644\n--- c/fetch-pack.c\n+++ w/fetch-pack.c\n@@ -1911,10 +1911,10 @@ static void fetch_pack_setup(void)\n \tif (did_setup)\n \t\treturn;\n \tfetch_pack_config();\n-\tif (0 <= transfer_unpack_limit)\n-\t\tunpack_limit = transfer_unpack_limit;\n-\telse if (0 <= fetch_unpack_limit)\n+\tif (0 <= fetch_unpack_limit)\n \t\tunpack_limit = fetch_unpack_limit;\n+\telse if (0 <= transfer_unpack_limit)\n+\t\tunpack_limit = transfer_unpack_limit;\n \tdid_setup = 1;\n }\n \ndiff --git c/t/t5510-fetch.sh w/t/t5510-fetch.sh\nindex 4f289063ce..19c36b57f4 100755\n--- c/t/t5510-fetch.sh\n+++ w/t/t5510-fetch.sh\n@@ -1127,6 +1127,52 @@ do\n \t'\n done\n \n+test_expect_success 'prepare source branch' '\n+\techo one >onebranch &&\n+\tgit checkout --orphan onebranch &&\n+\tgit rm --cached -r . &&\n+\tgit add onebranch &&\n+\tgit commit -m onebranch &&\n+\tgit rev-list --objects onebranch -- >actual &&\n+\t# 3 objects should be created, at least ...\n+\ttest 3 -le $(wc -l <actual)\n+'\n+\n+validate_store_type () {\n+\tgit -C dest count-objects -v >actual &&\n+\tcase \"$store_type\" in\n+\tpacked)\n+\t\tgrep \"^count: 0$\" actual ;;\n+\tloose)\n+\t\tgrep \"^packs: 0$\" actual ;;\n+\tesac || {\n+\t\techo \"store_type is $store_type\"\n+\t\tcat actual\n+\t\tfalse\n+\t}\n+}\n+\n+test_unpack_limit () {\n+\tstore_type=$1\n+\n+\tcase \"$store_type\" in\n+\tpacked) fetch_limit=1 transfer_limit=10000 ;;\n+\tloose) fetch_limit=10000 transfer_limit=1 ;;\n+\tesac\n+\n+\ttest_expect_success \"fetch trumps transfer limit\" '\n+\t\trm -fr dest &&\n+\t\tgit --bare init dest &&\n+\t\tgit -C dest config fetch.unpacklimit $fetch_limit &&\n+\t\tgit -C dest config transfer.unpacklimit $transfer_limit &&\n+\t\tgit -C dest fetch .. onebranch &&\n+\t\tvalidate_store_type\n+\t'\n+}\n+\n+test_unpack_limit packed\n+test_unpack_limit loose\n+\n setup_negotiation_tip () {\n \tSERVER=\"$1\"\n \tURL=\"$2\"\ndiff --git c/t/t5546-receive-limits.sh w/t/t5546-receive-limits.sh\nindex eed3c9d81a..9fc9ba552f 100755\n--- c/t/t5546-receive-limits.sh\n+++ w/t/t5546-receive-limits.sh\n@@ -9,10 +9,26 @@ TEST_PASSES_SANITIZE_LEAK=true\n # When the limit is 1, `git receive-pack` will call `git index-pack`.\n # When the limit is 10000, `git receive-pack` will call `git unpack-objects`.\n \n+validate_store_type () {\n+\tgit -C dest count-objects -v >actual &&\n+\tcase \"$store_type\" in\n+\tindex)\n+\t\tgrep \"^count: 0$\" actual ;;\n+\tunpack)\n+\t\tgrep \"^packs: 0$\" actual ;;\n+\tesac || {\n+\t\techo \"store_type is $store_type\"\n+\t\tcat actual\n+\t\tfalse;\n+\t}\n+}\n+\n test_pack_input_limit () {\n-\tcase \"$1\" in\n-\tindex) unpack_limit=1 ;;\n-\tunpack) unpack_limit=10000 ;;\n+\tstore_type=$1\n+\n+\tcase \"$store_type\" in\n+\tindex) unpack_limit=1 other_limit=10000 ;;\n+\tunpack) unpack_limit=10000 other_limit=1 ;;\n \tesac\n \n \ttest_expect_success 'prepare destination repository' '\n@@ -43,6 +59,19 @@ test_pack_input_limit () {\n \t\tgit --git-dir=dest config receive.maxInputSize 0 &&\n \t\tgit push dest HEAD\n \t'\n+\n+\ttest_expect_success 'prepare destination repository (once more)' '\n+\t\trm -fr dest &&\n+\t\tgit --bare init dest\n+\t'\n+\n+\ttest_expect_success 'receive trumps transfer' '\n+\t\tgit --git-dir=dest config receive.unpacklimit \"$unpack_limit\" &&\n+\t\tgit --git-dir=dest config transfer.unpacklimit \"$other_limit\" &&\n+\t\tgit push dest HEAD &&\n+\t\tvalidate_store_type\n+\t'\n+\n }\n \n test_expect_success \"create known-size (1024 bytes) commit\" '\n"},{"id":"480942","messageId":"xmqqcyzeh6a2.fsf@gitster.g","threadId":"60123","inReplyTo":"CAKacvadS8X_nb6Z=yub9eJ54hRYWq4B7CYrWaw=uXBY8dPChYA@mail.gmail.com","subject":"Re: [PATCH 1/1] Fix the order of consuming unpackLimit config settings.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-23T01:55:01Z","receivedAt":"2023-08-23T01:56:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Santiago <taylorsantiago@google.com> writes:\n\n> Thank you! How would you like me to proceed? Should I submit the above as a\n> v2 of the earlier patch?\n\nThere is nothing \"above\" as you seem to be top posting ;-)\n\nWhen somebody else helps by supplying an \"squashable\" patch, often\npeople are expected to review it and then update their patch(es)\nusing the given material to produce a v2.\n\nBut as I said, the \"squashable\" one was only about the receive-pack\nside; even if you combined it with your original, tests for the\nfetch side were still missing, so it was not sufficient for a v2.\n\nAs I didn't see your reply message (to which I am responding to)\nuntil now, mostly because it was dropped by the mailing list\n(perhaps it was an HTML e-mail from GMail or something???), I've\nfurther worked on the tests to cover the fetch side and sent out a\nfull version (not a squashable, but just a replacement for the whole\nthing).  It is archived and viewable at\n\n  https://lore.kernel.org/git/xmqqpm3eh7f6.fsf@gitster.g\n\nPart of it is still your original patch, some material in its\nproposed commit log message was given by Peff, and the rest was\nwritten by me, so it carries names of three people.\n\nIf the result looks acceptable to you, then saying \"Yup, that looks\ngood\" would be the simplest answer to give to move things forward.\n\nThanks.\n"},{"id":"480944","messageId":"CAKacvadHXrgVmAfT_W+wPyhhRP2hVXXcVRB3rX8ArKvqUpe=Ug@mail.gmail.com","threadId":"60123","inReplyTo":"xmqqcyzeh6a2.fsf@gitster.g","subject":"Re: [PATCH 1/1] Fix the order of consuming unpackLimit config settings.","fromName":"Taylor Santiago","fromEmail":"taylorsantiago@google.com","sentAt":"2023-08-23T03:32:24Z","receivedAt":"2023-08-23T03:32:47Z","isPatch":true,"sender":{"key":"taylorsantiago@google.com","avatar":null},"body":"Looks good to me.\n\nThanks for the info on the patch process. I also am sending this mail\nin plain text mode so hopefully the mailing list doesn't drop it.\n\n\nOn Tue, Aug 22, 2023 at 6:55 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Taylor Santiago <taylorsantiago@google.com> writes:\n>\n> > Thank you! How would you like me to proceed? Should I submit the above as a\n> > v2 of the earlier patch?\n>\n> There is nothing \"above\" as you seem to be top posting ;-)\n>\n> When somebody else helps by supplying an \"squashable\" patch, often\n> people are expected to review it and then update their patch(es)\n> using the given material to produce a v2.\n>\n> But as I said, the \"squashable\" one was only about the receive-pack\n> side; even if you combined it with your original, tests for the\n> fetch side were still missing, so it was not sufficient for a v2.\n>\n> As I didn't see your reply message (to which I am responding to)\n> until now, mostly because it was dropped by the mailing list\n> (perhaps it was an HTML e-mail from GMail or something???), I've\n> further worked on the tests to cover the fetch side and sent out a\n> full version (not a squashable, but just a replacement for the whole\n> thing).  It is archived and viewable at\n>\n>   https://lore.kernel.org/git/xmqqpm3eh7f6.fsf@gitster.g\n>\n> Part of it is still your original patch, some material in its\n> proposed commit log message was given by Peff, and the rest was\n> written by me, so it carries names of three people.\n>\n> If the result looks acceptable to you, then saying \"Yup, that looks\n> good\" would be the simplest answer to give to move things forward.\n>\n> Thanks.\n"},{"id":"480964","messageId":"20230823190315.GB2355007@coredump.intra.peff.net","threadId":"60123","inReplyTo":"xmqqpm3eh7f6.fsf@gitster.g","subject":"Re: [PATCH 1/1] Fix the order of consuming unpackLimit config settings.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-23T19:03:15Z","receivedAt":"2023-08-23T19:04:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 22, 2023 at 06:30:21PM -0700, Junio C Hamano wrote:\n\n> This time, what I am sending is not a squashable patch, but the\n> whole thing as a single patch.\n> \n> ------- >8 ------------- >8 ------------- >8 -------\n> Subject: transfer.unpackLimit: fetch/receive.unpackLimit takes precedence\n\nThanks, this looks fine to me.\n\n-Peff\n"}]}