From: Felipe Contreras Date: Tue, 14 Feb 2012 21:52:11 GMT Subject: Re: [PATCH v3 0/2] test: tests for the "double > from mailmap" bug Message-ID: In-Reply-To: <20120214211402.GC23291@sigill.intra.peff.net> On Tue, Feb 14, 2012 at 11:14 PM, Jeff King wrote: > 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