From: Junio C Hamano <gitster@pobox.com>
Date: Sun, 31 May 2015 18:13:23 GMT
Subject: Re: [PATCH v2] blame: add blame.showEmail configuration
Message-ID: <xmqqa8wkaa4c.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1433021926-17762-1-git-send-email-qneill@quicinc.com>

Quentin Neill <quentin.neill@gmail.com> writes:

> From: Quentin Neill <quentin.neill@gmail.com>
>
> Complement existing --show-email option with fallback
> configuration variable, with tests.
> ---

The patch itself looks very reasonable.  Thanks for getting back to
us ;-)

A few minor nits:

    - Your in-body "From:" is redundant and unnecessary, as your
      e-mail is coming from the same address.

    - You need "Signed-off-by: Quentin Neill <quentin.neill@gmail.com>"
      after your log message (separate it with a blank line before
      the sign-off, and place the sign-off before the three-dash
      lines).

> diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh
> index 5cdf3f1..faf1660 100755
> --- a/t/t8002-blame.sh
> +++ b/t/t8002-blame.sh
> @@ -19,4 +19,66 @@ test_expect_success 'blame --show-email' '
>  		"<E at test dot git>" 1
>  '
>  
> +test_expect_success 'setup showEmail tests' '
> +	echo "bin: test number 1" >one &&
> +	git add . &&
> +	GIT_AUTHOR_NAME=name1 GIT_AUTHOR_EMAIL=email1@test.git git commit -a -m First --date="2010-01-01 01:00:00"
> +'
> +
> +cat >expected_n <<\EOF &&
> +(name1 2010-01-01 01:00:00 +0000 1) bin: test number 1
> +EOF
> +
> +cat >expected_e <<\EOF &&
> +(<email1@test.git> 2010-01-01 01:00:00 +0000 1) bin: test number 1
> +EOF

These two commands outside test_expect_success are part of setup, so

	test_expect_success 'setup showEmail tests' '
        	echo "bin: test number 1" >one &&
                git add one &&
                GIT_AUTHOR_NAME=name1 \
                GIT_AUTHOR_EMAIL=email1@test.git \
                git commit -m First --date="2010-01-01 01:00:00" &&
		cat >expected_n <<-\EOF &&
                (name1 ...
                EOF
                cat >expected_e <<-\EOF
                (<email1@...
		EOF
	'

Also do not hesitate to break overlong lines with "\".

> +find_blame() {

style: "find_blame () {"

Other than that, the patch looks good.

Thanks.