[PATCH 0/2] send-email: add --mailmap support

[PATCH 0/2] send-email: add --mailmap support

[Jacob Keller]

I recently sent a series to enable mailmap support in format patch. The
discussion led me to realize that the true problem we wanted solved is to
map addresses at send time, so that we do not accidentally include a dead
mail address when sending an old change.

Instead of worrying about what the formatted patch has, this series
implements support for mailmap at the send-email, which will translate all
addresses, and not just the author/commit addresses for a patch, but also
the email for any trailers.o

I considered also that it may be useful to make send-email read a mailmap
file from the identity config blocks, but have not figured out how to
implement this yet.

I ended up needing to extend git check-mailmap to handle addresses without
the angle brackets as well.

I think this is closer to solving the actual problem we have, which is
wanting to avoid adding dead email addresses for coworkers who have moved
on, but without completely removing their name from the works.

Link to previous: https://lore.kernel.org/r/20240813-jk-support-mailmap-git-format-patch-v1-1-1aea690ea5dd@gmail.com

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
Jacob Keller (2):
      check-mailmap: add --no-brackets mode
      send-email: add support for --mailmap

 builtin/check-mailmap.c             | 27 ++++++++++++-----
 Documentation/git-check-mailmap.txt |  8 ++++-
 git-send-email.perl                 | 14 +++++++++
 t/t4203-mailmap.sh                  | 60 +++++++++++++++++++++++++++++++++++++
 t/t9001-send-email.sh               | 49 ++++++++++++++++++++++++++++++
 5 files changed, 149 insertions(+), 9 deletions(-)
---
base-commit: 87a1768b93a67d0420255a43d9e07387b2e805ad
change-id: 20240816-jk-send-email-mailmap-support-1a9e86867c72

Best regards,
-- 
Jacob Keller <jacob.keller@gmail.com>

[PATCH 1/2] check-mailmap: add --no-brackets mode

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

The git check-mailmap command can be used to convert identities to their
canonical real name and email address. Currently, if a simple email
address is provided without surrounding angle brackets, git
check-mailmap will fail:

  $ git check-mailmap test@example.com
  fatal: unable to parse contact: test@example.com

This is generally fine since identifies are expected to be of the form
"name <email@domain>". However, requiring brackets around simple email
addresses can make it difficult to support mailmap operation in other
environments where angle brackets may be missing.

Specifically, attempting to support the mailmap within git send-email is
tricky, since angle brackets are not always provided for addresses.

Teach check-mailmap a new '--no-brackets' mode. In this mode, any
contact line which cannot be interpreted by split_ident_line is treated
as a simple address without a name. In addition, when any contact does
not have a name, output the mail address without the angle brackets.
Note that angle brackets are accepted if they are present, however the
output will strip them.

This mode will be useful for git send-email in a following feature
implementation to enable mapping any email addresses to their canonical
value.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 builtin/check-mailmap.c             | 27 ++++++++++++-----
 Documentation/git-check-mailmap.txt |  8 ++++-
 t/t4203-mailmap.sh                  | 60 +++++++++++++++++++++++++++++++++++++
 3 files changed, 86 insertions(+), 9 deletions(-)

diff --git a/builtin/check-mailmap.c b/builtin/check-mailmap.c
index b8a05b8e07b5..7c8cde370b97 100644
--- a/builtin/check-mailmap.c
+++ b/builtin/check-mailmap.c
@@ -9,6 +9,7 @@
 #include "write-or-die.h"
 
 static int use_stdin;
