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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 23, 2022, 23:26 UTC
Message-ID: <xmqqh78pp3k1.fsf@gitster.g>
In-Reply-To: <YhZtcEqczAFES+hQ@nand.local>

Taylor Blau <me@ttaylorr.com> writes:

Show 16 quoted lines

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

Show 7 quoted lines

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