On Wed, Feb 15, 2012 at 12:07 AM, Jeff King <peff@xxxxxxxx> wrote: > 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. Seriously? You want to add a note saying "These tests don't tackle any known issues". Well, if it did, logically, it would have been mentioned. >> > 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? You are right, it's impossible to decipher that the output of 'blame -e' is different than 'blame'. Surely, somebody must have made a mistake when the patch was applied. Or we can apply common sense. > 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? Because there's no need. Why would anybody add tests that are already there? Why do you have to assume the worst? >> > 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? If you mean the second patch, this was already pointed out by Junio, and I already said I would clarify that these are meant to target 'git blame' output. >> > 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. The why can be derived. Unless you seriously think Junio would allow patches that say they "add" tests for something (which imply there isn't tests for that), where they don't. >> 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. Better != more words. English, like code, is good as simple as possible (but not simpler). If you start from the premise that the patch is good; it has been s-o-b by Junio, and it has been applied, and released, what more do you need from the summary "t: mailmap: add 'git blame -e' tests". Can you derive the rest? 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