{"thread":{"id":"51034","subject":"[PATCH v2] t4253-am-keep-cr-dos: avoid using pipes","startedAt":"2019-05-05T08:16:55Z","lastAt":"2019-05-08T16:49:42Z","messageCount":5,"participants":["Boxuan Li","Junio C Hamano","LI, BO XUAN"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"374907","messageId":"20190505081633.41157-1-liboxuan@connect.hku.hk","threadId":"51034","inReplyTo":null,"subject":"[PATCH v2] t4253-am-keep-cr-dos: avoid using pipes","fromName":"Boxuan Li","fromEmail":"liboxuan@connect.hku.hk","sentAt":"2019-05-05T08:16:33Z","receivedAt":"2019-05-05T08:16:55Z","isPatch":true,"sender":{"key":"liboxuan@connect.hku.hk","avatar":"https://avatars.githubusercontent.com/u/25746010?v=4"},"body":"The exit code of the upstream in a pipe is ignored thus we should avoid\nusing it. By writing out the output of the git command to a file, we can\ntest the exit codes of both the commands.\n\nSigned-off-by: Boxuan Li \n---\nThanks to Eric Sunshine's review, I've removed spaces after '>'\nand changed 'actual' into 'output'.\n---\n t/t4253-am-keep-cr-dos.sh | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t4253-am-keep-cr-dos.sh b/t/t4253-am-keep-cr-dos.sh\nindex 553fe3e88e..6e1b73ec3a 100755\n--- a/t/t4253-am-keep-cr-dos.sh\n+++ b/t/t4253-am-keep-cr-dos.sh\n@@ -51,14 +51,16 @@ test_expect_success 'am with dos files without --keep-cr' '\n \n test_expect_success 'am with dos files with --keep-cr' '\n \tgit checkout -b dosfiles-keep-cr initial &&\n-\tgit format-patch -k --stdout initial..master | git am --keep-cr -k -3 &&\n+\tgit format-patch -k --stdout initial..master >output &&\n+\tgit am --keep-cr -k -3 output &&\n \tgit diff --exit-code master\n '\n \n test_expect_success 'am with dos files config am.keepcr' '\n \tgit config am.keepcr 1 &&\n \tgit checkout -b dosfiles-conf-keepcr initial &&\n-\tgit format-patch -k --stdout initial..master | git am -k -3 &&\n+\tgit format-patch -k --stdout initial..master >output &&\n+\tgit am -k -3 output &&\n \tgit diff --exit-code master\n '\n \n-- \n2.21.0.777.g83232e3864\n\n"},{"id":"375001","messageId":"xmqqy33iljo8.fsf@gitster-ct.c.googlers.com","threadId":"51034","inReplyTo":"20190505081633.41157-1-liboxuan@connect.hku.hk","subject":"Re: [PATCH v2] t4253-am-keep-cr-dos: avoid using pipes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-07T09:04:39Z","receivedAt":"2019-05-07T09:04:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Boxuan Li writes:\n\n> The exit code of the upstream in a pipe is ignored thus we should avoid\n> using it. By writing out the output of the git command to a file, we can\n> test the exit codes of both the commands.\n\nWe are trying to catch breakages in \"am\" in these two tests (see the\ntitle of the test file), and we rely on format-patch to correctly\nproduce its output---if we assume that the command being tested,\ni.e. \"am\", could be fed garbage, the tests become pointless.\n\nAnd once we decide to rely on the correctness of format-patch in\nthese two tests, there no longer is a reason to fear that\ninvocations of it upstream of a pipe would lose the exit status.\n\nSo while the patch is not wrong per-se, but these two changes are\nborderline Meh.\n\n> Signed-off-by: Boxuan Li \n> ---\n> Thanks to Eric Sunshine's review, I've removed spaces after '>'\n> and changed 'actual' into 'output'.\n> ---\n> t/t4253-am-keep-cr-dos.sh | 6 ++++--\n> 1 file changed, 4 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/t4253-am-keep-cr-dos.sh b/t/t4253-am-keep-cr-dos.sh\n> index 553fe3e88e..6e1b73ec3a 100755\n> --- a/t/t4253-am-keep-cr-dos.sh\n> +++ b/t/t4253-am-keep-cr-dos.sh\n> @@ -51,14 +51,16 @@ test_expect_success 'am with dos files without --keep-cr' '\n> \n> test_expect_success 'am with dos files with --keep-cr' '\n> \tgit checkout -b dosfiles-keep-cr initial &&\n> -\tgit format-patch -k --stdout initial..master | git am --keep-cr -k -3 &&\n> +\tgit format-patch -k --stdout initial..master >output &&\n> +\tgit am --keep-cr -k -3 output &&\n> \tgit diff --exit-code master\n> '\n> \n> test_expect_success 'am with dos files config am.keepcr' '\n> \tgit config am.keepcr 1 &&\n> \tgit checkout -b dosfiles-conf-keepcr initial &&\n> -\tgit format-patch -k --stdout initial..master | git am -k -3 &&\n> +\tgit format-patch -k --stdout initial..master >output &&\n> +\tgit am -k -3 output &&\n> \tgit diff --exit-code master\n> '\n"},{"id":"375026","messageId":"CALM0=-=5WeTJ2-r+u-8TYzOAih_3aCEESSpA4oKhxjRpK2yj6w@mail.gmail.com","threadId":"51034","inReplyTo":"xmqqy33iljo8.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] t4253-am-keep-cr-dos: avoid using pipes","fromName":"LI, BO XUAN","fromEmail":"liboxuan@connect.hku.hk","sentAt":"2019-05-07T12:42:45Z","receivedAt":"2019-05-07T12:43:23Z","isPatch":true,"sender":{"key":"liboxuan@connect.hku.hk","avatar":"https://avatars.githubusercontent.com/u/25746010?v=4"},"body":"Hi Junio,\n\nThanks for your review! I can understand your point, but I've got a\nquick question:\n\nWhat if format-patch really breaks and 'am' magically does not break?\nThen the two tests might still pass. On the contrary, with this patch,\nwe can verify the correctness of format-patch and safely rely on it.\n\nBest regards,\nBoxuan Li\n\nOn Tue, May 7, 2019 at 5:04 PM Junio C Hamano wrote:\n>\n> Boxuan Li writes:\n>\n> > The exit code of the upstream in a pipe is ignored thus we should avoid\n> > using it. By writing out the output of the git command to a file, we can\n> > test the exit codes of both the commands.\n>\n> We are trying to catch breakages in \"am\" in these two tests (see the\n> title of the test file), and we rely on format-patch to correctly\n> produce its output---if we assume that the command being tested,\n> i.e. \"am\", could be fed garbage, the tests become pointless.\n>\n> And once we decide to rely on the correctness of format-patch in\n> these two tests, there no longer is a reason to fear that\n> invocations of it upstream of a pipe would lose the exit status.\n>\n> So while the patch is not wrong per-se, but these two changes are\n> borderline Meh.\n>\n> > Signed-off-by: Boxuan Li \n> > ---\n> > Thanks to Eric Sunshine's review, I've removed spaces after '>'\n> > and changed 'actual' into 'output'.\n> > ---\n> > t/t4253-am-keep-cr-dos.sh | 6 ++++--\n> > 1 file changed, 4 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/t/t4253-am-keep-cr-dos.sh b/t/t4253-am-keep-cr-dos.sh\n> > index 553fe3e88e..6e1b73ec3a 100755\n> > --- a/t/t4253-am-keep-cr-dos.sh\n> > +++ b/t/t4253-am-keep-cr-dos.sh\n> > @@ -51,14 +51,16 @@ test_expect_success 'am with dos files without --keep-cr' '\n> >\n> > test_expect_success 'am with dos files with --keep-cr' '\n> > git checkout -b dosfiles-keep-cr initial &&\n> > - git format-patch -k --stdout initial..master | git am --keep-cr -k -3 &&\n> > + git format-patch -k --stdout initial..master >output &&\n> > + git am --keep-cr -k -3 output &&\n> > git diff --exit-code master\n> > '\n> >\n> > test_expect_success 'am with dos files config am.keepcr' '\n> > git config am.keepcr 1 &&\n> > git checkout -b dosfiles-conf-keepcr initial &&\n> > - git format-patch -k --stdout initial..master | git am -k -3 &&\n> > + git format-patch -k --stdout initial..master >output &&\n> > + git am -k -3 output &&\n> > git diff --exit-code master\n> > '\n"},{"id":"375093","messageId":"xmqqv9ylh4wl.fsf@gitster-ct.c.googlers.com","threadId":"51034","inReplyTo":"CALM0=-=5WeTJ2-r+u-8TYzOAih_3aCEESSpA4oKhxjRpK2yj6w@mail.gmail.com","subject":"Re: [PATCH v2] t4253-am-keep-cr-dos: avoid using pipes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-08T05:49:30Z","receivedAt":"2019-05-08T05:49:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"LI, BO XUAN\" writes:\n\n> Thanks for your review! I can understand your point, but I've got a\n> quick question:\n>\n> What if format-patch really breaks and 'am' magically does not break?\n\nDoesn't that indicate that you are not testing the result of \"am\"\nadequately?\n\nI am not saying it is *wrong* to split the pipe into two. I am\nmerely saying that it is borderline Meh, as the primary point of\nthese two tests are to see how the command downstream of the pipe\nbehaves and not the upstream one.\n"},{"id":"375167","messageId":"CALM0=-mDhCqfL0XpaH27cHxcfRq1Y2xJQ=GQLCVdACsOHj0hiA@mail.gmail.com","threadId":"51034","inReplyTo":"xmqqv9ylh4wl.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] t4253-am-keep-cr-dos: avoid using pipes","fromName":"LI, BO XUAN","fromEmail":"liboxuan@connect.hku.hk","sentAt":"2019-05-08T16:49:03Z","receivedAt":"2019-05-08T16:49:42Z","isPatch":true,"sender":{"key":"liboxuan@connect.hku.hk","avatar":"https://avatars.githubusercontent.com/u/25746010?v=4"},"body":"Hi Junio,\n\nUnderstood. Thanks for the clarification.\n\nBest regards,\nBoxuan Li\n\nOn Wed, May 8, 2019 at 1:49 PM Junio C Hamano wrote:\n>\n> \"LI, BO XUAN\" writes:\n>\n> > Thanks for your review! I can understand your point, but I've got a\n> > quick question:\n> >\n> > What if format-patch really breaks and 'am' magically does not break?\n>\n> Doesn't that indicate that you are not testing the result of \"am\"\n> adequately?\n>\n> I am not saying it is *wrong* to split the pipe into two. I am\n> merely saying that it is borderline Meh, as the primary point of\n> these two tests are to see how the command downstream of the pipe\n> behaves and not the upstream one.\n"}]}