| Message ID | 20190505081633.41157-1-liboxuan@connect.hku.hk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] t4253-am-keep-cr-dos: avoid using pipes | expand |
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 > '
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: > > 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 > > '
"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.
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: > > "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.
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 '
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(-)