{"thread":{"id":"56738","subject":"[PoC] mailmap and trailers","startedAt":"2021-10-19T17:31:19Z","lastAt":"2021-10-20T19:22:00Z","messageCount":2,"participants":["Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"439036","messageId":"YW8A5FznqLYs7MqH@coredump.intra.peff.net","threadId":"56738","inReplyTo":null,"subject":"[PoC] mailmap and trailers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-19T17:31:16Z","receivedAt":"2021-10-19T17:31:19Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The notion came up at the virtual contrib summit that we mailmap\nauthor/committer names, but not usually idents in trailers. We do for\n\"git shortlog --group=trailer\", but not in say, \"git log\", which is\nwhere most people will see them.\n\nSo here's a proof-of-concept at mapping idents in trailers for git-log.\nIt does enough that you can try:\n\n  $ git log --format='%(trailers)' >orig\n  $ git log --format='%(trailers:mailmap)' >mapped\n  $ diff -u orig mapped\n\nwhich does show the expected differences for git.git (e.g., people's\nSigned-off-by trailers getting adjusted for their current emails).\n\nOne of the main things I wondered was the speed difference. The first\ncommand runs in about 0.5s for me, and the second in about 3.5s. So\nthere's a pretty big cost, but perhaps acceptable in the context of\ninteractive use. I didn't profile much. I'd guess some of the time is\njust that parsing and handling individual trailers at all is a bit slow\n(so it's on par with %(trailers:unfold), etc).\n\nWhat's here should work reliably, I think, but is pretty incomplete:\n\n  - it only works for %(trailers). We'd probably want a\n    \"--mailmap-trailers\" options to affect --pretty=medium, etc. That\n    code would probably go in pretty.c:pp_remainder()\n\n  - docs/tests conspicuously missing\n\n  - there's some ugliness with the ownership of the mailmap data\n    structure itself. I followed the existing pattern there. At the very\n    least these two spots should use the _same_ static string_list, but\n    probably it should be refactored to put it into pretty_print_context\n    or something.\n\nAnyway, here it is. I'm not sure if I'll push it forward more, but\ncertainly anybody interested is welcome to pick it up and run with it. I\nmostly wanted to show how much and where such code would be, and see how\nit performed.\n\n---\ndiff --git a/pretty.c b/pretty.c\nindex 73b5ead509..0afa70058f 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1167,6 +1167,7 @@ int format_set_trailers_options(struct process_trailer_options *opts,\n \tfor (;;) {\n \t\tconst char *argval;\n \t\tsize_t arglen;\n+\t\tint bool_arg;\n \n \t\tif (**arg == ')')\n \t\t\tbreak;\n@@ -1200,6 +1201,15 @@ int format_set_trailers_options(struct process_trailer_options *opts,\n \t\t\tstrbuf_expand(kvsepbuf, fmt, strbuf_expand_literal_cb, NULL);\n \t\t\tfree(fmt);\n \t\t\topts->key_value_separator = kvsepbuf;\n+\t\t} else if (match_placeholder_bool_arg(*arg, \"mailmap\", arg, &bool_arg)) {\n+\t\t\tif (bool_arg) {\n+\t\t\t\t/* yuck but this is how mailmap_name() above does it */\n+\t\t\t\tstatic struct string_list mailmap = STRING_LIST_INIT_DUP;\n+\t\t\t\tread_mailmap(&mailmap);\n+\t\t\t\topts->mailmap = &mailmap;\n+\t\t\t} else {\n+\t\t\t\topts->mailmap = NULL;\n+\t\t\t}\n \t\t} else if (!match_placeholder_bool_arg(*arg, \"only\", arg, &opts->only_trailers) &&\n \t\t\t   !match_placeholder_bool_arg(*arg, \"unfold\", arg, &opts->unfold) &&\n \t\t\t   !match_placeholder_bool_arg(*arg, \"keyonly\", arg, &opts->key_only) &&\ndiff --git a/trailer.c b/trailer.c\nindex 7c7cb61a94..743fdd506d 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -6,6 +6,8 @@\n #include \"tempfile.h\"\n #include \"trailer.h\"\n #include \"list.h\"\n+#include \"mailmap.h\"\n+\n /*\n  * Copyright (c) 2013, 2014 Christian Couder <chriscool@tuxfamily.org>\n  */\n@@ -1138,6 +1140,29 @@ void trailer_info_release(struct trailer_info *info)\n \tfree(info->trailers);\n }\n \n+static int mailmap_value(struct string_list *mailmap,\n+\t\t\t struct strbuf *out, const struct strbuf *in)\n+{\n+\tconst char *mailbuf, *namebuf;\n+\tsize_t namelen, maillen;\n+\tstruct ident_split ident;\n+\n+\tif (split_ident_line(&ident, in->buf, in->len))\n+\t\treturn -1; /* not an ident */\n+\n+\tnamebuf = ident.name_begin;\n+\tnamelen = ident.name_end - ident.name_begin;\n+\tmailbuf = ident.mail_begin;\n+\tmaillen = ident.mail_end - ident.mail_begin;\n+\n+\tmap_user(mailmap, &mailbuf, &maillen, &namebuf, &namelen);\n+\tstrbuf_add(out, namebuf, namelen);\n+\tstrbuf_addstr(out, \" <\");\n+\tstrbuf_add(out, mailbuf, maillen);\n+\tstrbuf_addch(out, '>');\n+\treturn 0;\n+}\n+\n static void format_trailer_info(struct strbuf *out,\n \t\t\t\tconst struct trailer_info *info,\n \t\t\t\tconst struct process_trailer_options *opts)\n@@ -1148,7 +1173,7 @@ static void format_trailer_info(struct strbuf *out,\n \t/* If we want the whole block untouched, we can take the fast path. */\n \tif (!opts->only_trailers && !opts->unfold && !opts->filter &&\n \t    !opts->separator && !opts->key_only && !opts->value_only &&\n-\t    !opts->key_value_separator) {\n+\t    !opts->key_value_separator && !opts->mailmap) {\n \t\tstrbuf_add(out, info->trailer_start,\n \t\t\t   info->trailer_end - info->trailer_start);\n \t\treturn;\n@@ -1177,8 +1202,11 @@ static void format_trailer_info(struct strbuf *out,\n \t\t\t\t\telse\n \t\t\t\t\t\tstrbuf_addstr(out, \": \");\n \t\t\t\t}\n-\t\t\t\tif (!opts->key_only)\n-\t\t\t\t\tstrbuf_addbuf(out, &val);\n+\t\t\t\tif (!opts->key_only) {\n+\t\t\t\t\tif (!opts->mailmap ||\n+\t\t\t\t\t    mailmap_value(opts->mailmap, out, &val) < 0)\n+\t\t\t\t\t\tstrbuf_addbuf(out, &val);\n+\t\t\t\t}\n \t\t\t\tif (!opts->separator)\n \t\t\t\t\tstrbuf_addch(out, '\\n');\n \t\t\t}\ndiff --git a/trailer.h b/trailer.h\nindex 795d2fccfd..747d2a6bc6 100644\n--- a/trailer.h\n+++ b/trailer.h\n@@ -73,6 +73,7 @@ struct process_trailer_options {\n \tint no_divider;\n \tint key_only;\n \tint value_only;\n+\tstruct string_list *mailmap;\n \tconst struct strbuf *separator;\n \tconst struct strbuf *key_value_separator;\n \tint (*filter)(const struct strbuf *, void *);\n"},{"id":"439134","messageId":"xmqq4k9bpkml.fsf@gitster.g","threadId":"56738","inReplyTo":"YW8A5FznqLYs7MqH@coredump.intra.peff.net","subject":"Re: [PoC] mailmap and trailers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-20T19:21:54Z","receivedAt":"2021-10-20T19:22:00Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> What's here should work reliably, I think, but is pretty incomplete:\n>\n>   - it only works for %(trailers). We'd probably want a\n>     \"--mailmap-trailers\" options to affect --pretty=medium, etc. That\n>     code would probably go in pretty.c:pp_remainder()\n\nYup, that would make it much more useful.\n\n> Anyway, here it is. I'm not sure if I'll push it forward more, but\n> certainly anybody interested is welcome to pick it up and run with it. I\n> mostly wanted to show how much and where such code would be, and see how\n> it performed.\n\nNice.\n\nThanks.\n"}]}