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

[PATCH v2 0/3] 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

With v2, we now have a configuration option (sendemail.mailmap) to enable
this behavior. In addition, I enabled support for email-specific mailmap
files.

The intention of these is to allow a maintainer to map the known-dead
addresses of former colleagues onto a current email for an owner within the
team. This would be used to update the send addresses to avoid including
no-longer-valid addresses when sending patches. This is intended for cases
where the original author is no longer valid such as when they are no
longer employed to work on the project. While sometimes pointing to a
canonical public address of that person may make sense, in other contexts,
removing them from the email makes sense.

I believe this version solves the use case we have of ensuring that we stop
sending emails with invalid addresses, and may be useful for others as
well.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
Changes in v2:
- Loosen restriction on git check-mailmap by default, rather than
  introducing a specific --no-brackets option.
- Re-write commit message for the send-email changes.
- Add --mailmap-file and --mailmap-blob options to git check-mailmap.
- Add configuration options to git send-email for enabling mailmap support
  by default, as well as providing send-email specific mailmap files.
- Link to v1: https://lore.kernel.org/r/20240816-jk-send-email-mailmap-support-v1-0-68ca5b4a6078@gmail.com
- Link to previous "v0": https://lore.kernel.org/r/20240813-jk-support-mailmap-git-format-patch-v1-1-1aea690ea5dd@gmail.com

---
Jacob Keller (3):
      check-mailmap: accept "user@host" contacts
      [1] check-mailmap: add options for additional mailmap sources
      send-email: add mailmap support via sendemail.mailmap and --mailmap

 mailmap.h                           |   7 +++
 builtin/check-mailmap.c             |  25 +++++---
 mailmap.c                           |   9 +--
 Documentation/git-check-mailmap.txt |  18 ++++--
 git-send-email.perl                 |  20 ++++++
 t/t4203-mailmap.sh                  |  33 +++++++++-
 t/t9001-send-email.sh               | 122 ++++++++++++++++++++++++++++++++++++
 7 files changed, 215 insertions(+), 19 deletions(-)
---
base-commit: 87a1768b93a67d0420255a43d9e07387b2e805ad
change-id: 20240816-jk-send-email-mailmap-support-1a9e86867c72

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

[PATCH v2 1/3] check-mailmap: accept "user@host" contacts

[Jacob Keller]

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

git check-mailmap splits each provided contact using split_ident_line.
This function requires that the contact either be of the form "Name
<user@host>" or of the form "<user@host>". In particular, if the mail
portion of the contact is not surrounded by angle brackets,
split_ident_line will reject it.

This results in git check-mailmap rejecting attempts to translate simple
email addresses:

  $ git check-mailmap user@host
  fatal: unable to parse contact: user@host

This limits the usability of check-mailmap as it requires placing angle
brackets around plain email addresses.

In particular, attempting to use git check-mailmap to support mapping
addresses in git send-email is not straight forward. The sanitization
and validation functions in git send-email strip angle brackets from
plain email addresses. It is not trivial to add brackets prior to
invoking git check-mailmap.

Instead, modify check_mailmap() to allow such strings as contacts. In
particular, treat any line which cannot be split by split_ident_line as
a simple email address.

No attempt is made to actually parse the address line to validate that
it is actually an address. Doing so is non-trivial, and provides little
value. Either the provided input will correctly map via the map_user
call, or it will fail and be printed out, surrounded by angle brackets:

  $ git check-mailmap user@host
  <user@host>

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

diff --git a/builtin/check-mailmap.c b/builtin/check-mailmap.c
index b8a05b8e07b5..6b7fb53494f0 100644
--- a/builtin/check-mailmap.c
+++ b/builtin/check-mailmap.c
@@ -25,13 +25,17 @@ 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)))
-		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;
+	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 {
+		name = NULL;
+		namelen = 0;
+		mail = contact;
+		maillen = strlen(contact);
+	}
 
 	map_user(mailmap, &mail, &maillen, &name, &namelen);
 
