[PATCH 0/2] Specify the actual pack size limit which is breached

[PATCH 0/2] Specify the actual pack size limit which is breached

[Matt Cooper via GitGitGadget]

Git allows configuring a maximum pack size. GitHub (like presumably other
Git hosts) configures this setting to a generous but finite limit. When a
user attempts to push an oversized pack, their connection is terminated with
a message that they've exceeded the limit. The user has to find the limit
value elsewhere, probably in the host's documentation. This change adds a
small convenience -- specifying the limit itself in the error message -- so
that users no longer have to search elsewhere to discover the limit.

Matt Cooper (2):
  index-pack: clarify the breached limit
  t5302: confirm that large packs mention limit

 builtin/index-pack.c  | 8 ++++++--
 t/t5302-pack-index.sh | 8 ++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)


base-commit: e6ebfd0e8cbbd10878070c8a356b5ad1b3ca464e
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1158%2Fvtbassmatt%2Fmc%2Fhumanize-limit-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1158/vtbassmatt/mc/humanize-limit-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1158
-- 
gitgitgadget

[PATCH 1/2] index-pack: clarify the breached limit

[Matt Cooper via GitGitGadget]

From: Matt Cooper <vtbassmatt@gmail.com>

As a small courtesy to users, report what limit was breached. This
is especially useful when a push exceeds a server-defined limit, since
the user is unlikely to have configured the limit (their host did).

Signed-off-by: Matt Cooper <vtbassmatt@gmail.com>
---
 builtin/index-pack.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 3c2e6aee3cc..c45273de3b1 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -323,8 +323,12 @@ static void use(int bytes)
 	if (signed_add_overflows(consumed_bytes, bytes))
 		die(_("pack too large for current definition of off_t"));
 	consumed_bytes += bytes;
-	if (max_input_size && consumed_bytes > max_input_size)
-		die(_("pack exceeds maximum allowed size"));
+	if (max_input_size && consumed_bytes > max_input_size) {
+		struct strbuf size_limit = STRBUF_INIT;
+		strbuf_humanise_bytes(&size_limit, max_input_size);
+		die(_("pack exceeds maximum allowed size (%s)"),
+		    size_limit.buf);
+	}
 }
 
 static const char *open_pack_file(const char *pack_name)
-- 
gitgitgadget

[PATCH 2/2] t5302: confirm that large packs mention limit

[Matt Cooper via GitGitGadget]

From: Matt Cooper <vtbassmatt@gmail.com>

When a pack can't be processed because it's too large, we report the
exact nature of the breach. This test ensures that the error message has
a human-readable size included.

Signed-off-by: Matt Cooper <vtbassmatt@gmail.com>
Helped-by: Taylor Blau <me@ttaylorr.com>
Helped-by: Derrick Stolee <derrickstolee@github.com>
---
 t/t5302-pack-index.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh
index 8ee67df38f6..b0095ab41d3 100755
--- a/t/t5302-pack-index.sh
+++ b/t/t5302-pack-index.sh
@@ -284,4 +284,12 @@ test_expect_success 'index-pack -v --stdin produces progress for both phases' '
 	test_i18ngrep "Resolving deltas" err
 '
 
+test_expect_success 'too-large packs report the breach' '
+	pack=$(git pack-objects --all pack </dev/null) &&
+	sz="$(test_file_size pack-$pack.pack)" &&
+	test "$sz" -gt 20 &&
+	test_must_fail git index-pack --max-input-size=20 pack-$pack.pack 2>err &&
+	grep "maximum allowed size (20 bytes)" err
+'
+
 test_done
-- 
gitgitgadget

Re: [PATCH 0/2] Specify the actual pack size limit which is breached

[Taylor Blau]

On Wed, Feb 23, 2022 at 04:03:11PM +0000, Matt Cooper via GitGitGadget wrote:
> Matt Cooper (2):
>   index-pack: clarify the breached limit
>   t5302: confirm that large packs mention limit
>
>  builtin/index-pack.c  | 8 ++++++--
>  t/t5302-pack-index.sh | 8 ++++++++
>  2 files changed, 14 insertions(+), 2 deletions(-)

