Re: new CommitFest states (was: YAML)

Robert Haas wrote:
> On a related note, Greg Smith requested a state called "Discussing
> Review", which would logically follow "Needs Review" and precede
> "Waiting for Author"/"Ready for Committer"/"Returned with Feedback".
> I'm not altogether convinced of the value of that state, but I'm not
> altogether opposed to it either.  If we're going to have a discussion
> of CommitFest states, we probably ought to talk about that one, too.
>   
Don't know what it is about YAML that it encourages slipping into CF 
management..."Discussing Review" is a state patches seem to fall into 
for a time.  I'd like to see it added mainly because it simplifies work 
for a lazier (than Robert) CF manager like myself, which I think is a 
more appropriate target for this process.  Some of the process that 
works for him I don't think can be replicated by other personalities 
very well.

If a patch is being actively discussed on the list, often the author is 
at the mercy of said discussion ending before they can make another 
forward step; this is why "Waiting for Author" isn't appropriate.  
Having the patch sitting in "Needs Review" instead is unfair to the 
reviewer, who would like to be able to mark it as "I'm done" and move 
on.  That's also why sitting in that state takes up time for the CF 
manager.  They need to scan all "waiting for [author|review]" patches to 
get a list of people to nudge, and in this case there is no one to 
nudge--we're all at the mercy of the list to reach some sort of conclusion.

The obvious concern here is "who has the action item them?"  In this 
case, that's the CF manager's job--to help wrap the discussion up once 
it's died down and figure out what state the patch should go into next.  
Reviewers would mark a patch "Discussing Review" once they're done and 
have sent their review to the list when it doesn't fit any of the 
existing next states well, and they're not sure what happens next.  
Basically it allows them to formally push making a hard decision about a 
patch upwards, which is effectively what's happening now.  Then the CF 
manager or committer can mark it "returned with feedback" or flat-out 
"rejected" if the resulting discussion isn't favorable, rather than 
making the reviewer responsible for that, once discussion has wrapped 
up.  Or the author/CF manager can eventually mark it "waiting for 
author" once one of them has decided that's the logical next step.  I 
plan to turn the whole thing into a fairly easy to follow state diagram 
as documentation on the process, there's just this one common state I 
don't have a label for now:  when things are trapped on the list, and 
nobody has an obvious next move until that settles down.

There is no need or want for a "Needs discussion" before review or code 
submission.  That just encourages abusing the CF app and process for 
something you can do quite nicely with the mailing list.  If you believe 
in a feature but don't want to get community buy-in on the list first, 
write a patch to prove it works.  If the reviewer torpedos your patch, 
don't expect you'll get a free round of discussing whether everyone 
wants that feature or not out of the deal.  For this YAML example, had 
the code in the patch been junk we wouldn't even have gotten to 
discussion of its utility--the reviewer would have rejected it and that 
would have been it.  And that IMHO is how it should be.

-- 
Greg Smith    2ndQuadrant   Baltimore, MD
PostgreSQL Training, Services and Support
greg@2ndQuadrant.com  www.2ndQuadrant.com