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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 5, 2012, 20:16 UTC
Message-ID: <7v39aphw85.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20120204182611.GA31091@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 24 quoted lines

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