{"thread":{"id":"61963","subject":"[PATCH 0/2] send-email: add --mailmap support","startedAt":"2024-08-16T23:06:33Z","lastAt":"2024-08-16T23:51:42Z","messageCount":8,"participants":["Jacob Keller","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"501165","messageId":"20240816-jk-send-email-mailmap-support-v1-0-68ca5b4a6078@gmail.com","threadId":"61963","inReplyTo":null,"subject":"[PATCH 0/2] send-email: add --mailmap support","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2024-08-16T23:06:22Z","receivedAt":"2024-08-16T23:06:33Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"I recently sent a series to enable mailmap support in format patch. The\ndiscussion led me to realize that the true problem we wanted solved is to\nmap addresses at send time, so that we do not accidentally include a dead\nmail address when sending an old change.\n\nInstead of worrying about what the formatted patch has, this series\nimplements support for mailmap at the send-email, which will translate all\naddresses, and not just the author/commit addresses for a patch, but also\nthe email for any trailers.o\n\nI considered also that it may be useful to make send-email read a mailmap\nfile from the identity config blocks, but have not figured out how to\nimplement this yet.\n\nI ended up needing to extend git check-mailmap to handle addresses without\nthe angle brackets as well.\n\nI think this is closer to solving the actual problem we have, which is\nwanting to avoid adding dead email addresses for coworkers who have moved\non, but without completely removing their name from the works.\n\nLink to previous: https://lore.kernel.org/r/20240813-jk-support-mailmap-git-format-patch-v1-1-1aea690ea5dd@gmail.com\n\nSigned-off-by: Jacob Keller <jacob.keller@gmail.com>\n---\nJacob Keller (2):\n      check-mailmap: add --no-brackets mode\n      send-email: add support for --mailmap\n\n builtin/check-mailmap.c             | 27 ++++++++++++-----\n Documentation/git-check-mailmap.txt |  8 ++++-\n git-send-email.perl                 | 14 +++++++++\n t/t4203-mailmap.sh                  | 60 +++++++++++++++++++++++++++++++++++++\n t/t9001-send-email.sh               | 49 ++++++++++++++++++++++++++++++\n 5 files changed, 149 insertions(+), 9 deletions(-)\n---\nbase-commit: 87a1768b93a67d0420255a43d9e07387b2e805ad\nchange-id: 20240816-jk-send-email-mailmap-support-1a9e86867c72\n\nBest regards,\n-- \nJacob Keller <jacob.keller@gmail.com>\n\n"},{"id":"501166","messageId":"20240816-jk-send-email-mailmap-support-v1-1-68ca5b4a6078@gmail.com","threadId":"61963","inReplyTo":"20240816-jk-send-email-mailmap-support-v1-0-68ca5b4a6078@gmail.com","subject":"[PATCH 1/2] check-mailmap: add --no-brackets mode","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2024-08-16T23:06:23Z","receivedAt":"2024-08-16T23:06:36Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller <jacob.keller@gmail.com>\n\nThe git check-mailmap command can be used to convert identities to their\ncanonical real name and email address. Currently, if a simple email\naddress is provided without surrounding angle brackets, git\ncheck-mailmap will fail:\n\n  $ git check-mailmap test@example.com\n  fatal: unable to parse contact: test@example.com\n\nThis is generally fine since identifies are expected to be of the form\n\"name <email@domain>\". However, requiring brackets around simple email\naddresses can make it difficult to support mailmap operation in other\nenvironments where angle brackets may be missing.\n\nSpecifically, attempting to support the mailmap within git send-email is\ntricky, since angle brackets are not always provided for addresses.\n\nTeach check-mailmap a new '--no-brackets' mode. In this mode, any\ncontact line which cannot be interpreted by split_ident_line is treated\nas a simple address without a name. In addition, when any contact does\nnot have a name, output the mail address without the angle brackets.\nNote that angle brackets are accepted if they are present, however the\noutput will strip them.\n\nThis mode will be useful for git send-email in a following feature\nimplementation to enable mapping any email addresses to their canonical\nvalue.\n\nSigned-off-by: Jacob Keller <jacob.keller@gmail.com>\n---\n builtin/check-mailmap.c             | 27 ++++++++++++-----\n Documentation/git-check-mailmap.txt |  8 ++++-\n t/t4203-mailmap.sh                  | 60 +++++++++++++++++++++++++++++++++++++\n 3 files changed, 86 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/check-mailmap.c b/builtin/check-mailmap.c\nindex b8a05b8e07b5..7c8cde370b97 100644\n--- a/builtin/check-mailmap.c\n+++ b/builtin/check-mailmap.c\n@@ -9,6 +9,7 @@\n #include \"write-or-die.h\"\n \n static int use_stdin;\n+static int no_brackets;\n static const char * const check_mailmap_usage[] = {\n N_(\"git check-mailmap [<options>] <contact>...\"),\n NULL\n@@ -16,6 +17,7 @@ NULL\n \n static const struct option check_mailmap_options[] = {\n \tOPT_BOOL(0, \"stdin\", &use_stdin, N_(\"also read contacts from stdin\")),\n+\tOPT_BOOL(0, \"no-brackets\", &no_brackets, N_(\"do not require or output brackets for nameless email addresses\")),\n \tOPT_END()\n };\n \n@@ -25,19 +27,28 @@ static void check_mailmap(struct string_list *mailmap, const char *contact)\n \tsize_t namelen, maillen;\n \tstruct ident_split ident;\n \n-\tif (split_ident_line(&ident, contact, strlen(contact)))\n+\tif (!split_ident_line(&ident, contact, strlen(contact))) {\n+\t\tname = ident.name_begin;\n+\t\tnamelen = ident.name_end - ident.name_begin;\n+\t\tmail = ident.mail_begin;\n+\t\tmaillen = ident.mail_end - ident.mail_begin;\n+\t} else if (no_brackets) {\n+\t\tname = contact;\n+\t\tnamelen = 0;\n+\t\tmail = contact;\n+\t\tmaillen = strlen(contact);\n+\t} else {\n \t\tdie(_(\"unable to parse contact: %s\"), contact);\n-\n-\tname = ident.name_begin;\n-\tnamelen = ident.name_end - ident.name_begin;\n-\tmail = ident.mail_begin;\n-\tmaillen = ident.mail_end - ident.mail_begin;\n+\t}\n \n \tmap_user(mailmap, &mail, &maillen, &name, &namelen);\n \n \tif (namelen)\n-\t\tprintf(\"%.*s \", (int)namelen, name);\n-\tprintf(\"<%.*s>\\n\", (int)maillen, mail);\n+\t\tprintf(\"%.*s <%.*s>\\n\", (int)namelen, name, (int)maillen, mail);\n+\telse if (no_brackets)\n+\t\tprintf(\"%.*s\\n\", (int)maillen, mail);\n+\telse\n+\t\tprintf(\"<%.*s>\\n\", (int)maillen, mail);\n }\n \n int cmd_check_mailmap(int argc, const char **argv, const char *prefix)\ndiff --git a/Documentation/git-check-mailmap.txt b/Documentation/git-check-mailmap.txt\nindex 02f441832321..30f44391a9dd 100644\n--- a/Documentation/git-check-mailmap.txt\n+++ b/Documentation/git-check-mailmap.txt\n@@ -27,13 +27,19 @@ OPTIONS\n \tRead contacts, one per line, from the standard input after exhausting\n \tcontacts provided on the command-line.\n \n+--no-brackets::\n+\tDo not require ``<`` and ``>`` angle brackets when interpreting\n+\tcontacts without a name. Additionally, do not output brackets when\n+\toutputting an email without a name.\n+\n \n OUTPUT\n ------\n \n For each contact, a single line is output, terminated by a newline.  If the\n name is provided or known to the 'mailmap', ``Name $$<user@host>$$'' is\n-printed; otherwise only ``$$<user@host>$$'' is printed.\n+printed; otherwise only ``$$<user@host>$$'' is printed. If ``--no-brackets``\n+is specified, output only ``<user@host>`` for contacts without a name.\n \n \n CONFIGURATION\ndiff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh\nindex 79e5f42760d9..83f012b34ab1 100755\n--- a/t/t4203-mailmap.sh\n+++ b/t/t4203-mailmap.sh\n@@ -80,6 +80,66 @@ test_expect_success 'check-mailmap bogus contact --stdin' '\n \ttest_must_fail git check-mailmap --stdin bogus </dev/null\n '\n \n+test_expect_success 'check-mailmap --no-brackets simple address: no mapping' '\n+\tcat >expect <<-EOF &&\n+\t\tbugs@company.xy\n+\tEOF\n+\tgit check-mailmap --no-brackets bugs@company.xy >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check-mailmap --no-brackets address with brackets: no mapping' '\n+\tcat >expect <<-EOF &&\n+\t\tbugs@company.xy\n+\tEOF\n+\tgit check-mailmap --no-brackets \"<bugs@company.xy>\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check-mailmap --no-brackets simple address: mapping' '\n+\tcat >.mailmap <<-EOF &&\n+\t\tNew Name <bugs@company.xy>\n+\tEOF\n+\tcat >expect <<-EOF &&\n+\t\tNew Name <bugs@company.xy>\n+\tEOF\n+\tgit check-mailmap --no-brackets bugs@company.xy >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check-mailmap --stdin --no-brackets simple address: mapping' '\n+\tcat >.mailmap <<-EOF &&\n+\t\tNew Name <bugs@company.xy>\n+\tEOF\n+\tcat >stdin <<-EOF &&\n+\t\tbugs@company.xy\n+\tEOF\n+\tcat >expect <<-EOF &&\n+\t\tNew Name <bugs@company.xy>\n+\tEOF\n+\tgit check-mailmap --stdin --no-brackets <stdin >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check-mailmap --no-brackets simple address: mapping, no name' '\n+\tcat >.mailmap <<-EOF &&\n+\t\t<bugs@company.xz> <bugs@company.xy>\n+\tEOF\n+\tcat >expect <<-EOF &&\n+\t\tbugs@company.xz\n+\tEOF\n+\tgit check-mailmap --no-brackets bugs@company.xy >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check-mailmap --no-brackets bogus address' '\n+\tcat >expect <<-EOF &&\n+\t\tbogus\n+\tEOF\n+\tgit check-mailmap --no-brackets bogus >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'No mailmap' '\n \tcat >expect <<-EOF &&\n \t$GIT_AUTHOR_NAME (1):\n\n-- \n2.46.0.124.g2dc1a81c8933\n\n"},{"id":"501167","messageId":"20240816-jk-send-email-mailmap-support-v1-2-68ca5b4a6078@gmail.com","threadId":"61963","inReplyTo":"20240816-jk-send-email-mailmap-support-v1-0-68ca5b4a6078@gmail.com","subject":"[PATCH 2/2] send-email: add support for --mailmap","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2024-08-16T23:06:24Z","receivedAt":"2024-08-16T23:06:36Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller <jacob.keller@gmail.com>\n\nIn certain cases, a user may be generating a patch for an old commit\nwhich now has an out-of-date author or other identity. For example,\nconsider a team member who contributes to an internal fork of a project,\nand then later leaves the company.\n\nIt may be desired to submit this change upstream, but the author\nidentity now points to an invalid email address which will bounce. This\nis likely to annoy users who respond to the email on the public mailing\nlist.\n\nThis can be manually corrected, but requires a bit of effort, as it may\nrequire --suppress-cc or otherwise formatting a patch separately and\nmanually removing any unintended email addresses.\n\nGit already has support for the mailmap, which allows mapping addresses\nfor old commits to new canonical names and addresses.\n\nTeach git send-email the --mailmap option. When supplied, use git\ncheck-mailmap (with the --no-brackets mode) as a final stage when\nprocessing address lists. This will convert all addresses to their\ncanonical name and email according to the mailmap file.\n\nA mailmap file can then be configured to point the invalid addresses\neither to their current canonical email (if they still participate in\nthe open source project), or possibly to new owner within the company.\n\nThis enables the sender to avoid accidentally listing an invalid address\nwhen sending such a change.\n\nSigned-off-by: Jacob Keller <jacob.keller@gmail.com>\n---\n git-send-email.perl   | 14 ++++++++++++++\n t/t9001-send-email.sh | 49 +++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 63 insertions(+)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 72044e5ef3a8..9a081e9f9b41 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -46,6 +46,8 @@ sub usage {\n     --compose-encoding      <str>  * Encoding to assume for introduction.\n     --8bit-encoding         <str>  * Encoding to assume 8bit mails if undeclared\n     --transfer-encoding     <str>  * Transfer encoding to use (quoted-printable, 8bit, base64)\n+    --[no-]mailmap                 * Use mailmap file to map all email addresses to canonical\n+                                     real names and email addresses.\n \n   Sending:\n     --envelope-sender       <str>  * Email envelope sender.\n@@ -278,6 +280,7 @@ sub do_edit {\n my $chain_reply_to = 0;\n my $use_xmailer = 1;\n my $validate = 1;\n+my $mailmap = 0;\n my $target_xfer_encoding = 'auto';\n my $forbid_sendmail_variables = 1;\n \n@@ -524,6 +527,8 @@ sub config_regexp {\n \t\t    \"thread!\" => \\$thread,\n \t\t    \"validate!\" => \\$validate,\n \t\t    \"transfer-encoding=s\" => \\$target_xfer_encoding,\n+\t\t    \"mailmap!\" => \\$mailmap,\n+\t\t    \"use-mailmap!\" => \\$mailmap,\n \t\t    \"format-patch!\" => \\$format_patch,\n \t\t    \"8bit-encoding=s\" => \\$auto_8bit_encoding,\n \t\t    \"compose-encoding=s\" => \\$compose_encoding,\n@@ -1085,6 +1090,14 @@ sub expand_one_alias {\n our ($message_id, %mail, $subject, $in_reply_to, $references, $message,\n \t$needs_confirm, $message_num, $ask_default);\n \n+sub mailmap_address_list {\n+\tmy @addr_list = @_;\n+\tif ($mailmap and @addr_list) {\n+\t\t@addr_list = Git::command('check-mailmap', '--no-brackets', @_);\n+\t}\n+\treturn @addr_list;\n+}\n+\n sub extract_valid_address {\n \tmy $address = shift;\n \tmy $local_part_regexp = qr/[^<>\"\\s@]+/;\n@@ -1294,6 +1307,7 @@ sub process_address_list {\n \t@addr_list = expand_aliases(@addr_list);\n \t@addr_list = sanitize_address_list(@addr_list);\n \t@addr_list = validate_address_list(@addr_list);\n+\t@addr_list = mailmap_address_list(@addr_list);\n \treturn @addr_list;\n }\n \ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex 64a4ab3736ef..185697d22563 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -2379,6 +2379,55 @@ test_expect_success $PREREQ 'leading and trailing whitespaces are removed' '\n \ttest_cmp expected-list actual-list\n '\n \n+test_expect_success $PREREQ 'mailmap support with --to' '\n+\tclean_fake_sendmail &&\n+\ttest_config mailmap.file \"mailmap.test\" &&\n+\tcat >mailmap.test <<-EOF &&\n+\tSome Body <someone@example.com> <someone@example.org>\n+\tEOF\n+\tgit format-patch --stdout -1 >a.patch &&\n+\tgit send-email \\\n+\t\t--from=\"Example <nobody@example.com>\" \\\n+\t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n+\t\t--to=someone@example.org \\\n+\t\t--mailmap \\\n+\t\ta.patch \\\n+\t\t2>errors >out &&\n+\tgrep \"^!someone@example\\.com!$\" commandline1\n+'\n+\n+test_expect_success $PREREQ 'mailmap support in To header' '\n+\tclean_fake_sendmail &&\n+\ttest_config mailmap.file \"mailmap.test\" &&\n+\tcat >mailmap.test <<-EOF &&\n+\t<someone@example.com> <someone@example.org>\n+\tEOF\n+\tgit format-patch --stdout -1 --to=someone@example.org >a.patch &&\n+\tgit send-email \\\n+\t\t--from=\"Example <nobody@example.com>\" \\\n+\t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n+\t\t--mailmap \\\n+\t\ta.patch \\\n+\t\t2>errors >out &&\n+\tgrep \"^!someone@example\\.com!$\" commandline1\n+'\n+\n+test_expect_success $PREREQ 'mailmap support in Cc header' '\n+\tclean_fake_sendmail &&\n+\ttest_config mailmap.file \"mailmap.test\" &&\n+\tcat >mailmap.test <<-EOF &&\n+\t<someone@example.com> <someone@example.org>\n+\tEOF\n+\tgit format-patch --stdout -1 --cc=someone@example.org >a.patch &&\n+\tgit send-email \\\n+\t\t--from=\"Example <nobody@example.com>\" \\\n+\t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n+\t\t--mailmap \\\n+\t\ta.patch \\\n+\t\t2>errors >out &&\n+\tgrep \"^!someone@example\\.com!$\" commandline1\n+'\n+\n test_expect_success $PREREQ 'test using command name with --sendmail-cmd' '\n \tclean_fake_sendmail &&\n \tPATH=\"$PWD:$PATH\" \\\n\n-- \n2.46.0.124.g2dc1a81c8933\n\n"},{"id":"501168","messageId":"CAPig+cSN-KU-TQ5UBR9KfxjmQ4zzjL-Y-anxDe9FaeqsV=1Xww@mail.gmail.com","threadId":"61963","inReplyTo":"20240816-jk-send-email-mailmap-support-v1-1-68ca5b4a6078@gmail.com","subject":"Re: [PATCH 1/2] check-mailmap: add --no-brackets mode","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-08-16T23:22:24Z","receivedAt":"2024-08-16T23:22:35Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Aug 16, 2024 at 7:06 PM Jacob Keller <jacob.e.keller@intel.com> wrote:\n> The git check-mailmap command can be used to convert identities to their\n> canonical real name and email address. Currently, if a simple email\n> address is provided without surrounding angle brackets, git\n> check-mailmap will fail:\n>\n>   $ git check-mailmap test@example.com\n>   fatal: unable to parse contact: test@example.com\n>\n> This is generally fine since identifies are expected to be of the form\n\ns/identifies/identities/\n\n> \"name <email@domain>\". However, requiring brackets around simple email\n> addresses can make it difficult to support mailmap operation in other\n> environments where angle brackets may be missing.\n>\n> Specifically, attempting to support the mailmap within git send-email is\n> tricky, since angle brackets are not always provided for addresses.\n>\n> Teach check-mailmap a new '--no-brackets' mode. In this mode, any\n> contact line which cannot be interpreted by split_ident_line is treated\n> as a simple address without a name. In addition, when any contact does\n> not have a name, output the mail address without the angle brackets.\n> Note that angle brackets are accepted if they are present, however the\n> output will strip them.\n\nWhat is not explained by the commit message is why we need this new\noption as opposed to merely making git-check-mailmap accept such\naddresses by default. Are there difficulties or downsides to making\nthis the default behavior? Do other things break if this new behavior\nbecomes the default as opposed to being an explicit opt-in?\n\n> This mode will be useful for git send-email in a following feature\n> implementation to enable mapping any email addresses to their canonical\n> value.\n\nI'm a bit skeptical about how this new flag also changes the output\nformat to suppress the angle brackets. It seems like that's something\nthe caller could do easily enough if desired. For instance:\n\n    $addr =~ s/^<(.*)>$/\\1/;\n\nOr, is there some deeper reason for doing this?\n\n> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>\n"},{"id":"501169","messageId":"CAPig+cQr+n2dTN9OAbmCVc30GwUH2K67X-HGaF=-LazY1y+2bg@mail.gmail.com","threadId":"61963","inReplyTo":"20240816-jk-send-email-mailmap-support-v1-2-68ca5b4a6078@gmail.com","subject":"Re: [PATCH 2/2] send-email: add support for --mailmap","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-08-16T23:41:18Z","receivedAt":"2024-08-16T23:41:31Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Aug 16, 2024 at 7:06 PM Jacob Keller <jacob.e.keller@intel.com> wrote:\n> In certain cases, a user may be generating a patch for an old commit\n> which now has an out-of-date author or other identity. For example,\n> consider a team member who contributes to an internal fork of a project,\n> and then later leaves the company.\n>\n> It may be desired to submit this change upstream, but the author\n> identity now points to an invalid email address which will bounce. This\n> is likely to annoy users who respond to the email on the public mailing\n> list.\n>\n> This can be manually corrected, but requires a bit of effort, as it may\n> require --suppress-cc or otherwise formatting a patch separately and\n> manually removing any unintended email addresses.\n>\n> Git already has support for the mailmap, which allows mapping addresses\n> for old commits to new canonical names and addresses.\n>\n> Teach git send-email the --mailmap option. When supplied, use git\n> check-mailmap (with the --no-brackets mode) as a final stage when\n> processing address lists. This will convert all addresses to their\n> canonical name and email according to the mailmap file.\n>\n> A mailmap file can then be configured to point the invalid addresses\n> either to their current canonical email (if they still participate in\n> the open source project), or possibly to new owner within the company.\n>\n> This enables the sender to avoid accidentally listing an invalid address\n> when sending such a change.\n\nNit: The final two paragraphs appear to repeat what was already stated\nor implied earlier, thus don't seem to add any value to the commit\nmessage.\n\nNit aside, similar to the question I asked about [1/2], are there\ndownsides to merely enabling this new behavior by default? It seems\nlike it would be generally desirable to have this translation happen\nby default, so making everyone opt-in may be a disservice. On the\nother hand, starting out with it disabled by default is understandable\nas a cautious first step, though it might be nice to explain that in\nthe commit message. Similarly, one can imagine a world in which people\nwant to enable this and forget about it, thus would like it to be\ncontrolled by configuration (though that can, of course, be left for a\nfuture change).\n\n> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>\n> ---\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> @@ -1085,6 +1090,14 @@ sub expand_one_alias {\n> +sub mailmap_address_list {\n> +       my @addr_list = @_;\n> +       if ($mailmap and @addr_list) {\n> +               @addr_list = Git::command('check-mailmap', '--no-brackets', @_);\n> +       }\n> +       return @addr_list;\n> +}\n\nFor some reason, I found this logic more difficult to follow than\nexpected, possibly because it doesn't feel quite Perlish, or possibly\nbecause in this codebase, we often take care of the easy cases first\nand return early. Thus, I may have been expecting the above to be\nwritten more along the lines of:\n\n    sub mailmap_address_list {\n        return @_ unless @_ && $mailmap;\n        return Git::command('check-mailmap', '--no-brackets', @_);\n    }\n\nOf course, it's highly subjective and not at all worth a reroll.\n"},{"id":"501170","messageId":"cf58f786-06b0-4ad3-a6fd-f593af639c98@intel.com","threadId":"61963","inReplyTo":"CAPig+cSN-KU-TQ5UBR9KfxjmQ4zzjL-Y-anxDe9FaeqsV=1Xww@mail.gmail.com","subject":"Re: [PATCH 1/2] check-mailmap: add --no-brackets mode","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2024-08-16T23:42:32Z","receivedAt":"2024-08-16T23:42:41Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"\n\nOn 8/16/2024 4:22 PM, Eric Sunshine wrote:\n> On Fri, Aug 16, 2024 at 7:06 PM Jacob Keller <jacob.e.keller@intel.com> wrote:\n>> The git check-mailmap command can be used to convert identities to their\n>> canonical real name and email address. Currently, if a simple email\n>> address is provided without surrounding angle brackets, git\n>> check-mailmap will fail:\n>>\n>>   $ git check-mailmap test@example.com\n>>   fatal: unable to parse contact: test@example.com\n>>\n>> This is generally fine since identifies are expected to be of the form\n> \n> s/identifies/identities/\n> \n>> \"name <email@domain>\". However, requiring brackets around simple email\n>> addresses can make it difficult to support mailmap operation in other\n>> environments where angle brackets may be missing.\n>>\n>> Specifically, attempting to support the mailmap within git send-email is\n>> tricky, since angle brackets are not always provided for addresses.\n>>\n>> Teach check-mailmap a new '--no-brackets' mode. In this mode, any\n>> contact line which cannot be interpreted by split_ident_line is treated\n>> as a simple address without a name. In addition, when any contact does\n>> not have a name, output the mail address without the angle brackets.\n>> Note that angle brackets are accepted if they are present, however the\n>> output will strip them.\n> \n> What is not explained by the commit message is why we need this new\n> option as opposed to merely making git-check-mailmap accept such\n> addresses by default. Are there difficulties or downsides to making\n> this the default behavior? Do other things break if this new behavior\n> becomes the default as opposed to being an explicit opt-in?\n> \n\nMostly I did it this way out of conservative caution to avoid breaking\nexisting users. It could be that nothing breaks and loosening the\nrestriction on what we pass it would be sufficient.\n\n>> This mode will be useful for git send-email in a following feature\n>> implementation to enable mapping any email addresses to their canonical\n>> value.\n> \n> I'm a bit skeptical about how this new flag also changes the output\n> format to suppress the angle brackets. It seems like that's something\n> the caller could do easily enough if desired. For instance:\n> \n>     $addr =~ s/^<(.*)>$/\\1/;\n> \n> Or, is there some deeper reason for doing this?\n> \n\nFor one, the documentation of git check-mailmap specifies that it passes\nother values through \"as-is\", but this mode would convert\n\"user@example.com\" to \"<user@example.com>\"\n\nI suppose thats not a big deal, and it was more a matter of not wanting\nto bother the caller to force it to strip any brackets if it didn't want\nthem. I think its probably fine to require that.\n\n>> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>\n"},{"id":"501171","messageId":"b706f695-c1db-48df-9095-664a13293f99@intel.com","threadId":"61963","inReplyTo":"CAPig+cQr+n2dTN9OAbmCVc30GwUH2K67X-HGaF=-LazY1y+2bg@mail.gmail.com","subject":"Re: [PATCH 2/2] send-email: add support for --mailmap","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2024-08-16T23:49:31Z","receivedAt":"2024-08-16T23:49:38Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"\n\nOn 8/16/2024 4:41 PM, Eric Sunshine wrote:\n> On Fri, Aug 16, 2024 at 7:06 PM Jacob Keller <jacob.e.keller@intel.com> wrote:\n>> In certain cases, a user may be generating a patch for an old commit\n>> which now has an out-of-date author or other identity. For example,\n>> consider a team member who contributes to an internal fork of a project,\n>> and then later leaves the company.\n>>\n>> It may be desired to submit this change upstream, but the author\n>> identity now points to an invalid email address which will bounce. This\n>> is likely to annoy users who respond to the email on the public mailing\n>> list.\n>>\n>> This can be manually corrected, but requires a bit of effort, as it may\n>> require --suppress-cc or otherwise formatting a patch separately and\n>> manually removing any unintended email addresses.\n>>\n>> Git already has support for the mailmap, which allows mapping addresses\n>> for old commits to new canonical names and addresses.\n>>\n>> Teach git send-email the --mailmap option. When supplied, use git\n>> check-mailmap (with the --no-brackets mode) as a final stage when\n>> processing address lists. This will convert all addresses to their\n>> canonical name and email according to the mailmap file.\n>>\n>> A mailmap file can then be configured to point the invalid addresses\n>> either to their current canonical email (if they still participate in\n>> the open source project), or possibly to new owner within the company.\n>>\n>> This enables the sender to avoid accidentally listing an invalid address\n>> when sending such a change.\n> \n> Nit: The final two paragraphs appear to repeat what was already stated\n> or implied earlier, thus don't seem to add any value to the commit\n> message.\n> \n\nSure, I think I got a bit verbose here.\n\n> Nit aside, similar to the question I asked about [1/2], are there\n> downsides to merely enabling this new behavior by default? It seems\n> like it would be generally desirable to have this translation happen\n> by default, so making everyone opt-in may be a disservice. On the\n> other hand, starting out with it disabled by default is understandable\n> as a cautious first step, though it might be nice to explain that in\n> the commit message. Similarly, one can imagine a world in which people\n> want to enable this and forget about it, thus would like it to be\n> controlled by configuration (though that can, of course, be left for a\n> future change).\n\nI definitely did it mostly out of conservative: \"don't change the\ndefault behavior\".\n\nFor a general mailmap I think enabling it by default, with a config\noption to disable it. I think it might also make sense to have\nper-identity configuration, so that different identities could point at\na different mailmap.\n\nFor example, one of the use cases we have is to have a mailmap file that\ntakes the now-invalid addresses and points them all to the current\nmaintainer. This way, the maintainer (who is sending these patches)\nwould have all the old addresses automatically point to him instead of\ngenerating the bounced messages as currently happens on accident. This\ntype of mailmap file likely does not make sense as a general-purpose file.\n\nI think for per-identity configurations, we would either need to add an\noption to git check-mailmap to pass the mailmap file, or I would need to\nfigure out how to set config when calling Git::command() to force a\nspecific mailmap.\n\n> \n>> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>\n>> ---\n>> diff --git a/git-send-email.perl b/git-send-email.perl\n>> @@ -1085,6 +1090,14 @@ sub expand_one_alias {\n>> +sub mailmap_address_list {\n>> +       my @addr_list = @_;\n>> +       if ($mailmap and @addr_list) {\n>> +               @addr_list = Git::command('check-mailmap', '--no-brackets', @_);\n>> +       }\n>> +       return @addr_list;\n>> +}\n> \n> For some reason, I found this logic more difficult to follow than\n> expected, possibly because it doesn't feel quite Perlish, or possibly\n> because in this codebase, we often take care of the easy cases first\n> and return early. Thus, I may have been expecting the above to be\n> written more along the lines of:\n> \n>     sub mailmap_address_list {\n>         return @_ unless @_ && $mailmap;\n>         return Git::command('check-mailmap', '--no-brackets', @_);\n>     }\n> \n\nAh, yea that makes more sense. I had been trying to figure that out but\nI am not as used to the unless syntax.\n\n> Of course, it's highly subjective and not at all worth a reroll.\n> \n"},{"id":"501172","messageId":"CAPig+cT8EOSmt+7bbNPsXa36AiPpbu2Et37o5TZb8JvRTxKXHA@mail.gmail.com","threadId":"61963","inReplyTo":"cf58f786-06b0-4ad3-a6fd-f593af639c98@intel.com","subject":"Re: [PATCH 1/2] check-mailmap: add --no-brackets mode","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-08-16T23:51:30Z","receivedAt":"2024-08-16T23:51:42Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Aug 16, 2024 at 7:42 PM Jacob Keller <jacob.e.keller@intel.com> wrote:\n> On 8/16/2024 4:22 PM, Eric Sunshine wrote:\n> > What is not explained by the commit message is why we need this new\n> > option as opposed to merely making git-check-mailmap accept such\n> > addresses by default. Are there difficulties or downsides to making\n> > this the default behavior? Do other things break if this new behavior\n> > becomes the default as opposed to being an explicit opt-in?\n>\n> Mostly I did it this way out of conservative caution to avoid breaking\n> existing users. It could be that nothing breaks and loosening the\n> restriction on what we pass it would be sufficient.\n\nI figured that was probably the reason for making this an opt-in.\n\nI think my somewhat negative reaction to this new option -- and the\nreason I asked the above question -- is that it feels very\nspecial-case, thus it lacks generality (or at least the way the commit\nmessage sells it, it makes it difficult to see it as anything other\nthan a bandaid added just to support mailmap in git-send-email).\n"}]}