[PATCH] blame: add blame.showemail config option

[PATCH] blame: add blame.showemail config option

[Quentin Neill]

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

	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.
---
 Documentation/blame-options.txt |  5 +++++
 Documentation/git-blame.txt     |  4 ----
 builtin/blame.c                 | 11 ++++++++---
 3 files changed, 13 insertions(+), 7 deletions(-)

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::
+	Show the author email instead of author name (Default: off).
+	This can also be controlled via the `blame.showemail` config
+	option.
 
 --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;
@@ -1949,7 +1950,7 @@ static void emit_other(struct scoreboard *sb, struct blame_entry *ent, int opt)
 			if (!(opt & OUTPUT_NO_AUTHOR)) {
 				const char *name;
 				int pad;
-				if (opt & OUTPUT_SHOW_EMAIL)
+				if ((opt & OUTPUT_SHOW_EMAIL) || show_email)
 					name = ci.author_mail.buf;
 				else
 					name = ci.author.buf;
@@ -2098,7 +2099,7 @@ static void find_alignment(struct scoreboard *sb, int *option)
 			struct commit_info ci;
 			suspect->commit->object.flags |= METAINFO_SHOWN;
 			get_commit_info(suspect->commit, &ci, 1);
-			if (*option & OUTPUT_SHOW_EMAIL)
+			if ((*option & OUTPUT_SHOW_EMAIL) || show_email)
 				num = utf8_strwidth(ci.author_mail.buf);
 			else
 				num = utf8_strwidth(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);
-- 
1.9.1

Re: [PATCH] blame: add blame.showemail config option

[Eric Sunshine]

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.

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.

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

Re: [PATCH] blame: add blame.showemail config option

[Quentin Neill]

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.
As for the comments on behavior, see my responses below.

--
Quentin

"There! His Majesty can now read my name without glasses. And he can
double the reward on my head!" - John Hancock, upon signing the
Declaration of Independence



On Fri, Apr 24, 2015 at 12:22 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> 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.

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.

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

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'?

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

Agreed, but again I don't see tests for the other switches with options.

Re: [PATCH] blame: add blame.showemail config option

[Junio C Hamano]

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

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

Please do not do anything before you understand why there are two
places, and if you don't understand, ask.

If you do this:

    $ git grep blame-options Documentation/

you would discover that blame-options.txt is meant to hold things
that are shared across "git annotate" and "git blame".  What is
understood only by "git blame" but not by "git annotate" is to be
described in git-blame.txt, I think.  So the criteria is not "does
it have configuration?"; it is "does git-annotate understand it?"

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

I do not think that is quite true.

The code strucure for other existing options is perfectly fine.
What they do is:

    - show_root is initialized to the fallback default value of
      false by being in BSS;

    - configuration is read to tweak that default;

    - command line parser may override the default with --show-root
      or --no-show-root.

And then show_root is used throughout the system.

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.

Re: [PATCH] blame: add blame.showemail config option

[Eric Sunshine]

On Mon, Apr 27, 2015 at 2:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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(...);
        ...
    }

Re: [PATCH] blame: add blame.showemail config option

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> 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;
>             }

Don't forget to clear the bit when the bool is set to false, too.

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

Re: [PATCH] blame: add blame.showemail config option

[Quentin Neill]

On Mon, Apr 27, 2015 at 1:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> 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.

Points well taken.  I'll admit I posted the patch as a way to find out these
sorts of things, but I see I could have done a bit more digging and asking
before posting.  Apologies for the noise.

On Tue, Apr 28, 2015 at 2:08 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>> .. instead set the OUTPUT_SHOW_EMAIL bit in git_blame_config().
>
> Don't forget to clear the bit when the bool is set to false, too.

I think I have this now, and some tests that check it, but have a pair
of questions.

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?

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

Please guide.

Re: [PATCH] blame: add blame.showemail config option

[Junio C Hamano]

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

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

Correct.  Write "section.variableName" in documentation and
invocations of "git config" command in scripts.  Compare key
with "section.variablename" in git_config() callback.

> 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'd suggest

 [PATCH 1/2] blame: add blame.showEmail configuration

which would be the polished version of the patch we have been
discussing, plus tests for this particular feature, and

 [PATCH 2/2] blame: more tests

which would widen test coverage to other configuration variables and
features.

Re: [PATCH] blame: add blame.showemail config option

[Eric Sunshine]

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.

Re: [PATCH] blame: add blame.showemail config option

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

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

