Re: [PATCH 2/3] t6025: replace pipe with redirection operator

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 17, 2020, 21:24 UTC
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.)

Show 8 quoted lines

> 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.)