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

> On Tue, May 27, 2025 at 10:03 AM Patrick Steinhardt <ps@pks.im> wrote:
> > The TAP format does not allow arbitrary output outside of a specific
> > test case. If a test suite wants to print any such diagnostic output,
> > then this output has to be prefixed with "#" to mark it accordingly.
> > A bunch of our tests generate output outside of `test_expect_*`
> > testcases anyway without such a mark, which breaks strict TAP parsers.
> >
> > Upon further inspection, all of the output generated by such tests is
> > rather uninteresting. Refactor them so that we don't break the TAP
> > format.
> 
> Nit: Can we avoid the word "refactor" for changes such as those made
> by this patch which clearly are not refactoring[*].
> 
> [*]: From Wikipedia: "... code refactoring is the process of
> restructuring existing source code—changing the factoring—without
> changing its external behavior."

> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > diff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh
> > @@ -30,7 +30,7 @@ setup_repo() {
> >  test_repo=test
> >  push_repo() {
> > -       test_create_repo $test_repo
> > +       test_create_repo $test_repo >/dev/null
> >         cd $test_repo
> >         setup_repo
> 
> Yuck, but certainly the simplest "fix" in this particular case
> considering that, ultimately, this entire script ought to be reworked
> since it cd's around outside of tests with abandon. It would be nice
> to see this script get overhauled eventually but such an undertaking
> doesn't need to be part of this patch series.

> > 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)
>     '
> 
> Note that I also dropped the ">/dev/null" redirect from the add_file()
> invocation.
> 
> The same comment applies to similar changes made by this patch to
> other scripts, such as t4060, t7401.

> > 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?