Re: [PATCH v2 2/2] t9813: avoid using pipes

[Stefan Beller <sbeller@google.com>]

On Mon, Jan 2, 2017 at 10:45 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> The exit code of the upstream in a pipe is ignored thus we should avoid
> using it.

for commands under test, i.e. git things. Other parts can be piped if that makes
the test easier. Though I guess that can be guessed by the reader as well,
as you only convert git commands on upstream pipes.

> By writing out the output of the git command to a file, we can
> test the exit codes of both the commands.
>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>

Thanks for taking ownership of this issue as well. :)

> ---
>  t/t9813-git-p4-preserve-users.sh | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/t/t9813-git-p4-preserve-users.sh b/t/t9813-git-p4-preserve-users.sh
> index 798bf2b67..9d7550ff3 100755
> --- a/t/t9813-git-p4-preserve-users.sh
> +++ b/t/t9813-git-p4-preserve-users.sh
> @@ -118,12 +118,12 @@ test_expect_success 'not preserving user with mixed authorship' '
>                 make_change_by_user usernamefile3 Derek derek@example.com &&
>                 P4EDITOR=cat P4USER=alice P4PASSWD=secret &&
>                 export P4EDITOR P4USER P4PASSWD &&
> -               git p4 commit |\
> -               grep "git author derek@example.com does not match" &&
> +               git p4 commit >actual 2>&1 &&

Why do we need to pipe 2>&1 here?
Originally the piping only fed the stdout to grep, so this patch changes the
test? Maybe

    2>actual.err &&
    test_must_be_empty actual.err

instead?

Thanks,
Stefan