Thread: Patch application
I think it is time to start giving people official responsibility for
certain areas of the code.
In the old says, we didn't have many _exports_, and people submitting
patches often knew more than we did because they had spent time studying
the code.
Now, we have much more expertise, to the point that people not involved
in those areas can't really contribute very well without the assistance
of those experts.
For example, I can't seem to evaluate any Makefile changes because Peter
E. knows how the system is designed much better. The same is true for
JDBC and many other areas.
I would like to create a web page in the developer's corner that
contains module names and the people who are most knowledgeable. I will
no longer apply changes to those areas without getting approval from
those people. My recent attempts have made things worse rather than
better. I suggest other committers do the same.
My short list right now is:
Makefiles/configure Peter E.
psql Peter E.
Jdbc Peter M.
Odbc Hiroshi?
Ecpg Michael
Python D'Arcy
Optimizer Tom Lane
Rewrite Jan
Locking Tom
Cache Tom
Date/Time Thomas
Pl/PgSQL Jan
SGML Peter E, Thomas
WAL Vadim, Tom
FAQ/TODO Bruce
Regression Peter E?
Multibyte Tatsuo
GIST Oleg
Comments?
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> I think it is time to start giving people official responsibility for
> certain areas of the code.
This strikes me as overly formalistic, and more likely to lead to
arteriosclerosis than any improvement in code quality. Particularly
with a breakdown such as you have proposed, which would likely mean
asking multiple people to approve any given patch.
I think the procedural error in this past weekend's contrib mess was
simply that you didn't pay attention to the fact that Oleg's patch was
based on an out-of-date copy of the contrib module. You should have
either merged the changes or bounced it back to Oleg for him to do so.
Insisting on CVS $Header$ or $Id$ markers in all code files might help
to detect this kind of error --- but nothing will help if you are
willing to overwrite other people's changes simply because you didn't
recall the reason for them at the moment.
regards, tom lane
The below basically summarizes my opinion quite well ... On Mon, 19 Mar 2001, Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > I think it is time to start giving people official responsibility for > > certain areas of the code. > > This strikes me as overly formalistic, and more likely to lead to > arteriosclerosis than any improvement in code quality. Particularly > with a breakdown such as you have proposed, which would likely mean > asking multiple people to approve any given patch. > > I think the procedural error in this past weekend's contrib mess was > simply that you didn't pay attention to the fact that Oleg's patch was > based on an out-of-date copy of the contrib module. You should have > either merged the changes or bounced it back to Oleg for him to do so. > > Insisting on CVS $Header$ or $Id$ markers in all code files might help > to detect this kind of error --- but nothing will help if you are > willing to overwrite other people's changes simply because you didn't > recall the reason for them at the moment. > > regards, tom lane > > ---------------------------(end of broadcast)--------------------------- > TIP 5: Have you checked our extensive FAQ? > > http://www.postgresql.org/users-lounge/docs/faq.html > Marc G. Fournier ICQ#7615664 IRC Nick: Scrappy Systems Administrator @ hub.org primary: scrappy@hub.org secondary: scrappy@{freebsd|postgresql}.org
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > I think it is time to start giving people official responsibility for
> > certain areas of the code.
>
> This strikes me as overly formalistic, and more likely to lead to
> arteriosclerosis than any improvement in code quality. Particularly
> with a breakdown such as you have proposed, which would likely mean
> asking multiple people to approve any given patch.
>
> I think the procedural error in this past weekend's contrib mess was
> simply that you didn't pay attention to the fact that Oleg's patch was
> based on an out-of-date copy of the contrib module. You should have
> either merged the changes or bounced it back to Oleg for him to do so.
>
> Insisting on CVS $Header$ or $Id$ markers in all code files might help
> to detect this kind of error --- but nothing will help if you are
> willing to overwrite other people's changes simply because you didn't
> recall the reason for them at the moment.
I understand the formalistic problem, and maybe I overstated its
formality, but it seems it would be good to maintain a list for two
reasons:
1) With formalize experts in various areas, if someone replies
to an email, the recipient can clearly know this person is an expert in
that area. It also helps focus attention on certain people for
development assistance.
2) The number of patches that I apply that need fixing by
someone else is getting more frequent. The most recent patch is just
one of many that had to be cleaned up for various reasons. I reviewed
the patch and still didn't see the intent of the Makefile change. In
this case, the CVS logs would have helped, but in others there is a
design goal that I just can not comprehend.
Looking at the list, I feel I would have to contact someone before
making any changes to these areas. Even if I can get the patch applied
properly, I doubt I would do it the _right_ way. Sometimes it is just
that the style of the patcher doesn't match the style in our sources.
Maybe we don't have to make it required, but plain patches from people I
don't know really need some review. Perhaps I can attach the patch to
the PATCHES list when I apply it so people can see exactly what was
changed.
Aren't people upset about the minor fixes they have to make to patches I
apply? Is it easier to just clean up things rather than find/apply the
patches?
For example, almost any change to an SGML file seems to require Peter E
to fix some part of it, usually the markup. Is that OK, Peter? Most of
the interfaces require an interface expert's comment I would think.
---------------------------------------------------------------------------
Makefiles/configure Peter E. psql Peter E. Jdbc Peter M.
Odbc Hiroshi? Ecpg Michael Python D'Arcy
Optimizer Tom Lane Rewrite Jan Locking Tom Cache
Tom Date/Time Thomas Pl/PgSQL Jan SGML Peter
E,Thomas WAL Vadim, Tom FAQ/TODO Bruce Regression
PeterE? Multibyte Tatsuo GIST Oleg
-- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610)
853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill,
Pennsylvania19026
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> I understand the formalistic problem, and maybe I overstated its
> formality, but it seems it would be good to maintain a list for two
> reasons:
I don't have a problem with keeping an informal list of area experts.
I was just objecting to the notion of formal signoffs (I doubt Peter E.
wants to look at every single Makefile change, for example).
Also, there's a point that keeps coming up in these discussions: the
standards need to be different depending on what time of the release
cycle it is. Perhaps formal signoffs *are* appropriate when we're
this late in beta.
regards, tom lane
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> I understand the formalistic problem, and maybe I overstated its
> formality, but it seems it would be good to maintain a list for two
> reasons:
In projects like gcc and the GNU binutils, we use a MAINTAINERS file.
Some people have blanket write privileges. Some people have write
priviliges to certain areas of the code. Anybody else needs a patch
to be approved before they can check it in. Patches which are
``obviously correct'' are always OK.
The MAINTAINERS file can be used as a guide for who to ask in certain
areas of the code.
This may be overly complex for Postgres now. But I believe that you
will need something of this nature as the project continues to grow.
This permits you to scale to more developers.
Note that the MAINTAINERS file is not enforced by a program. It is
only enforced by people noticing an unapproved checkin message, and
theoreticalliy removing write privileges.
For example, I have appended the gcc MAINTAINERS file.
Ian
Blanket Write Privs.
Craig Burley craig@jcb-sc.com
John Carr jfc@mit.edu
Richard Earnshaw rearnsha@arm.com
Richard Henderson rth@redhat.com
Geoffrey Keating geoffk@redhat.com
Richard Kenner kenner@nyu.edu
Jeff Law law@redhat.com
Jason Merrill jason@redhat.com
Michael Meissner meissner@redhat.com
David S. Miller davem@redhat.com
Mark Mitchell mark@codesourcery.com
Bernd Schmidt bernds@redhat.com
Jim Wilson wilson@redhat.com
Various Maintainers
sh port Joern Rennecke amylaar@redhat.com Alexandre Oliva aoliva@redhat.com
v850 port Nick Clifton nickc@redhat.com
v850 port Michael Meissner meissner@redhat.com
arm port Nick Clifton nickc@redhat.com
arm port Richard Earnshaw rearnsha@arm.com
m32r port Nick Clifton nickc@redhat.com Michael Meissner meissner@redhat.com
h8 port Jeff Law law@redhat.com
mcore Nick Clifton nickc@redhat.com Jim Dein jdein@windriver.com
mn10200 port Jeff Law law@redhat.com
mn10300 port Jeff Law law@redhat.com Alexandre Oliva aoliva@redhat.com
hppa port Jeff Law law@redhat.com
m68hc11 port Stephane Carrez Stephane.Carrez@worldnet.fr
m68k port (?) Jeff Law law@redhat.com
m68k-motorola-sysv port Philippe De Muyter phdm@macqel.be
rs6000 port Geoff Keating geoffk@redhat.com
rs6000 port David Edelsohn dje@watson.ibm.com
mips port Gavin Romig-Koch gavin@redhat.com
ia64 port Jim Wilson wilson@redhat.com
i860 port Jason Eckhardt jle@redhat.com
i960 port Jim Wilson wilson@redhat.com
a29k port Jim Wilson wilson@redhat.com
alpha port Richard Henderson rth@redhat.com
sparc port Richard Henderson rth@redhat.com
sparc port David S. Miller davem@redhat.com
sparc port Jakub Jelinek jakub@redhat.com
x86 ports Stan Cox scox@redhat.com
c4x port Michael Hayes m.hayes@elec.canterbury.ac.nz
arc port Richard Kenner kenner@nyu.edu
fr30 port Nick Clifton niclc@redhat.com
vax port Dave Anglin dave.anglin@nrc.ca
fortran Richard Henderson rth@redhat.com
fortran Toon Moene toon@moene.indiv.nluug.nl
c++ Jason Merrill jason@redhat.com
c++ Mark Mitchell mark@codesourcery.com
chill Dave Brolley brolley@redhat.com
chill Per Bothner per@bothner.com
java Per Bothner per@bothner.com
java Alexandre Petit-Bianco apbianco@redhat.com
mercury Fergus Henderson fjh@cs.mu.oz.au
objective-c Stan Shebs shebs@apple.com
objective-c Ovidiu Predescu ovidiu@cup.hp.com
cpplib Dave Brolley brolley@redhat.com
cpplib Per Bothner per@bothner.com
cpplib Zack Weinberg zackw@stanford.edu
cpplib Neil Booth neil@daikokuya.demon.co.uk
alias analysis John Carr jfc@mit.edu
loop unrolling Jim Wilson wilson@redhat.com
loop discovery Michael Hayes m.hayes@elec.canterbury.ac.nz
scheduler (+ haifa) Jim Wilson wilson@redhat.com
scheduler (+ haifa) Michael Meissner meissner@redhat.com
scheduler (+ haifa) Jeff Law law@redhat.com
reorg Jeff Law law@redhat.com
caller-save.c Jeff Law law@redhat.com
debugging code Jim Wilson wilson@redhat.com
dwarf debugging code Jason Merrill jason@redhat.com
c++ runtime libs Gabriel Dos Reis dosreis@cmla.ens-cachan.fr
c++ runtime libs Ulrich Drepper drepper@redhat.com
c++ runtime libs Phil Edwards pedwards@jaj.com
c++ runtime libs Benjamin Kosnik bkoz@redhat.com
*synthetic multiply Torbjorn Granlund tege@swox.com
*c-torture Torbjorn Granlund tege@swox.com
*f-torture Kate Hedstrom kate@ahab.rutgers.edu
sco5, unixware, sco udk Robert Lipe robertlipe@usa.net
fixincludes Bruce Korb bkorb@gnu.org
gcse.c Jeff Law law@redhat.com
global opt framework Jeff Law law@redhat.com
jump.c David S. Miller davem@redhat.com
web pages Gerald Pfeifer pfeifer@dbai.tuwien.ac.at
C front end/ISO C99 Gavin Romig-Koch gavin@redhat.com
config.sub/config.guess Ben Elliston bje@redhat.com
avr port Denis Chertykov denisc@overta.ru Marek Michalkiewicz marekm@linux.org.pl
basic block reordering Jason Eckhardt jle@redhat.com
i18n Philipp Thomas pthomas@suse.de
diagnostic messages Gabriel Dos Reis gdr@codesourcery.com
windows, cygwin, mingw Christopher Faylor cgf@redhat.com
windows, cygwin, mingw DJ Delorie dj@redhat.com
DJGPP DJ Delorie dj@delorie.com
libiberty DJ Delorie dj@redhat.com
build machinery (*.in) DJ Delorie dj@redhat.com
build machinery (*.in) Alexandre Oliva aoliva@redhat.com
Note individuals who maintain parts of the compiler need approval to check
in changes outside of the parts of the compiler they maintain.
Write After Approval
Scott Bambrough scottb@netwinder.org
Laurynas Biveinis lauras@softhome.net
Phil Blundell pb@futuretv.com
Hans Boehm hboehm@gcc.gnu.org
Andrew cagney cagney@redhat.com
Eric Christopher echristo@redhat.com
William Cohen wcohen@redhat.com
*Paul Eggert eggert@twinsun.com
Ben Elliston bje@redhat.com
Marc Espie espie@cvs.openbsd.org
Kaveh Ghazi ghazi@caip.rutgers.edu
Anthony Green green@redhat.com
Stu Grossman grossman@redhat.com
Andrew Haley aph@redhat.com
Aldy Hernandez aldyh@redhat.com
Kazu Hirata kazu@hxi.com
Manfred Hollstein mhollstein@redhat.com
Jan Hubicka hubicka@freesoft.cz
Andreas Jaeger aj@suse.de
Jakub Jelinek jakub@redhat.com
Klaus Kaempf kkaempf@progis.de
Brendan Kehoe brendan@redhat.com
Mumit Khan khan@xraylith.wisc.edu
Marc Lehmann pcg@goof.com
Alan Lehotsky apl@alum.mit.edu
Warren Levy warrenl@redhat.com
Kriang Lerdsuwanakij lerdsuwa@users.sourceforge.net
Don Lindsay dlindsay@redhat.com
Dave Love d.love@dl.ac.uk
Martin v. Löwis loewis@informatik.hu-berlin.de
*HJ Lu hjl@lucon.org
Andrew Macleod amacleod@redhat.com
Vladimir Makarov vmakarov@redhat.com
Greg McGary gkm@gnu.org
Bryce McKinlay bryce@gcc.gnu.org
Alan Modra alan@linuxcare.com.au
Toon Moene toon@moene.indiv.nluug.nl
Catherine Moore clm@redhat.com
Joseph Myers jsm28@cam.ac.uk
Hans-Peter Nilsson hp@bitrange.com
Diego Novillo dnovillo@redhat.com
David O'Brien obrien@FreeBSD.org
Jeffrey D. Oldham oldham@codesourcery.com
Alexandre Petit-Bianco apbianco@redhat.com
Clinton Popetz cpopetz@cpopetz.com
Ken Raeburn raeburn@redhat.com
Rolf Rasmussen rolfwr@gcc.gnu.org
Gabriel Dos Reis dosreis@cmla.ens-cachan.fr
Alex Samuel samuel@codesourcery.com
Bernd Schmidt bernds@redhat.com
Andreas Schwab schwab@suse.de
Stan Shebs shebs@apple.com
Nathan Sidwell nathan@acm.org
Franz Sirl franz.sirl-kernel@lauterbach.com
Michael Sokolov msokolov@ivan.Harhan.ORG
Mike Stump mrs@windriver.com
Ian Taylor ian@zembu.com
Philipp Thomas pthomas@suse.de
Kresten Krab Thorup krab@gcc.gnu.org
Tom Tromey tromey@redhat.com
John Wehle john@feith.com
Mark Wielaard mark@gcc.gnu.org
* Indicates folks we need to get Kerberos/ssh accounts ready so they
can write in the source tree
> Bruce Momjian <pgman@candle.pha.pa.us> writes: > > I understand the formalistic problem, and maybe I overstated its > > formality, but it seems it would be good to maintain a list for two > > reasons: > > I don't have a problem with keeping an informal list of area experts. > I was just objecting to the notion of formal signoffs (I doubt Peter E. > wants to look at every single Makefile change, for example). OK, that is good. I think it will make a nice web page. > Also, there's a point that keeps coming up in these discussions: the > standards need to be different depending on what time of the release > cycle it is. Perhaps formal signoffs *are* appropriate when we're > this late in beta. Oh, yes, agreed, beta requires signoffs. But I can't seem to get patches in that don't require some _expert_ to come along and improve it. If the _experts_ are OK with that, then that is fine, but if they want things done differently somehow, I want to meet their needs. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
Ian Lance Taylor <ian@airs.com> writes:
> In projects like gcc and the GNU binutils, we use a MAINTAINERS file.
> Some people have blanket write privileges. Some people have write
> priviliges to certain areas of the code. Anybody else needs a patch
> to be approved before they can check it in. Patches which are
> ``obviously correct'' are always OK.
Would you enlarge on what that fourth sentence means in practice?
Seems like the sticky issue here is what constitutes "approval".
We already have a policy that changes originating from non-committers
are supposed to be reviewed before they get applied, but what Bruce
is worried about is the quality of the review process.
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> writes: > Ian Lance Taylor <ian@airs.com> writes: > > In projects like gcc and the GNU binutils, we use a MAINTAINERS file. > > Some people have blanket write privileges. Some people have write > > priviliges to certain areas of the code. Anybody else needs a patch > > to be approved before they can check it in. Patches which are > > ``obviously correct'' are always OK. > > Would you enlarge on what that fourth sentence means in practice? > > Seems like the sticky issue here is what constitutes "approval". > We already have a policy that changes originating from non-committers > are supposed to be reviewed before they get applied, but what Bruce > is worried about is the quality of the review process. In practice, what it means is that somebody who has a patch, and does not have the appropriate privileges, sends it to the mailing list. Most patches apply to a single part of the code. The person responsible for that part of the code, or a person with blanket write privileges, reviews the patch, and approves it, or denies it, or approves it with changes. If approved, and the original submitter has write privileges, he or she checks it in. Otherwise, the maintainer who did the review checks it in. If approved with changes, and the original submitter has write privileges, he or she makes the changes and checks it in. Otherwise he or she makes the changes, sends them back, and the maintainer who did the review checks it in. One advantage of the MAINTAINERS file with respect to the review process is that it tells you who knows most about particular areas of the code. For example, in the gcc MAINTAINERS file I sent earlier, there are people with blanket write privileges who are also listed as responsible for particular areas of gcc. Another advantage is that it reduces load on the maintainers, since many people can check in their own patches. Since there are many people, some sort of control is needed. The goal is not excessive formalism; as I said, there is nothing which actually prevents anybody with write privileges from checking in a patch to any part of the code. The goal is to guide people to the right person to approve a particular patch. Ian
Bruce, what is the point of even an informal list of "experts"? There
have always been areas that folks have "adopted" or have developed an
interest and expertise in. And we've gotten lots of really great
contributions both large and small from people we barely knew existed
until the patch arrived.
Unless there is a "process improvement" which comes with developing this
list, I don't really see what the benefit will be wrt existance of the
list, developing the list, of deciding who should be on a list, etc etc.
istm that the list of active developers serves the function of
acknowledging contributors. Not sure what another list will do for us,
and it may have the effect of being an artificial barrier or distinction
between folks.
All imho of course ;)
- Thomas
At 05:36 20/03/01 +0000, Thomas Lockhart wrote: > >Unless there is a "process improvement" which comes with developing this >list, I don't really see what the benefit will be wrt existance of the >list, developing the list, of deciding who should be on a list, etc etc. > Totally agree; such formality will be barrier and we will gain almost nothing. It will also further disenfranchise the wider developer community. ISTM that the motivation is based on one or two isolated incidents where patches were applied when they should not have been. Tom's suggestion of insisting on CVS headers will deal with the specific case in point, at far lesser social and labour overhead. The last thing we want to do to people who contribute heavily to the project is say 'Gee, thanks. And you are now responsible for all approvals on this area of code', especially in late beta, when they are likely to be quite busy. Similarly, when we are outside the beta phase, we don't need the process. I suspect that anybody who has worked on a chunk of code in a release cycle will keep an eye on what is happening to the code. My suggestion for handling this process would be to allow an opt-in 'watch' to be placed on files/modules in CVS (CVS supports this, from memory). Then, eg, I can say 'send me info when someone makes a match to pg_dump'. Similarly, when I start working on a module, I can add myself to the list to be informed when someone changes it underneath me. This would be genuinely useful to me as a part-time developer. As a further development from this, it would be good to see a way for bug reports to be redirected to 'subsystems' (or something like that), which developers could opt into. So, using myself as an example, I would like special notification when a pg_dump bug is reported. Similarly, I might like to be notified when changes are made to the WAL code... If you want to make cute web pages, and define domain experts, make it useful, make it opt-in, and make it dynamic. ---------------------------------------------------------------- Philip Warner | __---_____ Albatross Consulting Pty. Ltd. |----/ - \ (A.B.N. 75 008 659 498) | /(@) ______---_ Tel: (+61) 0500 83 82 81 | _________ \ Fax: (+61) 0500 83 82 82 | ___________ | Http://www.rhyme.com.au | / \| | --________-- PGP key available upon request, | / and from pgp5.ai.mit.edu:11371 |/
OK, seems there have been enough objections that I will not implement a "experts" page, nor change the way patches are applied. I will be posting a diff -c of any patches I have to munge into place, so people can see how stuff was merged into the code. It seems the problem of people having to massage patches after they are applied is either not a big deal, or the other ways of applying patches are considered worse. > At 05:36 20/03/01 +0000, Thomas Lockhart wrote: > > > >Unless there is a "process improvement" which comes with developing this > >list, I don't really see what the benefit will be wrt existance of the > >list, developing the list, of deciding who should be on a list, etc etc. > > > > Totally agree; such formality will be barrier and we will gain almost > nothing. It will also further disenfranchise the wider developer community. > ISTM that the motivation is based on one or two isolated incidents where > patches were applied when they should not have been. Tom's suggestion of > insisting on CVS headers will deal with the specific case in point, at far > lesser social and labour overhead. > > The last thing we want to do to people who contribute heavily to the > project is say 'Gee, thanks. And you are now responsible for all approvals > on this area of code', especially in late beta, when they are likely to be > quite busy. Similarly, when we are outside the beta phase, we don't need > the process. > > I suspect that anybody who has worked on a chunk of code in a release cycle > will keep an eye on what is happening to the code. > > My suggestion for handling this process would be to allow an opt-in 'watch' > to be placed on files/modules in CVS (CVS supports this, from memory). > Then, eg, I can say 'send me info when someone makes a match to pg_dump'. > Similarly, when I start working on a module, I can add myself to the list > to be informed when someone changes it underneath me. This would be > genuinely useful to me as a part-time developer. > > As a further development from this, it would be good to see a way for bug > reports to be redirected to 'subsystems' (or something like that), which > developers could opt into. So, using myself as an example, I would like > special notification when a pg_dump bug is reported. Similarly, I might > like to be notified when changes are made to the WAL code... > > If you want to make cute web pages, and define domain experts, make it > useful, make it opt-in, and make it dynamic. > > > ---------------------------------------------------------------- > Philip Warner | __---_____ > Albatross Consulting Pty. Ltd. |----/ - \ > (A.B.N. 75 008 659 498) | /(@) ______---_ > Tel: (+61) 0500 83 82 81 | _________ \ > Fax: (+61) 0500 83 82 82 | ___________ | > Http://www.rhyme.com.au | / \| > | --________-- > PGP key available upon request, | / > and from pgp5.ai.mit.edu:11371 |/ > > ---------------------------(end of broadcast)--------------------------- > TIP 4: Don't 'kill -9' the postmaster > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026