Re: September 2015 Commitfest - Mailing list pgsql-hackers

On Sat, Oct 31, 2015 at 08:03:59AM +0100, Robert Haas wrote:
> +1.  FWIW, I'm willing to review some patches for this CommitFest, but
> if the committers have to do first-round review as well as
> committer-review of every patch in the CommitFest, this is going to be
> long, ugly, and painful.  We need to have a substantial pool of
> non-committers involved in the review process so that at least some of
> the work gets spread out.

As a non-committer, let me offer my thoughts.

First, I would ask that every patch include a commit id that the patch
was generated against.  For example, I was looking at the "group command
option for psql" patch.  I created a branch off of master, but the patch
doesn't apply cleanly.  On further investigation, it looks like Adam
Brightwell noted this on September 21, but the patch hasn't been
updated.  If I knew which commit id the patch was created against, I
could create a branch from there and test the patch, but without, I need
to figure that out which is more work, or I need to re-create the patch,
which is also more work.

Second, it would be convenient if there were a make target that would
set up a test environment.  Effectively do what the 'make check' does,
but don't run the tests and leave the database up.  It should probably
drop you into a shell that has the paths set up as well.  Another
target should be available to shut it down.  So, what would be cool,
and make testing easier is if I could do a 'git checkout -b feature
abcdef' (or whatever the commit id is and branch name you want to use)
Then from there a

make
make check
make testrig
<whatever tests you want to do>
make testshutdown

These two would go a long way to making the process of actually
doing a review easier.

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?  Is the answer different if the patch author has supplied
a commit id that the patch was generated from?

There was then some further discussion on the interface, and what to do
with startup files, and nothing was really decided, and then the thread
petered out.  This potential reviewer is then left with the conclusion
that this patch really can't be reviewed, and it's not sure if it's even
wanted as is anyway.  So I move on to something else.  There was a bunch
of discussion by a bunch of committers, and no-one updated the status of
the patch in the commitfest, and there's no clear guidelines on what the
status should be in this case.

If I were needing to decide, I would say that the patch should either be
marked as "Waiting on Author" on the grounds that the patch doesn't
currently apply, or "Returned with feedback" on the grounds that there
was unaddressed feedback on the details of the patch, and it was noted
as a "proof of concept" when submitted anyway.  But I'm unwilling to
just change it to that without more clear definitions of the meaning of
each status.  A link to definitions and when the status should be
changed would help.

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?

-- 
nw