Re: review process

Re: review process

[Gerald Pfeifer]

[ gcc-patches -> gcc; please omit the former when replying! ]

On Mon, 31 Mar 2003, Fergus Henderson wrote:
>>> 	- define a new category of "self-approve" developers?
> Well, the difference is that you're only allowed to approve your own
> patches, not someone else's patches.  The intent here is to create
> a new position which is less trusted than a normal "maintainer",
> and whose approval powers are thus significantly limited --
> limited to only approving their own patches, only after these patches
> have been reviewed by another human, and only after a one-week delay.

So, wouldn't it be more natural, in some way, to have maintainers that
may approve all patches _but_ their own?  (I'm not sure how well reviews
by arbitrary third parties would work, both procedurally and related to
quality.)

Managing droped patches using Bugzilla sounds like a good idea, and in
fact I have suggested it at least twice in the past.

My message at http://gcc.gnu.org/ml/gcc/2003-03/msg01529.html essentially
has all three suggestions which came up in this thread, BTW. :->

Gerald

-- 
Gerald "Jerry"   pfeifer@dbai.tuwien.ac.at   http://www.pfeifer.com/gerald/

Re: review process

[Daniel Berlin]

On Monday, March 31, 2003, at 02:46  AM, Gerald Pfeifer wrote:

> [ gcc-patches -> gcc; please omit the former when replying! ]
>
> On Mon, 31 Mar 2003, Fergus Henderson wrote:
>>>> 	- define a new category of "self-approve" developers?
>> Well, the difference is that you're only allowed to approve your own
>> patches, not someone else's patches.  The intent here is to create
>> a new position which is less trusted than a normal "maintainer",
>> and whose approval powers are thus significantly limited --
>> limited to only approving their own patches, only after these patches
>> have been reviewed by another human, and only after a one-week delay.
>
> So, wouldn't it be more natural, in some way, to have maintainers that
> may approve all patches _but_ their own?  (I'm not sure how well 
> reviews
> by arbitrary third parties would work, both procedurally and related to
> quality.)
>
> Managing droped patches using Bugzilla sounds like a good idea, and in
> fact I have suggested it at least twice in the past.

I can *almost* automate detecting missed patches after a day, too, 
since patch submission is generally in one of a small number of formats.
It's actually detecting approval that is the tricky part right now.

If we could
1. Be okay with the idea that any patch not submitted in a reasonable 
format (IE changelog + patch in message, or changelog in message + 
attached file with patch), and with [PATCH] in the subject line, 
wouldn't be tracked.
2. Be okay with the idea that: patches approved by people without 
reasonable mailers (reasonable being those that can tell us whether a 
message is a reply to another message), when it's not easily detectable 
otherwise, will be assumed to not be approved.
3. Standardize the text + possible placement of approval messages.

then I'm pretty sure i could auto-track missed patches.

But it's worthless if we can't detect already-approved patches, thus 
the need to come up with a format for approval messages.

Obviously, i don't need something in some strict format, I just need to 
be able to determine that a given message containing patch was approved 
(or approved with changes, etc).  Thus, simply saying that approval 
messages should contain a line starting with "approved".
>
> My message at http://gcc.gnu.org/ml/gcc/2003-03/msg01529.html 
> essentially
> has all three suggestions which came up in this thread, BTW. :->
>
> Gerald
>
> -- 
> Gerald "Jerry"   pfeifer@dbai.tuwien.ac.at   
> http://www.pfeifer.com/gerald/

Re: review process

[Phil Edwards]

Moving to gcc@.


On Mon, Mar 31, 2003 at 11:06:50AM -0500, Daniel Berlin wrote:
> 
> If we could
> 1. Be okay with the idea that any patch not submitted in a reasonable 
> format (IE changelog + patch in message, or changelog in message + 
> attached file with patch), and with [PATCH] in the subject line, 
> wouldn't be tracked.

This seems obvious.

> 2. Be okay with the idea that: patches approved by people without 
> reasonable mailers (reasonable being those that can tell us whether a 
> message is a reply to another message), when it's not easily detectable 
> otherwise, will be assumed to not be approved.

There are only two or three people in the MAINTAINERS file that have
mailers like this.  This sounds good too (democracy at work).


> 3. Standardize the text + possible placement of approval messages.
> 
> then I'm pretty sure i could auto-track missed patches.
> 
> But it's worthless if we can't detect already-approved patches, thus 
> the need to come up with a format for approval messages.
> 
> Obviously, i don't need something in some strict format, I just need to 
> be able to determine that a given message containing patch was approved 
> (or approved with changes, etc).  Thus, simply saying that approval 
> messages should contain a line starting with "approved".

