[v2] index-pack: clarify the breached limit

Commit Message

Matt Cooper Feb. 24, 2022, 12:07 a.m. UTC

From: Matt Cooper <[e-mail]>

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 <[e-mail]>
Helped-by: Derrick Stolee <[e-mail]>
Signed-off-by: Matt Cooper <[e-mail]>
---
    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 <[e-mail]>
     +    Helped-by: Derrick Stolee <[e-mail]>
          Signed-off-by: Matt Cooper <[e-mail]>
      
       ## 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(-)


base-commit: e6ebfd0e8cbbd10878070c8a356b5ad1b3ca464e

Comments

Ævar Arnfjörð Bjarmason Feb. 24, 2022, 12:14 a.m. UTC | #1

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

> From: Matt Cooper <[e-mail]>
> [...]
> +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...

Patch

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