{"thread":{"id":"39151","subject":"[PATCH] blame: add blame.showemail config option","startedAt":"2015-04-24T02:13:32Z","lastAt":"2015-05-31T19:27:37Z","messageCount":17,"participants":["Quentin Neill","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"259927","messageId":"1429841612-5131-1-git-send-email-qneill@quicinc.com","threadId":"39151","inReplyTo":null,"subject":"[PATCH] blame: add blame.showemail config option","fromName":"Quentin Neill","fromEmail":"quentin.neill@gmail.com","sentAt":"2015-04-24T02:13:32Z","receivedAt":"2015-04-24T02:13:32Z","isPatch":true,"sender":{"key":"quentin.neill@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3028797?v=4"},"body":"From: Quentin Neill \n\n\tIf you prefer seeing emails in your git blame output, rather\n\tthan sprinkling '-e' options everywhere you can just set\n\tthe new config blame.showemail to true.\n---\n Documentation/blame-options.txt | 5 +++++\n Documentation/git-blame.txt | 4 ----\n builtin/blame.c | 11 ++++++++---\n 3 files changed, 13 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt\nindex b299b59..9326115 100644\n--- a/Documentation/blame-options.txt\n+++ b/Documentation/blame-options.txt\n@@ -1,6 +1,11 @@\n -b::\n \tShow blank SHA-1 for boundary commits. This can also\n \tbe controlled via the `blame.blankboundary` config option.\n+-e::\n+--show-email::\n+\tShow the author email instead of author name (Default: off).\n+\tThis can also be controlled via the `blame.showemail` config\n+\toption.\n \n --root::\n \tDo not treat root commits as boundaries. This can also be\ndiff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt\nindex 9f23a86..50a9030 100644\n--- a/Documentation/git-blame.txt\n+++ b/Documentation/git-blame.txt\n@@ -73,10 +73,6 @@ include::blame-options.txt[]\n -s::\n \tSuppress the author name and timestamp from the output.\n \n--e::\n---show-email::\n-\tShow the author email instead of author name (Default: off).\n-\n -w::\n \tIgnore whitespace when comparing the parent's version and\n \tthe child's to find where the lines came from.\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 06484c2..70bfd0a 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -44,6 +44,7 @@ static int max_score_digits;\n static int show_root;\n static int reverse;\n static int blank_boundary;\n+static int show_email;\n static int incremental;\n static int xdl_opts;\n static int abbrev = -1;\n@@ -1926,7 +1927,7 @@ static void emit_other(struct scoreboard *sb, struct blame_entry *ent, int opt)\n \t\tprintf(\"%.*s\", length, hex);\n \t\tif (opt & OUTPUT_ANNOTATE_COMPAT) {\n \t\t\tconst char *name;\n-\t\t\tif (opt & OUTPUT_SHOW_EMAIL)\n+\t\t\tif ((opt & OUTPUT_SHOW_EMAIL) || show_email)\n \t\t\t\tname = ci.author_mail.buf;\n \t\t\telse\n \t\t\t\tname = ci.author.buf;\n@@ -1949,7 +1950,7 @@ static void emit_other(struct scoreboard *sb, struct blame_entry *ent, int opt)\n \t\t\tif (!(opt & OUTPUT_NO_AUTHOR)) {\n \t\t\t\tconst char *name;\n \t\t\t\tint pad;\n-\t\t\t\tif (opt & OUTPUT_SHOW_EMAIL)\n+\t\t\t\tif ((opt & OUTPUT_SHOW_EMAIL) || show_email)\n \t\t\t\t\tname = ci.author_mail.buf;\n \t\t\t\telse\n \t\t\t\t\tname = ci.author.buf;\n@@ -2098,7 +2099,7 @@ static void find_alignment(struct scoreboard *sb, int *option)\n \t\t\tstruct commit_info ci;\n \t\t\tsuspect->commit->object.flags |= METAINFO_SHOWN;\n \t\t\tget_commit_info(suspect->commit, &ci, 1);\n-\t\t\tif (*option & OUTPUT_SHOW_EMAIL)\n+\t\t\tif ((*option & OUTPUT_SHOW_EMAIL) || show_email)\n \t\t\t\tnum = utf8_strwidth(ci.author_mail.buf);\n \t\t\telse\n \t\t\t\tnum = utf8_strwidth(ci.author.buf);\n@@ -2185,6 +2186,10 @@ static int git_blame_config(const char *var, const char *value, void *cb)\n \t\tblank_boundary = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"blame.showemail\")) {\n+\t\tshow_email = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n \tif (!strcmp(var, \"blame.date\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n-- \n1.9.1\n"},{"id":"259929","messageId":"CAPig+cQrUPHOaKjNhsmLho+bFdAOQxb0NK2YK2QsFmdDBF6h4g@mail.gmail.com","threadId":"39151","inReplyTo":"1429841612-5131-1-git-send-email-qneill@quicinc.com","subject":"Re: [PATCH] blame: add blame.showemail config option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-04-24T05:22:33Z","receivedAt":"2015-04-24T05:22:33Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Thanks for the submission. See comments below...\n\nOn Thu, Apr 23, 2015 at 10:13 PM, Quentin Neill wrote:\n> From: Quentin Neill \n\nYou should drop this line. git-am will pluck your name and email\nautomatically from the email From: header.\n\n> If you prefer seeing emails in your git blame output, rather\n> than sprinkling '-e' options everywhere you can just set\n> the new config blame.showemail to true.\n\nDrop the indentation from the commit message.\n\nIt's not clear what \"everywhere\" means in the above. It might be\nsufficient to rephrase more simply as:\n\n Complement existing --show-email option with fallback\n configuration variable.\n\nor something.\n\n> ---\n\nMissing Signed-off-by:. See Documentation/SubmittingPatches.\n\n> diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt\n> index b299b59..9326115 100644\n> --- a/Documentation/blame-options.txt\n> +++ b/Documentation/blame-options.txt\n> @@ -1,6 +1,11 @@\n> -b::\n> Show blank SHA-1 for boundary commits. This can also\n> be controlled via the `blame.blankboundary` config option.\n> +-e::\n> +--show-email::\n\nInsert a blank line before the \"-e\" line to separate it from the \"-b\" paragraph.\n\n> + Show the author email instead of author name (Default: off).\n> + This can also be controlled via the `blame.showemail` config\n> + option.\n\nDespite being case-insensitive and despite existing inconsistencies,\nin documentation, it is customary to use camelCase for configuration\noptions, so \"blame.showEmail\".\n\nAlso blame.showEmail probably ought to be documented in\nDocumentation/config.txt (although there is some inconsistency here in\nthat documentation for the other blame-specific variables is missing\nfrom config.txt, so perhaps that's something that could be addressed\nseparately).\n\n> --root::\n> Do not treat root commits as boundaries. This can also be\n> diff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt\n> index 9f23a86..50a9030 100644\n> --- a/Documentation/git-blame.txt\n> +++ b/Documentation/git-blame.txt\n> @@ -73,10 +73,6 @@ include::blame-options.txt[]\n> -s::\n> Suppress the author name and timestamp from the output.\n>\n> --e::\n> ---show-email::\n> - Show the author email instead of author name (Default: off).\n> -\n\nIt's not clear why you relocated documentation of --show-email from\ngit-blame.txt to blame-options.txt, and the commit message does not\nexplain the move. If there's a good reason for the relocation, the\njustification should be spelled out so that people reviewing the patch\nor looking through history later on will not have to guess about it.\n\nIt might also make sense to do the relocation as a separate\npreparatory patch of a 2-patch series, in which the patch adding\nblame.showemail is the second of the two.\n\n> -w::\n> Ignore whitespace when comparing the parent's version and\n> the child's to find where the lines came from.\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 06484c2..70bfd0a 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -44,6 +44,7 @@ static int max_score_digits;\n> static int show_root;\n> static int reverse;\n> static int blank_boundary;\n> +static int show_email;\n> static int incremental;\n> static int xdl_opts;\n> static int abbrev = -1;\n> @@ -1926,7 +1927,7 @@ static void emit_other(struct scoreboard *sb, struct blame_entry *ent, int opt)\n> printf(\"%.*s\", length, hex);\n> if (opt & OUTPUT_ANNOTATE_COMPAT) {\n> const char *name;\n> - if (opt & OUTPUT_SHOW_EMAIL)\n> + if ((opt & OUTPUT_SHOW_EMAIL) || show_email)\n\nThe desired behavior is for a configuration setting to provide a\nfallback, and for a command-line option to override the fallback. So,\nfor instance, if blame.showemail is \"true\", then --no-show-email\nshould countermand that. Unfortunately, the way this patch is\nimplemented, it's impossible to override the setting from the\ncommand-line since show_email==true will always win (when\nblame.showemail is \"true\").\n\nMore below.\n\n> name = ci.author_mail.buf;\n> else\n> name = ci.author.buf;\n> @@ -2185,6 +2186,10 @@ static int git_blame_config(const char *var, const char *value, void *cb)\n> blank_boundary = git_config_bool(var, value);\n> return 0;\n> }\n> + if (!strcmp(var, \"blame.showemail\")) {\n> + show_email = git_config_bool(var, value);\n> + return 0;\n> + }\n> if (!strcmp(var, \"blame.date\")) {\n> if (!value)\n> return config_error_nonbool(var);\n\nYou'll also want to add tests for the new blame.showemail\nconfiguration. There's already one test in t8002-blame.sh which checks\nthat --show-email works, but you will want tests to ensure that you\nget the expected results for all combinations of blame.showemail and\n--show-email (including when --show-email is not specified).\n"},{"id":"260057","messageId":"CACfD1vvaAGkx2P3yMfZPOZrRvG3-H96zQVOCevnd-O0rBJ7wjw@mail.gmail.com","threadId":"39151","inReplyTo":"CAPig+cQrUPHOaKjNhsmLho+bFdAOQxb0NK2YK2QsFmdDBF6h4g@mail.gmail.com","subject":"Re: [PATCH] blame: add blame.showemail config option","fromName":"Quentin Neill","fromEmail":"quentin.neill@gmail.com","sentAt":"2015-04-27T13:46:14Z","receivedAt":"2015-04-27T13:46:14Z","isPatch":true,"sender":{"key":"quentin.neill@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3028797?v=4"},"body":"Thanks for the thorough review!\nI have adjusted the commit messages and updated the documentation changes.\nI'm in trying to add tests, I'll probably have some issues but will\npost something that works soon.\nAs for the comments on behavior, see my responses below.\n\n--\nQuentin\n\n\"There! His Majesty can now read my name without glasses. And he can\ndouble the reward on my head!\" - John Hancock, upon signing the\nDeclaration of Independence\n\n\n\nOn Fri, Apr 24, 2015 at 12:22 AM, Eric Sunshine wrote:\n> Thanks for the submission. See comments below...\n>\n> On Thu, Apr 23, 2015 at 10:13 PM, Quentin Neill wrote:\n>> From: Quentin Neill \n>\n> You should drop this line. git-am will pluck your name and email\n> automatically from the email From: header.\n>\n>> If you prefer seeing emails in your git blame output, rather\n>> than sprinkling '-e' options everywhere you can just set\n>> the new config blame.showemail to true.\n>\n> Drop the indentation from the commit message.\n>\n> It's not clear what \"everywhere\" means in the above. It might be\n> sufficient to rephrase more simply as:\n>\n> Complement existing --show-email option with fallback\n> configuration variable.\n>\n> or something.\n>\n>> ---\n>\n> Missing Signed-off-by:. See Documentation/SubmittingPatches.\n>\n>> diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt\n>> index b299b59..9326115 100644\n>> --- a/Documentation/blame-options.txt\n>> +++ b/Documentation/blame-options.txt\n>> @@ -1,6 +1,11 @@\n>> -b::\n>> Show blank SHA-1 for boundary commits. This can also\n>> be controlled via the `blame.blankboundary` config option.\n>> +-e::\n>> +--show-email::\n>\n> Insert a blank line before the \"-e\" line to separate it from the \"-b\" paragraph.\n>\n>> + Show the author email instead of author name (Default: off).\n>> + This can also be controlled via the `blame.showemail` config\n>> + option.\n>\n> Despite being case-insensitive and despite existing inconsistencies,\n> in documentation, it is customary to use camelCase for configuration\n> options, so \"blame.showEmail\".\n>\n> Also blame.showEmail probably ought to be documented in\n> Documentation/config.txt (although there is some inconsistency here in\n> that documentation for the other blame-specific variables is missing\n> from config.txt, so perhaps that's something that could be addressed\n> separately).\n>\n>> --root::\n>> Do not treat root commits as boundaries. This can also be\n>> diff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt\n>> index 9f23a86..50a9030 100644\n>> --- a/Documentation/git-blame.txt\n>> +++ b/Documentation/git-blame.txt\n>> @@ -73,10 +73,6 @@ include::blame-options.txt[]\n>> -s::\n>> Suppress the author name and timestamp from the output.\n>>\n>> --e::\n>> ---show-email::\n>> - Show the author email instead of author name (Default: off).\n>> -\n>\n> It's not clear why you relocated documentation of --show-email from\n> git-blame.txt to blame-options.txt, and the commit message does not\n> explain the move. If there's a good reason for the relocation, the\n> justification should be spelled out so that people reviewing the patch\n> or looking through history later on will not have to guess about it.\n\nI moved it to be with the other variables that had configuration\noptions, but I will move it back.\n\n> It might also make sense to do the relocation as a separate\n> preparatory patch of a 2-patch series, in which the patch adding\n> blame.showemail is the second of the two.\n\nIf you think it should be relocated, I will address in a separate patch.\n\n>> -w::\n>> Ignore whitespace when comparing the parent's version and\n>> the child's to find where the lines came from.\n>> diff --git a/builtin/blame.c b/builtin/blame.c\n>> index 06484c2..70bfd0a 100644\n>> --- a/builtin/blame.c\n>> +++ b/builtin/blame.c\n>> @@ -44,6 +44,7 @@ static int max_score_digits;\n>> static int show_root;\n>> static int reverse;\n>> static int blank_boundary;\n>> +static int show_email;\n>> static int incremental;\n>> static int xdl_opts;\n>> static int abbrev = -1;\n>> @@ -1926,7 +1927,7 @@ static void emit_other(struct scoreboard *sb, struct blame_entry *ent, int opt)\n>> printf(\"%.*s\", length, hex);\n>> if (opt & OUTPUT_ANNOTATE_COMPAT) {\n>> const char *name;\n>> - if (opt & OUTPUT_SHOW_EMAIL)\n>> + if ((opt & OUTPUT_SHOW_EMAIL) || show_email)\n>\n> The desired behavior is for a configuration setting to provide a\n> fallback, and for a command-line option to override the fallback. So,\n> for instance, if blame.showemail is \"true\", then --no-show-email\n> should countermand that. Unfortunately, the way this patch is\n> implemented, it's impossible to override the setting from the\n> command-line since show_email==true will always win (when\n> blame.showemail is \"true\").\n>\n> More below.\n\nI followed the code for blame.showRoot and blame.blankboundary.\n\nI think the desired behavior for the other switches would go in a\nseparate patch, the question is should it precede this one adding\n'blame.showemail'?\n\n>> name = ci.author_mail.buf;\n>> else\n>> name = ci.author.buf;\n>> @@ -2185,6 +2186,10 @@ static int git_blame_config(const char *var, const char *value, void *cb)\n>> blank_boundary = git_config_bool(var, value);\n>> return 0;\n>> }\n>> + if (!strcmp(var, \"blame.showemail\")) {\n>> + show_email = git_config_bool(var, value);\n>> + return 0;\n>> + }\n>> if (!strcmp(var, \"blame.date\")) {\n>> if (!value)\n>> return config_error_nonbool(var);\n>\n> You'll also want to add tests for the new blame.showemail\n> configuration. There's already one test in t8002-blame.sh which checks\n> that --show-email works, but you will want tests to ensure that you\n> get the expected results for all combinations of blame.showemail and\n> --show-email (including when --show-email is not specified).\n\nAgreed, but again I don't see tests for the other switches with options.\n"},{"id":"260064","messageId":"xmqqwq0xwige.fsf@gitster.dls.corp.google.com","threadId":"39151","inReplyTo":"CACfD1vvaAGkx2P3yMfZPOZrRvG3-H96zQVOCevnd-O0rBJ7wjw@mail.gmail.com","subject":"Re: [PATCH] blame: add blame.showemail config option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-27T18:10:25Z","receivedAt":"2015-04-27T18:10:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Quentin Neill writes:\n\n>> It's not clear why you relocated documentation of --show-email from\n>> git-blame.txt to blame-options.txt, and the commit message does not\n>> explain the move. If there's a good reason for the relocation, the\n>> justification should be spelled out so that people reviewing the patch\n>> or looking through history later on will not have to guess about it.\n>\n> I moved it to be with the other variables that had configuration\n> options, but I will move it back.\n\nPlease do not do anything before you understand why there are two\nplaces, and if you don't understand, ask.\n\nIf you do this:\n\n $ git grep blame-options Documentation/\n\nyou would discover that blame-options.txt is meant to hold things\nthat are shared across \"git annotate\" and \"git blame\". What is\nunderstood only by \"git blame\" but not by \"git annotate\" is to be\ndescribed in git-blame.txt, I think. So the criteria is not \"does\nit have configuration?\"; it is \"does git-annotate understand it?\"\n\n>>> @@ -1926,7 +1927,7 @@ static void emit_other(struct scoreboard *sb, struct blame_entry *ent, int opt)\n>>> printf(\"%.*s\", length, hex);\n>>> if (opt & OUTPUT_ANNOTATE_COMPAT) {\n>>> const char *name;\n>>> - if (opt & OUTPUT_SHOW_EMAIL)\n>>> + if ((opt & OUTPUT_SHOW_EMAIL) || show_email)\n>>\n>> The desired behavior is for a configuration setting to provide a\n>> fallback, and for a command-line option to override the fallback. So,\n>> for instance, if blame.showemail is \"true\", then --no-show-email\n>> should countermand that. Unfortunately, the way this patch is\n>> implemented, it's impossible to override the setting from the\n>> command-line since show_email==true will always win (when\n>> blame.showemail is \"true\").\n>>\n>> More below.\n>\n> I followed the code for blame.showRoot and blame.blankboundary.\n\nI do not think that is quite true.\n\nThe code strucure for other existing options is perfectly fine.\nWhat they do is:\n\n - show_root is initialized to the fallback default value of\n false by being in BSS;\n\n - configuration is read to tweak that default;\n\n - command line parser may override the default with --show-root\n or --no-show-root.\n\nAnd then show_root is used throughout the system.\n\nIf you truly followed this code pattern, I would expect that there\nwill not be a separate show_email variable introduced to support\nthis new configuration variable. The OUTPUT_SHOW_EMAIL bit in the\nopt flag word corresponds to show_root and blank_boundary variables,\nso the code to read configuration variable would set that bit in the\nopt flag word before the command line parser would kick in. And the\ncode that checks \"opt & OUTPUT_SHOW_EMAIL\" currently does not have\nto change at all.\n"},{"id":"260077","messageId":"CAPig+cRTfB9aiinhSJp=jY7-__6dRvfAqhWZYVN3+Ut=Y79LcQ@mail.gmail.com","threadId":"39151","inReplyTo":"xmqqwq0xwige.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] blame: add blame.showemail config option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-04-27T19:23:25Z","receivedAt":"2015-04-27T19:23:25Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 27, 2015 at 2:10 PM, Junio C Hamano wrote:\n> Quentin Neill writes:\n>> Eric Sunshine writes:\n>>> Quentin Neill writes:\n>>>> - if (opt & OUTPUT_SHOW_EMAIL)\n>>>> + if ((opt & OUTPUT_SHOW_EMAIL) || show_email)\n>>>\n>>> The desired behavior is for a configuration setting to provide a\n>>> fallback, and for a command-line option to override the fallback. So,\n>>> for instance, if blame.showemail is \"true\", then --no-show-email\n>>> should countermand that. Unfortunately, the way this patch is\n>>> implemented, it's impossible to override the setting from the\n>>> command-line since show_email==true will always win (when\n>>> blame.showemail is \"true\").\n>>>\n>> I followed the code for blame.showRoot and blame.blankboundary.\n>\n> If you truly followed this code pattern, I would expect that there\n> will not be a separate show_email variable introduced to support\n> this new configuration variable. The OUTPUT_SHOW_EMAIL bit in the\n> opt flag word corresponds to show_root and blank_boundary variables,\n> so the code to read configuration variable would set that bit in the\n> opt flag word before the command line parser would kick in. And the\n> code that checks \"opt & OUTPUT_SHOW_EMAIL\" currently does not have\n> to change at all.\n\nRight. Rather than having a separate global 'show_email' variable and\nconsulting that variable in parallel with OUTPUT_SHOW_EMAIL throughout\nthe code, instead set the OUTPUT_SHOW_EMAIL bit in git_blame_config().\nTo do this, take advantage of the \"callback data\" argument of\ngit_config(), which will arrive in git_blame_config() as its 'void\n*cb' argument. So, for instance, something like this:\n\n static int git_blame_config(var, value, void *cb)\n {\n ...\n if (!strcmp(var, \"blame.showemail\")) {\n if (git_config_bool(var, value)) {\n int *output_options = cb;\n *output_options |= OUTPUT_SHOW_EMAIL;\n }\n return 0;\n }\n ...\n }\n\n int cmd_blame(...)\n {\n ...\n git_config(git_blame_config, &output_options);\n ...\n parse_options(...);\n ...\n }\n"},{"id":"260081","messageId":"CAPig+cRYUng43rxCJExMEs1ti6wp0oangTzwJbbesmsqM3c+tA@mail.gmail.com","threadId":"39151","inReplyTo":"CACfD1vvaAGkx2P3yMfZPOZrRvG3-H96zQVOCevnd-O0rBJ7wjw@mail.gmail.com","subject":"Re: [PATCH] blame: add blame.showemail config option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-04-27T19:57:14Z","receivedAt":"2015-04-27T19:57:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 27, 2015 at 9:46 AM, Quentin Neill wrote:\n> On Fri, Apr 24, 2015 at 12:22 AM, Eric Sunshine wrote:\n>> It's not clear why you relocated documentation of --show-email from\n>> git-blame.txt to blame-options.txt, and the commit message does not\n>> explain the move. If there's a good reason for the relocation, the\n>> justification should be spelled out so that people reviewing the patch\n>> or looking through history later on will not have to guess about it.\n>\n> I moved it to be with the other variables that had configuration\n> options, but I will move it back.\n>\n>> It might also make sense to do the relocation as a separate\n>> preparatory patch of a 2-patch series, in which the patch adding\n>> blame.showemail is the second of the two.\n>\n> If you think it should be relocated, I will address in a separate patch.\n\nJunio's response[1] addresses both points nicely. To be clear, I\nwasn't suggesting that you should do the relocation, but instead that\nthe relocation seemed unrelated to the overall intent of the patch and\nthat its purpose wasn't clear. So, as a general statement, when the\nmotive for a change is unclear, it deserves explanation in the commit\nmessage; and when a change is not directly related to the patch\nitself, it often deserves to be placed in its own patch. In this case,\nneither applies since the relocation is unwarranted.\n\n>>> - if (opt & OUTPUT_SHOW_EMAIL)\n>>> + if ((opt & OUTPUT_SHOW_EMAIL) || show_email)\n>>\n>> The desired behavior is for a configuration setting to provide a\n>> fallback, and for a command-line option to override the fallback. So,\n>> for instance, if blame.showemail is \"true\", then --no-show-email\n>> should countermand that. Unfortunately, the way this patch is\n>> implemented, it's impossible to override the setting from the\n>> command-line since show_email==true will always win (when\n>> blame.showemail is \"true\").\n>>\n>> More below.\n>\n> I followed the code for blame.showRoot and blame.blankboundary.\n>\n> I think the desired behavior for the other switches would go in a\n> separate patch, the question is should it precede this one adding\n> 'blame.showemail'?\n\nAs per Junio's response[1], logic for the other configuration options\nseems to be fine, so I'm not quite sure what changes you propose.\n\n>> You'll also want to add tests for the new blame.showemail\n>> configuration. There's already one test in t8002-blame.sh which checks\n>> that --show-email works, but you will want tests to ensure that you\n>> get the expected results for all combinations of blame.showemail and\n>> --show-email (including when --show-email is not specified).\n>\n> Agreed, but again I don't see tests for the other switches with options.\n\nUnfortunately, test coverage is sometimes sparse, however, patches\nwith accompanying tests are looked upon with favor and instill greater\nconfidence, so they are encouraged. If you need assistance with the\ntests, feel free to ask.\n\nIt's not your responsibility to fill the gaps of missing tests for\nother options which you're not touching, but you're welcome to add\ntests for them if you want to.\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/267720/focus=267862\n"},{"id":"260108","messageId":"xmqqmw1svigd.fsf@gitster.dls.corp.google.com","threadId":"39151","inReplyTo":"CAPig+cRTfB9aiinhSJp=jY7-__6dRvfAqhWZYVN3+Ut=Y79LcQ@mail.gmail.com","subject":"Re: [PATCH] blame: add blame.showemail config option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-28T07:08:02Z","receivedAt":"2015-04-28T07:08:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> Right. Rather than having a separate global 'show_email' variable and\n> consulting that variable in parallel with OUTPUT_SHOW_EMAIL throughout\n> the code, instead set the OUTPUT_SHOW_EMAIL bit in git_blame_config().\n> To do this, take advantage of the \"callback data\" argument of\n> git_config(), which will arrive in git_blame_config() as its 'void\n> *cb' argument. So, for instance, something like this:\n>\n> static int git_blame_config(var, value, void *cb)\n> {\n> ...\n> if (!strcmp(var, \"blame.showemail\")) {\n> if (git_config_bool(var, value)) {\n> int *output_options = cb;\n> *output_options |= OUTPUT_SHOW_EMAIL;\n> }\n\nDon't forget to clear the bit when the bool is set to false, too.\n\n> return 0;\n> }\n> ...\n> }\n>\n> int cmd_blame(...)\n> {\n> ...\n> git_config(git_blame_config, &output_options);\n> ...\n> parse_options(...);\n> ...\n> }\n"},{"id":"260273","messageId":"CACfD1vtfO_rqr4wv4oHm7W7DSn-Gk-du6nCuJzYvpPXK_d=y=g@mail.gmail.com","threadId":"39151","inReplyTo":"xmqqmw1svigd.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] blame: add blame.showemail config option","fromName":"Quentin Neill","fromEmail":"quentin.neill@gmail.com","sentAt":"2015-04-30T14:03:03Z","receivedAt":"2015-04-30T14:03:03Z","isPatch":true,"sender":{"key":"quentin.neill@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3028797?v=4"},"body":"On Mon, Apr 27, 2015 at 1:10 PM, Junio C Hamano wrote:\n>>> It's not clear why you relocated documentation of --show-email from\n>>>\n>> I moved it to be with the other variables that had configuration\n>>\n>> options, but I will move it back.\n>\n> Please do not do anything before you understand why there are two\n> places, and if you don't understand, ask.\n>\n>> I followed the code for blame.showRoot and blame.blankboundary.\n>\n> I do not think that is quite true.\n\nPoints well taken. I'll admit I posted the patch as a way to find out these\nsorts of things, but I see I could have done a bit more digging and asking\nbefore posting. Apologies for the noise.\n\nOn Tue, Apr 28, 2015 at 2:08 AM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n>> .. instead set the OUTPUT_SHOW_EMAIL bit in git_blame_config().\n>\n> Don't forget to clear the bit when the bool is set to false, too.\n\nI think I have this now, and some tests that check it, but have a pair\nof questions.\n\nOn Fri, Apr 24, 2015 at 12:22 AM, Eric Sunshine wrote:\n> Despite being case-insensitive and despite existing inconsistencies,\n> in documentation, it is customary to use camelCase for configuration\n> options, so \"blame.showEmail\".\n\nI noticed while testing that git_config()/git_value() lowercase\neverything, so to\nbe clear this camelCase custom for configuration names is for documentation\nonly, right?\n\nI'm thinking of a test file that will test all the git blame options,\nbut for this\npatch it will only test the new showEmail config. I read t/README and\ntentatively named the new test file \"t/8009-blame-config.sh\".\n\nPlease guide.\n"},{"id":"260275","messageId":"xmqq1tj138cf.fsf@gitster.dls.corp.google.com","threadId":"39151","inReplyTo":"CACfD1vtfO_rqr4wv4oHm7W7DSn-Gk-du6nCuJzYvpPXK_d=y=g@mail.gmail.com","subject":"Re: [PATCH] blame: add blame.showemail config option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-30T16:10:40Z","receivedAt":"2015-04-30T16:10:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Quentin Neill writes:\n\n> On Fri, Apr 24, 2015 at 12:22 AM, Eric Sunshine wrote:\n>> Despite being case-insensitive and despite existing inconsistencies,\n>> in documentation, it is customary to use camelCase for configuration\n>> options, so \"blame.showEmail\".\n>\n> I noticed while testing that git_config()/git_value() lowercase\n> everything, so to be clear this camelCase custom for configuration\n> names is for documentation only, right?\n\nCorrect. Write \"section.variableName\" in documentation and\ninvocations of \"git config\" command in scripts. Compare key\nwith \"section.variablename\" in git_config() callback.\n\n> I'm thinking of a test file that will test all the git blame options,\n> but for this\n> patch it will only test the new showEmail config. I read t/README and\n> tentatively named the new test file \"t/8009-blame-config.sh\".\n\nI'd suggest\n\n [PATCH 1/2] blame: add blame.showEmail configuration\n\nwhich would be the polished version of the patch we have been\ndiscussing, plus tests for this particular feature, and\n\n [PATCH 2/2] blame: more tests\n\nwhich would widen test coverage to other configuration variables and\nfeatures.\n"},{"id":"260305","messageId":"CAPig+cQ1JNeg5eoJib07nc6UrREE80pssokbk+xthXTXDcQz1g@mail.gmail.com","threadId":"39151","inReplyTo":"CACfD1vtfO_rqr4wv4oHm7W7DSn-Gk-du6nCuJzYvpPXK_d=y=g@mail.gmail.com","subject":"Re: [PATCH] blame: add blame.showemail config option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-04-30T21:33:30Z","receivedAt":"2015-04-30T21:33:30Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Apr 30, 2015 at 10:03 AM, Quentin Neill wrote:\n> I'm thinking of a test file that will test all the git blame options,\n> but for this\n> patch it will only test the new showEmail config. I read t/README and\n> tentatively named the new test file \"t/8009-blame-config.sh\".\n\nI'm not sure that these new proposed tests warrant a new test script.\nMoreover, the tests presumably will be testing combinations of\nconfiguration options and command-line switches, so having \"config\" in\nthe script name doesn't seem quite correct.\n\nt8002-blame.sh already contains a test for --show-email, so it may be\nlogical to add the new tests there, alongside the existing one. On the\nother hand, if you do add a new test script, then perhaps the existing\n--show-email test in t8002-blame.sh should be moved to the new script.\n"},{"id":"260306","messageId":"xmqqk2wtxpg5.fsf@gitster.dls.corp.google.com","threadId":"39151","inReplyTo":"CAPig+cQ1JNeg5eoJib07nc6UrREE80pssokbk+xthXTXDcQz1g@mail.gmail.com","subject":"Re: [PATCH] blame: add blame.showemail config option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-30T21:43:06Z","receivedAt":"2015-04-30T21:43:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Thu, Apr 30, 2015 at 10:03 AM, Quentin Neill wrote:\n>> I'm thinking of a test file that will test all the git blame options,\n>> but for this\n>> patch it will only test the new showEmail config. I read t/README and\n>> tentatively named the new test file \"t/8009-blame-config.sh\".\n>\n> I'm not sure that these new proposed tests warrant a new test script.\n> Moreover, the tests presumably will be testing combinations of\n> configuration options and command-line switches, so having \"config\" in\n> the script name doesn't seem quite correct.\n>\n> t8002-blame.sh already contains a test for --show-email, so it may be\n> logical to add the new tests there, alongside the existing one. On the\n> other hand, if you do add a new test script, then perhaps the existing\n> --show-email test in t8002-blame.sh should be moved to the new script.\n\nGood thinking. I would vote for the former, unless the number of\nadditional tests exceed 20 (as t8002 has about 100 tests).\n"},{"id":"262449","messageId":"xmqq617bcgva.fsf@gitster.dls.corp.google.com","threadId":"39151","inReplyTo":"CACfD1vvaAGkx2P3yMfZPOZrRvG3-H96zQVOCevnd-O0rBJ7wjw@mail.gmail.com","subject":"Re: [PATCH] blame: add blame.showemail config option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-29T19:40:09Z","receivedAt":"2015-05-29T19:40:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Quentin Neill writes:\n\n> Thanks for the thorough review!\n> I have adjusted the commit messages and updated the documentation changes.\n> I'm in trying to add tests, I'll probably have some issues but will\n> post something that works soon.\n\nHi, I was sweeping my old mailbox for leftover bits, and noticed\nthat this thread ended without seeing the final step.\n\nHas anything further happened to this topic, did you got too busy,\nor you lost interest?\n"},{"id":"262488","messageId":"CACfD1vv9gFFnYmMQmX_KmZFHBbzPuNmts7FE8cJwQz-r+33BcA@mail.gmail.com","threadId":"39151","inReplyTo":"xmqq617bcgva.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] blame: add blame.showemail config option","fromName":"Quentin Neill","fromEmail":"quentin.neill@gmail.com","sentAt":"2015-05-30T20:31:10Z","receivedAt":"2015-05-30T20:31:10Z","isPatch":true,"sender":{"key":"quentin.neill@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3028797?v=4"},"body":"On Fri, May 29, 2015 at 2:40 PM, Junio C Hamano wrote:\n>\n> Quentin Neill writes:\n>\n> > Thanks for the thorough review!\n> > I have adjusted the commit messages and updated the documentation changes.\n> > I'm in trying to add tests, I'll probably have some issues but will\n> > post something that works soon.\n>\n> Hi, I was sweeping my old mailbox for leftover bits, and noticed\n> that this thread ended without seeing the final step.\n>\n> Has anything further happened to this topic, did you got too busy,\n> or you lost interest?\n>\n>\n\n[One more time without html]\n\nHi Junio,\n\nI got too busy, my free time went to zero. And this week I am\ntraveling. I was tinkering with tests Thursday night and was in fact\npreparing a reply. When I get back I'll post the patch.\n"},{"id":"262489","messageId":"1433021926-17762-1-git-send-email-qneill@quicinc.com","threadId":"39151","inReplyTo":"xmqq617bcgva.fsf@gitster.dls.corp.google.com","subject":"[PATCH v2] blame: add blame.showEmail configuration","fromName":"Quentin Neill","fromEmail":"quentin.neill@gmail.com","sentAt":"2015-05-30T21:38:46Z","receivedAt":"2015-05-30T21:38:46Z","isPatch":true,"sender":{"key":"quentin.neill@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3028797?v=4"},"body":"From: Quentin Neill \n\nComplement existing --show-email option with fallback\nconfiguration variable, with tests.\n---\n Documentation/git-blame.txt | 2 ++\n builtin/blame.c | 10 +++++++-\n t/t8002-blame.sh | 62 +++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 73 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt\nindex 9f23a86..e6e947c 100644\n--- a/Documentation/git-blame.txt\n+++ b/Documentation/git-blame.txt\n@@ -76,6 +76,8 @@ include::blame-options.txt[]\n -e::\n --show-email::\n \tShow the author email instead of author name (Default: off).\n+\tThis can also be controlled via the `blame.showEmail` config\n+\toption.\n \n -w::\n \tIgnore whitespace when comparing the parent's version and\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 8d70623..60039d3 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2185,6 +2185,14 @@ static int git_blame_config(const char *var, const char *value, void *cb)\n \t\tblank_boundary = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"blame.showemail\")) {\n+\t\tint *output_option = cb;\n+\t\tif (git_config_bool(var, value))\n+\t\t\t*output_option |= OUTPUT_SHOW_EMAIL;\n+\t\telse\n+\t\t\t*output_option &= ~OUTPUT_SHOW_EMAIL;\n+\t\treturn 0;\n+\t}\n \tif (!strcmp(var, \"blame.date\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n@@ -2529,7 +2537,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \tunsigned int range_i;\n \tlong anchor;\n \n-\tgit_config(git_blame_config, NULL);\n+\tgit_config(git_blame_config, &output_option);\n \tinit_revisions(&revs, NULL);\n \trevs.date_mode = blame_date_mode;\n \tDIFF_OPT_SET(&revs.diffopt, ALLOW_TEXTCONV);\ndiff --git a/t/t8002-blame.sh b/t/t8002-blame.sh\nindex 5cdf3f1..faf1660 100755\n--- a/t/t8002-blame.sh\n+++ b/t/t8002-blame.sh\n@@ -19,4 +19,66 @@ test_expect_success 'blame --show-email' '\n \t\t\"\" 1\n '\n \n+test_expect_success 'setup showEmail tests' '\n+\techo \"bin: test number 1\" >one &&\n+\tgit add . &&\n+\tGIT_AUTHOR_NAME=name1 GIT_AUTHOR_EMAIL=email1@test.git git commit -a -m First --date=\"2010-01-01 01:00:00\"\n+'\n+\n+cat >expected_n <<\\EOF &&\n+(name1 2010-01-01 01:00:00 +0000 1) bin: test number 1\n+EOF\n+\n+cat >expected_e <<\\EOF &&\n+( 2010-01-01 01:00:00 +0000 1) bin: test number 1\n+EOF\n+\n+find_blame() {\n+\tsed -e 's/^[^(]*//'\n+}\n+\n+test_expect_success 'blame with no options and no config' '\n+\tgit blame one >blame &&\n+\tfind_blame result &&\n+\ttest_cmp expected_n result\n+'\n+\n+test_expect_success 'blame with showemail options' '\n+\tgit blame --show-email one >blame1 &&\n+\tfind_blame result &&\n+\ttest_cmp expected_e result &&\n+\tgit blame -e one >blame2 &&\n+\tfind_blame result &&\n+\ttest_cmp expected_e result &&\n+\tgit blame --no-show-email one >blame3 &&\n+\tfind_blame result &&\n+\ttest_cmp expected_n result\n+'\n+\n+test_expect_success 'blame with showEmail config false' '\n+\tgit config blame.showEmail false &&\n+\tgit blame one >blame1 &&\n+\tfind_blame result &&\n+\ttest_cmp expected_n result &&\n+\tgit blame --show-email one >blame2 &&\n+\tfind_blame result &&\n+\ttest_cmp expected_e result &&\n+\tgit blame -e one >blame3 &&\n+\tfind_blame result &&\n+\ttest_cmp expected_e result &&\n+\tgit blame --no-show-email one >blame4 &&\n+\tfind_blame result &&\n+\ttest_cmp expected_n result\n+'\n+\n+test_expect_success 'blame with showEmail config true' '\n+\tgit config blame.showEmail true &&\n+\tgit blame one >blame1 &&\n+\tfind_blame result &&\n+\ttest_cmp expected_e result &&\n+\tgit blame --no-show-email one >blame2 &&\n+\tfind_blame result &&\n+\ttest_cmp expected_n result\n+'\n+\n test_done\n-- \n2.4.2.340.g5ecd853\n"},{"id":"262521","messageId":"xmqqa8wkaa4c.fsf@gitster.dls.corp.google.com","threadId":"39151","inReplyTo":"1433021926-17762-1-git-send-email-qneill@quicinc.com","subject":"Re: [PATCH v2] blame: add blame.showEmail configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-31T18:13:23Z","receivedAt":"2015-05-31T18:13:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Quentin Neill writes:\n\n> From: Quentin Neill \n>\n> Complement existing --show-email option with fallback\n> configuration variable, with tests.\n> ---\n\nThe patch itself looks very reasonable. Thanks for getting back to\nus ;-)\n\nA few minor nits:\n\n - Your in-body \"From:\" is redundant and unnecessary, as your\n e-mail is coming from the same address.\n\n - You need \"Signed-off-by: Quentin Neill \"\n after your log message (separate it with a blank line before\n the sign-off, and place the sign-off before the three-dash\n lines).\n\n> diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh\n> index 5cdf3f1..faf1660 100755\n> --- a/t/t8002-blame.sh\n> +++ b/t/t8002-blame.sh\n> @@ -19,4 +19,66 @@ test_expect_success 'blame --show-email' '\n> \t\t\"\" 1\n> '\n> \n> +test_expect_success 'setup showEmail tests' '\n> +\techo \"bin: test number 1\" >one &&\n> +\tgit add . &&\n> +\tGIT_AUTHOR_NAME=name1 GIT_AUTHOR_EMAIL=email1@test.git git commit -a -m First --date=\"2010-01-01 01:00:00\"\n> +'\n> +\n> +cat >expected_n <<\\EOF &&\n> +(name1 2010-01-01 01:00:00 +0000 1) bin: test number 1\n> +EOF\n> +\n> +cat >expected_e <<\\EOF &&\n> +( 2010-01-01 01:00:00 +0000 1) bin: test number 1\n> +EOF\n\nThese two commands outside test_expect_success are part of setup, so\n\n\ttest_expect_success 'setup showEmail tests' '\n \techo \"bin: test number 1\" >one &&\n git add one &&\n GIT_AUTHOR_NAME=name1 \\\n GIT_AUTHOR_EMAIL=email1@test.git \\\n git commit -m First --date=\"2010-01-01 01:00:00\" &&\n\t\tcat >expected_n <<-\\EOF &&\n (name1 ...\n EOF\n cat >expected_e <<-\\EOF\n ( +find_blame() {\n\nstyle: \"find_blame () {\"\n\nOther than that, the patch looks good.\n\nThanks.\n"},{"id":"262527","messageId":"CACfD1vvHMYw6J4Gi8eAND7Sm=aUb23U8Q_MBSzrWek0uuxPwdQ@mail.gmail.com","threadId":"39151","inReplyTo":"xmqqa8wkaa4c.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] blame: add blame.showEmail configuration","fromName":"Quentin Neill","fromEmail":"quentin.neill@gmail.com","sentAt":"2015-05-31T19:24:07Z","receivedAt":"2015-05-31T19:24:07Z","isPatch":true,"sender":{"key":"quentin.neill@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3028797?v=4"},"body":"On Sun, May 31, 2015 at 1:13 PM, Junio C Hamano wrote:\n> Quentin Neill writes:\n>\n>> From: Quentin Neill \n>>\n>> Complement existing --show-email option with fallback\n>> configuration variable, with tests.\n>> ---\n>\n> The patch itself looks very reasonable. Thanks for getting back to\n> us ;-)\n>\n> A few minor nits:\n>\n> - Your in-body \"From:\" is redundant and unnecessary, as your\n> e-mail is coming from the same address.\n\nI tried using 'git send-email' directly on the commit, not sure how or\nwhy that occurred.\nI will fall back to 'git format-patch' and 'git send-email' as I did\nin my first post.\n\n> - You need \"Signed-off-by: Quentin Neill \"\n> after your log message (separate it with a blank line before\n> the sign-off, and place the sign-off before the three-dash\n> lines).\n>\n>> diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh\n>> index 5cdf3f1..faf1660 100755\n>> --- a/t/t8002-blame.sh\n>> +++ b/t/t8002-blame.sh\n>> @@ -19,4 +19,66 @@ test_expect_success 'blame --show-email' '\n>> \"\" 1\n>> '\n>>\n>> +test_expect_success 'setup showEmail tests' '\n>> + echo \"bin: test number 1\" >one &&\n>> + git add . &&\n>> + GIT_AUTHOR_NAME=name1 GIT_AUTHOR_EMAIL=email1@test.git git commit -a -m First --date=\"2010-01-01 01:00:00\"\n>> +'\n>> +\n>> +cat >expected_n <<\\EOF &&\n>> +(name1 2010-01-01 01:00:00 +0000 1) bin: test number 1\n>> +EOF\n>> +\n>> +cat >expected_e <<\\EOF &&\n>> +( 2010-01-01 01:00:00 +0000 1) bin: test number 1\n>> +EOF\n>\n> These two commands outside test_expect_success are part of setup, so\n>\n> test_expect_success 'setup showEmail tests' '\n> echo \"bin: test number 1\" >one &&\n> git add one &&\n> GIT_AUTHOR_NAME=name1 \\\n> GIT_AUTHOR_EMAIL=email1@test.git \\\n> git commit -m First --date=\"2010-01-01 01:00:00\" &&\n> cat >expected_n <<-\\EOF &&\n> (name1 ...\n> EOF\n> cat >expected_e <<-\\EOF\n> ( EOF\n> '\n>\n> Also do not hesitate to break overlong lines with \"\\\".\n>\n>> +find_blame() {\n>\n> style: \"find_blame () {\"\n>\n> Other than that, the patch looks good.\n>\n> Thanks.\n\nThanks for the help, one more version on the way.\n"},{"id":"262528","messageId":"1433100457-18828-1-git-send-email-quentin.neill@gmail.com","threadId":"39151","inReplyTo":"xmqq617bcgva.fsf@gitster.dls.corp.google.com","subject":"[PATCH v3] blame: add blame.showEmail configuration","fromName":"Quentin Neill","fromEmail":"quentin.neill@gmail.com","sentAt":"2015-05-31T19:27:37Z","receivedAt":"2015-05-31T19:27:37Z","isPatch":true,"sender":{"key":"quentin.neill@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3028797?v=4"},"body":"Complement existing --show-email option with fallback\nconfiguration variable, with tests.\n\nSigned-off-by: Quentin Neill \n---\n Documentation/git-blame.txt | 2 ++\n builtin/blame.c | 10 +++++++-\n t/t8002-blame.sh | 62 +++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 73 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt\nindex 9f23a86..e6e947c 100644\n--- a/Documentation/git-blame.txt\n+++ b/Documentation/git-blame.txt\n@@ -76,6 +76,8 @@ include::blame-options.txt[]\n -e::\n --show-email::\n \tShow the author email instead of author name (Default: off).\n+\tThis can also be controlled via the `blame.showEmail` config\n+\toption.\n \n -w::\n \tIgnore whitespace when comparing the parent's version and\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 8d70623..60039d3 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2185,6 +2185,14 @@ static int git_blame_config(const char *var, const char *value, void *cb)\n \t\tblank_boundary = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"blame.showemail\")) {\n+\t\tint *output_option = cb;\n+\t\tif (git_config_bool(var, value))\n+\t\t\t*output_option |= OUTPUT_SHOW_EMAIL;\n+\t\telse\n+\t\t\t*output_option &= ~OUTPUT_SHOW_EMAIL;\n+\t\treturn 0;\n+\t}\n \tif (!strcmp(var, \"blame.date\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n@@ -2529,7 +2537,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \tunsigned int range_i;\n \tlong anchor;\n \n-\tgit_config(git_blame_config, NULL);\n+\tgit_config(git_blame_config, &output_option);\n \tinit_revisions(&revs, NULL);\n \trevs.date_mode = blame_date_mode;\n \tDIFF_OPT_SET(&revs.diffopt, ALLOW_TEXTCONV);\ndiff --git a/t/t8002-blame.sh b/t/t8002-blame.sh\nindex 5cdf3f1..ff09ace 100755\n--- a/t/t8002-blame.sh\n+++ b/t/t8002-blame.sh\n@@ -19,4 +19,66 @@ test_expect_success 'blame --show-email' '\n \t\t\"\" 1\n '\n \n+test_expect_success 'setup showEmail tests' '\n+\techo \"bin: test number 1\" >one &&\n+\tgit add one &&\n+\tGIT_AUTHOR_NAME=name1 \\\n+\tGIT_AUTHOR_EMAIL=email1@test.git \\\n+\tgit commit -m First --date=\"2010-01-01 01:00:00\" &&\n+\tcat >expected_n <<-\\EOF &&\n+\t(name1 2010-01-01 01:00:00 +0000 1) bin: test number 1\n+\tEOF\n+\tcat >expected_e <<-\\EOF\n+\t( 2010-01-01 01:00:00 +0000 1) bin: test number 1\n+\tEOF\n+'\n+\n+find_blame () {\n+\tsed -e 's/^[^(]*//'\n+}\n+\n+test_expect_success 'blame with no options and no config' '\n+\tgit blame one >blame &&\n+\tfind_blame result &&\n+\ttest_cmp expected_n result\n+'\n+\n+test_expect_success 'blame with showemail options' '\n+\tgit blame --show-email one >blame1 &&\n+\tfind_blame result &&\n+\ttest_cmp expected_e result &&\n+\tgit blame -e one >blame2 &&\n+\tfind_blame result &&\n+\ttest_cmp expected_e result &&\n+\tgit blame --no-show-email one >blame3 &&\n+\tfind_blame result &&\n+\ttest_cmp expected_n result\n+'\n+\n+test_expect_success 'blame with showEmail config false' '\n+\tgit config blame.showEmail false &&\n+\tgit blame one >blame1 &&\n+\tfind_blame result &&\n+\ttest_cmp expected_n result &&\n+\tgit blame --show-email one >blame2 &&\n+\tfind_blame result &&\n+\ttest_cmp expected_e result &&\n+\tgit blame -e one >blame3 &&\n+\tfind_blame result &&\n+\ttest_cmp expected_e result &&\n+\tgit blame --no-show-email one >blame4 &&\n+\tfind_blame result &&\n+\ttest_cmp expected_n result\n+'\n+\n+test_expect_success 'blame with showEmail config true' '\n+\tgit config blame.showEmail true &&\n+\tgit blame one >blame1 &&\n+\tfind_blame result &&\n+\ttest_cmp expected_e result &&\n+\tgit blame --no-show-email one >blame2 &&\n+\tfind_blame result &&\n+\ttest_cmp expected_n result\n+'\n+\n test_done\n-- \n2.4.2.340.g5ecd853\n"}]}