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

On Wed, Feb 15, 2012 at 12:07 AM, Jeff King [off-list ref] wrote:

On Tue, Feb 14, 2012 at 11:52:11PM +0200, Felipe Contreras wrote:

quoted

On Tue, Feb 14, 2012 at 11:14 PM, Jeff King [off-list ref] wrote:

quoted

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.

quoted

quoted

    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?

quoted

quoted

    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.

quoted

quoted

 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.

quoted

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