Re: [PATCH 2/3] t: mailmap: add 'git blame -e' tests

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 5, 2012, 20:38 UTC
Message-ID: <7vvcnlggno.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CAMP44s2djXurzMSXLOAkx84Sm8P2YV67M1yS2AuidkfGbTdmEQ@mail.gmail.com>

Felipe Contreras <felipe.contreras@gmail.com> writes:

Show 13 quoted lines

>>> Look at the title:
>>> add 'git blame -e' tests
>>>
>>> s/blame/blame -e/
>>
>> And?  After copy/pasting this particular test with that substitution,
>> what do we get a test for?
>
> For 'git blame -e'.
>
>> What class of problem is it supposed to catch?
>
> Problems related to 'git blame -e'?

You very well know that we know you better than that, so it is no use to pretend to be dumb. It does not do anything other than wasting bandwidth and irritating readers.

You know that you are not addressing "git blame with the -e option shows wrong line numbers on its output". The symptom you are checking with is "e-mail address output from 'blame -e' used to add an extra '>' at the end when only name is mapped, and I fixed it with the previous patch."

Why is it so hard to either

 (1) give the more descriptive answer upfront when somebody who did not
     read the first patch of the 3-patch series pointed it out the comment
     is not descriptive enough the first time; or

 (2) give the more descriptive answer and then say "we could do that, but
     when somebody views this change in "git log" as a part of 3-patch
     series, it should be clear enough---so let's aim for brevity instead
     of adding that two-line description" to defend the line in the patch?

> Or just apply it. Don't let the perfect be the enemy of the good.

Perfect is the enemy of the good, but it is not an excuse to be sloppy.

I tend to think that a single line "# blame -e" is sufficient if this were a part of just a single patch that has the fix and the test to guard the fix against future breakage (i.e. "not sloppy"). I would even say that not even "# blame" is necessary in such a case.

But if this is a standalone patch to add this test, it does not describe what it wants to test very well.

In any case, I have doubts that the fix should go to blame and not to map_user(), so I'll see what happens in the further discussions.

Thanks.