{"thread":{"id":"51324","subject":"[PATCH] Use an intermediate file between between git blame and sed to avoid git blame's exit code being hidden.","startedAt":"2019-06-15T18:43:05Z","lastAt":"2019-06-16T22:44:15Z","messageCount":4,"participants":["michael@platin.gs","Denton Liu","Michael Platings","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"377306","messageId":"20190615184039.3711-1-michael@platin.gs","threadId":"51324","inReplyTo":null,"subject":"[PATCH] Use an intermediate file between between git blame and sed to avoid git blame's exit code being hidden.","fromName":"","fromEmail":"michael@platin.gs","sentAt":"2019-06-15T18:40:39Z","receivedAt":"2019-06-15T18:43:05Z","isPatch":true,"sender":{"key":"michael@platin.gs","avatar":"https://avatars.githubusercontent.com/u/1112348?v=4"},"body":"From: Michael Platings <michael@platin.gs>\n\n---\n t/t8014-blame-ignore-fuzzy.sh | 13 +++++++++----\n 1 file changed, 9 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t8014-blame-ignore-fuzzy.sh b/t/t8014-blame-ignore-fuzzy.sh\nindex 1ff59624e9..13f3313710 100755\n--- a/t/t8014-blame-ignore-fuzzy.sh\n+++ b/t/t8014-blame-ignore-fuzzy.sh\n@@ -332,7 +332,9 @@ test_expect_success setup '\n for i in $(test_seq 2 $last_test); do\n \teval title=\"\\$title$i\"\n \ttest_expect_success \"$title\" \\\n-\t\"git blame -M9 --ignore-rev $IGNOREME $i | sed -e \\\"$pick_author\\\" >actual && test_cmp expected$i actual\"\n+\t\"git blame -M9 --ignore-rev $IGNOREME $i >output &&\n+\tsed -e \\\"$pick_author\\\" <output >actual &&\n+\ttest_cmp expected$i actual\"\n done\n \n # This invoked a null pointer dereference when the chunk callback was called\n@@ -357,7 +359,8 @@ test_expect_success 'Diff chunks with no suspects' '\n \n \ttest_write_lines 1 1 >expected &&\n \n-\tgit blame --ignore-rev $REV_2 --ignore-rev $REV_3 file | sed -e \"$pick_author\" >actual &&\n+\tgit blame --ignore-rev $REV_2 --ignore-rev $REV_3 file >output &&\n+\tsed -e \"$pick_author\" <output >actual &&\n \n \ttest_cmp expected actual\n \t'\n@@ -387,7 +390,8 @@ test_expect_success 'position matching' '\n \n \ttest_write_lines 1 1 2 2 >expected &&\n \n-\tgit blame --ignore-rev $REV_3 --ignore-rev $REV_4 file2 | sed -e \"$pick_author\" >actual &&\n+\tgit blame --ignore-rev $REV_3 --ignore-rev $REV_4 file2 >output &&\n+\tsed -e \"$pick_author\" <output >actual &&\n \n \ttest_cmp expected actual\n \t'\n@@ -424,7 +428,8 @@ test_expect_success 'preserve order' '\n \n \ttest_write_lines 1 2 3 >expected &&\n \n-\tgit blame --ignore-rev $REV_4 --ignore-rev $REV_5 file3 | sed -e \"$pick_author\" >actual &&\n+\tgit blame --ignore-rev $REV_4 --ignore-rev $REV_5 file3 >output &&\n+\tsed -e \"$pick_author\" <output >actual &&\n \n \ttest_cmp expected actual\n \t'\n-- \n2.21.0\n\n"},{"id":"377332","messageId":"20190616190202.GA15262@archbookpro.localdomain","threadId":"51324","inReplyTo":"20190615184039.3711-1-michael@platin.gs","subject":"Re: [PATCH] Use an intermediate file between between git blame and sed to avoid git blame's exit code being hidden.","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-06-16T19:02:02Z","receivedAt":"2019-06-16T19:02:09Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Thanks for the patch, Michael!\n\nOn Sat, Jun 15, 2019 at 07:40:39PM +0100, michael@platin.gs wrote:\n> Subject: [PATCH] Use an intermediate file between between git blame and sed to avoid git blame's exit code being hidden.\n\nFor your commit message, the usual convention is to first specify the\narea you're working on followed by a colon and a brief summary.\nTypically, the subject starts with a lowercase character and also\ndoesn't end with any punctuation. See [[describe-changes]] under\nDocumentation/SubmittingPatches for more details.\n\nFor yours, I would reword your commit message to something like\n\n\tt8014: avoid git command in upstream pipe\n\t\n\tUse an intermediate file between between git blame and sed to avoid\n\tgit blame's exit code being hidden.\n\nIn addition, your commit message is missing a sign-off line. You can add\none by passing `-s` to git commit but you should read about what it\nmeans in [[sign-off]] in SubmittingPatches.\n\n> From: Michael Platings <michael@platin.gs>\n> \n> ---\n>  t/t8014-blame-ignore-fuzzy.sh | 13 +++++++++----\n>  1 file changed, 9 insertions(+), 4 deletions(-)\n> \n> diff --git a/t/t8014-blame-ignore-fuzzy.sh b/t/t8014-blame-ignore-fuzzy.sh\n> index 1ff59624e9..13f3313710 100755\n> --- a/t/t8014-blame-ignore-fuzzy.sh\n> +++ b/t/t8014-blame-ignore-fuzzy.sh\n> @@ -332,7 +332,9 @@ test_expect_success setup '\n>  for i in $(test_seq 2 $last_test); do\n>  \teval title=\"\\$title$i\"\n>  \ttest_expect_success \"$title\" \\\n> -\t\"git blame -M9 --ignore-rev $IGNOREME $i | sed -e \\\"$pick_author\\\" >actual && test_cmp expected$i actual\"\n> +\t\"git blame -M9 --ignore-rev $IGNOREME $i >output &&\n> +\tsed -e \\\"$pick_author\\\" <output >actual &&\n\nWe should take advantage of the fact that sed can open its own input\nhere. So we should drop the `<` and just pass the filename to sed. Same\napplies to the below.\n\nThanks,\n\nDenton\n\n> +\ttest_cmp expected$i actual\"\n>  done\n>  \n>  # This invoked a null pointer dereference when the chunk callback was called\n> @@ -357,7 +359,8 @@ test_expect_success 'Diff chunks with no suspects' '\n>  \n>  \ttest_write_lines 1 1 >expected &&\n>  \n> -\tgit blame --ignore-rev $REV_2 --ignore-rev $REV_3 file | sed -e \"$pick_author\" >actual &&\n> +\tgit blame --ignore-rev $REV_2 --ignore-rev $REV_3 file >output &&\n> +\tsed -e \"$pick_author\" <output >actual &&\n>  \n>  \ttest_cmp expected actual\n>  \t'\n> @@ -387,7 +390,8 @@ test_expect_success 'position matching' '\n>  \n>  \ttest_write_lines 1 1 2 2 >expected &&\n>  \n> -\tgit blame --ignore-rev $REV_3 --ignore-rev $REV_4 file2 | sed -e \"$pick_author\" >actual &&\n> +\tgit blame --ignore-rev $REV_3 --ignore-rev $REV_4 file2 >output &&\n> +\tsed -e \"$pick_author\" <output >actual &&\n>  \n>  \ttest_cmp expected actual\n>  \t'\n> @@ -424,7 +428,8 @@ test_expect_success 'preserve order' '\n>  \n>  \ttest_write_lines 1 2 3 >expected &&\n>  \n> -\tgit blame --ignore-rev $REV_4 --ignore-rev $REV_5 file3 | sed -e \"$pick_author\" >actual &&\n> +\tgit blame --ignore-rev $REV_4 --ignore-rev $REV_5 file3 >output &&\n> +\tsed -e \"$pick_author\" <output >actual &&\n>  \n>  \ttest_cmp expected actual\n>  \t'\n> -- \n> 2.21.0\n> \n"},{"id":"377334","messageId":"CAJDYR9SEoUMfB8iZ=CpaPz=YYRALvN-24Ws8LgOh4FAhLUq2zA@mail.gmail.com","threadId":"51324","inReplyTo":"20190616190202.GA15262@archbookpro.localdomain","subject":"Re: [PATCH] Use an intermediate file between between git blame and sed to avoid git blame's exit code being hidden.","fromName":"Michael Platings","fromEmail":"michael@platin.gs","sentAt":"2019-06-16T20:35:58Z","receivedAt":"2019-06-16T20:36:11Z","isPatch":true,"sender":{"key":"michael@platin.gs","avatar":"https://avatars.githubusercontent.com/u/1112348?v=4"},"body":"Hi Denton,\nThanks for the review. The patch was supposed to be in response to\nhttps://public-inbox.org/git/20190613151756.GA31952@szeder.dev/ but\napparently I didn't use --in-reply-to correctly. I'll resubmit with\nthe requested changes.\n-Michael\n\nOn Sun, 16 Jun 2019 at 20:02, Denton Liu <liu.denton@gmail.com> wrote:\n>\n> Thanks for the patch, Michael!\n>\n> On Sat, Jun 15, 2019 at 07:40:39PM +0100, michael@platin.gs wrote:\n> > Subject: [PATCH] Use an intermediate file between between git blame and sed to avoid git blame's exit code being hidden.\n>\n> For your commit message, the usual convention is to first specify the\n> area you're working on followed by a colon and a brief summary.\n> Typically, the subject starts with a lowercase character and also\n> doesn't end with any punctuation. See [[describe-changes]] under\n> Documentation/SubmittingPatches for more details.\n>\n> For yours, I would reword your commit message to something like\n>\n>         t8014: avoid git command in upstream pipe\n>\n>         Use an intermediate file between between git blame and sed to avoid\n>         git blame's exit code being hidden.\n>\n> In addition, your commit message is missing a sign-off line. You can add\n> one by passing `-s` to git commit but you should read about what it\n> means in [[sign-off]] in SubmittingPatches.\n>\n> > From: Michael Platings <michael@platin.gs>\n> >\n> > ---\n> >  t/t8014-blame-ignore-fuzzy.sh | 13 +++++++++----\n> >  1 file changed, 9 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/t/t8014-blame-ignore-fuzzy.sh b/t/t8014-blame-ignore-fuzzy.sh\n> > index 1ff59624e9..13f3313710 100755\n> > --- a/t/t8014-blame-ignore-fuzzy.sh\n> > +++ b/t/t8014-blame-ignore-fuzzy.sh\n> > @@ -332,7 +332,9 @@ test_expect_success setup '\n> >  for i in $(test_seq 2 $last_test); do\n> >       eval title=\"\\$title$i\"\n> >       test_expect_success \"$title\" \\\n> > -     \"git blame -M9 --ignore-rev $IGNOREME $i | sed -e \\\"$pick_author\\\" >actual && test_cmp expected$i actual\"\n> > +     \"git blame -M9 --ignore-rev $IGNOREME $i >output &&\n> > +     sed -e \\\"$pick_author\\\" <output >actual &&\n>\n> We should take advantage of the fact that sed can open its own input\n> here. So we should drop the `<` and just pass the filename to sed. Same\n> applies to the below.\n>\n> Thanks,\n>\n> Denton\n>\n> > +     test_cmp expected$i actual\"\n> >  done\n> >\n> >  # This invoked a null pointer dereference when the chunk callback was called\n> > @@ -357,7 +359,8 @@ test_expect_success 'Diff chunks with no suspects' '\n> >\n> >       test_write_lines 1 1 >expected &&\n> >\n> > -     git blame --ignore-rev $REV_2 --ignore-rev $REV_3 file | sed -e \"$pick_author\" >actual &&\n> > +     git blame --ignore-rev $REV_2 --ignore-rev $REV_3 file >output &&\n> > +     sed -e \"$pick_author\" <output >actual &&\n> >\n> >       test_cmp expected actual\n> >       '\n> > @@ -387,7 +390,8 @@ test_expect_success 'position matching' '\n> >\n> >       test_write_lines 1 1 2 2 >expected &&\n> >\n> > -     git blame --ignore-rev $REV_3 --ignore-rev $REV_4 file2 | sed -e \"$pick_author\" >actual &&\n> > +     git blame --ignore-rev $REV_3 --ignore-rev $REV_4 file2 >output &&\n> > +     sed -e \"$pick_author\" <output >actual &&\n> >\n> >       test_cmp expected actual\n> >       '\n> > @@ -424,7 +428,8 @@ test_expect_success 'preserve order' '\n> >\n> >       test_write_lines 1 2 3 >expected &&\n> >\n> > -     git blame --ignore-rev $REV_4 --ignore-rev $REV_5 file3 | sed -e \"$pick_author\" >actual &&\n> > +     git blame --ignore-rev $REV_4 --ignore-rev $REV_5 file3 >output &&\n> > +     sed -e \"$pick_author\" <output >actual &&\n> >\n> >       test_cmp expected actual\n> >       '\n> > --\n> > 2.21.0\n> >\n"},{"id":"377338","messageId":"xmqqy321rw4t.fsf@gitster-ct.c.googlers.com","threadId":"51324","inReplyTo":"20190616190202.GA15262@archbookpro.localdomain","subject":"Re: [PATCH] Use an intermediate file between between git blame and sed to avoid git blame's exit code being hidden.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-06-16T22:41:22Z","receivedAt":"2019-06-16T22:44:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu <liu.denton@gmail.com> writes:\n\n> For yours, I would reword your commit message to something like\n>\n> \tt8014: avoid git command in upstream pipe\n> \t\n> \tUse an intermediate file between between git blame and sed to avoid\n> \tgit blame's exit code being hidden.\n\nI agree that the main \"points\" of this patch that should be\nhighlighted on the title line are that it is about a test, and it is\nabout not hiding a failure of a Git command by placing it on the\nupstream side of a pipe---the above title is very nicely written.\n\n>> +\t\"git blame -M9 --ignore-rev $IGNOREME $i >output &&\n>> +\tsed -e \\\"$pick_author\\\" <output >actual &&\n>\n> We should take advantage of the fact that sed can open its own input\n> here. So we should drop the `<` and just pass the filename to sed. Same\n> applies to the below.\n\nWhile I do not think it matters too much in this case, I agree it is\na good habit to get into, because it would give the command a chance\nto produce a better error diagnosis (i.e. \"malformed input on line\nX\" vs \"malformed input on line X in file F\"), when it wants to\nreport an error in input, if we give the name of the file to open to\nthe command instead of an already-open file descriptor, for one thing.\n\n\n"}]}