threads / patch / 51034

v2t4253-am-keep-cr-dos: avoid using pipes

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

## tl;dr

5 messages between May 5, 2019 and May 8, 2019. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Boxuan Li· May 5, 2019, 08:16 UTC · lore

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.

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(-)
Show changes to t/t4253-am-keep-cr-dos.sh +4 −2
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
 '
 
-- 
2.21.0.777.g83232e3864
Junio C Hamano· May 7, 2019, 09:04 UTC · re: Boxuan Li · lore

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

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.

Show 30 quoted lines
> 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
>  '
LI, BO XUAN· May 7, 2019, 12:42 UTC · re: Junio C Hamano · lore

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

Hi Junio,

Thanks for your review! I can understand your point, but I've got a quick question:

What if format-patch really breaks and 'am' magically does not break? Then the two tests might still pass. On the contrary, with this patch, we can verify the correctness of format-patch and safely rely on it.

Best regards, Boxuan Li

On Tue, May 7, 2019 at 5:04 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 49 quoted lines
>
> 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
> >  '
Junio C Hamano· May 8, 2019, 05:49 UTC · re: LI, BO XUAN · lore

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

"LI, BO XUAN" <liboxuan@connect.hku.hk> writes:
> Thanks for your review! I can understand your point, but I've got a
> quick question:
>
> What if format-patch really breaks and 'am' magically does not break?

Doesn't that indicate that you are not testing the result of "am" adequately?

I am not saying it is *wrong* to split the pipe into two. I am merely saying that it is borderline Meh, as the primary point of these two tests are to see how the command downstream of the pipe behaves and not the upstream one.

LI, BO XUAN· May 8, 2019, 16:49 UTC · re: Junio C Hamano · lore

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

Hi Junio,
Understood. Thanks for the clarification.

Best regards, Boxuan Li

On Wed, May 8, 2019 at 1:49 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 15 quoted lines
>
> "LI, BO XUAN" <liboxuan@connect.hku.hk> writes:
>
> > Thanks for your review! I can understand your point, but I've got a
> > quick question:
> >
> > What if format-patch really breaks and 'am' magically does not break?
>
> Doesn't that indicate that you are not testing the result of "am"
> adequately?
>
> I am not saying it is *wrong* to split the pipe into two.  I am
> merely saying that it is borderline Meh, as the primary point of
> these two tests are to see how the command downstream of the pipe
> behaves and not the upstream one.

← back to recent threads