On Tue, Feb 14, 2012 at 11:52:11PM +0200, Felipe Contreras wrote: > 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. Really? I didn't see it mentioned in your commit message, which consisted entirely of: t: mailmap: add 'git blame -e' tests Yes, I know you mentioned it elsewhere in the thread. But review should happen on what is actually in the posted patch. That is what reviewers are guaranteed to have read, and it is what people reading "git log" in a year will see. If there was other discussion or context that led to the patch, it's helpful in both cases to summarize it. > > 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. Are there already git-blame tests, but not "blame -e" tests? If there are already "blame" tests, why do we additionally need "blame -e" tests? I can guess, or I can do my own digging in the history to find out, but that makes review a lot more painful. You already did the digging and came to understand the problem when you made your patch. Why not just share it? > > 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 Sorry, that should have been s/mailmap/blame/ above. But if I am making wrong assumptions about the rationale, then isn't that a sign that the commit message is insufficient? > > 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. Yes, I think you did describe the "what", which in this case is very simple. But as I mentioned before, it is not just knowing the "what" but evaluating that the "what" meets the "why" from step 1. > 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. I don't necessarily assume the worst. If I were the maintainer, I might even have taken your patch as-is, as it's pretty simple. But I found a description like the one Jonathan included to be _much_ easier to review. Whether yours was above a minimum threshold or not, I think it's worth striving to be better. -Peff -- 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