+static int no_brackets;
 static const char * const check_mailmap_usage[] = {
 N_("git check-mailmap [<options>] <contact>..."),
 NULL
@@ -16,6 +17,7 @@ NULL
 
 static const struct option check_mailmap_options[] = {
 	OPT_BOOL(0, "stdin", &use_stdin, N_("also read contacts from stdin")),
+	OPT_BOOL(0, "no-brackets", &no_brackets, N_("do not require or output brackets for nameless email addresses")),
 	OPT_END()
 };
 
@@ -25,19 +27,28 @@ static void check_mailmap(struct string_list *mailmap, const char *contact)
 	size_t namelen, maillen;
 	struct ident_split ident;
 
-	if (split_ident_line(&ident, contact, strlen(contact)))
+	if (!split_ident_line(&ident, contact, strlen(contact))) {
+		name = ident.name_begin;
+		namelen = ident.name_end - ident.name_begin;
+		mail = ident.mail_begin;
+		maillen = ident.mail_end - ident.mail_begin;
+	} else if (no_brackets) {
+		name = contact;
+		namelen = 0;
+		mail = contact;
+		maillen = strlen(contact);
+	} else {
 		die(_("unable to parse contact: %s"), contact);
-
-	name = ident.name_begin;
-	namelen = ident.name_end - ident.name_begin;
-	mail = ident.mail_begin;
-	maillen = ident.mail_end - ident.mail_begin;
+	}
 
 	map_user(mailmap, &mail, &maillen, &name, &namelen);
 
 	if (namelen)
-		printf("%.*s ", (int)namelen, name);
-	printf("<%.*s>\n", (int)maillen, mail);
+		printf("%.*s <%.*s>\n", (int)namelen, name, (int)maillen, mail);
+	else if (no_brackets)
+		printf("%.*s\n", (int)maillen, mail);
+	else
+		printf("<%.*s>\n", (int)maillen, mail);
 }
 
 int cmd_check_mailmap(int argc, const char **argv, const char *prefix)
diff --git a/Documentation/git-check-mailmap.txt b/Documentation/git-check-mailmap.txt
index 02f441832321..30f44391a9dd 100644
--- a/Documentation/git-check-mailmap.txt
+++ b/Documentation/git-check-mailmap.txt
@@ -27,13 +27,19 @@ OPTIONS
 	Read contacts, one per line, from the standard input after exhausting
 	contacts provided on the command-line.
 
+--no-brackets::
+	Do not require ``<`` and ``>`` angle brackets when interpreting
+	contacts without a name. Additionally, do not output brackets when
+	outputting an email without a name.
+
 
 OUTPUT
 ------
 
 For each contact, a single line is output, terminated by a newline.  If the
 name is provided or known to the 'mailmap', ``Name $$<user@host>$$'' is
-printed; otherwise only ``$$<user@host>$$'' is printed.
+printed; otherwise only ``$$<user@host>$$'' is printed. If ``--no-brackets``
+is specified, output only ``<user@host>`` for contacts without a name.
 
 
 CONFIGURATION
diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
index 79e5f42760d9..83f012b34ab1 100755
--- a/t/t4203-mailmap.sh
+++ b/t/t4203-mailmap.sh
@@ -80,6 +80,66 @@ test_expect_success 'check-mailmap bogus contact --stdin' '
 	test_must_fail git check-mailmap --stdin bogus </dev/null
 '
 
+test_expect_success 'check-mailmap --no-brackets simple address: no mapping' '
+	cat >expect <<-EOF &&
+		bugs@company.xy
+	EOF
+	git check-mailmap --no-brackets bugs@company.xy >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check-mailmap --no-brackets address with brackets: no mapping' '
+	cat >expect <<-EOF &&
+		bugs@company.xy
+	EOF
+	git check-mailmap --no-brackets "<bugs@company.xy>" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check-mailmap --no-brackets simple address: mapping' '
+	cat >.mailmap <<-EOF &&
+		New Name <bugs@company.xy>
+	EOF
+	cat >expect <<-EOF &&
+		New Name <bugs@company.xy>
+	EOF
+	git check-mailmap --no-brackets bugs@company.xy >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check-mailmap --stdin --no-brackets simple address: mapping' '
+	cat >.mailmap <<-EOF &&
+		New Name <bugs@company.xy>
+	EOF
+	cat >stdin <<-EOF &&
+		bugs@company.xy
+	EOF
+	cat >expect <<-EOF &&
+		New Name <bugs@company.xy>
+	EOF
+	git check-mailmap --stdin --no-brackets <stdin >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check-mailmap --no-brackets simple address: mapping, no name' '
+	cat >.mailmap <<-EOF &&
+		<bugs@company.xz> <bugs@company.xy>
+	EOF
+	cat >expect <<-EOF &&
+		bugs@company.xz
+	EOF
+	git check-mailmap --no-brackets bugs@company.xy >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check-mailmap --no-brackets bogus address' '
+	cat >expect <<-EOF &&
+		bogus
+	EOF
+	git check-mailmap --no-brackets bogus >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'No mailmap' '
 	cat >expect <<-EOF &&
 	$GIT_AUTHOR_NAME (1):

-- 
2.46.0.124.g2dc1a81c8933

