git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Denton Liu <liu.denton@gmail.com>
To: michael@platin.gs
Cc: git@vger.kernel.org, "Jeff King" <peff@peff.net>,
	"Stefan Beller" <stefanbeller@gmail.com>,
	"Jeff Smith" <whydoubt@gmail.com>,
	"Junio C Hamano" <gitster@pobox.com>,
	"René Scharfe" <l.s.r@web.de>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"David Kastrup" <dak@gnu.org>,
	"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
	"Barret Rhoden" <brho@google.com>,
	"SZEDER Gábor" <szeder.dev@gmail.com>
Subject: Re: [PATCH] Use an intermediate file between between git blame and sed to avoid git blame's exit code being hidden.
Date: Sun, 16 Jun 2019 15:02:02 -0400	[thread overview]
Message-ID: <20190616190202.GA15262@archbookpro.localdomain> (raw)
In-Reply-To: <20190615184039.3711-1-michael@platin.gs>

Thanks for the patch, Michael!

On Sat, Jun 15, 2019 at 07:40:39PM +0100, michael@platin.gs wrote:
> Subject: [PATCH] Use an intermediate file between between git blame and sed to avoid git blame's exit code being hidden.

For your commit message, the usual convention is to first specify the
area you're working on followed by a colon and a brief summary.
Typically, the subject starts with a lowercase character and also
doesn't end with any punctuation. See [[describe-changes]] under
Documentation/SubmittingPatches for more details.

For yours, I would reword your commit message to something like

	t8014: avoid git command in upstream pipe
	
	Use an intermediate file between between git blame and sed to avoid
	git blame's exit code being hidden.

In addition, your commit message is missing a sign-off line. You can add
one by passing `-s` to git commit but you should read about what it
means in [[sign-off]] in SubmittingPatches.

> From: Michael Platings <michael@platin.gs>
> 
> ---
>  t/t8014-blame-ignore-fuzzy.sh | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/t/t8014-blame-ignore-fuzzy.sh b/t/t8014-blame-ignore-fuzzy.sh
> index 1ff59624e9..13f3313710 100755
> --- a/t/t8014-blame-ignore-fuzzy.sh
> +++ b/t/t8014-blame-ignore-fuzzy.sh
> @@ -332,7 +332,9 @@ test_expect_success setup '
>  for i in $(test_seq 2 $last_test); do
>  	eval title="\$title$i"
>  	test_expect_success "$title" \
> -	"git blame -M9 --ignore-rev $IGNOREME $i | sed -e \"$pick_author\" >actual && test_cmp expected$i actual"
> +	"git blame -M9 --ignore-rev $IGNOREME $i >output &&
> +	sed -e \"$pick_author\" <output >actual &&

We should take advantage of the fact that sed can open its own input
here. So we should drop the `<` and just pass the filename to sed. Same
applies to the below.

Thanks,

Denton

> +	test_cmp expected$i actual"
>  done
>  
>  # This invoked a null pointer dereference when the chunk callback was called
> @@ -357,7 +359,8 @@ test_expect_success 'Diff chunks with no suspects' '
>  
>  	test_write_lines 1 1 >expected &&
>  
> -	git blame --ignore-rev $REV_2 --ignore-rev $REV_3 file | sed -e "$pick_author" >actual &&
> +	git blame --ignore-rev $REV_2 --ignore-rev $REV_3 file >output &&
> +	sed -e "$pick_author" <output >actual &&
>  
>  	test_cmp expected actual
>  	'
> @@ -387,7 +390,8 @@ test_expect_success 'position matching' '
>  
>  	test_write_lines 1 1 2 2 >expected &&
>  
> -	git blame --ignore-rev $REV_3 --ignore-rev $REV_4 file2 | sed -e "$pick_author" >actual &&
> +	git blame --ignore-rev $REV_3 --ignore-rev $REV_4 file2 >output &&
> +	sed -e "$pick_author" <output >actual &&
>  
>  	test_cmp expected actual
>  	'
> @@ -424,7 +428,8 @@ test_expect_success 'preserve order' '
>  
>  	test_write_lines 1 2 3 >expected &&
>  
> -	git blame --ignore-rev $REV_4 --ignore-rev $REV_5 file3 | sed -e "$pick_author" >actual &&
> +	git blame --ignore-rev $REV_4 --ignore-rev $REV_5 file3 >output &&
> +	sed -e "$pick_author" <output >actual &&
>  
>  	test_cmp expected actual
>  	'
> -- 
> 2.21.0
> 

  reply	other threads:[~2019-06-16 19:02 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <Re: [PATCH v8 7/9] blame: add a fingerprint heuristic to match ignored lines>
2019-06-15 18:40 ` [PATCH] Use an intermediate file between between git blame and sed to avoid git blame's exit code being hidden michael
2019-06-16 19:02   ` Denton Liu [this message]
2019-06-16 20:35     ` Michael Platings
2019-06-16 22:41     ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190616190202.GA15262@archbookpro.localdomain \
    --to=liu.denton@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=avarab@gmail.com \
    --cc=brho@google.com \
    --cc=dak@gnu.org \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=l.s.r@web.de \
    --cc=michael@platin.gs \
    --cc=peff@peff.net \
    --cc=stefanbeller@gmail.com \
    --cc=szeder.dev@gmail.com \
    --cc=whydoubt@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).