| Message ID | 20240816-jk-send-email-mailmap-support-v1-1-68ca5b4a6078@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | send-email: add --mailmap support | expand |
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>
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>
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).
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):