{"thread":{"id":"57462","subject":"[PATCH 0/2] Specify the actual pack size limit which is breached","startedAt":"2022-02-23T16:03:22Z","lastAt":"2022-02-24T00:17:57Z","messageCount":9,"participants":["Matt Cooper via GitGitGadget","Taylor Blau","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"449271","messageId":"pull.1158.git.1645632193.gitgitgadget@gmail.com","threadId":"57462","inReplyTo":null,"subject":"[PATCH 0/2] Specify the actual pack size limit which is breached","fromName":"Matt Cooper via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-23T16:03:11Z","receivedAt":"2022-02-23T16:03:22Z","isPatch":true,"sender":{"key":"name:Matt Cooper","avatar":null},"body":"Git allows configuring a maximum pack size. GitHub (like presumably other\nGit hosts) configures this setting to a generous but finite limit. When a\nuser attempts to push an oversized pack, their connection is terminated with\na message that they've exceeded the limit. The user has to find the limit\nvalue elsewhere, probably in the host's documentation. This change adds a\nsmall convenience -- specifying the limit itself in the error message -- so\nthat users no longer have to search elsewhere to discover the limit.\n\nMatt Cooper (2):\n  index-pack: clarify the breached limit\n  t5302: confirm that large packs mention limit\n\n builtin/index-pack.c  | 8 ++++++--\n t/t5302-pack-index.sh | 8 ++++++++\n 2 files changed, 14 insertions(+), 2 deletions(-)\n\n\nbase-commit: e6ebfd0e8cbbd10878070c8a356b5ad1b3ca464e\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1158%2Fvtbassmatt%2Fmc%2Fhumanize-limit-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1158/vtbassmatt/mc/humanize-limit-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1158\n-- \ngitgitgadget\n"},{"id":"449272","messageId":"a2eb3956f3e811a1d96618b94738ce862f2c6483.1645632193.git.gitgitgadget@gmail.com","threadId":"57462","inReplyTo":"pull.1158.git.1645632193.gitgitgadget@gmail.com","subject":"[PATCH 1/2] index-pack: clarify the breached limit","fromName":"Matt Cooper via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-23T16:03:12Z","receivedAt":"2022-02-23T16:03:24Z","isPatch":true,"sender":{"key":"name:Matt Cooper","avatar":null},"body":"From: Matt Cooper <vtbassmatt@gmail.com>\n\nAs a small courtesy to users, report what limit was breached. This\nis especially useful when a push exceeds a server-defined limit, since\nthe user is unlikely to have configured the limit (their host did).\n\nSigned-off-by: Matt Cooper <vtbassmatt@gmail.com>\n---\n builtin/index-pack.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 3c2e6aee3cc..c45273de3b1 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -323,8 +323,12 @@ static void use(int bytes)\n \tif (signed_add_overflows(consumed_bytes, bytes))\n \t\tdie(_(\"pack too large for current definition of off_t\"));\n \tconsumed_bytes += bytes;\n-\tif (max_input_size && consumed_bytes > max_input_size)\n-\t\tdie(_(\"pack exceeds maximum allowed size\"));\n+\tif (max_input_size && consumed_bytes > max_input_size) {\n+\t\tstruct strbuf size_limit = STRBUF_INIT;\n+\t\tstrbuf_humanise_bytes(&size_limit, max_input_size);\n+\t\tdie(_(\"pack exceeds maximum allowed size (%s)\"),\n+\t\t    size_limit.buf);\n+\t}\n }\n \n static const char *open_pack_file(const char *pack_name)\n-- \ngitgitgadget\n\n"},{"id":"449273","messageId":"43990408a10d65058d872f13ea9619e85de7081d.1645632193.git.gitgitgadget@gmail.com","threadId":"57462","inReplyTo":"pull.1158.git.1645632193.gitgitgadget@gmail.com","subject":"[PATCH 2/2] t5302: confirm that large packs mention limit","fromName":"Matt Cooper via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-23T16:03:13Z","receivedAt":"2022-02-23T16:03:25Z","isPatch":true,"sender":{"key":"name:Matt Cooper","avatar":null},"body":"From: Matt Cooper <vtbassmatt@gmail.com>\n\nWhen a pack can't be processed because it's too large, we report the\nexact nature of the breach. This test ensures that the error message has\na human-readable size included.\n\nSigned-off-by: Matt Cooper <vtbassmatt@gmail.com>\nHelped-by: Taylor Blau <me@ttaylorr.com>\nHelped-by: Derrick Stolee <derrickstolee@github.com>\n---\n t/t5302-pack-index.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh\nindex 8ee67df38f6..b0095ab41d3 100755\n--- a/t/t5302-pack-index.sh\n+++ b/t/t5302-pack-index.sh\n@@ -284,4 +284,12 @@ test_expect_success 'index-pack -v --stdin produces progress for both phases' '\n \ttest_i18ngrep \"Resolving deltas\" err\n '\n \n+test_expect_success 'too-large packs report the breach' '\n+\tpack=$(git pack-objects --all pack </dev/null) &&\n+\tsz=\"$(test_file_size pack-$pack.pack)\" &&\n+\ttest \"$sz\" -gt 20 &&\n+\ttest_must_fail git index-pack --max-input-size=20 pack-$pack.pack 2>err &&\n+\tgrep \"maximum allowed size (20 bytes)\" err\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"449274","messageId":"YhZhvPcdD/C/LSMP@nand.local","threadId":"57462","inReplyTo":"pull.1158.git.1645632193.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] Specify the actual pack size limit which is breached","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-02-23T16:33:00Z","receivedAt":"2022-02-23T16:33:06Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Feb 23, 2022 at 04:03:11PM +0000, Matt Cooper via GitGitGadget wrote:\n> Matt Cooper (2):\n>   index-pack: clarify the breached limit\n>   t5302: confirm that large packs mention limit\n>\n>  builtin/index-pack.c  | 8 ++++++--\n>  t/t5302-pack-index.sh | 8 ++++++++\n>  2 files changed, 14 insertions(+), 2 deletions(-)\n\nI took a look and reviewed this series internally. The patches here\nmatch what I looked at within GitHub, so these look good to me also.\n\nFor what it's worth, I wouldn't mind to see these two patches squashed\ntogether, since it may be easier for future readers to see the new code\nand test together in the same patch.\n\nBut I don't feel strongly about it, so (with or without that suggestion)\nI'd be happy to see this get picked up. Thanks, Matt!\n\nThanks,\nTaylor\n"},{"id":"449275","messageId":"YhZtcEqczAFES+hQ@nand.local","threadId":"57462","inReplyTo":"43990408a10d65058d872f13ea9619e85de7081d.1645632193.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] t5302: confirm that large packs mention limit","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-02-23T17:22:56Z","receivedAt":"2022-02-23T17:23:01Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Feb 23, 2022 at 04:03:13PM +0000, Matt Cooper via GitGitGadget wrote:\n> From: Matt Cooper <vtbassmatt@gmail.com>\n>\n> When a pack can't be processed because it's too large, we report the\n> exact nature of the breach. This test ensures that the error message has\n> a human-readable size included.\n>\n> Signed-off-by: Matt Cooper <vtbassmatt@gmail.com>\n> Helped-by: Taylor Blau <me@ttaylorr.com>\n> Helped-by: Derrick Stolee <derrickstolee@github.com>\n\nAh, one small note here: typically we try to keep commit trailers in\nreverse-chronological order, with the most recent thing at the bottom.\nThat doesn't really matter for the Helped-by's, but keeping your s-o-b\nat the bottom indicates that you signed off on the result of your patch\nafter Stolee and I gave some suggestions.\n\nIt's not a huge deal, and I'm sure we have plenty of examples of\nslightly out-of-order commit trailers throughout our history. Personally\nI don't consider it worth rerolling, but perhaps something to keep in\nmind for future contributions :-).\n\n> ---\n>  t/t5302-pack-index.sh | 8 ++++++++\n>  1 file changed, 8 insertions(+)\n>\n> diff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh\n> index 8ee67df38f6..b0095ab41d3 100755\n> --- a/t/t5302-pack-index.sh\n> +++ b/t/t5302-pack-index.sh\n> @@ -284,4 +284,12 @@ test_expect_success 'index-pack -v --stdin produces progress for both phases' '\n>  \ttest_i18ngrep \"Resolving deltas\" err\n>  '\n>\n> +test_expect_success 'too-large packs report the breach' '\n> +\tpack=$(git pack-objects --all pack </dev/null) &&\n> +\tsz=\"$(test_file_size pack-$pack.pack)\" &&\n> +\ttest \"$sz\" -gt 20 &&\n> +\ttest_must_fail git index-pack --max-input-size=20 pack-$pack.pack 2>err &&\n> +\tgrep \"maximum allowed size (20 bytes)\" err\n> +'\n\nThis grep is inconsistent with the rest of t5302 (which uses the\nnow-outdated test_i18ngrep). The choice to deviate from the norm of this\ntest script in favor of the general guidance against using test_i18ngrep\nis intentional AFAIK.\n\nThanks,\nTaylor\n"},{"id":"449368","messageId":"xmqqh78pp3k1.fsf@gitster.g","threadId":"57462","inReplyTo":"YhZtcEqczAFES+hQ@nand.local","subject":"Re: [PATCH 2/2] t5302: confirm that large packs mention limit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-23T23:26:54Z","receivedAt":"2022-02-23T23:27:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> On Wed, Feb 23, 2022 at 04:03:13PM +0000, Matt Cooper via GitGitGadget wrote:\n>> From: Matt Cooper <vtbassmatt@gmail.com>\n>>\n>> When a pack can't be processed because it's too large, we report the\n>> exact nature of the breach. This test ensures that the error message has\n>> a human-readable size included.\n>>\n>> Signed-off-by: Matt Cooper <vtbassmatt@gmail.com>\n>> Helped-by: Taylor Blau <me@ttaylorr.com>\n>> Helped-by: Derrick Stolee <derrickstolee@github.com>\n>\n> Ah, one small note here: typically we try to keep commit trailers in\n> reverse-chronological order, with the most recent thing at the bottom.\n> That doesn't really matter for the Helped-by's, but keeping your s-o-b\n> at the bottom indicates that you signed off on the result of your patch\n> after Stolee and I gave some suggestions.\n\nIt is very much appreciated to point these things out.\n\n> It's not a huge deal, and I'm sure we have plenty of examples of\n> slightly out-of-order commit trailers throughout our history. Personally\n> I don't consider it worth rerolling, but perhaps something to keep in\n> mind for future contributions :-).\n\nPeople need to understand that each such contributor robs maintainer\nbandwidth by not rerolling.\n\n>> +test_expect_success 'too-large packs report the breach' '\n>> +\tpack=$(git pack-objects --all pack </dev/null) &&\n>> +\tsz=\"$(test_file_size pack-$pack.pack)\" &&\n>> +\ttest \"$sz\" -gt 20 &&\n>> +\ttest_must_fail git index-pack --max-input-size=20 pack-$pack.pack 2>err &&\n>> +\tgrep \"maximum allowed size (20 bytes)\" err\n>> +'\n\nThis test looks OK to me.\n\nShouldn't it be squashed into the previous patch?  After all, it is\na test for the new behaviour introduced by the previous step, right?\n\nThanks.\n\n"},{"id":"449374","messageId":"YhbGIeksK7Df1QzS@nand.local","threadId":"57462","inReplyTo":"xmqqh78pp3k1.fsf@gitster.g","subject":"Re: [PATCH 2/2] t5302: confirm that large packs mention limit","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-02-23T23:41:21Z","receivedAt":"2022-02-23T23:41:30Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Feb 23, 2022 at 03:26:54PM -0800, Junio C Hamano wrote:\n> > It's not a huge deal, and I'm sure we have plenty of examples of\n> > slightly out-of-order commit trailers throughout our history. Personally\n> > I don't consider it worth rerolling, but perhaps something to keep in\n> > mind for future contributions :-).\n>\n> People need to understand that each such contributor robs maintainer\n> bandwidth by not rerolling.\n\nI figured you weren't going to tweak the trailers when applying, hence\n\"not a huge deal\" in my comment above. But if it's going to cause you to\nspend extra effort in order to pick these patches up, then I'm sure Matt\nwould be happy to reroll.\n\n> >> +test_expect_success 'too-large packs report the breach' '\n> >> +\tpack=$(git pack-objects --all pack </dev/null) &&\n> >> +\tsz=\"$(test_file_size pack-$pack.pack)\" &&\n> >> +\ttest \"$sz\" -gt 20 &&\n> >> +\ttest_must_fail git index-pack --max-input-size=20 pack-$pack.pack 2>err &&\n> >> +\tgrep \"maximum allowed size (20 bytes)\" err\n> >> +'\n>\n> This test looks OK to me.\n>\n> Shouldn't it be squashed into the previous patch?  After all, it is\n> a test for the new behaviour introduced by the previous step, right?\n\nYeah. I mentioned in my response to Matt's cover letter that I figured\nthe two could be squashed together, and that there was no reason to keep\nthem separate.\n\n(That was written under the assumption that he wasn't going to send a\nreroll, in which case the advice was along the lines of \"not a big deal\nfor this instance, but in the future...\")\n\nThanks,\nTaylor\n"},{"id":"449377","messageId":"pull.1158.v2.git.1645661240356.gitgitgadget@gmail.com","threadId":"57462","inReplyTo":"pull.1158.git.1645632193.gitgitgadget@gmail.com","subject":"[PATCH v2] index-pack: clarify the breached limit","fromName":"Matt Cooper via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-24T00:07:20Z","receivedAt":"2022-02-24T00:07:28Z","isPatch":true,"sender":{"key":"name:Matt Cooper","avatar":null},"body":"From: Matt Cooper <vtbassmatt@gmail.com>\n\nAs a small courtesy to users, report what limit was breached. This\nis especially useful when a push exceeds a server-defined limit, since\nthe user is unlikely to have configured the limit (their host did).\nAlso demonstrate the human-readable message in a test.\n\nHelped-by: Taylor Blau <me@ttaylorr.com>\nHelped-by: Derrick Stolee <derrickstolee@github.com>\nSigned-off-by: Matt Cooper <vtbassmatt@gmail.com>\n---\n    Specify the actual pack size limit which is breached\n    \n    Git allows configuring a maximum pack size. GitHub (like presumably\n    other Git hosts) configures this setting to a generous but finite limit.\n    When a user attempts to push an oversized pack, their connection is\n    terminated with a message that they've exceeded the limit. The user has\n    to find the limit value elsewhere, probably in the host's documentation.\n    This change adds a small convenience -- specifying the limit itself in\n    the error message -- so that users no longer have to search elsewhere to\n    discover the limit.\n    \n    v2 squashes the changes into one commit and corrects the commit trailer\n    misordering.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1158%2Fvtbassmatt%2Fmc%2Fhumanize-limit-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1158/vtbassmatt/mc/humanize-limit-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1158\n\nRange-diff vs v1:\n\n 1:  a2eb3956f3e ! 1:  abf21ec109a index-pack: clarify the breached limit\n     @@ Commit message\n          As a small courtesy to users, report what limit was breached. This\n          is especially useful when a push exceeds a server-defined limit, since\n          the user is unlikely to have configured the limit (their host did).\n     +    Also demonstrate the human-readable message in a test.\n      \n     +    Helped-by: Taylor Blau <me@ttaylorr.com>\n     +    Helped-by: Derrick Stolee <derrickstolee@github.com>\n          Signed-off-by: Matt Cooper <vtbassmatt@gmail.com>\n      \n       ## builtin/index-pack.c ##\n     @@ builtin/index-pack.c: static void use(int bytes)\n       }\n       \n       static const char *open_pack_file(const char *pack_name)\n     +\n     + ## t/t5302-pack-index.sh ##\n     +@@ t/t5302-pack-index.sh: test_expect_success 'index-pack -v --stdin produces progress for both phases' '\n     + \ttest_i18ngrep \"Resolving deltas\" err\n     + '\n     + \n     ++test_expect_success 'too-large packs report the breach' '\n     ++\tpack=$(git pack-objects --all pack </dev/null) &&\n     ++\tsz=\"$(test_file_size pack-$pack.pack)\" &&\n     ++\ttest \"$sz\" -gt 20 &&\n     ++\ttest_must_fail git index-pack --max-input-size=20 pack-$pack.pack 2>err &&\n     ++\tgrep \"maximum allowed size (20 bytes)\" err\n     ++'\n     ++\n     + test_done\n 2:  43990408a10 < -:  ----------- t5302: confirm that large packs mention limit\n\n\n builtin/index-pack.c  | 8 ++++++--\n t/t5302-pack-index.sh | 8 ++++++++\n 2 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 3c2e6aee3cc..c45273de3b1 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -323,8 +323,12 @@ static void use(int bytes)\n \tif (signed_add_overflows(consumed_bytes, bytes))\n \t\tdie(_(\"pack too large for current definition of off_t\"));\n \tconsumed_bytes += bytes;\n-\tif (max_input_size && consumed_bytes > max_input_size)\n-\t\tdie(_(\"pack exceeds maximum allowed size\"));\n+\tif (max_input_size && consumed_bytes > max_input_size) {\n+\t\tstruct strbuf size_limit = STRBUF_INIT;\n+\t\tstrbuf_humanise_bytes(&size_limit, max_input_size);\n+\t\tdie(_(\"pack exceeds maximum allowed size (%s)\"),\n+\t\t    size_limit.buf);\n+\t}\n }\n \n static const char *open_pack_file(const char *pack_name)\ndiff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh\nindex 8ee67df38f6..b0095ab41d3 100755\n--- a/t/t5302-pack-index.sh\n+++ b/t/t5302-pack-index.sh\n@@ -284,4 +284,12 @@ test_expect_success 'index-pack -v --stdin produces progress for both phases' '\n \ttest_i18ngrep \"Resolving deltas\" err\n '\n \n+test_expect_success 'too-large packs report the breach' '\n+\tpack=$(git pack-objects --all pack </dev/null) &&\n+\tsz=\"$(test_file_size pack-$pack.pack)\" &&\n+\ttest \"$sz\" -gt 20 &&\n+\ttest_must_fail git index-pack --max-input-size=20 pack-$pack.pack 2>err &&\n+\tgrep \"maximum allowed size (20 bytes)\" err\n+'\n+\n test_done\n\nbase-commit: e6ebfd0e8cbbd10878070c8a356b5ad1b3ca464e\n-- \ngitgitgadget\n"},{"id":"449380","messageId":"220224.86wnhl9ky8.gmgdl@evledraar.gmail.com","threadId":"57462","inReplyTo":"pull.1158.v2.git.1645661240356.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] index-pack: clarify the breached limit","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-24T00:14:25Z","receivedAt":"2022-02-24T00:17:57Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Feb 24 2022, Matt Cooper via GitGitGadget wrote:\n\n> From: Matt Cooper <vtbassmatt@gmail.com>\n> [...]\n> +test_expect_success 'too-large packs report the breach' '\n> +\tpack=$(git pack-objects --all pack </dev/null) &&\n> +\tsz=\"$(test_file_size pack-$pack.pack)\" &&\n\nI don't think this needs a re-roll, but FWIW I have an old-ish local\npatch I haven't submitted yet to s/test_file_size/test-tool path-utils\nfile-size/g in the test suite.\n\nI.e. we've been moving more in the direction of using test-tool\ndirectly, which some tests already do in that case, with some using\ntest_file_size.\n\n> +\ttest \"$sz\" -gt 20 &&\n> +\ttest_must_fail git index-pack --max-input-size=20 pack-$pack.pack 2>err &&\n> +\tgrep \"maximum allowed size (20 bytes)\" err\n> +'\n\nMaybe this is covered by another test, but in case we don't have a test\nfor this error already: This doesn't cover that we don't error on packs\nsmaller than the limit, so in terms of black-box testing it's\nindistinguishable from us erroring out about this all the time.\n\nBut that's probably too paranoid in the first place, and maybe we do\nhave that other test...\n"}]}