diff --git a/Documentation/git-check-mailmap.txt b/Documentation/git-check-mailmap.txt
index 02f441832321..7747e38e25e3 100644
--- a/Documentation/git-check-mailmap.txt
+++ b/Documentation/git-check-mailmap.txt
@@ -15,10 +15,10 @@ SYNOPSIS
 DESCRIPTION
 -----------
 
-For each ``Name $$<user@host>$$'' or ``$$<user@host>$$'' from the command-line
-or standard input (when using `--stdin`), look up the person's canonical name
-and email address (see "Mapping Authors" below). If found, print them;
-otherwise print the input as-is.
+For each ``Name $$<user@host>$$'', ``$$<user@host>$$'', or ``$$user@host$$''
+from the command-line or standard input (when using `--stdin`), look up the
+person's canonical name and email address (see "Mapping Authors" below). If
+found, print them; otherwise print the input as-is.
 
 
 OPTIONS
diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
index 79e5f42760d9..0c1efe0b2e17 100755
--- a/t/t4203-mailmap.sh
+++ b/t/t4203-mailmap.sh
@@ -73,11 +73,40 @@ test_expect_success 'check-mailmap --stdin arguments: mapping' '
 '
 
 test_expect_success 'check-mailmap bogus contact' '
-	test_must_fail git check-mailmap bogus
+	cat >expect <<-EOF &&
+	<bogus>
+	EOF
+	git check-mailmap bogus >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'check-mailmap bogus contact --stdin' '
-	test_must_fail git check-mailmap --stdin bogus </dev/null
+	cat >expect <<-EOF &&
+	<bogus>
+	EOF
+	cat >stdin <<-EOF &&
+	bogus
+	EOF
+	git check-mailmap --stdin <stdin >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check-mailmap simple address: mapping' '
+	test_when_finished "rm .mailmap" &&
+	cat >.mailmap <<-EOF &&
+	New Name <$GIT_AUTHOR_EMAIL>
+	EOF
+	cat .mailmap >expect &&
+	git check-mailmap "$GIT_AUTHOR_EMAIL" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check-mailmap simple address: no mapping' '
+	cat >expect <<-EOF &&
+	<bugs@company.xx>
+	EOF
+	git check-mailmap "bugs@company.xx" >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'No mailmap' '

-- 
2.46.0.124.g2dc1a81c8933

[PATCH 1 v2 2/3] check-mailmap: add options for additional mailmap sources

[Jacob Keller]

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

The git check-mailmap command reads the mailmap from either the default
.mailmap location and then from the mailmap.blob and mailmap.file
configurations.

A following change to git send-email will want to support new
configuration options based on the configured identity. The
identity-based configuration and options only make sense in the context
of git send-email.

Expose the read_mailmap_file and read_mailmap_blob functions from
mailmap.c.  Teach git check-mailmap the --mailmap-file and
--mailmap-blob options which load the additional mailmap sources.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 mailmap.h                           |  7 +++++++
 builtin/check-mailmap.c             |  7 +++++++
 mailmap.c                           |  9 +++------
 Documentation/git-check-mailmap.txt | 10 ++++++++++
 4 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/mailmap.h b/mailmap.h
index cbda9bc5e0c1..908365e1bffa 100644
--- a/mailmap.h
+++ b/mailmap.h
@@ -6,6 +6,13 @@ struct string_list;
 extern char *git_mailmap_file;
 extern char *git_mailmap_blob;
 
+/* Flags for read_mailmap_file() */
+#define MAILMAP_NOFOLLOW (1<<0)
+
+int read_mailmap_file(struct string_list *map, const char *filename,
+		      unsigned flags);
+int read_mailmap_blob(struct string_list *map, const char *name);
+
 int read_mailmap(struct string_list *map);
 void clear_mailmap(struct string_list *map);
 
