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.
> > > diff --git a/t/t9822-git-p4-path-encoding.sh b/t/t9822-git-p4-path-encoding.sh
> > > @@ -7,12 +7,17 @@ test_description='Clone repositories with non ASCII paths'
> > > -ISO8859="$(printf "$ISO8859_ESCAPED")" &&
> > > -echo content123 >"$ISO8859" &&
> > > -rm "$ISO8859" || {
> > > +test_lazy_prereq FS_ACCEPTS_ISO_8859_1 '
> > > + ISO8859="$(printf "$ISO8859_ESCAPED")" &&
> > > + echo content123 >"$ISO8859" 2>/dev/null &&
> > > + rm "$ISO8859"
> > > +'
> >
> > Was the problem here that the `echo content123 > "$..."` was
> > potentially spitting out an error message to stderr, thus you had to
> > redirect it to /dev/null to silence it?
>
> Ah, this redirect is not required anymore. I had it in a previous
> version due to the exact problem that you mentioned, that echo spit out
> an error.
Okay. I'm perfectly fine with you turning this into a lazy prereq --
it's cleaner, more modern, and possibly easier to understand as a
prereq -- so no problem there. I asked about it merely because I
didn't understand why it was included in this patch, and the commit
message didn't explain its presence. It would do very well as a
separate patch, either within this series or standalone.
