From: Junio C Hamano Date: Wed, 23 Feb 2022 23:26:54 GMT Subject: Re: [PATCH 2/2] t5302: confirm that large packs mention limit Message-ID: In-Reply-To: Taylor Blau writes: > On Wed, Feb 23, 2022 at 04:03:13PM +0000, Matt Cooper via GitGitGadget wrote: >> From: Matt Cooper >> >> 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 >> Helped-by: Taylor Blau >> Helped-by: Derrick Stolee > > 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 > + 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.