diff --git a/builtin/check-mailmap.c b/builtin/check-mailmap.c
index 6b7fb53494f0..2334b5722275 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 const char *mailmap_file, *mailmap_blob;
 static const char * const check_mailmap_usage[] = {
 N_("git check-mailmap [<options>] <contact>..."),
 NULL
@@ -16,6 +17,8 @@ NULL
 
 static const struct option check_mailmap_options[] = {
 	OPT_BOOL(0, "stdin", &use_stdin, N_("also read contacts from stdin")),
+	OPT_FILENAME(0, "mailmap-file", &mailmap_file, N_("read additional mailmap entries from file")),
+	OPT_STRING(0, "mailmap-blob", &mailmap_blob, N_("blob"), N_("read additional mailmap entries from blob")),
 	OPT_END()
 };
 
@@ -56,6 +59,10 @@ int cmd_check_mailmap(int argc, const char **argv, const char *prefix)
 		die(_("no contacts specified"));
 
 	read_mailmap(&mailmap);
+	if (mailmap_blob)
+		read_mailmap_blob(&mailmap, mailmap_blob);
+	if (mailmap_file)
+		read_mailmap_file(&mailmap, mailmap_file, 0);
 
 	for (i = 0; i < argc; ++i)
 		check_mailmap(&mailmap, argv[i]);
diff --git a/mailmap.c b/mailmap.c
index 2acf97f30760..9f9fa3199a85 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -142,11 +142,8 @@ static void read_mailmap_line(struct string_list *map, char *buffer)
 		add_mapping(map, name1, email1, name2, email2);
 }
 
-/* Flags for read_mailmap_file() */
-#define MAILMAP_NOFOLLOW (1<<0)
-
-static int read_mailmap_file(struct string_list *map, const char *filename,
-			     unsigned flags)
+int read_mailmap_file(struct string_list *map, const char *filename,
+		      unsigned flags)
 {
 	char buffer[1024];
 	FILE *f;
@@ -186,7 +183,7 @@ static void read_mailmap_string(struct string_list *map, char *buf)
 	}
 }
 
