Thread: "rejected" vs "returned with feedback" in new CF app

I tried to mark the "UPDATE SET (*)" patch as "returned with feedback",
but the CF app informed me that if I did that the patch would
automatically be moved to the next commitfest.  That seems completely
stupid.  There is no need to reconsider it unless a new version of the
patch is forthcoming (which there may or may not ever be, but that's
beside the point for now).  When and if the author does submit a new
patch, that would be the time to include it in the next commitfest, no?

I ended up marking it "rejected" instead, but that seems a bit harsh.
        regards, tom lane

On 4/7/15 3:33 PM, Tom Lane wrote:
> I tried to mark the "UPDATE SET (*)" patch as "returned with feedback",
> but the CF app informed me that if I did that the patch would
> automatically be moved to the next commitfest.  That seems completely
> stupid.  There is no need to reconsider it unless a new version of the
> patch is forthcoming (which there may or may not ever be, but that's
> beside the point for now).  When and if the author does submit a new
> patch, that would be the time to include it in the next commitfest, no?

I noticed that as well and have avoided closing some patches because of it.

On Tue, Apr 7, 2015 at 3:35 PM, Peter Eisentraut <peter_e@gmx.net> wrote:
> On 4/7/15 3:33 PM, Tom Lane wrote:
>> I tried to mark the "UPDATE SET (*)" patch as "returned with feedback",
>> but the CF app informed me that if I did that the patch would
>> automatically be moved to the next commitfest.  That seems completely
>> stupid.  There is no need to reconsider it unless a new version of the
>> patch is forthcoming (which there may or may not ever be, but that's
>> beside the point for now).  When and if the author does submit a new
>> patch, that would be the time to include it in the next commitfest, no?
>
> I noticed that as well and have avoided closing some patches because of it.

Several people, including me, have complained about this before.  I
hope that Magnus will fix it soon.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Apr 8, 2015 at 11:57 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Apr 7, 2015 at 3:35 PM, Peter Eisentraut <peter_e@gmx.net> wrote:
>> On 4/7/15 3:33 PM, Tom Lane wrote:
>>> I tried to mark the "UPDATE SET (*)" patch as "returned with feedback",
>>> but the CF app informed me that if I did that the patch would
>>> automatically be moved to the next commitfest.  That seems completely
>>> stupid.  There is no need to reconsider it unless a new version of the
>>> patch is forthcoming (which there may or may not ever be, but that's
>>> beside the point for now).  When and if the author does submit a new
>>> patch, that would be the time to include it in the next commitfest, no?
>>
>> I noticed that as well and have avoided closing some patches because of it.
>
> Several people, including me, have complained about this before.  I
> hope that Magnus will fix it soon.

Yeah, you can find references about that here and there... And the
current behavior is confusing.
-- 
Michael

On April 8, 2015 9:28:50 PM GMT+02:00, Magnus Hagander <magnus@hagander.net> wrote:
>On Wed, Apr 8, 2015 at 4:57 AM, Robert Haas <robertmhaas@gmail.com>
>wrote:
>
>> On Tue, Apr 7, 2015 at 3:35 PM, Peter Eisentraut <peter_e@gmx.net>
>wrote:
>> > On 4/7/15 3:33 PM, Tom Lane wrote:
>> >> I tried to mark the "UPDATE SET (*)" patch as "returned with
>feedback",
>> >> but the CF app informed me that if I did that the patch would
>> >> automatically be moved to the next commitfest.  That seems
>completely
>> >> stupid.  There is no need to reconsider it unless a new version of
>the
>> >> patch is forthcoming (which there may or may not ever be, but
>that's
>> >> beside the point for now).  When and if the author does submit a
>new
>> >> patch, that would be the time to include it in the next
>commitfest, no?
>> >
>> > I noticed that as well and have avoided closing some patches
>because of
>> it.
>>
>> Several people, including me, have complained about this before.  I
>> hope that Magnus will fix it soon.
>>
>
>
>Yeah, I think my doing so is more or less down to one of the hardest
>problems in IT - naming things. As in, what should we call that level.
>
>Right now we have "Committed", "Returned with feedback" and "Rejected"
>as
>the statuses that indicates something is "done for this commitfest". I
>do
>think we want to add another one of those to differentiate between
>these
>two states -- we could flag it as just "returned with feedback" and not
>move it, but if we do that we loose the ability to do statistics on it
>for
>example, and in order to figure out what happened you have to go look
>at
>the history details int he box at the bottom.
>
>So i think we need a specific label for it. Any suggestions for what it
>should be?

I'm not convinced we really need a version that closes and moves a entry. But if we indeed want it we can just name it
"moved".

--- 
Please excuse brevity and formatting - I am writing this on my mobile phone.

