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(at)2ndQuadrant(dot)com www.2ndQuadrant.com