Re: [PATCH 1/2] check-mailmap: add --no-brackets mode

[Eric Sunshine]

On Fri, Aug 16, 2024 at 7:06 PM Jacob Keller <jacob.e.keller@intel.com> wrote:
> The git check-mailmap command can be used to convert identities to their
> canonical real name and email address. Currently, if a simple email
> address is provided without surrounding angle brackets, git
> check-mailmap will fail:
>
>   $ git check-mailmap test@example.com
>   fatal: unable to parse contact: test@example.com
>
> This is generally fine since identifies are expected to be of the form

s/identifies/identities/

> "name <email@domain>". However, requiring brackets around simple email
> addresses can make it difficult to support mailmap operation in other
> environments where angle brackets may be missing.
>
> Specifically, attempting to support the mailmap within git send-email is
> tricky, since angle brackets are not always provided for addresses.
>
> Teach check-mailmap a new '--no-brackets' mode. In this mode, any
> contact line which cannot be interpreted by split_ident_line is treated
> as a simple address without a name. In addition, when any contact does
> not have a name, output the mail address without the angle brackets.
> Note that angle brackets are accepted if they are present, however the
> output will strip them.

What is not explained by the commit message is why we need this new
option as opposed to merely making git-check-mailmap accept such
addresses by default. Are there difficulties or downsides to making
this the default behavior? Do other things break if this new behavior
becomes the default as opposed to being an explicit opt-in?

> This mode will be useful for git send-email in a following feature
> implementation to enable mapping any email addresses to their canonical
> value.

I'm a bit skeptical about how this new flag also changes the output
format to suppress the angle brackets. It seems like that's something
the caller could do easily enough if desired. For instance:

    $addr =~ s/^<(.*)>$/\1/;

Or, is there some deeper reason for doing this?

> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>

Re: [PATCH 1/2] check-mailmap: add --no-brackets mode

[Jacob Keller]

On 8/16/2024 4:22 PM, Eric Sunshine wrote:
> On Fri, Aug 16, 2024 at 7:06 PM Jacob Keller <jacob.e.keller@intel.com> wrote:
>> The git check-mailmap command can be used to convert identities to their
>> canonical real name and email address. Currently, if a simple email
>> address is provided without surrounding angle brackets, git
>> check-mailmap will fail:
>>
>>   $ git check-mailmap test@example.com
>>   fatal: unable to parse contact: test@example.com
>>
>> This is generally fine since identifies are expected to be of the form
> 
> s/identifies/identities/
> 
>> "name <email@domain>". However, requiring brackets around simple email
>> addresses can make it difficult to support mailmap operation in other
>> environments where angle brackets may be missing.
>>
>> Specifically, attempting to support the mailmap within git send-email is
>> tricky, since angle brackets are not always provided for addresses.
>>
>> Teach check-mailmap a new '--no-brackets' mode. In this mode, any
>> contact line which cannot be interpreted by split_ident_line is treated
>> as a simple address without a name. In addition, when any contact does
>> not have a name, output the mail address without the angle brackets.
>> Note that angle brackets are accepted if they are present, however the
>> output will strip them.
> 
> What is not explained by the commit message is why we need this new
> option as opposed to merely making git-check-mailmap accept such
> addresses by default. Are there difficulties or downsides to making
> this the default behavior? Do other things break if this new behavior
> becomes the default as opposed to being an explicit opt-in?
> 

Mostly I did it this way out of conservative caution to avoid breaking
existing users. It could be that nothing breaks and loosening the
restriction on what we pass it would be sufficient.

>> This mode will be useful for git send-email in a following feature
>> implementation to enable mapping any email addresses to their canonical
>> value.
> 
> I'm a bit skeptical about how this new flag also changes the output
> format to suppress the angle brackets. It seems like that's something
> the caller could do easily enough if desired. For instance:
> 
>     $addr =~ s/^<(.*)>$/\1/;
> 
> Or, is there some deeper reason for doing this?
> 

For one, the documentation of git check-mailmap specifies that it passes
other values through "as-is", but this mode would convert
"user@example.com" to "<user@example.com>"

I suppose thats not a big deal, and it was more a matter of not wanting
to bother the caller to force it to strip any brackets if it didn't want
them. I think its probably fine to require that.

>> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>

Re: [PATCH 1/2] check-mailmap: add --no-brackets mode

[Eric Sunshine]

