Re: [PATCH v2 1/6] t: fix cases where output breaks TAP format

> On Wed, May 28, 2025 at 11:55 AM Patrick Steinhardt <ps@pks.im> wrote:
> > On Tue, May 27, 2025 at 03:47:16PM -0400, Eric Sunshine wrote:
> > > On Tue, May 27, 2025 at 10:03 AM Patrick Steinhardt <ps@pks.im> wrote:
> > > > diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh
> > > > @@ -48,7 +48,7 @@ commit_file () {
> > > > -test_create_repo sm1 &&
> > > > +test_create_repo sm1 >/dev/null &&
> > > >  add_file . foo >/dev/null
> > > >
> > > >  head1=$(add_file sm1 foo1 foo2)
> > >
> > > Unlike the case with t1007, in which the entire script needs an
> > > overhaul, it is much easier to fix the problems in this script without
> > > papering over them via ">/dev/null". In particular, it would be
> > > preferable to resolve the issue by wrapping test_expect_success around
> > > the code which currently resides outside of any test. So, for example,
> > > the above could become:
> > >
> > >     test_expect_success 'setup submodule 1' '
> > >         test_create_repo sm1 &&
> > >         add_file . foo &&
> > >         head1=$(add_file sm1 foo1 foo2) &&
> > >         fullhead1=$(cd sm1; git rev-parse --verify HEAD)
> > >     '
> >
> > Yes, it isn't particularly hard. But it does result in a bunch of
> > shuffling that makes the patch way harder to read.
> 
> I'm not sure why such a change would require shuffling code around (or
> perhaps I misunderstand the idea you are trying to convey). Unlike
> t1007 which needs a major overhaul, each block of code which currently
> exists outside of test_expect_success in this script can simply be
> wrapped in test_expect_success (with a distinct "setup whatever"
> title) in situ without shuffling the code around. Yes, such changes
> would be noisy, but it would be very localized noise in each case,
> thus not particularly difficult to review.
> 
> As such, since such a fix is so simple (even if a bit noisy) I'd
> rather see it done properly rather than merely papering over the
> problem. However, I'm just one reviewer; others, including yourself,
> may feel differently.