I took a look and reviewed this series internally. The patches here
match what I looked at within GitHub, so these look good to me also.

For what it's worth, I wouldn't mind to see these two patches squashed
together, since it may be easier for future readers to see the new code
and test together in the same patch.

But I don't feel strongly about it, so (with or without that suggestion)
I'd be happy to see this get picked up. Thanks, Matt!

Thanks,
Taylor

Re: [PATCH 2/2] t5302: confirm that large packs mention limit

[Taylor Blau]

On Wed, Feb 23, 2022 at 04:03:13PM +0000, Matt Cooper via GitGitGadget wrote:
> From: Matt Cooper <vtbassmatt@gmail.com>
>
> When a pack can't be processed because it's too large, we report the
> exact nature of the breach. This test ensures that the error message has
> a human-readable size included.
>
> Signed-off-by: Matt Cooper <vtbassmatt@gmail.com>
> Helped-by: Taylor Blau <me@ttaylorr.com>
> Helped-by: Derrick Stolee <derrickstolee@github.com>

Ah, one small note here: typically we try to keep commit trailers in
reverse-chronological order, with the most recent thing at the bottom.
That doesn't really matter for the Helped-by's, but keeping your s-o-b
at the bottom indicates that you signed off on the result of your patch
after Stolee and I gave some suggestions.

It's not a huge deal, and I'm sure we have plenty of examples of
slightly out-of-order commit trailers throughout our history. Personally
I don't consider it worth rerolling, but perhaps something to keep in
mind for future contributions :-).

> ---
>  t/t5302-pack-index.sh | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh
> index 8ee67df38f6..b0095ab41d3 100755
> --- a/t/t5302-pack-index.sh
> +++ b/t/t5302-pack-index.sh
> @@ -284,4 +284,12 @@ test_expect_success 'index-pack -v --stdin produces progress for both phases' '
>  	test_i18ngrep "Resolving deltas" err
>  '
>
> +test_expect_success 'too-large packs report the breach' '
> +	pack=$(git pack-objects --all pack </dev/null) &&
> +	sz="$(test_file_size pack-$pack.pack)" &&
> +	test "$sz" -gt 20 &&
> +	test_must_fail git index-pack --max-input-size=20 pack-$pack.pack 2>err &&
> +	grep "maximum allowed size (20 bytes)" err
> +'

This grep is inconsistent with the rest of t5302 (which uses the
now-outdated test_i18ngrep). The choice to deviate from the norm of this
test script in favor of the general guidance against using test_i18ngrep
is intentional AFAIK.

Thanks,
Taylor

Re: [PATCH 2/2] t5302: confirm that large packs mention limit

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> On Wed, Feb 23, 2022 at 04:03:13PM +0000, Matt Cooper via GitGitGadget wrote:
>> From: Matt Cooper <vtbassmatt@gmail.com>
>>
>> When a pack can't be processed because it's too large, we report the
>> exact nature of the breach. This test ensures that the error message has
>> a human-readable size included.
>>
>> Signed-off-by: Matt Cooper <vtbassmatt@gmail.com>
>> Helped-by: Taylor Blau <me@ttaylorr.com>
>> Helped-by: Derrick Stolee <derrickstolee@github.com>
>
> Ah, one small note here: typically we try to keep commit trailers in
> reverse-chronological order, with the most recent thing at the bottom.
> That doesn't really matter for the Helped-by's, but keeping your s-o-b
> at the bottom indicates that you signed off on the result of your patch
> after Stolee and I gave some suggestions.

It is very much appreciated to point these things out.

> It's not a huge deal, and I'm sure we have plenty of examples of
> slightly out-of-order commit trailers throughout our history. Personally
> I don't consider it worth rerolling, but perhaps something to keep in
> mind for future contributions :-).

People need to understand that each such contributor robs maintainer
bandwidth by not rerolling.

