Re: September 2015 Commitfest - Mailing list pgsql-hackers

[ separate response for questions that are about process not mechanics ]

Nathan Wagner <nw+pg@hydaspes.if.org> writes:
> Back to the patch in question, so Mr Brightwell noted that the patch
> doesn't apply against master.  Should someone then mark the patch as
> waiting on author?  Is failing to apply against master a 'waiting on
> author' cause?

I would say so, if the fix isn't obvious.  ("Obvious" might be different
for different people, but if you wanted to review the patch and couldn't
make it apply, I think it's fair to put the ball into the author's court.)

> Is the answer different if the patch author has supplied
> a commit id that the patch was generated from?

While that would give reviewers a chance to play with the original form
of the patch, it's still gonna have to get updated at some point if it's
ever to get committed.  And if there are conflicting patches that already
went in, there's a substantial risk that those patches may have broken
the new patch in some non-obvious way.  So I'm not sure that testing an
already-obsolete patch is all that valuable.  This is going to be
situation-dependent in many cases, but there is certainly nothing wrong
with just booting the patch back to the author when in doubt.

> What is "ready for committer" anyway?  It's clearly not "a committer
> will apply the patch", since things sit in that status without being
> committed.  If I think the patch is good and should be applied, do I
> mark it as ready for committer, and then later a committer will also
> review the patch?  If so, is doing anything other than the sanity
> checks, and possibly offering an opinion, on the patch even useful?

Our expectation is that committers will review whatever they commit,
because ultimately what goes in is their responsibility.  But that doesn't
make reviews by non-committers useless.  In the first place, you might
spot something the committer would have missed (nobody's perfect, so more
eyeballs are always better than fewer).  Even if the committer would have
found it, committer cycles are a limited resource around here, so it's
better if a bug gets found before it gets to the committer's review.
That's why we'd like at least one non-committer to have signed off on a
patch before a committer picks it up.

And there's a meta-agenda behind the whole CF process: getting more people
to study the Postgres code, even if it's a byproduct of examining patches
that ultimately get rejected, improves the community's abilities overall.
The way you get to having committer-grade skills is to spend a lot of time
reading and modifying the PG code.  Reviewing other peoples' patches is
one means to that end, especially if you spend time thinking about how
they did whatever they are trying to do and whether there are other better
ways to do it.  So ultimately we hope to get more committers out of the
process as well.
        regards, tom lane