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 -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers