[PATCH] blame: add blame.showemail config option

From: Quentin Neill <redacted>

	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

Thanks for the submission. See comments below...

On Thu, Apr 23, 2015 at 10:13 PM, Quentin Neill [off-list ref] wrote:

From: Quentin Neill <redacted>

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.

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

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.

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.

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

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 [off-list ref] wrote:

Thanks for the submission. See comments below...

On Thu, Apr 23, 2015 at 10:13 PM, Quentin Neill [off-list ref] wrote:

quoted

From: Quentin Neill <redacted>

You should drop this line. git-am will pluck your name and email
automatically from the email From: header.

quoted

        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.

quoted

---

Missing Signed-off-by:. See Documentation/SubmittingPatches.

quoted

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.

quoted

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

quoted

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

quoted

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

quoted

                                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.

On Mon, Apr 27, 2015 at 9:46 AM, Quentin Neill [off-list ref] wrote:

On Fri, Apr 24, 2015 at 12:22 AM, Eric Sunshine [off-list ref] wrote:

quoted

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.

quoted

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.

quoted

quoted

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

quoted

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