On 21.01.2013 02:07, Jeff Janes wrote:
> As a junior reviewer, I'd like to know if my main task should be to decide
> between 1) writing a review convincing you or Tom that your judgement is
> hasty, or 2) to convince the author that your judgement is correct. That
> would provide me with some direction, rather than just having me pondering
> whether a certain variable name ought to have one more or one fewer
> underscores in it. On the other hand if a summary judgement is that the
> patch is fundamentally sound but needs some line-by-line code-lawyering, or
> some performance testing, or documentation/usability testing, or needs to
> ponder the implications to part XYZ of the code base (which neither I nor
> the other have even heard of before), that would also be good to know.
>
> My desire is not for you to tell me what the outcome of the review should
> be, but rather what the focus of it should be.
Often a patch contains a contentious change buried deep in the patch.
The patch submitter might not realize there's anything out of the
ordinary in changing parser behavior based on a GUC, or doing things in
the executor that should be done in the planner, so he doesn't mention
it in the submission email or highlight it with any comments. The longer
the patch, the easier it is for things like that to hide in the crowd.
If a reviewer can bring those potentially contentious things that don't
"smell right" to light, that's extremely helpful. It's not so important
what judgment you make on it; just bringing it up, in a short, separate
reply to the email thread, will allow the submitter to reconsider, and a
committer to jump in early to say "yeah, you can't do that, because X.".
IMHO that's the single most important task of a review.
- Heikki