On Tue, Feb 14, 2012 at 11:14 PM, Jeff King <peff@xxxxxxxx> 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 -- 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