On 04/08/2015 03:28 PM, Magnus Hagander wrote:
> On Wed, Apr 8, 2015 at 4:57 AM, Robert Haas <robertmhaas@gmail.com 
> <mailto:robertmhaas@gmail.com>> wrote:
>
>     On Tue, Apr 7, 2015 at 3:35 PM, Peter Eisentraut <peter_e@gmx.net
>     <mailto:peter_e@gmx.net>> wrote:
>     > On 4/7/15 3:33 PM, Tom Lane wrote:
>     >> I tried to mark the "UPDATE SET (*)" patch as "returned with
>     feedback",
>     >> but the CF app informed me that if I did that the patch would
>     >> automatically be moved to the next commitfest. That seems
>     completely
>     >> stupid.  There is no need to reconsider it unless a new version
>     of the
>     >> patch is forthcoming (which there may or may not ever be, but
>     that's
>     >> beside the point for now).  When and if the author does submit
>     a new
>     >> patch, that would be the time to include it in the next
>     commitfest, no?
>     >
>     > I noticed that as well and have avoided closing some patches
>     because of it.
>
>     Several people, including me, have complained about this before.  I
>     hope that Magnus will fix it soon.
>
>
>
> Yeah, I think my doing so is more or less down to one of the hardest 
> problems in IT - naming things. As in, what should we call that level.
>
> Right now we have "Committed", "Returned with feedback" and "Rejected" 
> as the statuses that indicates something is "done for this 
> commitfest". I do think we want to add another one of those to 
> differentiate between these two states -- we could flag it as just 
> "returned with feedback" and not move it, but if we do that we loose 
> the ability to do statistics on it for example, and in order to figure 
> out what happened you have to go look at the history details int he 
> box at the bottom.
>
> So i think we need a specific label for it. Any suggestions for what 
> it should be?
>
>

If we're moving it to the next commitfest, maybe "Delayed with 
feedback". "Returned with feedback" should be putting the ball back in 
the submitter's court for further action.

cheers

andrew

On Apr 9, 2015, at 1:08 AM, Andres Freund <andres@anarazel.de> wrote:
> I'm not convinced we really need a version that closes and moves a entry. But if we indeed want it we can just name
it"moved". 

+1.

...Robert

On Thu, Apr 9, 2015 at 9:20 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Apr 9, 2015, at 1:08 AM, Andres Freund <andres@anarazel.de> wrote:
>> I'm not convinced we really need a version that closes and moves a entry. But if we indeed want it we can just name
it"moved".
 
>
> +1.

+1. Sounds like a good idea. It would be good to get something in this
area before the virtual deadline of 4/15, switching the current CF to
money time...
-- 
Michael

<p dir="ltr"><br /> On Apr 9, 2015 2:20 AM, "Robert Haas" <<a
href="mailto:robertmhaas@gmail.com">robertmhaas@gmail.com</a>>wrote:<br /> ><br /> > On Apr 9, 2015, at 1:08
AM,Andres Freund <<a href="mailto:andres@anarazel.de">andres@anarazel.de</a>> wrote:<br /> > > I'm not
convincedwe really need a version that closes and moves a entry. But if we indeed want it we can just name it
"moved".<br/> ><br /> > +1.<p dir="ltr">Is that at +1 for naming it moved, or for not having it? :-)<p
dir="ltr">Ican definitely go with moved. Buy I would like to keep it - the reason for having it in the first place is
tomake the history of the patch follow along when it goes to the next cf. If we don't have the move option, I think
it'slikely that we'll be back to the same patch having multiple completely unrelated entries in different cfs. <p
dir="ltr">/Magnus<br /> 

Magnus Hagander <magnus@hagander.net> writes:
> On Apr 9, 2015 2:20 AM, "Robert Haas" <robertmhaas@gmail.com> wrote:
>> +1.

> Is that at +1 for naming it moved, or for not having it? :-)

> I can definitely go with moved. Buy I would like to keep it - the reason
> for having it in the first place is to make the history of the patch follow
> along when it goes to the next cf. If we don't have the move option, I
> think it's likely that we'll be back to the same patch having multiple
> completely unrelated entries in different cfs.

The problem with the whole thing is that you're asking the person doing
the "returned" marking to guess whether the patch will be resubmitted in
a future CF.

The right workflow here, IMO, is that a patch should be marked returned or
rejected, full stop; and then when/if the author submits a new version for
a future CF, there should be a way *at that time* to re-link the email
thread into that future CF.

"Moved" is really only applicable, I think, for cases where we punt a
patch to the next CF for lack of time.
        regards, tom lane

On 2015-04-09 15:09:55 +0200, Magnus Hagander wrote:
> If we just link the email thread, that would mean we loose all those
> precious annotations we just added support for. Is that really what you
> meant? We also loose all history of a patch, and can't see that a previous
> version existed in a previous commitfest, without manually checking each
> and every one. But if that's a history we don't *want*, that's of course
> doable, but it seems wrong to me?

It'd be better if we kept them, but it's not *that* important imo. But
if the (documented) workflow would be to go to the old cf and click the
'move to next CF' button that'd not be a problem anyway.

Greetings,

Andres Freund

