From: Eric Sunshine <sunshine@sunshineco.com>
Date: Fri, 17 Jan 2020 21:24:36 GMT
Subject: Re: [PATCH 2/3] t6025: replace pipe with redirection operator
Message-ID: <CAPig+cQX=jB1KTKcOMVE9u0jX-ZXt_vQBndkzqqQWORu5iFxeA@mail.gmail.com>
In-Reply-To: <20200117204426.9347-3-shouryashukla.oo@gmail.com>

On Fri, Jan 17, 2020 at 3:45 PM Shourya Shukla
<shouryashukla.oo@gmail.com> wrote:
> The exit code of pipes(|) are always ignored, which will create
> errors in subsequent statements. Let's handle it by redirecting
> its output to a file and capturing return values. Replace pipe
> with redirect(>) operator.

This is not an accurate description. The proper way to explain this is
that exit code of a command upstream of a pipe is lost; the exit code
of a command downstream is not lost. We don't want to lose the exit
code of a git command, so a git command should not be upstream. (We
don't care about non-git commands being upstream when that command's
exit code is not relevant.)

> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
> ---
> diff --git a/t/t6025-merge-symlinks.sh b/t/t6025-merge-symlinks.sh
> @@ -17,14 +17,13 @@ test_expect_success 'setup' '
>         git commit -m initial &&
>         git branch b-symlink &&
>         git branch b-file &&
> -       l=$(printf file | git hash-object -t blob -w --stdin) &&

This command is fine as-is. We are interested in the exit code of the
git command (which is correctly downstream), and we don't care about
the exit code of 'printf' (which is upstream), so there is no reason
to rewrite this to use temporary files instead.

> +       printf file >file &&
> +       l=$(git hash-object -t blob -w --stdin) &&

Sorry, but this just doesn't make sense. You're telling "git
hash-object" to take its input from the standard input stream, yet you
don't feed anything to it on that stream. If anything, it should have
been written like this:

     l=$(git hash-object -t blob -w --stdin <file) &&

however, as noted above, there is no reason to avoid pipes in this
case, so this rewrite is unnecessary.

By the way, it's hard to imagine that this test passed once this
change was made (and, if it did pass, then that would likely indicate
that the test is somehow flawed.)