On Fri, Aug 16, 2024 at 7:42 PM Jacob Keller <jacob.e.keller@intel.com> wrote:
> On 8/16/2024 4:22 PM, Eric Sunshine wrote:
> > What is not explained by the commit message is why we need this new
> > option as opposed to merely making git-check-mailmap accept such
> > addresses by default. Are there difficulties or downsides to making
> > this the default behavior? Do other things break if this new behavior
> > becomes the default as opposed to being an explicit opt-in?
>
> Mostly I did it this way out of conservative caution to avoid breaking
> existing users. It could be that nothing breaks and loosening the
> restriction on what we pass it would be sufficient.

I figured that was probably the reason for making this an opt-in.

I think my somewhat negative reaction to this new option -- and the
reason I asked the above question -- is that it feels very
special-case, thus it lacks generality (or at least the way the commit
message sells it, it makes it difficult to see it as anything other
than a bandaid added just to support mailmap in git-send-email).

[PATCH 2/2] send-email: add support for --mailmap

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

In certain cases, a user may be generating a patch for an old commit
which now has an out-of-date author or other identity. For example,
consider a team member who contributes to an internal fork of a project,
and then later leaves the company.

It may be desired to submit this change upstream, but the author
identity now points to an invalid email address which will bounce. This
is likely to annoy users who respond to the email on the public mailing
list.

This can be manually corrected, but requires a bit of effort, as it may
require --suppress-cc or otherwise formatting a patch separately and
manually removing any unintended email addresses.

Git already has support for the mailmap, which allows mapping addresses
for old commits to new canonical names and addresses.

Teach git send-email the --mailmap option. When supplied, use git
check-mailmap (with the --no-brackets mode) as a final stage when
processing address lists. This will convert all addresses to their
canonical name and email according to the mailmap file.

A mailmap file can then be configured to point the invalid addresses
either to their current canonical email (if they still participate in
the open source project), or possibly to new owner within the company.

This enables the sender to avoid accidentally listing an invalid address
when sending such a change.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 git-send-email.perl   | 14 ++++++++++++++
 t/t9001-send-email.sh | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+)

