On Tue, Feb 7, 2012 at 12:04 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Felipe Contreras <felipe.contreras@xxxxxxxxx> 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@xxxxxxx>>'. 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html