Most such systems also have a "stop processing" keyword or phrase, e.g.,

    approved
    thankyou

    J.Random User wrote:
    > Here's my patch, yadda yadda yadda.

    Please also consider a followup patch that makes this work on platforms
    which do not necessarily obey the laws of physics during runtime loading.


Phil

-- 
If ye love wealth greater than liberty, the tranquility of servitude greater
than the animating contest for freedom, go home and leave us in peace.  We seek
not your counsel, nor your arms.  Crouch down and lick the hand that feeds you;
and may posterity forget that ye were our countrymen.            - Samuel Adams

Re: review process

[Daniel Berlin]

On Monday, March 31, 2003, at 12:04  PM, Phil Edwards wrote:

>
> Moving to gcc@.
>

Whoops, didn't notice it was somewhere else.
:)

>
> On Mon, Mar 31, 2003 at 11:06:50AM -0500, Daniel Berlin wrote:
>>
>> If we could
>> 1. Be okay with the idea that any patch not submitted in a reasonable
>> format (IE changelog + patch in message, or changelog in message +
>> attached file with patch), and with [PATCH] in the subject line,
>> wouldn't be tracked.
>
> This seems obvious.
>
>> 2. Be okay with the idea that: patches approved by people without
>> reasonable mailers (reasonable being those that can tell us whether a
>> message is a reply to another message), when it's not easily 
>> detectable
>> otherwise, will be assumed to not be approved.
>
> There are only two or three people in the MAINTAINERS file that have
> mailers like this.  This sounds good too (democracy at work).
>
I just wanted to make it explicit that i can't handle it otherwise.
My current view is that this is a pretty simple SQL application (using 
a new database and the bug database), like so:
1. Any mail with [PATCH] in the title that we can see a well-formed 
patch in gets inserted into the new db (indexed by subject and message 
id)
2. Any mail approvals/denials for that message delete the [PATCH] 
record from the new db and update the bug (closing it), if one exists 
in the bug db. Since approvals/rejections can come at any time, we may 
or may not have created the bug report yet.
3. Every night, a cron job runs, and anything still in the database 
that is 1 day old and has no bug report created, gets one created.

That should work, no?

>
>> 3. Standardize the text + possible placement of approval messages.
>>
>> then I'm pretty sure i could auto-track missed patches.
>>
>> But it's worthless if we can't detect already-approved patches, thus
>> the need to come up with a format for approval messages.
>>
>> Obviously, i don't need something in some strict format, I just need 
>> to
>> be able to determine that a given message containing patch was 
>> approved
>> (or approved with changes, etc).  Thus, simply saying that approval
>> messages should contain a line starting with "approved".
>
> Most such systems also have a "stop processing" keyword or phrase, 
> e.g.,
>
That would work, if it's amenable to people.


>     approved
>     thankyou
>
>     J.Random User wrote:
>> Here's my patch, yadda yadda yadda.
>
>     Please also consider a followup patch that makes this work on 
> platforms
>     which do not necessarily obey the laws of physics during runtime 
> loading.
>
>
> Phil
>
> -- 
> If ye love wealth greater than liberty, the tranquility of servitude 
> greater
> than the animating contest for freedom, go home and leave us in peace. 
>  We seek
> not your counsel, nor your arms.  Crouch down and lick the hand that 
> feeds you;
> and may posterity forget that ye were our countrymen.            - 
> Samuel Adams

Re: review process

[Tom Tromey]

>>>>> "Dan" == Daniel Berlin <dberlin@dberlin.org> writes:

Dan> 1. Be okay with the idea that any patch not submitted in a reasonable
Dan>    format (IE changelog + patch in message, or changelog in message +
Dan>    attached file with patch), and with [PATCH] in the subject line,
Dan>    wouldn't be tracked.

I prefer getting patches with embedded ChangeLog entries that have
been processed with Alexandre Oliva's clcleanup script.  This makes it
simpler for me to apply patches that span multiple directories.  So
I'd like it if we didn't have to require that the ChangeLog entry
appear in the body of the message.

Dan> 3. Standardize the text + possible placement of approval messages.

I think the benefits of automated patch tracking are definitely worth
it, even if it means a new rule or two.  Plus, those of us with
programmable mailers can automate.

I'd also like a way to associate a given commit with an entry in the
patch database.  Then we could track the `applied' state in addition
to the `approved' state.  Ideally this would happen automatically
somehow, but I know that's asking a lot.

