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

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Feb 5, 2012, 21:07 UTC
Message-ID: <CAMP44s0gf196UWQabdqPXF=YwJ0WYtU2P4Y6DCEONvkCDhbJAw@mail.gmail.com>
In-Reply-To: <7vvcnlggno.fsf@alter.siamese.dyndns.org>

On Sun, Feb 5, 2012 at 10:38 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 19 quoted lines

> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>>>> 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.

That description is *perfectly* fine. It's succinct, there's nothing wrong with that.

There's *nothing* else to say. There's no tests for 'git blame -e', the patch adds tests for 'git blame -e', that's all, thus the title "add 'git blame -e' tests.

The patch is good by itself, it doesn't _need_ any other context. It would detect regressions on 'git blame -e', obviously, which is good.

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

No, that's not true. This patch doesn't do that.

Show 5 quoted lines

> 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

Because it's not needed. Adding tests for 'git blame -e' is good in itself.

Some tests = better than no tests.

>  (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?

What is unclear of "add 'git blame -e' tests"? It adds tests, that's good.

Show 11 quoted lines

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

It wants to test the output of 'git blame -e', as it can be obviates from the title. Is there something wrong with that?

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

This patch is orthogonal to that; there are no tests for 'git blame -e'.

-- 
Felipe Contreras