Good thinking. I would vote for the former, unless the number of
additional tests exceed 20 (as t8002 has about 100 tests).

Re: [PATCH] blame: add blame.showemail config option

[Eric Sunshine]

On Mon, Apr 27, 2015 at 9:46 AM, Quentin Neill <quentin.neill@gmail.com> wrote:
> 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.

Junio's response[1] addresses both points nicely. To be clear, I
wasn't suggesting that you should do the relocation, but instead that
the relocation seemed unrelated to the overall intent of the patch and
that its purpose wasn't clear. So, as a general statement, when the
motive for a change is unclear, it deserves explanation in the commit
message; and when a change is not directly related to the patch
itself, it often deserves to be placed in its own patch. In this case,
neither applies since the relocation is unwarranted.

>>> -                       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'?

As per Junio's response[1], logic for the other configuration options
seems to be fine, so I'm not quite sure what changes you propose.

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

Unfortunately, test coverage is sometimes sparse, however, patches
with accompanying tests are looked upon with favor and instill greater
confidence, so they are encouraged. If you need assistance with the
tests, feel free to ask.

It's not your responsibility to fill the gaps of missing tests for
other options which you're not touching, but you're welcome to add
tests for them if you want to.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/267720/focus=267862

Re: [PATCH] blame: add blame.showemail config option

[Junio C Hamano]

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?

Re: [PATCH] blame: add blame.showemail config option

[Quentin Neill]

On Fri, May 29, 2015 at 2:40 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> 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?
>
>

[One more time without html]

Hi Junio,

I got too busy, my free time went to zero.  And this week I am
traveling. I was tinkering with tests Thursday night and was in fact
preparing a reply. When I get back I'll post the patch.

[PATCH v2] blame: add blame.showEmail configuration

[Quentin Neill]

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

Complement existing --show-email option with fallback
configuration variable, with tests.
---
 Documentation/git-blame.txt |  2 ++
 builtin/blame.c             | 10 +++++++-
 t/t8002-blame.sh            | 62 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 73 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt
index 9f23a86..e6e947c 100644
--- a/Documentation/git-blame.txt
+++ b/Documentation/git-blame.txt
@@ -76,6 +76,8 @@ include::blame-options.txt[]
 -e::
 --show-email::
 	Show the author email instead of author name (Default: off).
+	This can also be controlled via the `blame.showEmail` config
+	option.
 
 -w::
 	Ignore whitespace when comparing the parent's version and
