Thread: Re: [HACKERS] Patch Submission Guidelines
On Tuesday 14 February 2006 20:42, Robert Treat wrote:
> On Tuesday 14 February 2006 16:00, Martijn van Oosterhout wrote:
> > > I would like to suggest that we increase substantially the FAQ entries
> > > relating to patch submission. By we, I actually mean please could the
> > > committers sit down and agree some clarified written guidelines?
> >
> > As I remember, there is a disinclination to increase the size of the
> > FAQ very much. This suggests maintaining it as a seperate document. Or
> > alternatively attach it as an appendix to the main documentation.
>
> Huh? The current developers FAQ is at least 1/2 the size of the main FAQ.
> I think adding a submission on patch submission guidelines is a great idea.
> I'll have a patch based on Simon's post to -patches ready in the next 24
> hours unless someone is really going to object.
As stated, the following patch adds a list of patch submission guidelines
based on Simon Riggs suggestions to the developers FAQ.
--
Robert Treat
Build A Brighter Lamp :: Linux Apache {middleware} PostgreSQL
Attachment
Robert Treat <xzilla@users.sourceforge.net> writes:
> As stated, the following patch adds a list of patch submission guidelines
> based on Simon Riggs suggestions to the developers FAQ.
A couple minor comments ...
> ! <li>Ensure that your patch is generated against the most recent version
> ! of the code. If you are developing new features, this should be
> ! CVS HEAD; if it is a bug fix, this will be the most recent version of
> ! the branch which suffers from the bug. For more on branches in
> ! PostgreSQL, see <a href="#1.15">1.15</a>.</li>
Actually, I'd suggest working against HEAD in all cases; the committers
are used to adapting patches backwards, less so to adapting forwards.
(If a bug is fixed in newer releases and not older ones, there is
probably a good reason why not; so I don't see the point of encouraging
people to submit patches for bugs that only exist in older releases,
as this text seems to do.)
> ! <li>The patch should be generated in contextual diff format and should
> ! be applicable from the root directory. If you are unfamiliar with
> ! this, you might find the script <I>src/tools/makediff/difforig</I>
> ! useful. Unified diffs are only preferrable if the file changes are
> ! single-line changes and do not rely on the surrounding lines.</li>
I'd like the policy to be "contextual diffs are preferred", full stop.
Unidiffs are more compact but they sacrifice readability of the patch
(IMHO anyway) and when you are preparing a patch you should be thinking
first in terms of making it readable for the reviewers/committers.
Some things that follow along with the readability mandate, and should
be brought out somewhere here:
* avoid unnecessary whitespace changes. They just distract the
reviewer, and your formatting changes will probably not survive
the next pgindent run anyway.
* try to follow the project's code-layout conventions; again, this
makes it easier for the reviewer, and there's no long-term point
in trying to do it differently than pgindent would.
> ! <li>If your patch changes any existing defaults, you will need to
> ! explain why this is *required* or the patch will likely be rejected.
> ! New feature patches should also be accompanied by doc patches, and
> ! pointers to any relevant sections of the SQL standard are recommended
> ! as well. See <a href="#1.16">1.16</a> for more information on the
> ! SQL standards</li>
The above should be two items not one --- as written it downplays the
importance of providing documentation, which is something we must talk
up not down. (Bruce's original wording of the FAQ item I think
underplays it; we should absolutely not give the impression that
documentation is optional.) I'm not sticky about the docs being
properly-marked-up SGML, but I think you should at least have expended
the effort to explain what you are doing in English separate from the
code.
regards, tom lane
On Thursday 16 February 2006 00:27, Tom Lane wrote:
> Robert Treat <xzilla@users.sourceforge.net> writes:
> > ! <li>The patch should be generated in contextual diff format and
> > should ! be applicable from the root directory. If you are unfamiliar
> > with ! this, you might find the script
> > <I>src/tools/makediff/difforig</I> ! useful. Unified diffs are only
> > preferrable if the file changes are ! single-line changes and do not
> > rely on the surrounding lines.</li>
>
> I'd like the policy to be "contextual diffs are preferred", full stop.
> Unidiffs are more compact but they sacrifice readability of the patch
> (IMHO anyway) and when you are preparing a patch you should be thinking
> first in terms of making it readable for the reviewers/committers.
>
> Some things that follow along with the readability mandate, and should
> be brought out somewhere here:
> * avoid unnecessary whitespace changes. They just distract the
> reviewer, and your formatting changes will probably not survive
> the next pgindent run anyway.
would diff -c --ignore-space-change be better?
> * try to follow the project's code-layout conventions; again, this
> makes it easier for the reviewer, and there's no long-term point
> in trying to do it differently than pgindent would.
>
--
Robert Treat
Build A Brighter Lamp :: Linux Apache {middleware} PostgreSQL
Robert Treat wrote: > On Thursday 16 February 2006 00:27, Tom Lane wrote: > > Robert Treat <xzilla@users.sourceforge.net> writes: > > > ! <li>The patch should be generated in contextual diff format and > > > should ! be applicable from the root directory. If you are unfamiliar > > > with ! this, you might find the script > > > <I>src/tools/makediff/difforig</I> ! useful. Unified diffs are only > > > preferrable if the file changes are ! single-line changes and do not > > > rely on the surrounding lines.</li> > > > > I'd like the policy to be "contextual diffs are preferred", full stop. > > Unidiffs are more compact but they sacrifice readability of the patch > > (IMHO anyway) and when you are preparing a patch you should be thinking > > first in terms of making it readable for the reviewers/committers. > > > > Some things that follow along with the readability mandate, and should > > be brought out somewhere here: > > * avoid unnecessary whitespace changes. They just distract the > > reviewer, and your formatting changes will probably not survive > > the next pgindent run anyway. > > would diff -c --ignore-space-change be better? No, because some whitespace changes are important. For example when you indent a piece of code one level higher. The submitter should eyeball the patch (in diff form) and clean things up when something unexpected appears, like a no-op whitespace change. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Robert Treat <xzilla@users.sourceforge.net> writes:
> On Thursday 16 February 2006 00:27, Tom Lane wrote:
>> * avoid unnecessary whitespace changes. They just distract the
>> reviewer, and your formatting changes will probably not survive
>> the next pgindent run anyway.
> would diff -c --ignore-space-change be better?
Hmm. Not sure --- there are situations where whitespace *does* matter,
so having that as a blanket policy doesn't seem wise. Also I'm worried
that a diff made this way would confuse patch (for instance, because
line numbers following an omitted whitespace change wouldn't match up).
Probably best not to go there, but just focus on the point about keeping
the patch readable.
regards, tom lane
On Thursday 16 February 2006 00:27, Tom Lane wrote:
> Robert Treat <xzilla@users.sourceforge.net> writes:
> > As stated, the following patch adds a list of patch submission guidelines
> > based on Simon Riggs suggestions to the developers FAQ.
>
> A couple minor comments ...
>
Attached patch updated based on previous feedback.
--
Robert Treat
Build A Brighter Lamp :: Linux Apache {middleware} PostgreSQL
Attachment
On Thu, 2006-02-16 at 15:09 -0500, Robert Treat wrote: > On Thursday 16 February 2006 00:27, Tom Lane wrote: > > Robert Treat <xzilla@users.sourceforge.net> writes: > > > As stated, the following patch adds a list of patch submission guidelines > > > based on Simon Riggs suggestions to the developers FAQ. > > > > A couple minor comments ... > > > > Attached patch updated based on previous feedback. Looks like a useful addition to me and I'll do my best to follow it. Best Regards, Simon Riggs
Tom Lane wrote: > > ! <li>The patch should be generated in contextual diff format and should > > ! be applicable from the root directory. If you are unfamiliar with > > ! this, you might find the script <I>src/tools/makediff/difforig</I> > > ! useful. Unified diffs are only preferrable if the file changes are > > ! single-line changes and do not rely on the surrounding lines.</li> > > I'd like the policy to be "contextual diffs are preferred", full stop. > Unidiffs are more compact but they sacrifice readability of the patch > (IMHO anyway) and when you are preparing a patch you should be thinking > first in terms of making it readable for the reviewers/committers. This unified diff sentence was added recently, because I had a case where I was posting a diff, and a unified version was actually clearer than the context diff version because it was a file were we were changing discrete lines, rather than blocks of code. It might be a small enough number of cases that it isn't worth mentioning, but we have had people say they find unified diffs clearer, so I wanted to mention _where_ unified diffs are clearer, and where they are not. I thought this might encourage people to use content diffs more often if they understood _why_? -- Bruce Momjian http://candle.pha.pa.us SRA OSS, Inc. http://www.sraoss.com + If your life is a hard drive, Christ can be your backup. +
Tom Lane wrote: > Robert Treat <xzilla@users.sourceforge.net> writes: > > On Thursday 16 February 2006 00:27, Tom Lane wrote: > >> * avoid unnecessary whitespace changes. They just distract the > >> reviewer, and your formatting changes will probably not survive > >> the next pgindent run anyway. > > > would diff -c --ignore-space-change be better? > > Hmm. Not sure --- there are situations where whitespace *does* matter, > so having that as a blanket policy doesn't seem wise. Also I'm worried > that a diff made this way would confuse patch (for instance, because > line numbers following an omitted whitespace change wouldn't match up). > Probably best not to go there, but just focus on the point about keeping > the patch readable. Agreed. It isn't that we don't want whitespace changes, just that we don't want insignificant whitespace changes. -- Bruce Momjian http://candle.pha.pa.us SRA OSS, Inc. http://www.sraoss.com + If your life is a hard drive, Christ can be your backup. +
Robert Treat wrote: > On Thursday 16 February 2006 00:27, Tom Lane wrote: > > Robert Treat <xzilla@users.sourceforge.net> writes: > > > As stated, the following patch adds a list of patch submission guidelines > > > based on Simon Riggs suggestions to the developers FAQ. > > > > A couple minor comments ... > > > > Attached patch updated based on previous feedback. I have applied your patch (attached) after some cleanup, and applied to 8.1.X too. The only part I removed was the requirement to research previous discussion of the patch. That usually is not an issue, and can always be requested after the patch is submitted. Thanks, nice improvement. -- Bruce Momjian http://candle.pha.pa.us SRA OSS, Inc. http://www.sraoss.com + If your life is a hard drive, Christ can be your backup. + Index: doc/src/FAQ/FAQ_DEV.html =================================================================== RCS file: /cvsroot/pgsql/doc/src/FAQ/FAQ_DEV.html,v retrieving revision 1.107 diff -c -c -r1.107 FAQ_DEV.html *** doc/src/FAQ/FAQ_DEV.html 24 Dec 2005 19:29:38 -0000 1.107 --- doc/src/FAQ/FAQ_DEV.html 1 Mar 2006 22:20:01 -0000 *************** *** 156,180 **** <H3 id="item1.5">1.5) I've developed a patch, what next?</H3> ! <P>Generate the patch in contextual diff format. If you are unfamiliar with this, you might find the script ! <I>src/tools/makediff/difforig</I> useful. Unified diffs are ! only preferrable if the file changes are single-line changes and ! do not rely on the surrounding lines.</P> ! ! <P>Ensure that your patch is generated against the most recent ! version of the code. If it is a patch adding new functionality, the ! most recent version is CVS HEAD; if it is a bug fix, this will be ! the most recently version of the branch which suffers from the bug ! (for more on branches in PostgreSQL, see <A href= ! "#1.15">1.15</A>).</P> ! <P>Finally, submit the patch to pgsql-patches@postgresql.org. It ! will be reviewed by other contributors to the project and will be ! either accepted or sent back for further work. Also, please try to ! include documentation changes as part of the patch. If you can't do ! that, let us know and we will manually update the documentation when ! the patch is applied.</P> <H3 id="item1.6">1.6) Where can I learn more about the code?</H3> --- 156,220 ---- <H3 id="item1.5">1.5) I've developed a patch, what next?</H3> ! <P>You will need to submit the patch to pgsql-patches@postgresql.org. It ! will be reviewed by other contributors to the project and will be ! either accepted or sent back for further work. To help ensure your patch ! is reviewed and committed in a timely fashion, please try to make sure your ! submission conforms to the following guidelines: ! ! <ol> ! <li>Ensure that your patch is generated against the most recent version ! of the code, which for developers is CVS HEAD. For more on branches in ! PostgreSQL, see <a href="#1.15">1.15</a>.</li> ! ! <li>Try to make your patch as readable as possible by following the ! project's code-layout conventions. This makes it easier for the ! reviewer, and there's no point in trying to layout things ! differently than pgindent. Also avoid unnecessary whitespace ! changes because they just distract the reviewer, and formatting ! changes will be removed by the next run of pgindent.</li> ! ! <li>The patch should be generated in contextual diff format (<i>diff ! -c</i> and should be applicable from the root directory. If you are unfamiliar with this, you might find the script ! <I>src/tools/makediff/difforig</I> useful. (Unified diffs are only ! preferable if the file changes are single-line changes and do not ! rely on surrounding lines.)</li> ! ! <li>PostgreSQL is licensed under a BSD license, so any submissions must ! conform to the BSD license to be included. If you use code that is ! available under some other license that is BSD compatible (eg. public ! domain) please note that code in your email submission</li> ! ! <li>Confirm that your changes can pass the regression tests. If your ! changes are port specific, please list the ports you have tested it ! on.</li> ! ! <li>Provide an implementation overview, preferably in code comments. ! Following the surrounding code commenting style is usually a good ! approach.</li> ! ! <li>New feature patches should also be accompanied by documentation ! patches. If you need help checking the SQL standard, see <a href= ! "#1.16">1.16</a>.</li> ! ! <li>If you are adding a new feature, confirm that it has been tested ! thoughly. Try to test the feature in all conceivable ! scenarios.</li> ! ! <li>If it is a performance patch, please provide confirming test ! results to show the benefit of your patch. It is OK to post patches ! without this information, though the patch will not be applied until ! somebody has tested the patch and found a significant performance ! improvement.</li> ! </ol> ! ! <p>Even if you pass all of the above, the patch might still be ! rejected for other reasons. Please be prepared to listen to comments ! and make modifications.</p> ! <p>You will be notified via email when the patch is applied, and ! your name will appear in the next version of the release notes.</p> <H3 id="item1.6">1.6) Where can I learn more about the code?</H3>