From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Sat, 04 Feb 2012 19:30:42 GMT
Subject: Re: [bug] blame duplicates trailing ">" in mailmapped emails
Message-ID: <CAMP44s2r-fcnfpdT4u5U7TwF1z6Abp+J1U7oqfsSrYMuD6weOQ@mail.gmail.com>
In-Reply-To: <20120204182611.GA31091@sigill.intra.peff.net>

On Sat, Feb 4, 2012 at 8:26 PM, Jeff King <peff@peff.net> wrote:
> On Sat, Feb 04, 2012 at 05:46:05PM +0200, Felipe Contreras wrote:
>
>> 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:

Yes, I found that as well.

> 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';
>                }

Yes, I already did this. I'm writing tests for this right now, but I
think I found yet another bug...

> 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).

Perhaps, but I though the idea was to make it efficient. I think the
above fix should be ok.

We should have tests for this though, to make sure it doesn't get
broken again. I'm on that.

Cheers.

-- 
Felipe Contreras