| Message ID | 43990408a10d65058d872f13ea9619e85de7081d.1645632193.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Specify the actual pack size limit which is breached | expand |
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
Taylor Blau <me@ttaylorr.com> writes: > 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. >> +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.
On Wed, Feb 23, 2022 at 03:26:54PM -0800, Junio C Hamano wrote: > > 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. 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. > >> +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? 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
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