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

From: Taylor Blau <me@ttaylorr.com>
To: Matt Cooper via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, gitster@pobox.com, derrickstolee@github.com,
	Matt Cooper <vtbassmatt@gmail.com>
Subject: Re: [PATCH 2/2] t5302: confirm that large packs mention limit
Date: Wed, 23 Feb 2022 12:22:56 -0500	[thread overview]
Message-ID: <YhZtcEqczAFES+hQ@nand.local> (raw)
In-Reply-To: <43990408a10d65058d872f13ea9619e85de7081d.1645632193.git.gitgitgadget@gmail.com>

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