{"thread":{"id":"32444","subject":"[PATCH 0/2] Mailmap in log improvements","startedAt":"2012-12-22T16:58:30Z","lastAt":"2012-12-28T20:37:42Z","messageCount":15,"participants":["Antoine Pelisse","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"205420","messageId":"1356195512-4846-1-git-send-email-apelisse@gmail.com","threadId":"32444","inReplyTo":null,"subject":"[PATCH 0/2] Mailmap in log improvements","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2012-12-22T16:58:30Z","receivedAt":"2012-12-22T16:58:30Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"The goal of these patches are to:\n\n - allow the use of mailmap when looking for commits\n   authored/committed by a mapped name/email.\n\n - add an option so that --use-mailmap option can be used\n   automatically.\n\nTests are included.\n\nThis series is based on ap/log-mailmap.\n\nAntoine Pelisse (2):\n  log: grep author/committer using mailmap\n  log: add log.mailmap configuration option\n\n Documentation/config.txt |    4 ++++\n builtin/log.c            |    8 ++++++-\n revision.c               |   53 ++++++++++++++++++++++++++++++++++++++++++++++\n t/t4203-mailmap.sh       |   42 ++++++++++++++++++++++++++++++++++++\n 4 files changed, 106 insertions(+), 1 deletion(-)\n\n--\n1.7.9.5\n"},{"id":"205422","messageId":"1356195512-4846-2-git-send-email-apelisse@gmail.com","threadId":"32444","inReplyTo":"1356195512-4846-1-git-send-email-apelisse@gmail.com","subject":"[PATCH 1/2] log: grep author/committer using mailmap","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2012-12-22T16:58:31Z","receivedAt":"2012-12-22T16:58:31Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"Currently mailmap can be used to display log authors and committers\nbut there no way to use mailmap to find commits with mapped values.\n\nThis commit allows those commands to work:\n\n    git log --use-mailmap --author mapped_name_or_email\n    git log --use-mailmap --committer mapped_name_or_email\n\nOf course it only works if the --use-mailmap option is used.\n\nSigned-off-by: Antoine Pelisse <apelisse@gmail.com>\n---\nI probably missed something but I didn't find the connection with\ncommit 2d10c55. Let me know if I went the wrong direction.\n\n revision.c         |   53 ++++++++++++++++++++++++++++++++++++++++++++++++++++\n t/t4203-mailmap.sh |   18 ++++++++++++++++++\n 2 files changed, 71 insertions(+)\n\ndiff --git a/revision.c b/revision.c\nindex 95d21e6..fb9fd41 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -13,6 +13,7 @@\n #include \"decorate.h\"\n #include \"log-tree.h\"\n #include \"string-list.h\"\n+#include \"mailmap.h\"\n\n volatile show_early_output_fn_t show_early_output;\n\n@@ -2219,6 +2220,50 @@ static int rewrite_parents(struct rev_info *revs, struct commit *commit)\n \treturn 0;\n }\n\n+static int commit_rewrite_authors(struct strbuf *buf, const char *what, struct string_list *mailmap)\n+{\n+\tchar *author, *endp;\n+\tsize_t len;\n+\tstruct strbuf name = STRBUF_INIT;\n+\tstruct strbuf mail = STRBUF_INIT;\n+\tstruct ident_split ident;\n+\n+\tauthor = strstr(buf->buf, what);\n+\tif (!author)\n+\t\tgoto error;\n+\n+\tauthor += strlen(what);\n+\tendp = strstr(author, \"\\n\");\n+\tif (!endp)\n+\t\tgoto error;\n+\n+\tlen = endp - author;\n+\n+\tif (split_ident_line(&ident, author, len)) {\n+\terror:\n+\t\tstrbuf_release(&name);\n+\t\tstrbuf_release(&mail);\n+\n+\t\treturn 1;\n+\t}\n+\n+\tstrbuf_add(&name, ident.name_begin, ident.name_end - ident.name_begin);\n+\tstrbuf_add(&mail, ident.mail_begin, ident.mail_end - ident.mail_begin);\n+\n+\tmap_user(mailmap, &mail, &name);\n+\n+\tstrbuf_addf(&name, \" <%s>\", mail.buf);\n+\n+\tstrbuf_splice(buf, ident.name_begin - buf->buf,\n+\t\t      ident.mail_end - ident.name_begin + 1,\n+\t\t      name.buf, name.len);\n+\n+\tstrbuf_release(&name);\n+\tstrbuf_release(&mail);\n+\n+\treturn 0;\n+}\n+\n static int commit_match(struct commit *commit, struct rev_info *opt)\n {\n \tint retval;\n@@ -2237,6 +2282,14 @@ static int commit_match(struct commit *commit, struct rev_info *opt)\n \tif (buf.len)\n \t\tstrbuf_addstr(&buf, commit->buffer);\n\n+\tif (opt->mailmap) {\n+\t\tif (!buf.len)\n+\t\t\tstrbuf_addstr(&buf, commit->buffer);\n+\n+\t\tcommit_rewrite_authors(&buf, \"\\nauthor \", opt->mailmap);\n+\t\tcommit_rewrite_authors(&buf, \"\\ncommitter \", opt->mailmap);\n+\t}\n+\n \t/* Append \"fake\" message parts as needed */\n \tif (opt->show_notes) {\n \t\tif (!buf.len)\ndiff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh\nindex db043dc..e16187f 100755\n--- a/t/t4203-mailmap.sh\n+++ b/t/t4203-mailmap.sh\n@@ -248,11 +248,29 @@ Author: Other Author <other@author.xx>\n Author: Some Dude <some@dude.xx>\n Author: A U Thor <author@example.com>\n EOF\n+\n test_expect_success 'Log output with --use-mailmap' '\n \tgit log --use-mailmap | grep Author >actual &&\n \ttest_cmp expect actual\n '\n\n+cat >expect <<\\EOF\n+Author: Santa Claus <santa.claus@northpole.xx>\n+Author: Santa Claus <santa.claus@northpole.xx>\n+EOF\n+\n+test_expect_success 'Grep author with --use-mailmap' '\n+\tgit log --use-mailmap --author Santa | grep Author >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+>expect\n+\n+test_expect_success 'Only grep replaced author with --use-mailmap' '\n+\tgit log --use-mailmap --author \"<cto@coompany.xx>\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n # git blame\n cat >expect <<\\EOF\n ^OBJI (A U Thor     DATE 1) one\n--\n1.7.9.5\n"},{"id":"205421","messageId":"1356195512-4846-3-git-send-email-apelisse@gmail.com","threadId":"32444","inReplyTo":"1356195512-4846-1-git-send-email-apelisse@gmail.com","subject":"[PATCH 2/2] log: add log.mailmap configuration option","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2012-12-22T16:58:32Z","receivedAt":"2012-12-22T16:58:32Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"This patch provides a new configuration option 'log.mailmap' to\nautomatically use the --use-mailmap option from git-show, git-log and\ngit-whatchanged commands.\n\nSigned-off-by: Antoine Pelisse <apelisse@gmail.com>\n---\nI'm wondering if it would be needed to add a no-use-mailmap option to\nlog command so that it can cancel this configuration option.\n\n Documentation/config.txt |    4 ++++\n builtin/log.c            |    8 +++++++-\n t/t4203-mailmap.sh       |   24 ++++++++++++++++++++++++\n 3 files changed, 35 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex bf8f911..226362a 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1509,6 +1509,10 @@ log.showroot::\n \tTools like linkgit:git-log[1] or linkgit:git-whatchanged[1], which\n \tnormally hide the root commit will now show it. True by default.\n\n+log.mailmap::\n+\tIf true, makes linkgit:git-log[1], linkgit:git-show[1], and\n+\tlinkgit:git-whatchanged[1] assume `--use-mailmap`.\n+\n mailmap.file::\n \tThe location of an augmenting mailmap file. The default\n \tmailmap, located in the root of the repository, is loaded\ndiff --git a/builtin/log.c b/builtin/log.c\nindex d2bd8ce..f6936ff 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -31,6 +31,7 @@ static int default_abbrev_commit;\n static int default_show_root = 1;\n static int decoration_style;\n static int decoration_given;\n+static int use_mailmap;\n static const char *fmt_patch_subject_prefix = \"PATCH\";\n static const char *fmt_pretty;\n\n@@ -138,7 +139,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n \tif (source)\n \t\trev->show_source = 1;\n\n-\tif (mailmap) {\n+\tif (mailmap || use_mailmap) {\n \t\trev->mailmap = xcalloc(1, sizeof(struct string_list));\n \t\tread_mailmap(rev->mailmap, NULL);\n \t}\n@@ -358,6 +359,11 @@ static int git_log_config(const char *var, const char *value, void *cb)\n \t}\n \tif (!prefixcmp(var, \"color.decorate.\"))\n \t\treturn parse_decorate_color_config(var, 15, value);\n+\tif (!strcmp(var, \"log.mailmap\")) {\n+\t\tuse_mailmap = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (grep_config(var, value, cb) < 0)\n \t\treturn -1;\n \treturn git_diff_ui_config(var, value, cb);\ndiff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh\nindex e16187f..7d4d31c 100755\n--- a/t/t4203-mailmap.sh\n+++ b/t/t4203-mailmap.sh\n@@ -255,6 +255,21 @@ test_expect_success 'Log output with --use-mailmap' '\n '\n\n cat >expect <<\\EOF\n+Author: CTO <cto@company.xx>\n+Author: Santa Claus <santa.claus@northpole.xx>\n+Author: Santa Claus <santa.claus@northpole.xx>\n+Author: Other Author <other@author.xx>\n+Author: Other Author <other@author.xx>\n+Author: Some Dude <some@dude.xx>\n+Author: A U Thor <author@example.com>\n+EOF\n+\n+test_expect_success 'Log output with log.mailmap' '\n+\tgit -c log.mailmap=True log | grep Author >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+cat >expect <<\\EOF\n Author: Santa Claus <santa.claus@northpole.xx>\n Author: Santa Claus <santa.claus@northpole.xx>\n EOF\n@@ -263,6 +278,15 @@ test_expect_success 'Grep author with --use-mailmap' '\n \tgit log --use-mailmap --author Santa | grep Author >actual &&\n \ttest_cmp expect actual\n '\n+cat >expect <<\\EOF\n+Author: Santa Claus <santa.claus@northpole.xx>\n+Author: Santa Claus <santa.claus@northpole.xx>\n+EOF\n+\n+test_expect_success 'Grep author with log.mailmap' '\n+\tgit -c log.mailmap=True log --author Santa | grep Author >actual &&\n+\ttest_cmp expect actual\n+'\n\n >expect\n\n--\n1.7.9.5\n"},{"id":"205442","messageId":"7v8v8ppf6f.fsf@alter.siamese.dyndns.org","threadId":"32444","inReplyTo":"1356195512-4846-3-git-send-email-apelisse@gmail.com","subject":"Re: [PATCH 2/2] log: add log.mailmap configuration option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-23T04:26:16Z","receivedAt":"2012-12-23T04:26:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antoine Pelisse <apelisse@gmail.com> writes:\n\n> This patch provides a new configuration option 'log.mailmap' to\n> automatically use the --use-mailmap option from git-show, git-log and\n> git-whatchanged commands.\n>\n> Signed-off-by: Antoine Pelisse <apelisse@gmail.com>\n> ---\n> I'm wondering if it would be needed to add a no-use-mailmap option to\n> log command so that it can cancel this configuration option.\n\nThe usual way for adding a new feature is to add a --enable-feature\nlong-option without any configuration variable to let users try it\nout in the field, and then add the configuration to let it be\ndefault for users who opt in.  The first step should also allow a\ncommand line option to disable (which should come for free if you\nuse parse-options API correctly).\n\n>  Documentation/config.txt |    4 ++++\n>  builtin/log.c            |    8 +++++++-\n>  t/t4203-mailmap.sh       |   24 ++++++++++++++++++++++++\n>  3 files changed, 35 insertions(+), 1 deletion(-)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index bf8f911..226362a 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1509,6 +1509,10 @@ log.showroot::\n>  \tTools like linkgit:git-log[1] or linkgit:git-whatchanged[1], which\n>  \tnormally hide the root commit will now show it. True by default.\n>\n> +log.mailmap::\n> +\tIf true, makes linkgit:git-log[1], linkgit:git-show[1], and\n> +\tlinkgit:git-whatchanged[1] assume `--use-mailmap`.\n> +\n>  mailmap.file::\n>  \tThe location of an augmenting mailmap file. The default\n>  \tmailmap, located in the root of the repository, is loaded\n> diff --git a/builtin/log.c b/builtin/log.c\n> index d2bd8ce..f6936ff 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -31,6 +31,7 @@ static int default_abbrev_commit;\n>  static int default_show_root = 1;\n>  static int decoration_style;\n>  static int decoration_given;\n> +static int use_mailmap;\n>  static const char *fmt_patch_subject_prefix = \"PATCH\";\n>  static const char *fmt_pretty;\n>\n> @@ -138,7 +139,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n>  \tif (source)\n>  \t\trev->show_source = 1;\n>\n> -\tif (mailmap) {\n> +\tif (mailmap || use_mailmap) {\n>  \t\trev->mailmap = xcalloc(1, sizeof(struct string_list));\n>  \t\tread_mailmap(rev->mailmap, NULL);\n>  \t}\n> @@ -358,6 +359,11 @@ static int git_log_config(const char *var, const char *value, void *cb)\n>  \t}\n>  \tif (!prefixcmp(var, \"color.decorate.\"))\n>  \t\treturn parse_decorate_color_config(var, 15, value);\n> +\tif (!strcmp(var, \"log.mailmap\")) {\n> +\t\tuse_mailmap = git_config_bool(var, value);\n> +\t\treturn 0;\n> +\t}\n> +\n>  \tif (grep_config(var, value, cb) < 0)\n>  \t\treturn -1;\n>  \treturn git_diff_ui_config(var, value, cb);\n> diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh\n> index e16187f..7d4d31c 100755\n> --- a/t/t4203-mailmap.sh\n> +++ b/t/t4203-mailmap.sh\n> @@ -255,6 +255,21 @@ test_expect_success 'Log output with --use-mailmap' '\n>  '\n>\n>  cat >expect <<\\EOF\n> +Author: CTO <cto@company.xx>\n> +Author: Santa Claus <santa.claus@northpole.xx>\n> +Author: Santa Claus <santa.claus@northpole.xx>\n> +Author: Other Author <other@author.xx>\n> +Author: Other Author <other@author.xx>\n> +Author: Some Dude <some@dude.xx>\n> +Author: A U Thor <author@example.com>\n> +EOF\n> +\n> +test_expect_success 'Log output with log.mailmap' '\n> +\tgit -c log.mailmap=True log | grep Author >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +cat >expect <<\\EOF\n>  Author: Santa Claus <santa.claus@northpole.xx>\n>  Author: Santa Claus <santa.claus@northpole.xx>\n>  EOF\n> @@ -263,6 +278,15 @@ test_expect_success 'Grep author with --use-mailmap' '\n>  \tgit log --use-mailmap --author Santa | grep Author >actual &&\n>  \ttest_cmp expect actual\n>  '\n> +cat >expect <<\\EOF\n> +Author: Santa Claus <santa.claus@northpole.xx>\n> +Author: Santa Claus <santa.claus@northpole.xx>\n> +EOF\n> +\n> +test_expect_success 'Grep author with log.mailmap' '\n> +\tgit -c log.mailmap=True log --author Santa | grep Author >actual &&\n> +\ttest_cmp expect actual\n> +'\n>\n>  >expect\n>\n> --\n> 1.7.9.5\n"},{"id":"205498","messageId":"7vlickpz81.fsf@alter.siamese.dyndns.org","threadId":"32444","inReplyTo":"7v8v8ppf6f.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] log: add log.mailmap configuration option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-26T16:14:38Z","receivedAt":"2012-12-26T16:14:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Antoine Pelisse <apelisse@gmail.com> writes:\n>\n>> I'm wondering if it would be needed to add a no-use-mailmap option to\n>> log command so that it can cancel this configuration option.\n>\n> The usual way for adding a new feature is to add a --enable-feature\n> long-option without any configuration variable to let users try it\n> out in the field, and then add the configuration to let it be\n> default for users who opt in.  The first step should also allow a\n> command line option to disable (which should come for free if you\n> use parse-options API correctly).\n\nIt should be sufficient to squash something like this in.  Use the\nconfigured value, if available, to initialize the existing \"mailmap\"\nvariable, which is in turn updated from the command line option with\neither --use-mailmap or --no-use-mailmap.  What is left in \"mailmap\"\nafter the command line parsing returns is what the user told us to\nuse.\n\nThanks.\n\n builtin/log.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex f6936ff..16e6520 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -31,7 +31,7 @@ static int default_abbrev_commit;\n static int default_show_root = 1;\n static int decoration_style;\n static int decoration_given;\n-static int use_mailmap;\n+static int use_mailmap_config;\n static const char *fmt_patch_subject_prefix = \"PATCH\";\n static const char *fmt_pretty;\n \n@@ -107,6 +107,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n \t\tOPT_END()\n \t};\n \n+\tmailmap = use_mailmap_config;\n \targc = parse_options(argc, argv, prefix,\n \t\t\t     builtin_log_options, builtin_log_usage,\n \t\t\t     PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN |\n@@ -139,7 +140,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n \tif (source)\n \t\trev->show_source = 1;\n \n-\tif (mailmap || use_mailmap) {\n+\tif (mailmap) {\n \t\trev->mailmap = xcalloc(1, sizeof(struct string_list));\n \t\tread_mailmap(rev->mailmap, NULL);\n \t}\n@@ -360,7 +361,7 @@ static int git_log_config(const char *var, const char *value, void *cb)\n \tif (!prefixcmp(var, \"color.decorate.\"))\n \t\treturn parse_decorate_color_config(var, 15, value);\n \tif (!strcmp(var, \"log.mailmap\")) {\n-\t\tuse_mailmap = git_config_bool(var, value);\n+\t\tuse_mailmap_config = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n \n"},{"id":"205499","messageId":"CALWbr2zZ=SUns25RMXjV9NNh3vfGwpdDzTFbD=ytDUBJkhNp6w@mail.gmail.com","threadId":"32444","inReplyTo":"7vlickpz81.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] log: add log.mailmap configuration option","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2012-12-26T16:42:18Z","receivedAt":"2012-12-26T16:42:18Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"I was planning to send you a fix pretty close to that,\n\nThanks a lot Junio!\n\nOn Wed, Dec 26, 2012 at 5:14 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Antoine Pelisse <apelisse@gmail.com> writes:\n>>\n>>> I'm wondering if it would be needed to add a no-use-mailmap option to\n>>> log command so that it can cancel this configuration option.\n>>\n>> The usual way for adding a new feature is to add a --enable-feature\n>> long-option without any configuration variable to let users try it\n>> out in the field, and then add the configuration to let it be\n>> default for users who opt in.  The first step should also allow a\n>> command line option to disable (which should come for free if you\n>> use parse-options API correctly).\n>\n> It should be sufficient to squash something like this in.  Use the\n> configured value, if available, to initialize the existing \"mailmap\"\n> variable, which is in turn updated from the command line option with\n> either --use-mailmap or --no-use-mailmap.  What is left in \"mailmap\"\n> after the command line parsing returns is what the user told us to\n> use.\n>\n> Thanks.\n>\n>  builtin/log.c | 7 ++++---\n>  1 file changed, 4 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/log.c b/builtin/log.c\n> index f6936ff..16e6520 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -31,7 +31,7 @@ static int default_abbrev_commit;\n>  static int default_show_root = 1;\n>  static int decoration_style;\n>  static int decoration_given;\n> -static int use_mailmap;\n> +static int use_mailmap_config;\n>  static const char *fmt_patch_subject_prefix = \"PATCH\";\n>  static const char *fmt_pretty;\n>\n> @@ -107,6 +107,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n>                 OPT_END()\n>         };\n>\n> +       mailmap = use_mailmap_config;\n>         argc = parse_options(argc, argv, prefix,\n>                              builtin_log_options, builtin_log_usage,\n>                              PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN |\n> @@ -139,7 +140,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n>         if (source)\n>                 rev->show_source = 1;\n>\n> -       if (mailmap || use_mailmap) {\n> +       if (mailmap) {\n>                 rev->mailmap = xcalloc(1, sizeof(struct string_list));\n>                 read_mailmap(rev->mailmap, NULL);\n>         }\n> @@ -360,7 +361,7 @@ static int git_log_config(const char *var, const char *value, void *cb)\n>         if (!prefixcmp(var, \"color.decorate.\"))\n>                 return parse_decorate_color_config(var, 15, value);\n>         if (!strcmp(var, \"log.mailmap\")) {\n> -               use_mailmap = git_config_bool(var, value);\n> +               use_mailmap_config = git_config_bool(var, value);\n>                 return 0;\n>         }\n>\n"},{"id":"205502","messageId":"7vr4mcobpu.fsf@alter.siamese.dyndns.org","threadId":"32444","inReplyTo":"1356195512-4846-2-git-send-email-apelisse@gmail.com","subject":"Re: [PATCH 1/2] log: grep author/committer using mailmap","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-26T19:27:41Z","receivedAt":"2012-12-26T19:27:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antoine Pelisse <apelisse@gmail.com> writes:\n\n> Currently mailmap can be used to display log authors and committers\n> but there no way to use mailmap to find commits with mapped values.\n>\n> This commit allows those commands to work:\n>\n>     git log --use-mailmap --author mapped_name_or_email\n>     git log --use-mailmap --committer mapped_name_or_email\n>\n> Of course it only works if the --use-mailmap option is used.\n>\n> Signed-off-by: Antoine Pelisse <apelisse@gmail.com>\n> ---\n> I probably missed something but I didn't find the connection with\n> commit 2d10c55. Let me know if I went the wrong direction.\n>\n>  revision.c         |   53 ++++++++++++++++++++++++++++++++++++++++++++++++++++\n>  t/t4203-mailmap.sh |   18 ++++++++++++++++++\n>  2 files changed, 71 insertions(+)\n>\n> diff --git a/revision.c b/revision.c\n> index 95d21e6..fb9fd41 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -13,6 +13,7 @@\n>  #include \"decorate.h\"\n>  #include \"log-tree.h\"\n>  #include \"string-list.h\"\n> +#include \"mailmap.h\"\n>\n>  volatile show_early_output_fn_t show_early_output;\n>\n> @@ -2219,6 +2220,50 @@ static int rewrite_parents(struct rev_info *revs, struct commit *commit)\n>  \treturn 0;\n>  }\n>\n> +static int commit_rewrite_authors(struct strbuf *buf, const char *what, struct string_list *mailmap)\n> +{\n> +\tchar *author, *endp;\n> +\tsize_t len;\n> +\tstruct strbuf name = STRBUF_INIT;\n> +\tstruct strbuf mail = STRBUF_INIT;\n> +\tstruct ident_split ident;\n> +\n> +\tauthor = strstr(buf->buf, what);\n> +\tif (!author)\n> +\t\tgoto error;\n\nThis does not stop at the end of the header part and would match a\nrandom line in the log message that happens to begin with \"author \";\nis this something we would worry about, or would we leave it to \"fsck\"?\n\n> +\tauthor += strlen(what);\n> +\tendp = strstr(author, \"\\n\");\n\nUsing strchr(author, '\\n') would feel more natural.  Also rename\n\"author\" to \"person\" or something, as you would be using this\nfunction for the committer information as well?\n\n> +\tif (!endp)\n> +\t\tgoto error;\n> +\n> +\tlen = endp - author;\n> +\n> +\tif (split_ident_line(&ident, author, len)) {\n> +\terror:\n> +\t\tstrbuf_release(&name);\n> +\t\tstrbuf_release(&mail);\n> +\n> +\t\treturn 1;\n\nWe usually signal error by returning a negative integer.  It does\nnot matter too much in this case as no callers seem to check the\nreturn value from this function, though.\n\n> +\t}\n> +\n> +\tstrbuf_add(&name, ident.name_begin, ident.name_end - ident.name_begin);\n> +\tstrbuf_add(&mail, ident.mail_begin, ident.mail_end - ident.mail_begin);\n> +\n> +\tmap_user(mailmap, &mail, &name);\n> +\n> +\tstrbuf_addf(&name, \" <%s>\", mail.buf);\n> +\n> +\tstrbuf_splice(buf, ident.name_begin - buf->buf,\n> +\t\t      ident.mail_end - ident.name_begin + 1,\n> +\t\t      name.buf, name.len);\n\nWould it give us better performance if we splice only when\nmap_user() tells us that we actually rewrote the ident?\n\n> +\tstrbuf_release(&name);\n> +\tstrbuf_release(&mail);\n> +\n> +\treturn 0;\n> +}\n> +\n>  static int commit_match(struct commit *commit, struct rev_info *opt)\n>  {\n>  \tint retval;\n> @@ -2237,6 +2282,14 @@ static int commit_match(struct commit *commit, struct rev_info *opt)\n>  \tif (buf.len)\n>  \t\tstrbuf_addstr(&buf, commit->buffer);\n>\n> +\tif (opt->mailmap) {\n> +\t\tif (!buf.len)\n> +\t\t\tstrbuf_addstr(&buf, commit->buffer);\n> +\n> +\t\tcommit_rewrite_authors(&buf, \"\\nauthor \", opt->mailmap);\n> +\t\tcommit_rewrite_authors(&buf, \"\\ncommitter \", opt->mailmap);\n> +\t}\n> +\n>  \t/* Append \"fake\" message parts as needed */\n>  \tif (opt->show_notes) {\n>  \t\tif (!buf.len)\n> diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh\n> index db043dc..e16187f 100755\n> --- a/t/t4203-mailmap.sh\n> +++ b/t/t4203-mailmap.sh\n> @@ -248,11 +248,29 @@ Author: Other Author <other@author.xx>\n>  Author: Some Dude <some@dude.xx>\n>  Author: A U Thor <author@example.com>\n>  EOF\n> +\n>  test_expect_success 'Log output with --use-mailmap' '\n>  \tgit log --use-mailmap | grep Author >actual &&\n>  \ttest_cmp expect actual\n>  '\n>\n> +cat >expect <<\\EOF\n> +Author: Santa Claus <santa.claus@northpole.xx>\n> +Author: Santa Claus <santa.claus@northpole.xx>\n> +EOF\n> +\n> +test_expect_success 'Grep author with --use-mailmap' '\n> +\tgit log --use-mailmap --author Santa | grep Author >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +>expect\n> +\n> +test_expect_success 'Only grep replaced author with --use-mailmap' '\n> +\tgit log --use-mailmap --author \"<cto@coompany.xx>\" >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>  # git blame\n>  cat >expect <<\\EOF\n>  ^OBJI (A U Thor     DATE 1) one\n> --\n> 1.7.9.5\n"},{"id":"205513","messageId":"CALWbr2xW6r5ysJ8KQZa1eGYehG8ZbEp6K+s5JkG2goK9ef7rcA@mail.gmail.com","threadId":"32444","inReplyTo":"7vr4mcobpu.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] log: grep author/committer using mailmap","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2012-12-26T21:12:16Z","receivedAt":"2012-12-26T21:12:16Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":">>\n>> +static int commit_rewrite_authors(struct strbuf *buf, const char *what, struct string_list *mailmap)\n>> +{\n>> +     char *author, *endp;\n>> +     size_t len;\n>> +     struct strbuf name = STRBUF_INIT;\n>> +     struct strbuf mail = STRBUF_INIT;\n>> +     struct ident_split ident;\n>> +\n>> +     author = strstr(buf->buf, what);\n>> +     if (!author)\n>> +             goto error;\n>\n> This does not stop at the end of the header part and would match a\n> random line in the log message that happens to begin with \"author \";\n> is this something we would worry about, or would we leave it to \"fsck\"?\n\nThe only worrying case would be:\n - commit doesn't have \"\\nauthor\" in the header (can that happen\nwithout corruption?)\n - commit has \"\\nauthor\" in the commit log\n - This line from commit log contains an <email> (split_ident_line works)\nThen, I guess it's going to replace the name in the commit log.\n\nOtherwise, it would not replace anything, as there is no author to\nreplace anyway.\n\nIt looks like most mechanisms using mailmap would have the same issue.\n\n>> +     author += strlen(what);\n>> +     endp = strstr(author, \"\\n\");\n>\n> Using strchr(author, '\\n') would feel more natural.  Also rename\n> \"author\" to \"person\" or something, as you would be using this\n> function for the committer information as well?\n\nBoth fixed\n\n>> +     if (!endp)\n>> +             goto error;\n>> +\n>> +     len = endp - author;\n>> +\n>> +     if (split_ident_line(&ident, author, len)) {\n>> +     error:\n>> +             strbuf_release(&name);\n>> +             strbuf_release(&mail);\n>> +\n>> +             return 1;\n>\n> We usually signal error by returning a negative integer.  It does\n> not matter too much in this case as no callers seem to check the\n> return value from this function, though.\n\nFixed, or would you rather see it `void` ?\n\n>> +     }\n>> +\n>> +     strbuf_add(&name, ident.name_begin, ident.name_end - ident.name_begin);\n>> +     strbuf_add(&mail, ident.mail_begin, ident.mail_end - ident.mail_begin);\n>> +\n>> +     map_user(mailmap, &mail, &name);\n>> +\n>> +     strbuf_addf(&name, \" <%s>\", mail.buf);\n>> +\n>> +     strbuf_splice(buf, ident.name_begin - buf->buf,\n>> +                   ident.mail_end - ident.name_begin + 1,\n>> +                   name.buf, name.len);\n>\n> Would it give us better performance if we splice only when\n> map_user() tells us that we actually rewrote the ident?\n\nMy intuition was that the cost of splice belongs to \"memoving\", when the\nsize is different. Yet, Fixed, as it removes two copies.\n"},{"id":"205515","messageId":"7vy5gkmr53.fsf@alter.siamese.dyndns.org","threadId":"32444","inReplyTo":"CALWbr2xW6r5ysJ8KQZa1eGYehG8ZbEp6K+s5JkG2goK9ef7rcA@mail.gmail.com","subject":"Re: [PATCH 1/2] log: grep author/committer using mailmap","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-26T21:37:28Z","receivedAt":"2012-12-26T21:37:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antoine Pelisse <apelisse@gmail.com> writes:\n\n>>>\n>>> +static int commit_rewrite_authors(struct strbuf *buf, const char *what, struct string_list *mailmap)\n>>> +{\n>>> +     char *author, *endp;\n>>> +     size_t len;\n>>> +     struct strbuf name = STRBUF_INIT;\n>>> +     struct strbuf mail = STRBUF_INIT;\n>>> +     struct ident_split ident;\n>>> +\n>>> +     author = strstr(buf->buf, what);\n>>> +     if (!author)\n>>> +             goto error;\n>>\n>> This does not stop at the end of the header part and would match a\n>> random line in the log message that happens to begin with \"author \";\n>> is this something we would worry about, or would we leave it to \"fsck\"?\n>\n> The only worrying case would be:\n> ...\n\nYeah, that pretty much matches what I had in mind (the short answer:\nleave it to \"git fsck\").\n\n>> We usually signal error by returning a negative integer.  It does\n>> not matter too much in this case as no callers seem to check the\n>> return value from this function, though.\n>\n> Fixed, or would you rather see it `void` ?\n\nJust like you can take advantage of map_user() that signals the\ncaller if it did anything to optimize this function, in the longer\nrun, it may help the (future) callers of this function if it gave \"I\ndid something\" vs \"I left it intact\".  In the particular case of\nthis function, the \"error\" cases fall into the latter (it merely\nexplains why it left it intact, and there is no sensible error\nrecovery the caller _could_ do in any case) and I think it is not\nnecessary to differenciate between \"Returned as-is because there is\nno mapping\" and \"Returned as-is because I couldn't parse the\ncommit\".\n\nSo \"return 0 when it didn't do anything, return 1 when it rewrote\"\nfeels good enough, at least to me.\n\n>>> +     }\n>>> +\n>>> +     strbuf_add(&name, ident.name_begin, ident.name_end - ident.name_begin);\n>>> +     strbuf_add(&mail, ident.mail_begin, ident.mail_end - ident.mail_begin);\n>>> +\n>>> +     map_user(mailmap, &mail, &name);\n>>> +\n>>> +     strbuf_addf(&name, \" <%s>\", mail.buf);\n>>> +\n>>> +     strbuf_splice(buf, ident.name_begin - buf->buf,\n>>> +                   ident.mail_end - ident.name_begin + 1,\n>>> +                   name.buf, name.len);\n>>\n>> Would it give us better performance if we splice only when\n>> map_user() tells us that we actually rewrote the ident?\n>\n> My intuition was that the cost of splice belongs to \"memoving\", when the\n> size is different. Yet, Fixed, as it removes two copies.\n\nThanks.\n\nI wonder if we can further restructure the code so that it first\ninspects the existing buffer to see if it even needs to copy the\noriginal commit buffer into a \"strbuf only for grepping\".  If that\ncan be easily done, then we will save even more copying, I think.\n\nThe reason I alluded to revamping the grep API to get rid of the use\nof \"header grep\" mode in this codepath was exactly that.  We could:\n\n - change the command line parser for --author= and --committer= so\n   that these do not become part of the main \"grep\" expression.\n   Instead we keep them as separate grep expressions (one \"author\"\n   expression that OR'es the --author= options together, the other\n   for the --committer= options);\n\n - in this codepath, inspect the \"author\" and \"committer\" in the\n   commit object buffer, map them if necessary via the mailmap\n   mechanism into temporary buffers (that is different from the\n   \"buf\" in the commit_match() function), then run grep_buffer()\n   with the author and committer grep expressions we separated in\n   the previous step. Then we combine the results from \"author\" and\n   \"committer\" grep and the main grep_buffer() result ourselves in\n   this function.\n\nThat may essentially amount to going in the totally opposite\ndirection from what 2d10c55 (git log: Unify header_filter and\nmessage_filter into one., 2006-09-20) attempted to do.  We used to\nhave two grep expressions (one for header, the other one for body)\ncommit_match() runs grep_buffer() on and combined the results.\n2d10c55 merged them into one grep expression by introducing a term\nthat matches only header elements.  But we would instead split the\n\"header\" expression into \"author\" and \"committer\" expressions\n(making it three from one) if we go the above route.\n\nThat would eliminate the need to copy and rewrite the contents of\nthe commit object in this codepath, which may be a big win when\nnames and emails that need to be rewritten are minority cases.\n\nBut I suspect that is a much larger change.  If we can reduce the\namount of copies necessary without changing the code structure, that\nmay be enough to reduce the performance hit from this change.\n\nThanks.\n"},{"id":"205553","messageId":"1356622318-19523-1-git-send-email-apelisse@gmail.com","threadId":"32444","inReplyTo":"7vy5gkmr53.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2] log: grep author/committer using mailmap","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2012-12-27T15:31:58Z","receivedAt":"2012-12-27T15:31:58Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"Currently you can use mailmap to display log authors and committers\nbut you can't use the mailmap to find commits with mapped values.\n\nThis commit allows you to run:\n\n    git log --use-mailmap --author mapped_name_or_email\n    git log --use-mailmap --committer mapped_name_or_email\n\nOf course it only works if the --use-mailmap option is used.\n\nSigned-off-by: Antoine Pelisse <apelisse@gmail.com>\n---\n revision.c         |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++++\n t/t4203-mailmap.sh |   18 ++++++++++++++++++\n 2 files changed, 72 insertions(+)\n\ndiff --git a/revision.c b/revision.c\nindex 95d21e6..fa16b9d 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -13,6 +13,7 @@\n #include \"decorate.h\"\n #include \"log-tree.h\"\n #include \"string-list.h\"\n+#include \"mailmap.h\"\n \n volatile show_early_output_fn_t show_early_output;\n \n@@ -2219,6 +2220,51 @@ static int rewrite_parents(struct rev_info *revs, struct commit *commit)\n \treturn 0;\n }\n \n+static int commit_rewrite_person(struct strbuf *buf, const char *what, struct string_list *mailmap)\n+{\n+\tchar *person, *endp;\n+\tsize_t len;\n+\tstruct strbuf name = STRBUF_INIT;\n+\tstruct strbuf mail = STRBUF_INIT;\n+\tstruct ident_split ident;\n+\n+\tperson = strstr(buf->buf, what);\n+\tif (!person)\n+\t\tgoto left_intact;\n+\n+\tperson += strlen(what);\n+\tendp = strchr(person, '\\n');\n+\tif (!endp)\n+\t\tgoto left_intact;\n+\n+\tlen = endp - person;\n+\n+\tif (split_ident_line(&ident, person, len))\n+\t\tgoto left_intact;\n+\n+\tstrbuf_add(&name, ident.name_begin, ident.name_end - ident.name_begin);\n+\tstrbuf_add(&mail, ident.mail_begin, ident.mail_end - ident.mail_begin);\n+\n+\tif (map_user(mailmap, &mail, &name)) {\n+\t\tstrbuf_addf(&name, \" <%s>\", mail.buf);\n+\n+\t\tstrbuf_splice(buf, ident.name_begin - buf->buf,\n+\t\t\t      ident.mail_end - ident.name_begin + 1,\n+\t\t\t      name.buf, name.len);\n+\n+\t\tstrbuf_release(&name);\n+\t\tstrbuf_release(&mail);\n+\n+\t\treturn 1;\n+\t}\n+\n+left_intact:\n+\tstrbuf_release(&name);\n+\tstrbuf_release(&mail);\n+\n+\treturn 0;\n+}\n+\n static int commit_match(struct commit *commit, struct rev_info *opt)\n {\n \tint retval;\n@@ -2237,6 +2283,14 @@ static int commit_match(struct commit *commit, struct rev_info *opt)\n \tif (buf.len)\n \t\tstrbuf_addstr(&buf, commit->buffer);\n \n+\tif (opt->mailmap) {\n+\t\tif (!buf.len)\n+\t\t\tstrbuf_addstr(&buf, commit->buffer);\n+\n+\t\tcommit_rewrite_person(&buf, \"\\nauthor \", opt->mailmap);\n+\t\tcommit_rewrite_person(&buf, \"\\ncommitter \", opt->mailmap);\n+\t}\n+\n \t/* Append \"fake\" message parts as needed */\n \tif (opt->show_notes) {\n \t\tif (!buf.len)\ndiff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh\nindex db043dc..e16187f 100755\n--- a/t/t4203-mailmap.sh\n+++ b/t/t4203-mailmap.sh\n@@ -248,11 +248,29 @@ Author: Other Author <other@author.xx>\n Author: Some Dude <some@dude.xx>\n Author: A U Thor <author@example.com>\n EOF\n+\n test_expect_success 'Log output with --use-mailmap' '\n \tgit log --use-mailmap | grep Author >actual &&\n \ttest_cmp expect actual\n '\n \n+cat >expect <<\\EOF\n+Author: Santa Claus <santa.claus@northpole.xx>\n+Author: Santa Claus <santa.claus@northpole.xx>\n+EOF\n+\n+test_expect_success 'Grep author with --use-mailmap' '\n+\tgit log --use-mailmap --author Santa | grep Author >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+>expect\n+\n+test_expect_success 'Only grep replaced author with --use-mailmap' '\n+\tgit log --use-mailmap --author \"<cto@coompany.xx>\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n # git blame\n cat >expect <<\\EOF\n ^OBJI (A U Thor     DATE 1) one\n-- \n1.7.9.5\n"},{"id":"205559","messageId":"7v1uebmizx.fsf@alter.siamese.dyndns.org","threadId":"32444","inReplyTo":"1356622318-19523-1-git-send-email-apelisse@gmail.com","subject":"Re: [PATCH v2] log: grep author/committer using mailmap","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-27T18:45:38Z","receivedAt":"2012-12-27T18:45:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antoine Pelisse <apelisse@gmail.com> writes:\n\n> Currently you can use mailmap to display log authors and committers\n> but you can't use the mailmap to find commits with mapped values.\n>\n> This commit allows you to run:\n>\n>     git log --use-mailmap --author mapped_name_or_email\n>     git log --use-mailmap --committer mapped_name_or_email\n>\n> Of course it only works if the --use-mailmap option is used.\n>\n> Signed-off-by: Antoine Pelisse <apelisse@gmail.com>\n> ---\n\nThanks.  I'll queue this on top.\n\n-- >8 --\nSubject: [PATCH] log --use-mailmap: optimize for cases without --author/--committer search\n\nWhen we taught the commit_match() mechanism to pay attention to the\nnew --use-mailmap option, we started to unconditionally copy the\ncommit object to a temporary buffer, just in case we need the author\nand committer lines updated via the mailmap mechanism.\n\nIt turns out that this has a rather unpleasant performance\nimplications.  In the linux kernel repository, running\n\n  $ git log --author='Junio C Hamano' --pretty=short >/dev/null\n\nunder /usr/bin/time, with and without --use-mailmap (the .mailmap\nfile is 118 entries long, the particular author does not appear in\nit), cost (with warm cache):\n\n  [without --use-mailmap]\n  5.34user 0.25system 0:05.60elapsed 100%CPU (0avgtext+0avgdata 2004832maxresident)k\n  0inputs+0outputs (0major+137600minor)pagefaults 0swaps\n\n  [with --use-mailmap]\n  6.87user 0.24system 0:07.11elapsed 99%CPU (0avgtext+0avgdata 2006352maxresident)k\n  0inputs+0outputs (0major+137696minor)pagefaults 0swaps\n\nwhich is with about 29% overhead.  The command is doing extra work,\nso the extra cost may be justified.\n\nBut it is inexcusable to pay the cost when we do not need\nauthor/committer match.  In the same repository,\n\n  $ git log --grep='fix menuconfig on debian lenny' --pretty=short >/dev/null\n\nshows very similar numbers as the above:\n\n  [without --use-mailmap]\n  5.30user 0.24system 0:05.55elapsed 99%CPU (0avgtext+0avgdata 2004896maxresident)k\n  0inputs+0outputs (0major+137604minor)pagefaults 0swaps\n\n  [with --use-mailmap]\n  6.82user 0.26system 0:07.07elapsed 100%CPU (0avgtext+0avgdata 2006352maxresident)k\n  0inputs+0outputs (0major+137697minor)pagefaults 0swaps\n\nThe latter case is an unnecessary performance regression.  We may\nwant to _show_ the result with mailmap applied, but we do not have\nto copy and rewrite the author/committer of all commits we try to\nmatch if we do not query for these fields.\n\nTrivially optimize this performace regression by limiting the\nrewrites for only when we are matching with author/committer fields.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n revision.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/revision.c b/revision.c\nindex fa16b9d..56b72f7 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2283,7 +2283,7 @@ static int commit_match(struct commit *commit, struct rev_info *opt)\n \tif (buf.len)\n \t\tstrbuf_addstr(&buf, commit->buffer);\n \n-\tif (opt->mailmap) {\n+\tif (opt->grep_filter.header_list && opt->mailmap) {\n \t\tif (!buf.len)\n \t\t\tstrbuf_addstr(&buf, commit->buffer);\n \n-- \n1.8.1.rc3.221.g8d09d94\n"},{"id":"205560","messageId":"7vwqw3l49z.fsf@alter.siamese.dyndns.org","threadId":"32444","inReplyTo":"7v1uebmizx.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] log: grep author/committer using mailmap","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-27T18:48:56Z","receivedAt":"2012-12-27T18:48:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Thanks.  I'll queue this on top.\n>\n> -- >8 --\n> Subject: [PATCH] log --use-mailmap: optimize for cases without --author/--committer search\n\nAnd this I will *not* queue further on top.\n\n-- >8 --\nSubject: [PATCH] [DO NOT USE] log --use-mailmap --author/--committer: further\n optimize identity rewriting\n\nWe used to always allocate a temporary buffer to search for\nauthor/committer names even when the author/committer does not\nappear in the mailmap.  Update the logic to do the allocation\non demand to reduce needless malloc()/free() calls.\n\nIt turns out that this does not affect performance at all; all the\ntime is spent in map_user() which is a look-up in string_list, so\nlet's not use this patch in the production, but it illustrates an\ninteresting point.\n\nIn map_identities() function, we already know who the author\nrecorded in the commit is, either in \"author\" strbuf, or in buffer\nbetween [a_at..a_len], so we could grep_buffer() the author\nregexp(s) specified from the command line right there, and combine\nthat result with the main grep_buffer() done for the --grep= option\nat the end of the commit_match() function.\n\nThat may essentially amount to going in the totally opposite\ndirection from what 2d10c55 (git log: Unify header_filter and\nmessage_filter into one., 2006-09-20) attempted to do.  We used to\nhave two grep expressions (one for header, the other one for body)\ncommit_match() runs grep_buffer() on and combined the results.\n2d10c55 merged them into one grep expression by introducing a term\nthat matches only header elements.  But we would instead split the\n\"header\" expression into \"author\" and \"committer\" expressions\n(making it three from one) if we go that route.\n\nIn any case, I *think* the bottleneck is in map_user() so until that\nis solved, such an update (or this patch) is not very useful.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n revision.c | 95 +++++++++++++++++++++++++++++++++++++-------------------------\n 1 file changed, 57 insertions(+), 38 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 56b72f7..4b66598 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2220,49 +2220,73 @@ static int rewrite_parents(struct rev_info *revs, struct commit *commit)\n \treturn 0;\n }\n \n-static int commit_rewrite_person(struct strbuf *buf, const char *what, struct string_list *mailmap)\n+static void map_person(const char *buf, size_t len, const char *head, int headlen,\n+\t\t       struct strbuf *result, struct string_list *mailmap,\n+\t\t       int *matchofs, int *matchlen)\n {\n-\tchar *person, *endp;\n-\tsize_t len;\n+\tstruct ident_split ident;\n+\tconst char *endp, *person;\n \tstruct strbuf name = STRBUF_INIT;\n \tstruct strbuf mail = STRBUF_INIT;\n-\tstruct ident_split ident;\n \n-\tperson = strstr(buf->buf, what);\n+\tperson = memmem(buf, len, head, headlen);\n \tif (!person)\n-\t\tgoto left_intact;\n-\n-\tperson += strlen(what);\n+\t\treturn;\n+\tperson += headlen;\n \tendp = strchr(person, '\\n');\n \tif (!endp)\n-\t\tgoto left_intact;\n-\n-\tlen = endp - person;\n-\n-\tif (split_ident_line(&ident, person, len))\n-\t\tgoto left_intact;\n-\n+\t\treturn;\n+\t*matchofs = person - buf;\n+\t*matchlen = endp - person;\n+\tif (split_ident_line(&ident, person, *matchlen))\n+\t\treturn;\n \tstrbuf_add(&name, ident.name_begin, ident.name_end - ident.name_begin);\n \tstrbuf_add(&mail, ident.mail_begin, ident.mail_end - ident.mail_begin);\n-\n-\tif (map_user(mailmap, &mail, &name)) {\n-\t\tstrbuf_addf(&name, \" <%s>\", mail.buf);\n-\n-\t\tstrbuf_splice(buf, ident.name_begin - buf->buf,\n-\t\t\t      ident.mail_end - ident.name_begin + 1,\n-\t\t\t      name.buf, name.len);\n-\n-\t\tstrbuf_release(&name);\n-\t\tstrbuf_release(&mail);\n-\n-\t\treturn 1;\n-\t}\n-\n-left_intact:\n+\tif (map_user(mailmap, &mail, &name))\n+\t\tstrbuf_addf(result, \"%s <%s>\", name.buf, mail.buf);\n \tstrbuf_release(&name);\n \tstrbuf_release(&mail);\n+}\n \n-\treturn 0;\n+static void map_identities(struct strbuf *buf, const char *buffer, struct string_list *mailmap)\n+{\n+\tconst char *eob;\n+\tstruct strbuf author = STRBUF_INIT;\n+\tstruct strbuf committer = STRBUF_INIT;\n+\tint a_at = -1, a_len, c_at = -1, c_len;\n+\n+\teob = strstr(buffer, \"\\n\\n\");\n+\tif (!eob)\n+\t\teob = buffer + strlen(buffer);\n+\tmap_person(buffer, eob - buffer, \"\\nauthor \", 8, &author, mailmap,\n+\t\t   &a_at, &a_len);\n+\tmap_person(buffer, eob - buffer, \"\\ncommitter \", 11, &committer, mailmap,\n+\t\t   &c_at, &c_len);\n+\tif (!author.len && !committer.len)\n+\t\tgoto done;\n+\n+\t/* Now, we know we need rewriting */\n+\tif (!buf->len)\n+\t\tstrbuf_addstr(buf, buffer);\n+\n+\tif (c_at < 0 || !committer.len) {\n+\t\tstrbuf_splice(buf, a_at, a_len, author.buf, author.len);\n+\t} else if (a_at < 0 || !author.len) {\n+\t\tstrbuf_splice(buf, c_at, c_len, committer.buf, committer.len);\n+\t} else if (a_at < c_at) {\n+\t\tstrbuf_splice(buf, c_at, c_len, committer.buf, committer.len);\n+\t\tstrbuf_splice(buf, a_at, a_len, author.buf, author.len);\n+\t} else {\n+\t\t/*\n+\t\t * The commit records committer before the author, which is malformed,\n+\t\t * which we may want to warn about.\n+\t\t */\n+\t\tstrbuf_splice(buf, a_at, a_len, author.buf, author.len);\n+\t\tstrbuf_splice(buf, c_at, c_len, committer.buf, committer.len);\n+\t}\n+ done:\n+\tstrbuf_release(&author);\n+\tstrbuf_release(&committer);\n }\n \n static int commit_match(struct commit *commit, struct rev_info *opt)\n@@ -2283,13 +2307,8 @@ static int commit_match(struct commit *commit, struct rev_info *opt)\n \tif (buf.len)\n \t\tstrbuf_addstr(&buf, commit->buffer);\n \n-\tif (opt->grep_filter.header_list && opt->mailmap) {\n-\t\tif (!buf.len)\n-\t\t\tstrbuf_addstr(&buf, commit->buffer);\n-\n-\t\tcommit_rewrite_person(&buf, \"\\nauthor \", opt->mailmap);\n-\t\tcommit_rewrite_person(&buf, \"\\ncommitter \", opt->mailmap);\n-\t}\n+\tif (opt->grep_filter.header_list && opt->mailmap)\n+\t\tmap_identities(&buf, commit->buffer, opt->mailmap);\n \n \t/* Append \"fake\" message parts as needed */\n \tif (opt->show_notes) {\n-- \n1.8.1.rc3.221.g8d09d94\n"},{"id":"205595","messageId":"CALWbr2y63L0-ykdUNGqUOb0LhG=vpXGRcb1KkvssEZmKFJEGeQ@mail.gmail.com","threadId":"32444","inReplyTo":"7vwqw3l49z.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] log: grep author/committer using mailmap","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2012-12-28T18:00:50Z","receivedAt":"2012-12-28T18:00:50Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"Actually, gprof seems to be unhappy about the number of call to\nstrbuf_grow() in map_user() (25% of the time spent in map_user() is\nspent in strbuf_grow()).\n\nThat probably comes from the repeated call to strbuf_addch() when\nlowering the email address.\nAt this point, we are also copying the '\\0' for every char we add,\ndoubling the copy.\nThis may not be much of a difference, but it seems to be called 15\nmillions times when running:\n$ git log --author='Junio C Hamano' --use-mailmap\n\nMaybe we should come up with another way to lower this email address afterall.\n"},{"id":"205596","messageId":"7va9syj9v1.fsf@alter.siamese.dyndns.org","threadId":"32444","inReplyTo":"CALWbr2y63L0-ykdUNGqUOb0LhG=vpXGRcb1KkvssEZmKFJEGeQ@mail.gmail.com","subject":"Re: [PATCH v2] log: grep author/committer using mailmap","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-28T18:43:30Z","receivedAt":"2012-12-28T18:43:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antoine Pelisse <apelisse@gmail.com> writes:\n\n> Actually, gprof seems to be unhappy about the number of call to\n> strbuf_grow() in map_user() (25% of the time spent in map_user() is\n> spent in strbuf_grow()).\n>\n> That probably comes from the repeated call to strbuf_addch() when\n> lowering the email address.\n\nThis is about your rewritten implementation that hasn't escaped to\nthe general public but sitting in 'next', right?\n\nTwo things that immediately come to mind are:\n\n - initialization of lowermail can use strbuf_init() instead;\n - downcasing can be done in place, i.e. \"lowermail.buf[i] = ...\".\n"},{"id":"205606","messageId":"CALWbr2xm1CR-dCCV8WjfHVRCrqehRLT=PdpgK4pQG505d8pNmw@mail.gmail.com","threadId":"32444","inReplyTo":"7va9syj9v1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] log: grep author/committer using mailmap","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2012-12-28T20:37:42Z","receivedAt":"2012-12-28T20:37:42Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"> This is about your rewritten implementation that hasn't escaped to\n> the general public but sitting in 'next', right?\n>\n> Two things that immediately come to mind are:\n>\n>  - initialization of lowermail can use strbuf_init() instead;\n>  - downcasing can be done in place, i.e. \"lowermail.buf[i] = ...\".\n\nYep,\nI don't think it's merged to 'next', I will squash those changes appropriately.\n"}]}