diff --git a/builtin/blame.c b/builtin/blame.c
index 8d70623..60039d3 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2185,6 +2185,14 @@ 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")) {
+		int *output_option = cb;
+		if (git_config_bool(var, value))
+			*output_option |= OUTPUT_SHOW_EMAIL;
+		else
+			*output_option &= ~OUTPUT_SHOW_EMAIL;
+		return 0;
+	}
 	if (!strcmp(var, "blame.date")) {
 		if (!value)
 			return config_error_nonbool(var);
@@ -2529,7 +2537,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	unsigned int range_i;
 	long anchor;
 
-	git_config(git_blame_config, NULL);
+	git_config(git_blame_config, &output_option);
 	init_revisions(&revs, NULL);
 	revs.date_mode = blame_date_mode;
 	DIFF_OPT_SET(&revs.diffopt, ALLOW_TEXTCONV);
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
+
+find_blame() {
+	sed -e 's/^[^(]*//'
+}
+
+test_expect_success 'blame with no options and no config' '
+	git blame one >blame &&
+	find_blame <blame >result &&
+	test_cmp expected_n result
+'
+
+test_expect_success 'blame with showemail options' '
+	git blame --show-email one >blame1 &&
+	find_blame <blame1 >result &&
+	test_cmp expected_e result &&
+	git blame -e one >blame2 &&
+	find_blame <blame2 >result &&
+	test_cmp expected_e result &&
+	git blame --no-show-email one >blame3 &&
+	find_blame <blame3 >result &&
+	test_cmp expected_n result
+'
+
+test_expect_success 'blame with showEmail config false' '
+	git config blame.showEmail false &&
+	git blame one >blame1 &&
+	find_blame <blame1 >result &&
+	test_cmp expected_n result &&
+	git blame --show-email one >blame2 &&
+	find_blame <blame2 >result &&
+	test_cmp expected_e result &&
+	git blame -e one >blame3 &&
+	find_blame <blame3 >result &&
+	test_cmp expected_e result &&
+	git blame --no-show-email one >blame4 &&
+	find_blame <blame4 >result &&
+	test_cmp expected_n result
+'
+
+test_expect_success 'blame with showEmail config true' '
+	git config blame.showEmail true &&
+	git blame one >blame1 &&
+	find_blame <blame1 >result &&
+	test_cmp expected_e result &&
+	git blame --no-show-email one >blame2 &&
+	find_blame <blame2 >result &&
+	test_cmp expected_n result
+'
+
 test_done
-- 
2.4.2.340.g5ecd853

Re: [PATCH v2] blame: add blame.showEmail configuration

[Junio C Hamano]

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.

Re: [PATCH v2] blame: add blame.showEmail configuration

[Quentin Neill]

On Sun, May 31, 2015 at 1:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

I tried using 'git send-email' directly on the commit, not sure how or
why that occurred.
I will fall back to 'git format-patch' and 'git send-email' as I did
in my first post.

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

Thanks for the help, one more version on the way.

[PATCH v3] blame: add blame.showEmail configuration

[Quentin Neill]

Complement existing --show-email option with fallback
configuration variable, with tests.

Signed-off-by: Quentin Neill <quentin.neill@gmail.com>
---
 Documentation/git-blame.txt |  2 ++
 builtin/blame.c             | 10 +++++++-
 t/t8002-blame.sh            | 62 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 73 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt
index 9f23a86..e6e947c 100644
--- a/Documentation/git-blame.txt
+++ b/Documentation/git-blame.txt
@@ -76,6 +76,8 @@ include::blame-options.txt[]
 -e::
 --show-email::
 	Show the author email instead of author name (Default: off).
+	This can also be controlled via the `blame.showEmail` config
+	option.
 
 -w::
 	Ignore whitespace when comparing the parent's version and
diff --git a/builtin/blame.c b/builtin/blame.c
index 8d70623..60039d3 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2185,6 +2185,14 @@ 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")) {
+		int *output_option = cb;
+		if (git_config_bool(var, value))
+			*output_option |= OUTPUT_SHOW_EMAIL;
+		else
+			*output_option &= ~OUTPUT_SHOW_EMAIL;
+		return 0;
+	}
 	if (!strcmp(var, "blame.date")) {
 		if (!value)
 			return config_error_nonbool(var);
@@ -2529,7 +2537,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	unsigned int range_i;
 	long anchor;
 
-	git_config(git_blame_config, NULL);
+	git_config(git_blame_config, &output_option);
 	init_revisions(&revs, NULL);
 	revs.date_mode = blame_date_mode;
 	DIFF_OPT_SET(&revs.diffopt, ALLOW_TEXTCONV);
diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh
index 5cdf3f1..ff09ace 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 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 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
+'
+
+find_blame () {
+	sed -e 's/^[^(]*//'
+}
+
+test_expect_success 'blame with no options and no config' '
+	git blame one >blame &&
+	find_blame <blame >result &&
+	test_cmp expected_n result
+'
+
+test_expect_success 'blame with showemail options' '
+	git blame --show-email one >blame1 &&
+	find_blame <blame1 >result &&
+	test_cmp expected_e result &&
+	git blame -e one >blame2 &&
+	find_blame <blame2 >result &&
+	test_cmp expected_e result &&
+	git blame --no-show-email one >blame3 &&
+	find_blame <blame3 >result &&
+	test_cmp expected_n result
+'
+
+test_expect_success 'blame with showEmail config false' '
+	git config blame.showEmail false &&
+	git blame one >blame1 &&
+	find_blame <blame1 >result &&
+	test_cmp expected_n result &&
+	git blame --show-email one >blame2 &&
+	find_blame <blame2 >result &&
+	test_cmp expected_e result &&
+	git blame -e one >blame3 &&
+	find_blame <blame3 >result &&
+	test_cmp expected_e result &&
+	git blame --no-show-email one >blame4 &&
+	find_blame <blame4 >result &&
+	test_cmp expected_n result
+'
+
+test_expect_success 'blame with showEmail config true' '
+	git config blame.showEmail true &&
+	git blame one >blame1 &&
+	find_blame <blame1 >result &&
+	test_cmp expected_e result &&
+	git blame --no-show-email one >blame2 &&
+	find_blame <blame2 >result &&
+	test_cmp expected_n result
+'
+
 test_done
-- 
2.4.2.340.g5ecd853