Re: [PATCH 1/2] t9832,t2200: avoid using pipes in git commands

[Eric Sunshine <sunshine@sunshineco.com>]

On Sat, Oct 17, 2020 at 12:02 PM Amanda Shafack via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> When the upstream of a pipe throws an error, the downstream still
> executes normally. This happens because the exit code of the upstream
> in a pipe is ignored. This behavior can make debugging very hard
> incase a test fails.

To be more precise, we're not worried about difficulty of debugging
when a Git command is upstream in a pipe. What we are more concerned
with is that an unexpected failure of the Git command will go
unnoticed (unless it also happens to produce wrong output which is
later caught somewhere downstream). By avoiding placing Git upstream
in a pipe, we can actively catch a failure of the Git command. The
commit message should focus on that reason instead.

> Also, pipes are prone to deadlocks. If the
> upstream gets full, the commands downstream will never start.

I don't think this is ever an issue in Git tests, so talking about it
here probably only muddies the waters, thus makes the commit message
less precise and more hand-wavy. Dropping this sentence is
recommended.

> Commit c6f44e1da5 (t9813: avoid using pipes, 2017-01-04) noticed that
> the exit code of upstream in the pipe is ignored, thus using pipes
> should be avoided.

There are a lot of commits in the project history which perform this
sort of transformation, so it's not clear to the reader why this
particular commit is called out as being important. If it's not, then
I'd recommend dropping this sentence, as well.

> diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh
>  test_expect_success '"add -u non-existent" should fail' '
>         test_must_fail git add -u non-existent &&
> -       ! (git ls-files | grep "non-existent")
> +       ! (
> +               git ls-files >actual &&
> +               grep "non-existent" actual
> +       )
>  '

This transformation appears to be incorrect. See my review of patch
[2/2] for details.

> diff --git a/t/t9832-unshelve.sh b/t/t9832-unshelve.sh
> @@ -68,7 +68,8 @@ EOF
> -               git show refs/remotes/p4-unshelved/$change | grep -q "Further description" &&
> +               git show refs/remotes/p4-unshelved/$change >actual &&
> +               grep -q "Further description" actual &&

This transformation looks fine.