Re: [PATCH v2] t4253-am-keep-cr-dos: avoid using pipes

From: Junio C Hamano <gitster@pobox.com>
To: Boxuan Li <liboxuan@connect.hku.hk>
Cc: git@vger.kernel.org, sunshine@sunshineco.com
Subject: Re: [PATCH v2] t4253-am-keep-cr-dos: avoid using pipes
Date: Tue, 07 May 2019 18:04:39 +0900	[thread overview]
Message-ID: <xmqqy33iljo8.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20190505081633.41157-1-liboxuan@connect.hku.hk> (Boxuan Li's message of "Sun, 5 May 2019 16:16:33 +0800")

Boxuan Li <liboxuan@connect.hku.hk> writes:

> The exit code of the upstream in a pipe is ignored thus we should avoid
> using it. By writing out the output of the git command to a file, we can
> test the exit codes of both the commands.

We are trying to catch breakages in "am" in these two tests (see the
title of the test file), and we rely on format-patch to correctly
produce its output---if we assume that the command being tested,
i.e. "am", could be fed garbage, the tests become pointless.

And once we decide to rely on the correctness of format-patch in
these two tests, there no longer is a reason to fear that
invocations of it upstream of a pipe would lose the exit status.

So while the patch is not wrong per-se, but these two changes are
borderline Meh.

> Signed-off-by: Boxuan Li <liboxuan@connect.hku.hk>
> ---
> Thanks to Eric Sunshine's review, I've removed spaces after '>'
> and changed 'actual' into 'output'.
> ---
>  t/t4253-am-keep-cr-dos.sh | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/t/t4253-am-keep-cr-dos.sh b/t/t4253-am-keep-cr-dos.sh
> index 553fe3e88e..6e1b73ec3a 100755
> --- a/t/t4253-am-keep-cr-dos.sh
> +++ b/t/t4253-am-keep-cr-dos.sh
> @@ -51,14 +51,16 @@ test_expect_success 'am with dos files without --keep-cr' '
>  
>  test_expect_success 'am with dos files with --keep-cr' '
>  	git checkout -b dosfiles-keep-cr initial &&
> -	git format-patch -k --stdout initial..master | git am --keep-cr -k -3 &&
> +	git format-patch -k --stdout initial..master >output &&
> +	git am --keep-cr -k -3 output &&
>  	git diff --exit-code master
>  '
>  
>  test_expect_success 'am with dos files config am.keepcr' '
>  	git config am.keepcr 1 &&
>  	git checkout -b dosfiles-conf-keepcr initial &&
> -	git format-patch -k --stdout initial..master | git am -k -3 &&
> +	git format-patch -k --stdout initial..master >output &&
> +	git am -k -3 output &&
>  	git diff --exit-code master
>  '