It would also be nice if followups on the mailing list were recorded
in (or linked to from) the patch database entry.  Then someone looking
at the patch database could immediately see the discussion, if there
was one.

Tom

Re: review process

[Alexandre Oliva]

On Mar 31, 2003, Daniel Berlin <dberlin@dberlin.org> wrote:

> Obviously, i don't need something in some strict format, I just need
> to be able to determine that a given message containing patch was
> approved (or approved with changes, etc).  Thus, simply saying that
> approval messages should contain a line starting with "approved".

Why is it so important to tell whether a patch was approved or not?
It seems to me that it would be enough to use the presence of a
follow-up as an heuristics.  If there's a follow-up, it's either 
approval, rejection, conditional approval or discussion that will end
up with either a revised patch or a late approval/rejection.  So it
appears to me that the only case that really matters to detect patches
that didn't get a review is that of a patch that didn't get any
followup whatsoever.  False positives can always be corrected by the
person who posted the patch originally, sending a new `Ok to install?'
message.

-- 
Alexandre Oliva   Enjoy Guarana', see http://www.ic.unicamp.br/~oliva/
Red Hat GCC Developer                 aoliva@{redhat.com, gcc.gnu.org}
CS PhD student at IC-Unicamp        oliva@{lsd.ic.unicamp.br, gnu.org}
Free Software Evangelist                Professional serial bug killer

Re: review process

[Daniel Berlin]

On Monday, March 31, 2003, at 02:32  PM, Alexandre Oliva wrote:

> On Mar 31, 2003, Daniel Berlin <dberlin@dberlin.org> wrote:
>
>> Obviously, i don't need something in some strict format, I just need
>> to be able to determine that a given message containing patch was
>> approved (or approved with changes, etc).  Thus, simply saying that
>> approval messages should contain a line starting with "approved".
>
> Why is it so important to tell whether a patch was approved or not?

Because our goal is to avoid patches being dropped on the floor.
There are plenty of times where there is a lot of discussion, and 
updates, but then it just sits there waiting for final 
approval/rejection.
This is "dropped on the floor".

Re: review process

[Alexandre Oliva]

On Mar 31, 2003, Daniel Berlin <dberlin@dberlin.org> wrote:

>> Why is it so important to tell whether a patch was approved or not?

> Because our goal is to avoid patches being dropped on the floor.

My take is that, if there's a lot of discussion but the proponent of
the patch feels the patch should get reviewed under the light of the
discussion that ensued, an `Ok to install?' message containing a new
copy of the patch, or just the URL to the original posting, should
suffice.

-- 
Alexandre Oliva   Enjoy Guarana', see http://www.ic.unicamp.br/~oliva/
Red Hat GCC Developer                 aoliva@{redhat.com, gcc.gnu.org}
CS PhD student at IC-Unicamp        oliva@{lsd.ic.unicamp.br, gnu.org}
Free Software Evangelist                Professional serial bug killer

Re: review process

[Hans-Peter Nilsson]

On Mon, 31 Mar 2003, Daniel Berlin wrote:
> I can *almost* automate detecting missed patches after a day, too,
> since patch submission is generally in one of a small number of formats.
> It's actually detecting approval that is the tricky part right now.
>
> If we could
> 1. Be okay with the idea that any patch not submitted in a reasonable
> format (IE changelog + patch in message, or changelog in message +
> attached file with patch), and with [PATCH] in the subject line,
> wouldn't be tracked.

And [RFA:] (Request For Approval.  No, I don't remember the
origin of this TLA.)  Though I'm not sure automatic tracking of
patches and approvals is doable; I think there'd be too much
noise for it to be useful.  Oh well, never mind: prove me wrong.

brgds, H-P

Re: review process

[Nathanael Nerode]

Daniel Berlin said:
 >If our problem is really in reviewing significant patches (ie i'm 
 >assuming we aren't dropping 3 line patches on the floor, and that 
 >people just approve these while reading through email regularly), we 
 >could track those that aren't approved in a day through bugzilla.

I was certainly under the impression that we were also dropping 3-line 
patches on the floor.

--Nathanael

Re: review process

[Richard Henderson]

On Mon, Mar 31, 2003 at 01:45:25PM -0500, Nathanael Nerode wrote:
> I was certainly under the impression that we were also dropping 3-line 
> patches on the floor.

It's not the size of the patch, but the (perceved) amount of
analysis required to validate it.


r~