* Re: review process @ 2003-03-31 19:52 Nathanael Nerode 2003-03-31 20:02 ` Richard Henderson 0 siblings, 1 reply; 11+ messages in thread From: Nathanael Nerode @ 2003-03-31 19:52 UTC (permalink / raw) To: gcc 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: review process 2003-03-31 19:52 review process Nathanael Nerode @ 2003-03-31 20:02 ` Richard Henderson 0 siblings, 0 replies; 11+ messages in thread From: Richard Henderson @ 2003-03-31 20:02 UTC (permalink / raw) To: Nathanael Nerode; +Cc: gcc 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~ ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <m3r88o255d.fsf@uniton.integrable-solutions.net>]
[parent not found: <1049055987.31893.378.camel@doubledemon.codesourcery.com>]
[parent not found: <m3r88oy3we.fsf@uniton.integrable-solutions.net>]
[parent not found: <1049059174.31893.385.camel@doubledemon.codesourcery.com>]
[parent not found: <m3d6k8y2eg.fsf@uniton.integrable-solutions.net>]
[parent not found: <1049059963.7575.391.camel@doubledemon.codesourcery.com>]
[parent not found: <m34r5ky1wc.fsf@uniton.integrable-solutions.net>]
[parent not found: <1049060738.31975.394.camel@doubledemon.codesourcery.com>]
[parent not found: <20030331041419.GA4687@ceres.cs.mu.oz.au>]
[parent not found: <20030331060732.GA10362@disaster.jaj.com>]
[parent not found: <20030331062256.GA6493@ceres.cs.mu.oz.au>]
* Re: review process [not found] ` <20030331062256.GA6493@ceres.cs.mu.oz.au> @ 2003-03-31 10:08 ` Gerald Pfeifer 2003-03-31 17:50 ` Daniel Berlin 0 siblings, 1 reply; 11+ messages in thread From: Gerald Pfeifer @ 2003-03-31 10:08 UTC (permalink / raw) To: gcc-patches, gcc, Fergus Henderson Cc: Phil Edwards, Mark Mitchell, Gabriel Dos Reis [ 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/ ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: review process 2003-03-31 10:08 ` Gerald Pfeifer @ 2003-03-31 17:50 ` Daniel Berlin 2003-03-31 18:13 ` Phil Edwards ` (3 more replies) 0 siblings, 4 replies; 11+ messages in thread From: Daniel Berlin @ 2003-03-31 17:50 UTC (permalink / raw) To: Gerald Pfeifer Cc: gcc-patches, gcc, Fergus Henderson, Phil Edwards, Mark Mitchell, Gabriel Dos Reis 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/ ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: review process 2003-03-31 17:50 ` Daniel Berlin @ 2003-03-31 18:13 ` Phil Edwards 2003-03-31 18:45 ` Daniel Berlin 2003-03-31 18:42 ` Tom Tromey ` (2 subsequent siblings) 3 siblings, 1 reply; 11+ messages in thread From: Phil Edwards @ 2003-03-31 18:13 UTC (permalink / raw) To: Daniel Berlin Cc: Gerald Pfeifer, gcc, Fergus Henderson, Mark Mitchell, Gabriel Dos Reis 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: review process 2003-03-31 18:13 ` Phil Edwards @ 2003-03-31 18:45 ` Daniel Berlin 0 siblings, 0 replies; 11+ messages in thread From: Daniel Berlin @ 2003-03-31 18:45 UTC (permalink / raw) To: Phil Edwards Cc: Gerald Pfeifer, gcc, Fergus Henderson, Mark Mitchell, Gabriel Dos Reis 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: review process 2003-03-31 17:50 ` Daniel Berlin 2003-03-31 18:13 ` Phil Edwards @ 2003-03-31 18:42 ` Tom Tromey 2003-03-31 20:06 ` Alexandre Oliva 2003-04-02 17:15 ` Hans-Peter Nilsson 3 siblings, 0 replies; 11+ messages in thread From: Tom Tromey @ 2003-03-31 18:42 UTC (permalink / raw) To: Daniel Berlin Cc: gcc-patches, gcc, Fergus Henderson, Phil Edwards, Mark Mitchell, Gabriel Dos Reis >>>>> "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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: review process 2003-03-31 17:50 ` Daniel Berlin 2003-03-31 18:13 ` Phil Edwards 2003-03-31 18:42 ` Tom Tromey @ 2003-03-31 20:06 ` Alexandre Oliva 2003-03-31 20:08 ` Daniel Berlin 2003-04-02 17:15 ` Hans-Peter Nilsson 3 siblings, 1 reply; 11+ messages in thread From: Alexandre Oliva @ 2003-03-31 20:06 UTC (permalink / raw) To: Daniel Berlin Cc: Gerald Pfeifer, gcc-patches, gcc, Fergus Henderson, Phil Edwards, Mark Mitchell, Gabriel Dos Reis 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: review process 2003-03-31 20:06 ` Alexandre Oliva @ 2003-03-31 20:08 ` Daniel Berlin 2003-03-31 21:02 ` Alexandre Oliva 0 siblings, 1 reply; 11+ messages in thread From: Daniel Berlin @ 2003-03-31 20:08 UTC (permalink / raw) To: Alexandre Oliva Cc: Gerald Pfeifer, gcc-patches, gcc, Fergus Henderson, Phil Edwards, Mark Mitchell, Gabriel Dos Reis 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". ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: review process 2003-03-31 20:08 ` Daniel Berlin @ 2003-03-31 21:02 ` Alexandre Oliva 0 siblings, 0 replies; 11+ messages in thread From: Alexandre Oliva @ 2003-03-31 21:02 UTC (permalink / raw) To: Daniel Berlin Cc: Gerald Pfeifer, gcc-patches, gcc, Fergus Henderson, Phil Edwards, Mark Mitchell, Gabriel Dos Reis 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: review process 2003-03-31 17:50 ` Daniel Berlin ` (2 preceding siblings ...) 2003-03-31 20:06 ` Alexandre Oliva @ 2003-04-02 17:15 ` Hans-Peter Nilsson 3 siblings, 0 replies; 11+ messages in thread From: Hans-Peter Nilsson @ 2003-04-02 17:15 UTC (permalink / raw) To: Daniel Berlin; +Cc: gcc 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2003-04-02 15:43 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2003-03-31 19:52 review process Nathanael Nerode 2003-03-31 20:02 ` Richard Henderson [not found] <m3r88o255d.fsf@uniton.integrable-solutions.net> [not found] ` <1049055987.31893.378.camel@doubledemon.codesourcery.com> [not found] ` <m3r88oy3we.fsf@uniton.integrable-solutions.net> [not found] ` <1049059174.31893.385.camel@doubledemon.codesourcery.com> [not found] ` <m3d6k8y2eg.fsf@uniton.integrable-solutions.net> [not found] ` <1049059963.7575.391.camel@doubledemon.codesourcery.com> [not found] ` <m34r5ky1wc.fsf@uniton.integrable-solutions.net> [not found] ` <1049060738.31975.394.camel@doubledemon.codesourcery.com> [not found] ` <20030331041419.GA4687@ceres.cs.mu.oz.au> [not found] ` <20030331060732.GA10362@disaster.jaj.com> [not found] ` <20030331062256.GA6493@ceres.cs.mu.oz.au> 2003-03-31 10:08 ` Gerald Pfeifer 2003-03-31 17:50 ` Daniel Berlin 2003-03-31 18:13 ` Phil Edwards 2003-03-31 18:45 ` Daniel Berlin 2003-03-31 18:42 ` Tom Tromey 2003-03-31 20:06 ` Alexandre Oliva 2003-03-31 20:08 ` Daniel Berlin 2003-03-31 21:02 ` Alexandre Oliva 2003-04-02 17:15 ` Hans-Peter Nilsson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).