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

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

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

Thanks.

The map_user() API takes an email address that is terminated by either NUL
or '>' to allow the caller to learn the corresponding up-to-date email
address that is NUL terminated, while indicating with its return value
that if the caller even needs to replace what it already has.  But the
function does not properly terminate email when it only touched the name
part. And I think that is the real bug.

So I agree that the real fix should go to map_user() so that when it says
"I've updated something, so pick up the updated result from the i/o
arguments you gave me, i.e. email and name", it makes sure what it claims
to be an e-mail address does not have the extra '>' in it.

Working around the current behaviour by forcing all callers that pass '>'
terminated e-mail address to have the code like the above quoted patch 
does not feel right.