On Thu, Jul 8, 2010 at 11:36 AM, Kevin Grittner
<Kevin.Grittner@wicourts.gov> wrote:
>> or had major design issues
>
> That's a much harder problem. The set of people who can review for
> that is rather smaller than the set who can see if a patch applies
> without error.
Well, true. But reporting whether the patch applies without error is
about the most minimal review possible, so that's not saying much.
Many of the patches that need review are relatively small ones, so we
need people to check whether they work, see if they have docs and
regression tests, try to break them, and also - importantly - offer an
opinion on whether the proposed feature is actually useful. Of course
others may disagree, but you can't reach consensus if nobody's willing
to offer a starting point for discussion.
Reviewers who can actually analyze the code even a little bit can find
many more issues. Does it fit the style of the surrounding code? Are
there unnecessary or unrelated changes in the patch? And for extra
credit, does it have bugs? What I do frequently is find a
"comparable" - something similar in the existing code - and compare
them side-by-side. For each change, I ask myself whether it's there
because the new functionality is different from the old, or whether
it's arbitrary.
I agree that the larger features need a different kind of analysis,
and if those get left for more experienced reviewers I think that's
fine. Hopefully, some of those people will volunteer to help out; I
noticed that Itagaki Takahiro has already started reviewing, and Bernd
Helmle has signed up to review one of my patches. But there is no
shortage of things that can be looked at by people who are doing this
first the first time, especially if they have even a passing ability
to read C code.
>> The good/bad news is that we don't have any major uncommitted
>> patches floating around unmerged at this pont, as we did last
>> cycle with HS and SR.
>
> I'm not sure I understand what you mean by "unmerged" -- are you
> excluding patches where they are in a git repository which is
> merging in HEAD on a regular basis?
By unmerged I meant simply uncommitted. I know we have a number of
fairly big patches, but I don't think they're as complex as HS and SR,
and they are not leftovers that were too big to swallow during the 9.0
cycle.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company