On Wed, May 28, 2025 at 04:14:17PM -0400, Eric Sunshine wrote:
> On Wed, May 28, 2025 at 11:55 AM Patrick Steinhardt <ps@xxxxxx> 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@xxxxxx> 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.
I was mostly shying away from this because it's hard to argue why I'm
doing these cleanups in one test file, but not in others. And the
reindents to make the diff way noisier. Anyway, I've split up this
commit into multiple now and did some more refactorings to make this a
bit more palatable.
I'll send the revised version to the mailing list later today.
Thanks!
Patrick
