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

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Feb 6, 2012, 23:11 UTC
Message-ID: <CAMP44s3i3eE8mVGHBB5zUdcX95+KEz9xcC1B3B6qON1StD=FXQ@mail.gmail.com>
In-Reply-To: <7vliof62ko.fsf@alter.siamese.dyndns.org>

On Tue, Feb 7, 2012 at 12:04 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 15 quoted lines

> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> This subject doesn't explain the *purpose* of the patch: always return
>> a plain mail address from map_user()
>
> That would be a much better subject.
>
>> I think the immediate problem should be here:
>>
>> Currently 'git blame -e' would add an extra '>' if map_user() returns
>> true, which would end up as '<foo@bar.com>>'. This is because
>> map_user() sometimes modifies, the mail string, but sometimes not. So
>> let's always modify it.
>
> That is just a symptom.

That's a matter of semantics. Is the API broken? Then the API has a problem, and you are fixing it, otherwise you are merely *improving* it.

> People who reached this commit by digging the
> history of mailmap.c would need to see the *cause* of the symptom
> described in the light of how the API is designed to be used.

Well, when you have a summary as "always return a plain mail address from map_user()", I think it's pretty clear what is the "problem"; the API does not always return a plain mail address. But any case, that is described below, in my suggestion.

Besides, even people digging the history would benefit from seeing the "symptom" first.

> In other
> words, "the code after the update has to be this way because these are the
> i/o constraints this API has".  "Otherwise you would see this breakage for
> example" is merely a supporting material.

I disagree. The reader of the current commit message will keep in his/her head the question "Why?", and it would be answered only at the very end. Besides, what more succinct way to describe a problem with the API than with an example (that happens to be real).

-- 
Felipe Contreras