-static int read_mailmap_blob(struct string_list *map, const char *name)
+int read_mailmap_blob(struct string_list *map, const char *name)
 {
 	struct object_id oid;
 	char *buf;
diff --git a/Documentation/git-check-mailmap.txt b/Documentation/git-check-mailmap.txt
index 7747e38e25e3..966c91c46af7 100644
--- a/Documentation/git-check-mailmap.txt
+++ b/Documentation/git-check-mailmap.txt
@@ -27,6 +27,16 @@ OPTIONS
 	Read contacts, one per line, from the standard input after exhausting
 	contacts provided on the command-line.
 
+--mailmap-file=<file>::
+	In addition to any configured mailmap files, read the specified
+	mailmap file. Entries in this file take precedence over entries in
+	either the default mailmap file or any configured mailmap file.
+
+--mailmap-blob=<blob>::
+	Like `--mailmap-file`, but consider the value as a reference to a
+	blob in the repository. If both `--mailmap-file` and
+	`--mailmap-blob` are specified, entries in `--mailmap-file` will
+	take precedence.
 
 OUTPUT
 ------

-- 
2.46.0.124.g2dc1a81c8933

[PATCH v2 3/3] send-email: add mailmap support via sendemail.mailmap and --mailmap

[Jacob Keller]

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

In some 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 an upstream project,
but leaves before this change is submitted upstream.

In this case, the team members company address may no longer be valid,
and will thus bounce when sending email.

This can be manually avoided by editing the generated patch files, or by
carefully using --suppress-<cc|to> options. This requires a lot of
manual intervention and is easy to forget.

Git has support for mapping old email addresses and names to a canonical
name and address via the .mailmap file (and its associated mailmap.file,
mailmap.blob, and log.mailmap options).

Teach git send-email to enable mailmap support for all addresses. This
ensures that addresses point to the canonical real name and email
address.

Add the sendemail.mailmap configuration option and its associated
--mailmap (and --use-mailmap for compatibility with git log) options.
For now, the default behavior is to disable the mailmap in order to
avoid any surprises or breaking any existing setups.

These options support per-identity configuration via the
sendemail.identity configuration blocks. This enables identity-specific
configuration in cases where users may not want to enable support.

In addition, support send-email specific mailmap data via
sendemail.mailmap.file, sendemail.mailmap.blob and their
identity-specific variants.

The intention of these options is to enable mapping addresses which are
no longer valid to a current project or team maintainer. Such mappings
may change the actual person being referred to, and may not make sense
in a traditional mailmap file which is intended for updating canonical
name and address for the same individual.

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

diff --git a/git-send-email.perl b/git-send-email.perl
index 72044e5ef3a8..8995d5f12d9e 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.
@@ -272,12 +274,14 @@ sub do_edit {
 my ($auto_8bit_encoding);
 my ($compose_encoding);
 my ($sendmail_cmd);
+my ($mailmap_file, $mailmap_blob);
 # Variables with corresponding config settings & hardcoded defaults
 my ($debug_net_smtp) = 0;		# Net::SMTP, see send_message()
 my $thread = 1;
 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;
 
@@ -294,6 +298,7 @@ sub do_edit {
     "annotate" => \$annotate,
     "xmailer" => \$use_xmailer,
     "forbidsendmailvariables" => \$forbid_sendmail_variables,
+    "mailmap" => \$mailmap,
 );
 
 my %config_settings = (
@@ -327,6 +332,8 @@ sub do_edit {
 my %config_path_settings = (
     "aliasesfile" => \@alias_files,
     "smtpsslcertpath" => \$smtp_ssl_cert_path,
+    "mailmap.file" => \$mailmap_file,
+    "mailmap.blob" => \$mailmap_blob,
 );
 
 # Handle Uncouth Termination
@@ -524,6 +531,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 +1094,16 @@ sub expand_one_alias {
 our ($message_id, %mail, $subject, $in_reply_to, $references, $message,
 	$needs_confirm, $message_num, $ask_default);
 
+sub mailmap_address_list {
+	return @_ unless @_ and $mailmap;
+	my @options = ();
+	push(@options, "--mailmap-file=$mailmap_file") if $mailmap_file;
+	push(@options, "--mailmap-blob=$mailmap_blob") if $mailmap_blob;
+	my @addr_list = Git::command('check-mailmap', @options, @_);
+	s/^<(.*)>$/$1/ for @addr_list;
+	return @addr_list;
+}
+
 sub extract_valid_address {
 	my $address = shift;
 	my $local_part_regexp = qr/[^<>"\s@]+/;
@@ -1294,6 +1313,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..4bdc793a53a2 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -2379,6 +2379,128 @@ 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 'sendemail.mailmap configuration' '
+	clean_fake_sendmail &&
+	test_config mailmap.file "mailmap.test" &&
+	test_config sendemail.mailmap "true" &&
+	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 \
+		a.patch \
+		2>errors >out &&
+	grep "^!someone@example\.com!$" commandline1
+'
+
+test_expect_success $PREREQ 'sendemail.mailmap.file configuration' '
+	clean_fake_sendmail &&
+	test_config sendemail.mailmap.file "mailmap.test" &&
+	test_config sendemail.mailmap "true" &&
+	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 \
+		a.patch \
+		2>errors >out &&
+	grep "^!someone@example\.com!$" commandline1
+'
+
+test_expect_success $PREREQ 'sendemail.mailmap identity overrides configuration' '
+	clean_fake_sendmail &&
+	test_config sendemail.cloud.mailmap.file "mailmap.test" &&
+	test_config sendemail.mailmap "false" &&
+	test_config sendemail.cloud.mailmap "true" &&
+	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" \
+		--identity=cloud \
+		--to=someone@example.org \
+		a.patch \
+		2>errors >out &&
+	grep "^!someone@example\.com!$" commandline1
+'
+
+test_expect_success $PREREQ '--no-mailmap overrides configuration' '
+	clean_fake_sendmail &&
+	test_config sendemail.cloud.mailmap.file "mailmap.test" &&
+	test_config sendemail.mailmap "false" &&
+	test_config sendemail.cloud.mailmap "true" &&
+	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" \
+		--identity=cloud \
+		--to=someone@example.org \
+		--no-mailmap \
+		a.patch \
+		2>errors >out &&
+	grep "^!someone@example\.org!$" 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 v2 1/3] check-mailmap: accept "user@host" contacts

[Josh Steadmon]

On 2024.08.19 17:07, Jacob Keller wrote:
> From: Jacob Keller <jacob.keller@gmail.com>
> 
> git check-mailmap splits each provided contact using split_ident_line.
> This function requires that the contact either be of the form "Name
> <user@host>" or of the form "<user@host>". In particular, if the mail
> portion of the contact is not surrounded by angle brackets,
> split_ident_line will reject it.
> 
> This results in git check-mailmap rejecting attempts to translate simple
> email addresses:
> 
>   $ git check-mailmap user@host
>   fatal: unable to parse contact: user@host
> 
> This limits the usability of check-mailmap as it requires placing angle
> brackets around plain email addresses.
> 
> In particular, attempting to use git check-mailmap to support mapping
> addresses in git send-email is not straight forward. The sanitization
> and validation functions in git send-email strip angle brackets from
> plain email addresses. It is not trivial to add brackets prior to
> invoking git check-mailmap.
> 
> Instead, modify check_mailmap() to allow such strings as contacts. In
> particular, treat any line which cannot be split by split_ident_line as
> a simple email address.
> 
> No attempt is made to actually parse the address line to validate that
> it is actually an address. Doing so is non-trivial, and provides little
> value. Either the provided input will correctly map via the map_user
> call, or it will fail and be printed out, surrounded by angle brackets:
> 
>   $ git check-mailmap user@host
>   <user@host>
> 
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
>  builtin/check-mailmap.c             | 18 +++++++++++-------
>  Documentation/git-check-mailmap.txt |  8 ++++----
>  t/t4203-mailmap.sh                  | 33 +++++++++++++++++++++++++++++++--
>  3 files changed, 46 insertions(+), 13 deletions(-)
> 
> diff --git a/builtin/check-mailmap.c b/builtin/check-mailmap.c
> index b8a05b8e07b5..6b7fb53494f0 100644
> --- a/builtin/check-mailmap.c
> +++ b/builtin/check-mailmap.c
> @@ -25,13 +25,17 @@ 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)))
> -		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;
> +	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 {
> +		name = NULL;
> +		namelen = 0;
> +		mail = contact;
> +		maillen = strlen(contact);
> +	}
>  
>  	map_user(mailmap, &mail, &maillen, &name, &namelen);
>  
> diff --git a/Documentation/git-check-mailmap.txt b/Documentation/git-check-mailmap.txt
> index 02f441832321..7747e38e25e3 100644
> --- a/Documentation/git-check-mailmap.txt
> +++ b/Documentation/git-check-mailmap.txt
> @@ -15,10 +15,10 @@ SYNOPSIS
>  DESCRIPTION
>  -----------
>  
> -For each ``Name $$<user@host>$$'' or ``$$<user@host>$$'' from the command-line
> -or standard input (when using `--stdin`), look up the person's canonical name
> -and email address (see "Mapping Authors" below). If found, print them;
> -otherwise print the input as-is.
> +For each ``Name $$<user@host>$$'', ``$$<user@host>$$'', or ``$$user@host$$''
> +from the command-line or standard input (when using `--stdin`), look up the
> +person's canonical name and email address (see "Mapping Authors" below). If
> +found, print them; otherwise print the input as-is.
>  
>  
>  OPTIONS
> diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
> index 79e5f42760d9..0c1efe0b2e17 100755
> --- a/t/t4203-mailmap.sh
> +++ b/t/t4203-mailmap.sh
> @@ -73,11 +73,40 @@ test_expect_success 'check-mailmap --stdin arguments: mapping' '
>  '
>  
>  test_expect_success 'check-mailmap bogus contact' '
> -	test_must_fail git check-mailmap bogus
> +	cat >expect <<-EOF &&
> +	<bogus>
> +	EOF
> +	git check-mailmap bogus >actual &&
> +	test_cmp expect actual
>  '

I think I'd just remove this test case altogether, IIUC it' doesn't
provide any additional value vs. the "check-mailmap simple address: no
mapping" test below.


>  test_expect_success 'check-mailmap bogus contact --stdin' '
> -	test_must_fail git check-mailmap --stdin bogus </dev/null
> +	cat >expect <<-EOF &&
> +	<bogus>
> +	EOF
> +	cat >stdin <<-EOF &&
> +	bogus
> +	EOF
> +	git check-mailmap --stdin <stdin >actual &&
> +	test_cmp expect actual
> +'

Similarly, I might change this to use a real address instead of "bogus",
as we're no longer checking for invalid input.


> +test_expect_success 'check-mailmap simple address: mapping' '
> +	test_when_finished "rm .mailmap" &&
> +	cat >.mailmap <<-EOF &&
> +	New Name <$GIT_AUTHOR_EMAIL>
> +	EOF
> +	cat .mailmap >expect &&
> +	git check-mailmap "$GIT_AUTHOR_EMAIL" >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'check-mailmap simple address: no mapping' '
> +	cat >expect <<-EOF &&
> +	<bugs@company.xx>
> +	EOF
> +	git check-mailmap "bugs@company.xx" >actual &&
> +	test_cmp expect actual
>  '
>  
>  test_expect_success 'No mailmap' '
> 
> -- 
> 2.46.0.124.g2dc1a81c8933
> 

Re: [PATCH v2 0/3] send-email: add --mailmap support

[Josh Steadmon]

On 2024.08.19 17:07, Jacob Keller wrote:
> 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
> 
> With v2, we now have a configuration option (sendemail.mailmap) to enable
> this behavior. In addition, I enabled support for email-specific mailmap
> files.
> 
> The intention of these is to allow a maintainer to map the known-dead
> addresses of former colleagues onto a current email for an owner within the
> team. This would be used to update the send addresses to avoid including
> no-longer-valid addresses when sending patches. This is intended for cases
> where the original author is no longer valid such as when they are no
> longer employed to work on the project. While sometimes pointing to a
> canonical public address of that person may make sense, in other contexts,
> removing them from the email makes sense.
> 
> I believe this version solves the use case we have of ensuring that we stop
> sending emails with invalid addresses, and may be useful for others as
> well.
> 
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
> Changes in v2:
> - Loosen restriction on git check-mailmap by default, rather than
>   introducing a specific --no-brackets option.
> - Re-write commit message for the send-email changes.
> - Add --mailmap-file and --mailmap-blob options to git check-mailmap.
> - Add configuration options to git send-email for enabling mailmap support
>   by default, as well as providing send-email specific mailmap files.
> - Link to v1: https://lore.kernel.org/r/20240816-jk-send-email-mailmap-support-v1-0-68ca5b4a6078@gmail.com
> - Link to previous "v0": https://lore.kernel.org/r/20240813-jk-support-mailmap-git-format-patch-v1-1-1aea690ea5dd@gmail.com
> 
> ---
> Jacob Keller (3):
>       check-mailmap: accept "user@host" contacts
>       [1] check-mailmap: add options for additional mailmap sources
>       send-email: add mailmap support via sendemail.mailmap and --mailmap
> 
>  mailmap.h                           |   7 +++
>  builtin/check-mailmap.c             |  25 +++++---
>  mailmap.c                           |   9 +--
>  Documentation/git-check-mailmap.txt |  18 ++++--
>  git-send-email.perl                 |  20 ++++++
>  t/t4203-mailmap.sh                  |  33 +++++++++-
>  t/t9001-send-email.sh               | 122 ++++++++++++++++++++++++++++++++++++
>  7 files changed, 215 insertions(+), 19 deletions(-)
> ---
> base-commit: 87a1768b93a67d0420255a43d9e07387b2e805ad
> change-id: 20240816-jk-send-email-mailmap-support-1a9e86867c72
> 
> Best regards,
> -- 
> Jacob Keller <jacob.keller@gmail.com>
> 

This looks good to me, just some small test complaints in Patch #1.
Thanks for sending the series!

Re: [PATCH v2 1/3] check-mailmap: accept "user@host" contacts

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

>>  test_expect_success 'check-mailmap bogus contact' '
>> -	test_must_fail git check-mailmap bogus
>> +	cat >expect <<-EOF &&
>> +	<bogus>
>> +	EOF
>> +	git check-mailmap bogus >actual &&
>> +	test_cmp expect actual
>>  '
>
> I think I'd just remove this test case altogether, IIUC it' doesn't
> provide any additional value vs. the "check-mailmap simple address: no
> mapping" test below.

Sorry, but I do not follow.  The other one is <bogus@company.xx>
that looks more globally routable address than a local-only <bogus>
mailbox.  Isn't it worth ensuring that we will keep treating them
the same way?

Having said that ...

>> -For each ``Name $$<user@host>$$'' or ``$$<user@host>$$'' from the command-line
>> -or standard input (when using `--stdin`), look up the person's canonical name
>> -and email address (see "Mapping Authors" below). If found, print them;
>> -otherwise print the input as-is.
>> +For each ``Name $$<user@host>$$'', ``$$<user@host>$$'', or ``$$user@host$$''
>> +from the command-line or standard input (when using `--stdin`), look up the
>> +person's canonical name and email address (see "Mapping Authors" below). If
>> +found, print them; otherwise print the input as-is.

... it seems that <user> without <@host> is a supported format.
Should we update the document, too? 

If the @host-less name is meant to trigger a random unspecified
behaviour, whatever the code happens to do, that is perfectly fine,
but then we probably should not be etching it in the stone by
writing a test for it.  So because of a  reason that is completely
different from yours, I'd support removal of the "bogus" test, if
that is the case.

Thanks.

Re: [PATCH v2 1/3] check-mailmap: accept "user@host" contacts

[Eric Sunshine]

On Wed, Aug 21, 2024 at 1:50 PM Josh Steadmon <steadmon@google.com> wrote:
> On 2024.08.19 17:07, Jacob Keller wrote:
> > diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
> > @@ -73,11 +73,40 @@ test_expect_success 'check-mailmap --stdin arguments: mapping' '
> >  test_expect_success 'check-mailmap bogus contact' '
> > -     test_must_fail git check-mailmap bogus
> > +     cat >expect <<-EOF &&
> > +     <bogus>
> > +     EOF
> > +     git check-mailmap bogus >actual &&
> > +     test_cmp expect actual
> >  '
>
> I think I'd just remove this test case altogether, IIUC it' doesn't
> provide any additional value vs. the "check-mailmap simple address: no
> mapping" test below.

I had the same thought upon reading this.

> >  test_expect_success 'check-mailmap bogus contact --stdin' '
> > -     test_must_fail git check-mailmap --stdin bogus </dev/null
> > +     cat >expect <<-EOF &&
> > +     <bogus>
> > +     EOF
> > +     cat >stdin <<-EOF &&
> > +     bogus
> > +     EOF
> > +     git check-mailmap --stdin <stdin >actual &&
> > +     test_cmp expect actual
> > +'
>
> Similarly, I might change this to use a real address instead of "bogus",
> as we're no longer checking for invalid input.

Ditto for this change, but even more so because this is a fairly
significant semantic change. In particular, the documented and
intended behavior of the command when --stdin is specified is that it
will consume email addresses from *both* the command-line and from
standard input, and I think the point of the original test was to
verify that it still correctly recognized a bogus email address
specified as an argument even when --stdin is requested. Given that
understanding (assuming it's correct), then the original test was
already perhaps somewhat iffy anyhow, but after this change, it is
even less meaningful.

Re: [PATCH v2 1/3] check-mailmap: accept "user@host" contacts

[Jacob Keller]

On 8/21/2024 11:26 AM, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> 
>>>  test_expect_success 'check-mailmap bogus contact' '
>>> -	test_must_fail git check-mailmap bogus
>>> +	cat >expect <<-EOF &&
>>> +	<bogus>
>>> +	EOF
>>> +	git check-mailmap bogus >actual &&
>>> +	test_cmp expect actual
>>>  '
>>
>> I think I'd just remove this test case altogether, IIUC it' doesn't
>> provide any additional value vs. the "check-mailmap simple address: no
>> mapping" test below.
> 
> Sorry, but I do not follow.  The other one is <bogus@company.xx>
> that looks more globally routable address than a local-only <bogus>
> mailbox.  Isn't it worth ensuring that we will keep treating them
> the same way?
> 
> Having said that ...
> 
>>> -For each ``Name $$<user@host>$$'' or ``$$<user@host>$$'' from the command-line
>>> -or standard input (when using `--stdin`), look up the person's canonical name
>>> -and email address (see "Mapping Authors" below). If found, print them;
>>> -otherwise print the input as-is.
>>> +For each ``Name $$<user@host>$$'', ``$$<user@host>$$'', or ``$$user@host$$''
>>> +from the command-line or standard input (when using `--stdin`), look up the
>>> +person's canonical name and email address (see "Mapping Authors" below). If
>>> +found, print them; otherwise print the input as-is.
> 
> ... it seems that <user> without <@host> is a supported format.
> Should we update the document, too? 
> 

Its supported by happenstance, but i didn't want to encourage it.

> If the @host-less name is meant to trigger a random unspecified
> behaviour, whatever the code happens to do, that is perfectly fine,
> but then we probably should not be etching it in the stone by
> writing a test for it.  So because of a  reason that is completely
> different from yours, I'd support removal of the "bogus" test, if
> that is the case.
> 

I prefer removing the test. In an ideal world, I think we would probably
only accept actual <user@host> or user@host, but I did not think I would
create sufficiently correct parsing for addresses, so I left it out.

I did try looking up what the rules for addresses are, but it looks like
it got pretty complicated. I guess we could do very basic "it must have
an @ symbol, but anything else goes?


> Thanks.
> 

Re: [PATCH v2 1/3] check-mailmap: accept "user@host" contacts

[Jacob Keller]

On 8/21/2024 11:27 AM, Eric Sunshine wrote:
> On Wed, Aug 21, 2024 at 1:50 PM Josh Steadmon <steadmon@google.com> wrote:
>> On 2024.08.19 17:07, Jacob Keller wrote:
>>> diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
>>> @@ -73,11 +73,40 @@ test_expect_success 'check-mailmap --stdin arguments: mapping' '
>>>  test_expect_success 'check-mailmap bogus contact' '
>>> -     test_must_fail git check-mailmap bogus
>>> +     cat >expect <<-EOF &&
>>> +     <bogus>
>>> +     EOF
>>> +     git check-mailmap bogus >actual &&
>>> +     test_cmp expect actual
>>>  '
>>
>> I think I'd just remove this test case altogether, IIUC it' doesn't
>> provide any additional value vs. the "check-mailmap simple address: no
>> mapping" test below.
> 
> I had the same thought upon reading this.
> 

Yea, I think I've been convinced. I'll remove these tests.

>>>  test_expect_success 'check-mailmap bogus contact --stdin' '
>>> -     test_must_fail git check-mailmap --stdin bogus </dev/null
>>> +     cat >expect <<-EOF &&
>>> +     <bogus>
>>> +     EOF
>>> +     cat >stdin <<-EOF &&
>>> +     bogus
>>> +     EOF
>>> +     git check-mailmap --stdin <stdin >actual &&
>>> +     test_cmp expect actual
>>> +'
>>
>> Similarly, I might change this to use a real address instead of "bogus",
>> as we're no longer checking for invalid input.>
> Ditto for this change, but even more so because this is a fairly
> significant semantic change. In particular, the documented and
> intended behavior of the command when --stdin is specified is that it
> will consume email addresses from *both* the command-line and from
> standard input, and I think the point of the original test was to
> verify that it still correctly recognized a bogus email address
> specified as an argument even when --stdin is requested. Given that
> understanding (assuming it's correct), then the original test was
> already perhaps somewhat iffy anyhow, but after this change, it is
> even less meaningful.
> 

I tried to ensure we have test cases covering both --stdin and a
combination. I'll double check this in a v3 and ensure test cases
covering the behavior.

Thanks for the feedback!