Re: Commitfest Bug (was: Re: Reusing abbreviated keys during second pass of ordered [set] aggregates)

On Tue, Mar 1, 2016 at 10:27 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> > Magnus Hagander wrote:
> >> That said, we can certainly reconsider that. Would we always copy the
> value
> >> over? Even if it was, say, rejected? (so it would be copied to the new
> CF
> >> but still marked rejected) Or is there a subset of behaviors you're
> looking
> >> for?
>
> > I think the states "Ready for Committer" and "Needs Review" ought to be
> > kept in the new CF.
>
> Definitely.
>
> > I'm unclear on what to do about "Returned with Feedback" and "Waiting on
> > Author"; my first instinct is that if a patch is in those states, then
> > it shouldn't be possible to move to the next CF. On the other hand, if
> > we force the state to change to "Needs Review" before moving it, we
> > would lose the information of what state it was closed with. So perhaps
> > for any patch in those two states, the state in the next CF should be
> > "needs review" too.
>
> +1 for not moving such patches to the new CF until the author does
> something --- at which point they'd change to "Needs Review" state.
> But we should not change them into that state without author input.
> And I don't see the value of having them in a new CF until the
> author does something.

Well, "waiting on author" is not considered to be a "closing". So as long
as we have patches in that state we want to do *something* with them in a
CF. Which currently can be "move to next cf", "reject" or "return with
feedback". So basically it means we have to decide if we want to reject it
or return-with-feedback it, if we say it should not be possible to "move to
next cf".

The problem is if the author already has something ready of course, in
which case it will become a new entry on the new CF instead of a moved one
-- which means we loose the historical connection between those two.

> > I am even more unclear on "Rejected". My instinct says we should refuse
> > a move-to-next-cf for such patches.
>
> Right. Rejected is dead, it shouldn't propagate forward.
>
>
Ok, so let's try to summarize. We want the following actinos when someone
clicks "move to next cf":

Needs Review -> Needs Review
Waiting on Author -> Refuse moving
Ready for committer -> Ready for Committer
Committed -> refuse moving
Moved to next cf -> refuse moving (if it's already set like this, it would
probably be a bug)
Returned with feedback -> Refuse moving

Which basically means we only move things that are in needs review or ready
for committer state, and that we never actually change the status of a
patch in the moving.

Is that a correct summary?

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/