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.

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

From: Matt Cooper <[email protected]>

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).

---
 builtin/index-pack.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

index 3c2e6aee3cc..c45273de3b1 100644
@@ -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

From: Matt Cooper <[email protected]>

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.

---
 t/t5302-pack-index.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

index 8ee67df38f6..b0095ab41d3 100755
@@ -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

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

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 :-).

It is very much appreciated to point these things out.

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

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.

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.

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

From: Matt Cooper <[email protected]>

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.

---
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 <[email protected]>
+ Helped-by: Derrick Stolee <[email protected]>

## 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(-)

index 3c2e6aee3cc..c45273de3b1 100644
@@ -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)

index 8ee67df38f6..b0095ab41d3 100755
@@ -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