Skip site navigation (1) Skip section navigation (2)

Re: [HACKERS] Patch Submission Guidelines

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Robert Treat <xzilla(at)users(dot)sourceforge(dot)net>
Cc: pgsql-patches(at)postgresql(dot)org
Subject: Re: [HACKERS] Patch Submission Guidelines
Date: 2006-02-16 05:27:40
Message-ID: 23536.1140067660@sss.pgh.pa.us (view raw or flat)
Thread:
Lists: pgsql-hackerspgsql-patches
Robert Treat <xzilla(at)users(dot)sourceforge(dot)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

In response to

Responses

pgsql-hackers by date

Next:From: Christopher Kings-LynneDate: 2006-02-16 07:14:41
Subject: Blog post on EnterpriseDB...maybe off topic
Previous:From: Tom LaneDate: 2006-02-16 04:54:54
Subject: Re: qsort again (was Re: [PERFORM] Strange Create Index behaviour)

pgsql-patches by date

Next:From: Robert TreatDate: 2006-02-16 12:17:51
Subject: Re: [HACKERS] Patch Submission Guidelines
Previous:From: Robert TreatDate: 2006-02-16 04:50:33
Subject: Re: [HACKERS] Patch Submission Guidelines

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group