>> +test_expect_success 'too-large packs report the breach' '
>> +	pack=$(git pack-objects --all pack </dev/null) &&
>> +	sz="$(test_file_size pack-$pack.pack)" &&
>> +	test "$sz" -gt 20 &&
>> +	test_must_fail git index-pack --max-input-size=20 pack-$pack.pack 2>err &&
>> +	grep "maximum allowed size (20 bytes)" err
>> +'

This test looks OK to me.

Shouldn't it be squashed into the previous patch?  After all, it is
a test for the new behaviour introduced by the previous step, right?

Thanks.

Re: [PATCH 2/2] t5302: confirm that large packs mention limit

[Taylor Blau]

On Wed, Feb 23, 2022 at 03:26:54PM -0800, Junio C Hamano wrote:
> > It's not a huge deal, and I'm sure we have plenty of examples of
> > slightly out-of-order commit trailers throughout our history. Personally
> > I don't consider it worth rerolling, but perhaps something to keep in
> > mind for future contributions :-).
>
> People need to understand that each such contributor robs maintainer
> bandwidth by not rerolling.

I figured you weren't going to tweak the trailers when applying, hence
"not a huge deal" in my comment above. But if it's going to cause you to
spend extra effort in order to pick these patches up, then I'm sure Matt
would be happy to reroll.

> >> +test_expect_success 'too-large packs report the breach' '
> >> +	pack=$(git pack-objects --all pack </dev/null) &&
> >> +	sz="$(test_file_size pack-$pack.pack)" &&
> >> +	test "$sz" -gt 20 &&
> >> +	test_must_fail git index-pack --max-input-size=20 pack-$pack.pack 2>err &&
> >> +	grep "maximum allowed size (20 bytes)" err
> >> +'
>
> This test looks OK to me.
>
> Shouldn't it be squashed into the previous patch?  After all, it is
> a test for the new behaviour introduced by the previous step, right?

Yeah. I mentioned in my response to Matt's cover letter that I figured
the two could be squashed together, and that there was no reason to keep
them separate.

(That was written under the assumption that he wasn't going to send a
reroll, in which case the advice was along the lines of "not a big deal
for this instance, but in the future...")

Thanks,
Taylor

[PATCH v2] index-pack: clarify the breached limit

[Matt Cooper via GitGitGadget]

From: Matt Cooper <vtbassmatt@gmail.com>

As a small courtesy to users, report what limit was breached. This
is especially useful when a push exceeds a server-defined limit, since
the user is unlikely to have configured the limit (their host did).
Also demonstrate the human-readable message in a test.

Helped-by: Taylor Blau <me@ttaylorr.com>
Helped-by: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Matt Cooper <vtbassmatt@gmail.com>
---
    Specify the actual pack size limit which is breached
    
    Git allows configuring a maximum pack size. GitHub (like presumably
    other Git hosts) configures this setting to a generous but finite limit.
    When a user attempts to push an oversized pack, their connection is
    terminated with a message that they've exceeded the limit. The user has
    to find the limit value elsewhere, probably in the host's documentation.
    This change adds a small convenience -- specifying the limit itself in
    the error message -- so that users no longer have to search elsewhere to
    discover the limit.
    
    v2 squashes the changes into one commit and corrects the commit trailer
    misordering.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1158%2Fvtbassmatt%2Fmc%2Fhumanize-limit-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1158/vtbassmatt/mc/humanize-limit-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1158

Range-diff vs v1:

 1:  a2eb3956f3e ! 1:  abf21ec109a index-pack: clarify the breached limit
     @@ Commit message
          As a small courtesy to users, report what limit was breached. This
          is especially useful when a push exceeds a server-defined limit, since
          the user is unlikely to have configured the limit (their host did).
     +    Also demonstrate the human-readable message in a test.
      
     +    Helped-by: Taylor Blau <me@ttaylorr.com>
     +    Helped-by: Derrick Stolee <derrickstolee@github.com>
          Signed-off-by: Matt Cooper <vtbassmatt@gmail.com>
      
       ## builtin/index-pack.c ##
     @@ builtin/index-pack.c: static void use(int bytes)
       }
       
       static const char *open_pack_file(const char *pack_name)
     +
     + ## t/t5302-pack-index.sh ##
     +@@ t/t5302-pack-index.sh: test_expect_success 'index-pack -v --stdin produces progress for both phases' '
     + 	test_i18ngrep "Resolving deltas" err
     + '
     + 
     ++test_expect_success 'too-large packs report the breach' '
     ++	pack=$(git pack-objects --all pack </dev/null) &&
     ++	sz="$(test_file_size pack-$pack.pack)" &&
     ++	test "$sz" -gt 20 &&
     ++	test_must_fail git index-pack --max-input-size=20 pack-$pack.pack 2>err &&
     ++	grep "maximum allowed size (20 bytes)" err
     ++'
     ++
     + test_done
 2:  43990408a10 < -:  ----------- t5302: confirm that large packs mention limit


 builtin/index-pack.c  | 8 ++++++--
 t/t5302-pack-index.sh | 8 ++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 3c2e6aee3cc..c45273de3b1 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -323,8 +323,12 @@ static void use(int bytes)
 	if (signed_add_overflows(consumed_bytes, bytes))
 		die(_("pack too large for current definition of off_t"));
 	consumed_bytes += bytes;
