On Tue, Feb 14, 2012 at 02:34:31PM -0600, Jonathan Nieder wrote:
> Felipe Contreras wrote:
>
> > I sent both the fix and the tests. Another fix was applied, but we are still
> > missing the tests.
> >
> > These are good before, and after the fix.
>
> To summarize the previous discussion[1]: some people had comments, and
> you seem to have found value in exactly none of them. OK. CC-ing
> Peff, since he at least probably has looked over this code before.
The short answer is that both patches look OK to me.
Here's a longer digression that you may feel free to ignore.
When I investigated this problem, I looked only at the code (and I think
Junio's fix is the right one). However, having just looked at your two
patches without having previously looked at those tests, I found them
quite painless to review.
Having participated in the bug-hunt, I knew they were probably related
to testing that bug, but it was very unclear to me from the original
series why there were two patches, and not one. Or why, when we have
mailmap tests, they did not catch this bug. And that is _now_, with the
context of having just participated in the discussion; six months from
now I would have even less hope of understanding the context.
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? The answer can
be a simple "nobody bothered to write them", and that's OK. 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").
2. Figure out what the change should be doing. This should also be in
the commit message, though at a high level. In the case of tests,
obviously we're adding new tests. But are we properly covering all
of the new cases? Does the "what" match the "why" from (1)?
3. Look at the patch. Do the changes match what the commit message
claimed in (2)?
4. Look at the patch for style, implementation, etc. Is the code
quality good enough to be included?
The steps are dependent on each other, and I usually quit if I can't get
past one step. Sometimes I will look at the patch to try to figure out
(1) or (2), especially if the patch is trivial. But in my ideal world,
every patch lets me just walk through those steps.
I won't claim that these steps create error-free reviews. Based on those
steps, your patches look good to me, and it seems from Felipe's response
that there might be some inaccuracies in your description. But it at
least gives us a starting point for discussion.
-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