Magnus Hagander wrote:
> On Thu, Apr 9, 2015 at 2:20 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> > The right workflow here, IMO, is that a patch should be marked
> > returned or rejected, full stop; and then when/if the author submits
> > a new version for a future CF, there should be a way *at that time*
> > to re-link the email thread into that future CF.
> 
> If we just link the email thread, that would mean we loose all those
> precious annotations we just added support for. Is that really what
> you meant? We also loose all history of a patch, and can't see that a
> previous version existed in a previous commitfest, without manually
> checking each and every one. But if that's a history we don't *want*,
> that's of course doable, but it seems wrong to me?
> 
> I'm not necessarily saying that what we have now is right, but just
> giving up on the history completely doesn't seem like a very good
> workflow to me.
> 
> We could always tell those people to "go back and find your old patch
> and re-open it", but in fairness, are people likely to actually do
> that?

I think it's convenient if the submitter can go to a previous commitfest
and set an RwF entry as again needing review in the open commitfest.
That would keep the CF-app-history intact.  This should probably only be
allowed for patches that are either RwF or Rejected, and only in
commitfests that are already closed (perhaps allow it for the commitfest
in progress also?).

> > "Moved" is really only applicable, I think, for cases where we punt
> > a patch to the next CF for lack of time.
> 
> Well, that's basically what "returned with feedback" is now, so I
> guess that one should just be renamed in that case.

Yes, keeping the current behavior with name "Moved to next CF" seems
good to me.

> And we add a new "returned with feedback" that closes out the patch
> and doesn't move it anywhere.

Sounds good.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 04/09/2015 09:09 AM, Magnus Hagander wrote:
>
>
>
>     "Moved" is really only applicable, I think, for cases where we punt a
>     patch to the next CF for lack of time.
>
>
> Well, that's basically what "returned with feedback" is now, so I 
> guess that one should just be renamed in that case. And we add a new 
> "returned with feedback" that closes out the patch and doesn't move it 
> anywhere. Which is pretty similar to the suggestion earlier in this 
> thread except it also swaps the two names.
>

I think we should keep it, and see how it works in practice. I'd prefer 
a name like "deferred" to "moved" - the latter is a workflow process 
rather than a patch status, ISTM.

cheers

andrew

On Thu, Apr 9, 2015 at 2:20 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Magnus Hagander <magnus@hagander.net> writes:
> On Apr 9, 2015 2:20 AM, "Robert Haas" <robertmhaas@gmail.com> wrote:
>> +1.

> Is that at +1 for naming it moved, or for not having it? :-)

> I can definitely go with moved. Buy I would like to keep it - the reason
> for having it in the first place is to make the history of the patch follow
> along when it goes to the next cf. If we don't have the move option, I
> think it's likely that we'll be back to the same patch having multiple
> completely unrelated entries in different cfs.

The problem with the whole thing is that you're asking the person doing
the "returned" marking to guess whether the patch will be resubmitted in
a future CF.

The right workflow here, IMO, is that a patch should be marked returned or
rejected, full stop; and then when/if the author submits a new version for
a future CF, there should be a way *at that time* to re-link the email
thread into that future CF.

If we just link the email thread, that would mean we loose all those precious annotations we just added support for. Is that really what you meant? We also loose all history of a patch, and can't see that a previous version existed in a previous commitfest, without manually checking each and every one. But if that's a history we don't *want*, that's of course doable, but it seems wrong to me?

I'm not necessarily saying that what we have now is right, but just giving up on the history completely doesn't seem like a very good workflow to me.

We could always tell those people to "go back and find your old patch and re-open it", but in fairness, are people likely to actually do that?

"Moved" is really only applicable, I think, for cases where we punt a
patch to the next CF for lack of time.

Well, that's basically what "returned with feedback" is now, so I guess that one should just be renamed in that case. And we add a new "returned with feedback" that closes out the patch and doesn't move it anywhere. Which is pretty similar to the suggestion earlier in this thread except it also swaps the two names.

On Apr 9, 2015, at 5:50 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> The problem with the whole thing is that you're asking the person doing
> the "returned" marking to guess whether the patch will be resubmitted in
> a future CF.
>
> The right workflow here, IMO, is that a patch should be marked returned or
> rejected, full stop; and then when/if the author submits a new version for
> a future CF, there should be a way *at that time* to re-link the email
> thread into that future CF.

Yeah, or a way to reactivate the old entry at that time.  Any kind of routine carryover to the next CF is going to lead
toan accumulation of dead patches with live CF entries; it should require some action to re-enter a previously returned
patchin the latest CF. 

...Robert

David G. Johnston wrote:

> Can we create a "fake" CF time period into which all of these "waiting on
> author" entries can be placed and readily browsed/found instead of leaving
> them in whatever CF they happened to stall in?

This seems a good idea to me -- not a "fake CF", but a page listing all
the Returned With Feedback patches, regardless of which commitfest(s)
they're linked onto.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services