-	if (max_input_size && consumed_bytes > max_input_size)
-		die(_("pack exceeds maximum allowed size"));
+	if (max_input_size && consumed_bytes > max_input_size) {
+		struct strbuf size_limit = STRBUF_INIT;
+		strbuf_humanise_bytes(&size_limit, max_input_size);
+		die(_("pack exceeds maximum allowed size (%s)"),
+		    size_limit.buf);
+	}
 }
 
 static const char *open_pack_file(const char *pack_name)
diff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh
index 8ee67df38f6..b0095ab41d3 100755
--- a/t/t5302-pack-index.sh
+++ b/t/t5302-pack-index.sh
@@ -284,4 +284,12 @@ test_expect_success 'index-pack -v --stdin produces progress for both phases' '
 	test_i18ngrep "Resolving deltas" err
 '
 
+test_expect_success 'too-large packs report the breach' '
+	pack=$(git pack-objects --all pack </dev/null) &&
+	sz="$(test_file_size pack-$pack.pack)" &&
+	test "$sz" -gt 20 &&
+	test_must_fail git index-pack --max-input-size=20 pack-$pack.pack 2>err &&
+	grep "maximum allowed size (20 bytes)" err
+'
+
 test_done

base-commit: e6ebfd0e8cbbd10878070c8a356b5ad1b3ca464e
-- 
gitgitgadget

Re: [PATCH v2] index-pack: clarify the breached limit

[Ævar Arnfjörð Bjarmason]

On Thu, Feb 24 2022, Matt Cooper via GitGitGadget wrote:

> From: Matt Cooper <vtbassmatt@gmail.com>
> [...]
> +test_expect_success 'too-large packs report the breach' '
> +	pack=$(git pack-objects --all pack </dev/null) &&
> +	sz="$(test_file_size pack-$pack.pack)" &&

I don't think this needs a re-roll, but FWIW I have an old-ish local
patch I haven't submitted yet to s/test_file_size/test-tool path-utils
file-size/g in the test suite.

I.e. we've been moving more in the direction of using test-tool
directly, which some tests already do in that case, with some using
test_file_size.

> +	test "$sz" -gt 20 &&
> +	test_must_fail git index-pack --max-input-size=20 pack-$pack.pack 2>err &&
> +	grep "maximum allowed size (20 bytes)" err
> +'

Maybe this is covered by another test, but in case we don't have a test
for this error already: This doesn't cover that we don't error on packs
smaller than the limit, so in terms of black-box testing it's
indistinguishable from us erroring out about this all the time.

But that's probably too paranoid in the first place, and maybe we do
have that other test...