[bug] blame duplicates trailing ">" in mailmapped emails

From: Jeff King <peff@peff.net>
Date: Feb 4, 2012, 18:26 UTC
Message-ID: <20120204182611.GA31091@sigill.intra.peff.net>
In-Reply-To: <CAMP44s2QdJ4+qgg4fF5-DOWHx3Btd0pTivTT9s_E=qqxg16YLQ@mail.gmail.com>

On Sat, Feb 04, 2012 at 05:46:05PM +0200, Felipe Contreras wrote:

Show 5 quoted lines

> In any case, the one to blame for the header corruption is git:
> [...]
> f2bb9f88 (<spearce@spearce.org>> 2006-11-27 03:41:01 -0500 952)
> 
> Notice the mail is wrong.

Ugh. The fault lies in this code:

  $ sed -n 1405,1414p builtin/blame.c 
          if (map_user(&mailmap, mail+1, mail_len-1, person, tmp-person-1)) {
                  /* Add a trailing '>' to email, since map_user returns plain emails
                     Note: It already has '<', since we replace from mail+1 */
                  mailpos = memchr(mail, '\0', mail_len);
                  if (mailpos && mailpos-mail < mail_len - 1) {
                          *mailpos = '>';
                          *(mailpos+1) = '\0';
                  }
          }
  }

But that comment is wrong. If there's no email mapping needed, map_user will leave the "mail" buffer intact, in which case it will have the trailing ">" (because we feed the address with enclosing angle brackets). So while map_user tries to accept either "foo@example.com\0" and "foo@example.com>", it is up to the contents of the mailmap whether you get back something with the closing angle bracket or not. Which is a pretty error-prone interface.

You can fix it with this:

diff --git a/builtin/blame.c b/builtin/blame.c
index 5a67c20..9b886fa 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1406,7 +1406,8 @@ static void get_ac_line(const char *inbuf, const char *what,
 		/* Add a trailing '>' to email, since map_user returns plain emails
 		   Note: It already has '<', since we replace from mail+1 */
 		mailpos = memchr(mail, '\0', mail_len);
-		if (mailpos && mailpos-mail < mail_len - 1) {
+		if (mailpos && mailpos-mail < mail_len - 1 &&
+		    mailpos > mail && *(mailpos-1) != '>') {
 			*mailpos = '>';
 			*(mailpos+1) = '\0';
 		}

but it feels like the fix should go into map_user.  I tried a few things,
like "git log -1 --format=%aE", and couldn't find other code paths with
this problem. So presumably they are all feeding email addresses without
the closing ">" (so one option is to just say "map_user needs to get
NUL-terminated strings).

-Peff