patchblame: add blame.showemail config option

> diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
> index b299b59..9326115 100644
> --- a/Documentation/blame-options.txt
> +++ b/Documentation/blame-options.txt
> @@ -1,6 +1,11 @@
>  -b::
>         Show blank SHA-1 for boundary commits.  This can also
>         be controlled via the `blame.blankboundary` config option.
> +-e::
> +--show-email::

>  --root::
>         Do not treat root commits as boundaries.  This can also be
> diff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt
> index 9f23a86..50a9030 100644
> --- a/Documentation/git-blame.txt
> +++ b/Documentation/git-blame.txt
> @@ -73,10 +73,6 @@ include::blame-options.txt[]
>  -s::
>         Suppress the author name and timestamp from the output.
>
> --e::
> ---show-email::
> -       Show the author email instead of author name (Default: off).
> -

>  -w::
>         Ignore whitespace when comparing the parent's version and
>         the child's to find where the lines came from.
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 06484c2..70bfd0a 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -44,6 +44,7 @@ static int max_score_digits;
>  static int show_root;
>  static int reverse;
>  static int blank_boundary;
> +static int show_email;
>  static int incremental;
>  static int xdl_opts;
>  static int abbrev = -1;
> @@ -1926,7 +1927,7 @@ static void emit_other(struct scoreboard *sb, struct blame_entry *ent, int opt)
>                 printf("%.*s", length, hex);
>                 if (opt & OUTPUT_ANNOTATE_COMPAT) {
>                         const char *name;
> -                       if (opt & OUTPUT_SHOW_EMAIL)
> +                       if ((opt & OUTPUT_SHOW_EMAIL) || show_email)

>                                 name = ci.author_mail.buf;
>                         else
>                                 name = ci.author.buf;
> @@ -2185,6 +2186,10 @@ static int git_blame_config(const char *var, const char *value, void *cb)
>                 blank_boundary = git_config_bool(var, value);
>                 return 0;
>         }
> +       if (!strcmp(var, "blame.showemail")) {
> +               show_email = git_config_bool(var, value);
> +               return 0;
> +       }
>         if (!strcmp(var, "blame.date")) {
>                 if (!value)
>                         return config_error_nonbool(var);

> Thanks for the submission. See comments below...
>
> On Thu, Apr 23, 2015 at 10:13 PM, Quentin Neill <quentin.neill@gmail.com> wrote:
>> From: Quentin Neill <quentin.neill@gmail.com>
>
> You should drop this line. git-am will pluck your name and email
> automatically from the email From: header.
>
>>         If you prefer seeing emails in your git blame output, rather
>>         than sprinkling '-e' options everywhere you can just set
>>         the new config blame.showemail to true.
>
> Drop the indentation from the commit message.
>
> It's not clear what "everywhere" means in the above. It might be
> sufficient to rephrase more simply as:
>
>     Complement existing --show-email option with fallback
>     configuration variable.
>
> or something.
>
>> ---
>
> Missing Signed-off-by:. See Documentation/SubmittingPatches.
>
>> diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
>> index b299b59..9326115 100644
>> --- a/Documentation/blame-options.txt
>> +++ b/Documentation/blame-options.txt
>> @@ -1,6 +1,11 @@
>>  -b::
>>         Show blank SHA-1 for boundary commits.  This can also
>>         be controlled via the `blame.blankboundary` config option.
>> +-e::
>> +--show-email::
>
> Insert a blank line before the "-e" line to separate it from the "-b" paragraph.
>
>> +       Show the author email instead of author name (Default: off).
>> +       This can also be controlled via the `blame.showemail` config
>> +       option.
>
> Despite being case-insensitive and despite existing inconsistencies,
> in documentation, it is customary to use camelCase for configuration
> options, so "blame.showEmail".
>
> Also blame.showEmail probably ought to be documented in
> Documentation/config.txt (although there is some inconsistency here in
> that documentation for the other blame-specific variables is missing
> from config.txt, so perhaps that's something that could be addressed
> separately).
>
>>  --root::
>>         Do not treat root commits as boundaries.  This can also be
>> diff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt
>> index 9f23a86..50a9030 100644
>> --- a/Documentation/git-blame.txt
>> +++ b/Documentation/git-blame.txt
>> @@ -73,10 +73,6 @@ include::blame-options.txt[]
>>  -s::
>>         Suppress the author name and timestamp from the output.
>>
>> --e::
>> ---show-email::
>> -       Show the author email instead of author name (Default: off).
>> -
>
> It's not clear why you relocated documentation of --show-email from
> git-blame.txt to blame-options.txt, and the commit message does not
> explain the move. If there's a good reason for the relocation, the
> justification should be spelled out so that people reviewing the patch
> or looking through history later on will not have to guess about it.

>>  -w::
>>         Ignore whitespace when comparing the parent's version and
>>         the child's to find where the lines came from.
>> diff --git a/builtin/blame.c b/builtin/blame.c
>> index 06484c2..70bfd0a 100644
>> --- a/builtin/blame.c
>> +++ b/builtin/blame.c
>> @@ -44,6 +44,7 @@ static int max_score_digits;
>>  static int show_root;
>>  static int reverse;
>>  static int blank_boundary;
>> +static int show_email;
>>  static int incremental;
>>  static int xdl_opts;
>>  static int abbrev = -1;
>> @@ -1926,7 +1927,7 @@ static void emit_other(struct scoreboard *sb, struct blame_entry *ent, int opt)
>>                 printf("%.*s", length, hex);
>>                 if (opt & OUTPUT_ANNOTATE_COMPAT) {
>>                         const char *name;
>> -                       if (opt & OUTPUT_SHOW_EMAIL)
>> +                       if ((opt & OUTPUT_SHOW_EMAIL) || show_email)
>
> The desired behavior is for a configuration setting to provide a
> fallback, and for a command-line option to override the fallback. So,
> for instance, if blame.showemail is "true", then --no-show-email
> should countermand that. Unfortunately, the way this patch is
> implemented, it's impossible to override the setting from the
> command-line since show_email==true will always win (when
> blame.showemail is "true").
>
> More below.

>>                                 name = ci.author_mail.buf;
>>                         else
>>                                 name = ci.author.buf;
>> @@ -2185,6 +2186,10 @@ static int git_blame_config(const char *var, const char *value, void *cb)
>>                 blank_boundary = git_config_bool(var, value);
>>                 return 0;
>>         }
>> +       if (!strcmp(var, "blame.showemail")) {
>> +               show_email = git_config_bool(var, value);
>> +               return 0;
>> +       }
>>         if (!strcmp(var, "blame.date")) {
>>                 if (!value)
>>                         return config_error_nonbool(var);
>
> You'll also want to add tests for the new blame.showemail
> configuration. There's already one test in t8002-blame.sh which checks
> that --show-email works, but you will want tests to ensure that you
> get the expected results for all combinations of blame.showemail and
> --show-email (including when --show-email is not specified).

>> It's not clear why you relocated documentation of --show-email from
>> git-blame.txt to blame-options.txt, and the commit message does not
>> explain the move. If there's a good reason for the relocation, the
>> justification should be spelled out so that people reviewing the patch
>> or looking through history later on will not have to guess about it.
>
> I moved it to be with the other variables that had configuration
> options, but I will move it back.

>>> @@ -1926,7 +1927,7 @@ static void emit_other(struct scoreboard *sb, struct blame_entry *ent, int opt)
>>>                 printf("%.*s", length, hex);
>>>                 if (opt & OUTPUT_ANNOTATE_COMPAT) {
>>>                         const char *name;
>>> -                       if (opt & OUTPUT_SHOW_EMAIL)
>>> +                       if ((opt & OUTPUT_SHOW_EMAIL) || show_email)
>>
>> The desired behavior is for a configuration setting to provide a
>> fallback, and for a command-line option to override the fallback. So,
>> for instance, if blame.showemail is "true", then --no-show-email
>> should countermand that. Unfortunately, the way this patch is
>> implemented, it's impossible to override the setting from the
>> command-line since show_email==true will always win (when
>> blame.showemail is "true").
>>
>> More below.
>
> I followed the code for blame.showRoot and blame.blankboundary.

> Quentin Neill <quentin.neill@gmail.com> writes:
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>> Quentin Neill <quentin.neill@gmail.com> writes:
>>>> -                       if (opt & OUTPUT_SHOW_EMAIL)
>>>> +                       if ((opt & OUTPUT_SHOW_EMAIL) || show_email)
>>>
>>> The desired behavior is for a configuration setting to provide a
>>> fallback, and for a command-line option to override the fallback. So,
>>> for instance, if blame.showemail is "true", then --no-show-email
>>> should countermand that. Unfortunately, the way this patch is
>>> implemented, it's impossible to override the setting from the
>>> command-line since show_email==true will always win (when
>>> blame.showemail is "true").
>>>
>> I followed the code for blame.showRoot and blame.blankboundary.
>
> If you truly followed this code pattern, I would expect that there
> will not be a separate show_email variable introduced to support
> this new configuration variable.  The OUTPUT_SHOW_EMAIL bit in the
> opt flag word corresponds to show_root and blank_boundary variables,
> so the code to read configuration variable would set that bit in the
> opt flag word before the command line parser would kick in.  And the
> code that checks "opt & OUTPUT_SHOW_EMAIL" currently does not have
> to change at all.

> Right. Rather than having a separate global 'show_email' variable and
> consulting that variable in parallel with OUTPUT_SHOW_EMAIL throughout
> the code, instead set the OUTPUT_SHOW_EMAIL bit in git_blame_config().
> To do this, take advantage of the "callback data" argument of
> git_config(), which will arrive in git_blame_config() as its 'void
> *cb' argument. So, for instance, something like this:
>
>     static int git_blame_config(var, value, void *cb)
>     {
>         ...
>         if (!strcmp(var, "blame.showemail")) {
>             if (git_config_bool(var, value)) {
>                 int *output_options = cb;
>                *output_options |= OUTPUT_SHOW_EMAIL;
>             }

>             return 0;
>         }
>         ...
>     }
>
>     int cmd_blame(...)
>     {
>         ...
>         git_config(git_blame_config, &output_options);
>         ...
>         parse_options(...);
>         ...
>     }

>>> It's not clear why you relocated documentation of --show-email from
>>>
>> I moved it to be with the other variables that had configuration
>>
>> options, but I will move it back.
>
> Please do not do anything before you understand why there are two
> places, and if you don't understand, ask.
>
>> I followed the code for blame.showRoot and blame.blankboundary.
>
> I do not think that is quite true.

> On Fri, Apr 24, 2015 at 12:22 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> Despite being case-insensitive and despite existing inconsistencies,
>> in documentation, it is customary to use camelCase for configuration
>> options, so "blame.showEmail".
>
> I noticed while testing that git_config()/git_value() lowercase
> everything, so to be clear this camelCase custom for configuration
> names is for documentation only, right?

> On Thu, Apr 30, 2015 at 10:03 AM, Quentin Neill <quentin.neill@gmail.com> wrote:
>> I'm thinking of a test file that will test all the git blame options,
>> but for this
>> patch it will only test the new showEmail config.  I read t/README and
>> tentatively named the new test file "t/8009-blame-config.sh".
>
> I'm not sure that these new proposed tests warrant a new test script.
> Moreover, the tests presumably will be testing combinations of
> configuration options and command-line switches, so having "config" in
> the script name doesn't seem quite correct.
>
> t8002-blame.sh already contains a test for --show-email, so it may be
> logical to add the new tests there, alongside the existing one. On the
> other hand, if you do add a new test script, then perhaps the existing
> --show-email test in t8002-blame.sh should be moved to the new script.

> On Fri, Apr 24, 2015 at 12:22 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> It's not clear why you relocated documentation of --show-email from
>> git-blame.txt to blame-options.txt, and the commit message does not
>> explain the move. If there's a good reason for the relocation, the
>> justification should be spelled out so that people reviewing the patch
>> or looking through history later on will not have to guess about it.
>
> I moved it to be with the other variables that had configuration
> options, but I will move it back.
>
>> It might also make sense to do the relocation as a separate
>> preparatory patch of a 2-patch series, in which the patch adding
>> blame.showemail is the second of the two.
>
> If you think it should be relocated, I will address in a separate patch.

>>> -                       if (opt & OUTPUT_SHOW_EMAIL)
>>> +                       if ((opt & OUTPUT_SHOW_EMAIL) || show_email)
>>
>> The desired behavior is for a configuration setting to provide a
>> fallback, and for a command-line option to override the fallback. So,
>> for instance, if blame.showemail is "true", then --no-show-email
>> should countermand that. Unfortunately, the way this patch is
>> implemented, it's impossible to override the setting from the
>> command-line since show_email==true will always win (when
>> blame.showemail is "true").
>>
>> More below.
>
> I followed the code for blame.showRoot and blame.blankboundary.
>
> I think the desired behavior for the other switches would go in a
> separate patch, the question is should it precede this one adding
> 'blame.showemail'?

>> You'll also want to add tests for the new blame.showemail
>> configuration. There's already one test in t8002-blame.sh which checks
>> that --show-email works, but you will want tests to ensure that you
>> get the expected results for all combinations of blame.showemail and
>> --show-email (including when --show-email is not specified).
>
> Agreed, but again I don't see tests for the other switches with options.

>
> Quentin Neill <quentin.neill@gmail.com> writes:
>
> > Thanks for the thorough review!
> > I have adjusted the commit messages and updated the documentation changes.
> > I'm in trying to add tests, I'll probably have some issues but will
> > post something that works soon.
>
> Hi, I was sweeping my old mailbox for leftover bits, and noticed
> that this thread ended without seeing the final step.
>
> Has anything further happened to this topic, did you got too busy,
> or you lost interest?
>
>

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

> 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

> 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.