Re: [PATCH v3 0/2] test: tests for the "double > from mailmap" bug

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Feb 14, 2012, 21:52 UTC
Message-ID: <CAMP44s0Dp9Av+ikFHa=QcqKFA5XL9ESBrzWLY0jkSCdH-NxhMw@mail.gmail.com>
In-Reply-To: <20120214211402.GC23291@sigill.intra.peff.net>

On Tue, Feb 14, 2012 at 11:14 PM, Jeff King <peff@peff.net> wrote:

Show 5 quoted lines

> My general review process is (in this order):
>
>  1. Figure out why a change is needed. This should be explained in the
>     commit message. And no, just adding tests is not assumed to be
>     needed.  Why did the old tests not cover this case?

As I already mentioned more than once; the first patch is not related to any fix.

>     The answer can
>     be a simple "nobody bothered to write them", and that's OK.

 That can be derived from the word "add". You can't add something that
is already there.

>     But
>     some description of the current state can help reviewers understand
>     the rationale (e.g., "we tested with shortlog, but not mailmap, and
>     certain code paths are only exposed through mailmap").

You are assuming too much. That's not the purpose, that's why I didn't menti

>  2. Figure out what the change should be doing.

t: mailmap: add 'git blame -e' tests

That's what the change should be doing; nothing more, nothing less.

I wonder why you have to assume the worst. When I see a commit message like that, I assume that there were no previous tests for that (thus the word 'add'), and that's all I need to know.

If you want to extrude meaning from where there isn't, well, go ahead, but there's nothing else really.

Cheers.

-- 
Felipe Contreras