diff --git a/git-send-email.perl b/git-send-email.perl
index 72044e5ef3a8..9a081e9f9b41 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -46,6 +46,8 @@ sub usage {
     --compose-encoding      <str>  * Encoding to assume for introduction.
     --8bit-encoding         <str>  * Encoding to assume 8bit mails if undeclared
     --transfer-encoding     <str>  * Transfer encoding to use (quoted-printable, 8bit, base64)
+    --[no-]mailmap                 * Use mailmap file to map all email addresses to canonical
+                                     real names and email addresses.
 
   Sending:
     --envelope-sender       <str>  * Email envelope sender.
@@ -278,6 +280,7 @@ sub do_edit {
 my $chain_reply_to = 0;
 my $use_xmailer = 1;
 my $validate = 1;
+my $mailmap = 0;
 my $target_xfer_encoding = 'auto';
 my $forbid_sendmail_variables = 1;
 
@@ -524,6 +527,8 @@ sub config_regexp {
 		    "thread!" => \$thread,
 		    "validate!" => \$validate,
 		    "transfer-encoding=s" => \$target_xfer_encoding,
+		    "mailmap!" => \$mailmap,
+		    "use-mailmap!" => \$mailmap,
 		    "format-patch!" => \$format_patch,
 		    "8bit-encoding=s" => \$auto_8bit_encoding,
 		    "compose-encoding=s" => \$compose_encoding,
@@ -1085,6 +1090,14 @@ sub expand_one_alias {
 our ($message_id, %mail, $subject, $in_reply_to, $references, $message,
 	$needs_confirm, $message_num, $ask_default);
 
+sub mailmap_address_list {
+	my @addr_list = @_;
+	if ($mailmap and @addr_list) {
+		@addr_list = Git::command('check-mailmap', '--no-brackets', @_);
+	}
+	return @addr_list;
+}
+
 sub extract_valid_address {
 	my $address = shift;
 	my $local_part_regexp = qr/[^<>"\s@]+/;
@@ -1294,6 +1307,7 @@ sub process_address_list {
 	@addr_list = expand_aliases(@addr_list);
 	@addr_list = sanitize_address_list(@addr_list);
 	@addr_list = validate_address_list(@addr_list);
+	@addr_list = mailmap_address_list(@addr_list);
 	return @addr_list;
 }
 
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 64a4ab3736ef..185697d22563 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -2379,6 +2379,55 @@ test_expect_success $PREREQ 'leading and trailing whitespaces are removed' '
 	test_cmp expected-list actual-list
 '
 
+test_expect_success $PREREQ 'mailmap support with --to' '
+	clean_fake_sendmail &&
+	test_config mailmap.file "mailmap.test" &&
+	cat >mailmap.test <<-EOF &&
+	Some Body <someone@example.com> <someone@example.org>
+	EOF
+	git format-patch --stdout -1 >a.patch &&
+	git send-email \
+		--from="Example <nobody@example.com>" \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		--to=someone@example.org \
+		--mailmap \
+		a.patch \
+		2>errors >out &&
+	grep "^!someone@example\.com!$" commandline1
+'
+
+test_expect_success $PREREQ 'mailmap support in To header' '
+	clean_fake_sendmail &&
+	test_config mailmap.file "mailmap.test" &&
+	cat >mailmap.test <<-EOF &&
+	<someone@example.com> <someone@example.org>
+	EOF
+	git format-patch --stdout -1 --to=someone@example.org >a.patch &&
+	git send-email \
+		--from="Example <nobody@example.com>" \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		--mailmap \
+		a.patch \
+		2>errors >out &&
+	grep "^!someone@example\.com!$" commandline1
+'
+
+test_expect_success $PREREQ 'mailmap support in Cc header' '
+	clean_fake_sendmail &&
+	test_config mailmap.file "mailmap.test" &&
+	cat >mailmap.test <<-EOF &&
+	<someone@example.com> <someone@example.org>
+	EOF
+	git format-patch --stdout -1 --cc=someone@example.org >a.patch &&
+	git send-email \
+		--from="Example <nobody@example.com>" \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		--mailmap \
+		a.patch \
+		2>errors >out &&
+	grep "^!someone@example\.com!$" commandline1
+'
+
 test_expect_success $PREREQ 'test using command name with --sendmail-cmd' '
 	clean_fake_sendmail &&
 	PATH="$PWD:$PATH" \

-- 
2.46.0.124.g2dc1a81c8933

Re: [PATCH 2/2] send-email: add support for --mailmap

[Eric Sunshine]

On Fri, Aug 16, 2024 at 7:06 PM Jacob Keller <jacob.e.keller@intel.com> wrote:
> In certain cases, a user may be generating a patch for an old commit
> which now has an out-of-date author or other identity. For example,
> consider a team member who contributes to an internal fork of a project,
> and then later leaves the company.
>
> It may be desired to submit this change upstream, but the author
> identity now points to an invalid email address which will bounce. This
> is likely to annoy users who respond to the email on the public mailing
> list.
>
> This can be manually corrected, but requires a bit of effort, as it may
> require --suppress-cc or otherwise formatting a patch separately and
> manually removing any unintended email addresses.
>
> Git already has support for the mailmap, which allows mapping addresses
> for old commits to new canonical names and addresses.
>
> Teach git send-email the --mailmap option. When supplied, use git
> check-mailmap (with the --no-brackets mode) as a final stage when
> processing address lists. This will convert all addresses to their
> canonical name and email according to the mailmap file.
>
> A mailmap file can then be configured to point the invalid addresses
> either to their current canonical email (if they still participate in
> the open source project), or possibly to new owner within the company.
>
> This enables the sender to avoid accidentally listing an invalid address
> when sending such a change.

Nit: The final two paragraphs appear to repeat what was already stated
or implied earlier, thus don't seem to add any value to the commit
message.

Nit aside, similar to the question I asked about [1/2], are there
downsides to merely enabling this new behavior by default? It seems
like it would be generally desirable to have this translation happen
by default, so making everyone opt-in may be a disservice. On the
other hand, starting out with it disabled by default is understandable
as a cautious first step, though it might be nice to explain that in
the commit message. Similarly, one can imagine a world in which people
want to enable this and forget about it, thus would like it to be
controlled by configuration (though that can, of course, be left for a
future change).

> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
> diff --git a/git-send-email.perl b/git-send-email.perl
> @@ -1085,6 +1090,14 @@ sub expand_one_alias {
> +sub mailmap_address_list {
> +       my @addr_list = @_;
> +       if ($mailmap and @addr_list) {
> +               @addr_list = Git::command('check-mailmap', '--no-brackets', @_);
> +       }
> +       return @addr_list;
> +}

For some reason, I found this logic more difficult to follow than
expected, possibly because it doesn't feel quite Perlish, or possibly
because in this codebase, we often take care of the easy cases first
and return early. Thus, I may have been expecting the above to be
written more along the lines of:

    sub mailmap_address_list {
        return @_ unless @_ && $mailmap;
        return Git::command('check-mailmap', '--no-brackets', @_);
    }

Of course, it's highly subjective and not at all worth a reroll.

Re: [PATCH 2/2] send-email: add support for --mailmap

[Jacob Keller]

On 8/16/2024 4:41 PM, Eric Sunshine wrote:
> On Fri, Aug 16, 2024 at 7:06 PM Jacob Keller <jacob.e.keller@intel.com> wrote:
>> In certain cases, a user may be generating a patch for an old commit
>> which now has an out-of-date author or other identity. For example,
>> consider a team member who contributes to an internal fork of a project,
>> and then later leaves the company.
>>
>> It may be desired to submit this change upstream, but the author
>> identity now points to an invalid email address which will bounce. This
>> is likely to annoy users who respond to the email on the public mailing
>> list.
>>
>> This can be manually corrected, but requires a bit of effort, as it may
>> require --suppress-cc or otherwise formatting a patch separately and
>> manually removing any unintended email addresses.
>>
>> Git already has support for the mailmap, which allows mapping addresses
>> for old commits to new canonical names and addresses.
>>
>> Teach git send-email the --mailmap option. When supplied, use git
>> check-mailmap (with the --no-brackets mode) as a final stage when
>> processing address lists. This will convert all addresses to their
>> canonical name and email according to the mailmap file.
>>
>> A mailmap file can then be configured to point the invalid addresses
>> either to their current canonical email (if they still participate in
>> the open source project), or possibly to new owner within the company.
>>
>> This enables the sender to avoid accidentally listing an invalid address
>> when sending such a change.
> 
> Nit: The final two paragraphs appear to repeat what was already stated
> or implied earlier, thus don't seem to add any value to the commit
> message.
> 

Sure, I think I got a bit verbose here.

> Nit aside, similar to the question I asked about [1/2], are there
> downsides to merely enabling this new behavior by default? It seems
> like it would be generally desirable to have this translation happen
> by default, so making everyone opt-in may be a disservice. On the
> other hand, starting out with it disabled by default is understandable
> as a cautious first step, though it might be nice to explain that in
> the commit message. Similarly, one can imagine a world in which people
> want to enable this and forget about it, thus would like it to be
> controlled by configuration (though that can, of course, be left for a
> future change).

I definitely did it mostly out of conservative: "don't change the
default behavior".

For a general mailmap I think enabling it by default, with a config
option to disable it. I think it might also make sense to have
per-identity configuration, so that different identities could point at
a different mailmap.

For example, one of the use cases we have is to have a mailmap file that
takes the now-invalid addresses and points them all to the current
maintainer. This way, the maintainer (who is sending these patches)
would have all the old addresses automatically point to him instead of
generating the bounced messages as currently happens on accident. This
type of mailmap file likely does not make sense as a general-purpose file.

I think for per-identity configurations, we would either need to add an
option to git check-mailmap to pass the mailmap file, or I would need to
figure out how to set config when calling Git::command() to force a
specific mailmap.

> 
>> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
>> ---
>> diff --git a/git-send-email.perl b/git-send-email.perl
>> @@ -1085,6 +1090,14 @@ sub expand_one_alias {
>> +sub mailmap_address_list {
>> +       my @addr_list = @_;
>> +       if ($mailmap and @addr_list) {
>> +               @addr_list = Git::command('check-mailmap', '--no-brackets', @_);
>> +       }
>> +       return @addr_list;
>> +}
> 
> For some reason, I found this logic more difficult to follow than
> expected, possibly because it doesn't feel quite Perlish, or possibly
> because in this codebase, we often take care of the easy cases first
> and return early. Thus, I may have been expecting the above to be
> written more along the lines of:
> 
>     sub mailmap_address_list {
>         return @_ unless @_ && $mailmap;
>         return Git::command('check-mailmap', '--no-brackets', @_);
>     }
> 

Ah, yea that makes more sense. I had been trying to figure that out but
I am not as used to the unless syntax.

> Of course, it's highly subjective and not at all worth a reroll.
>