Re: [PATCH] blame: add blame.showemail config option
From: Eric Sunshine <hidden>
Date: 2016-06-15 23:04:32
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.
quoted hunk ↗ jump to hunk
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).
quoted hunk ↗ jump to hunk
--root:: Do not treat root commits as boundaries. This can also bediff --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.
quoted hunk ↗ jump to hunk
-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.
quoted hunk ↗ jump to hunk
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).