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

[Eric Sunshine <sunshine@sunshineco.com>]

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? If so, did the file get
created in the error case? What I'm wondering is whether you also
should use `rm -f` when removing the file.