Tips on committing
With seven new committers, I thought it would be good for me to give
some tips I use in committing. I use a standard template that is this:
1 |--- gitweb/email subject limit -----------------|-------------|
2 Reported-by:
3 Bug:
4 Discussion:
5 Author:
6 Reviewed-by:
7 Tested-by:
8 Backpatch-through:
Any items left unchanged are automatically removed.
Line 1 shows me the length limits for the first commit line which will
appear in gitweb and commit subject lines. I always try to fit in the
commit subject line limit but the gitweb limit is too small for me in
many cases. (I automatically remove the first line, though that once
failed recently.)
If line 4 is a URL, I shorten it with this script:
sed 's;http\(s\?\)://www\.postgresql\.org/message-id/;http\1://postgr.es/m/;gi' "$@"
If line 4 is a message-id, I convert it to a shortened URL:
sed '/https\?:\/\//!s;^\(Discussion:[ \t]*\)\(.*@.*\)$;\1/messages/by-id/%5C2;g&
I check for files in the tree with 'rej' extensions, which might
indicate failed patch application.
I also remove repeated blank lines with:
cat --squeeze-blank
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +
I'm not sure pgsql-committers was the right audience. Cross-posting to
pg-hackers.
On 2018-Jun-28, Bruce Momjian wrote:
2 Reported-by:
5 Author:
6 Reviewed-by:
7 Tested-by:
Should these include email addresses?
I've also used "Diagnosed-by" to credit a person who spent time studying
a bug's mechanism and how to fix it. Sometimes that's the same as
Reported-by, but I think the weight is quite different.
Apparently, there's a recent trend to credit patch authors using
"Co-authored-by". Should we use that too?
https://stackoverflow.com/a/41847267/
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2018-06-28 11:14:38 -0400, Alvaro Herrera wrote:
I'm not sure pgsql-committers was the right audience. Cross-posting to
pg-hackers.On 2018-Jun-28, Bruce Momjian wrote:
2 Reported-by:
5 Author:
6 Reviewed-by:
7 Tested-by:Should these include email addresses?
I'm not sure it's that helpful, they tend to be out of date pretty soon.
I've also used "Diagnosed-by" to credit a person who spent time studying
a bug's mechanism and how to fix it. Sometimes that's the same as
Reported-by, but I think the weight is quite different.
Yea, same here.
Apparently, there's a recent trend to credit patch authors using
"Co-authored-by". Should we use that too?
https://stackoverflow.com/a/41847267/
I just put multiple people into Authors, with order roughly implying the
amount of work. Don't really see a reason to split it off further?
Greetings,
Andres Freund
On Thu, Jun 28, 2018 at 11:14:38AM -0400, Alvaro Herrera wrote:
I'm not sure pgsql-committers was the right audience. Cross-posting to
pg-hackers.On 2018-Jun-28, Bruce Momjian wrote:
2 Reported-by:
5 Author:
6 Reviewed-by:
7 Tested-by:Should these include email addresses?
Uh, since I am linking to the thread it seemed unnecessary.
I've also used "Diagnosed-by" to credit a person who spent time studying
a bug's mechanism and how to fix it. Sometimes that's the same as
Reported-by, but I think the weight is quite different.
Good point. I have never used it but I can see its value. I have added
it to my template.
Apparently, there's a recent trend to credit patch authors using
"Co-authored-by". Should we use that too?
https://stackoverflow.com/a/41847267/
I would just list multiple authors.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +
On Thu, Jun 28, 2018 at 11:20 AM, Andres Freund <andres@anarazel.de> wrote:
Apparently, there's a recent trend to credit patch authors using
"Co-authored-by". Should we use that too?
https://stackoverflow.com/a/41847267/I just put multiple people into Authors, with order roughly implying the
amount of work. Don't really see a reason to split it off further?
One of the reasons that I've stuck with free-form text for denoting
authors rather than using tags is precisely so I can try to clarify
relative levels of contribution. I think being able to have multiple
Author: tags -- each such author being a major author -- and also
multiple Co-authored-by tags -- each such coauthor having made a
relatively small contribution -- would be an adequate amount of
distinction for almost all cases that I deal with.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Thu, Jun 28, 2018 at 8:21 AM, Bruce Momjian <bruce@momjian.us> wrote:
Good point. I have never used it but I can see its value. I have added
it to my template.
FWIW, I developed a document on committing for my own reference, with
some help from Andres. A lot of it is about commit message style, the
use of fields, and so on. But I've also developed a check list for
committing, knowing that there are a few classic mistakes that
committers will make from time to time despite knowing better. These
are worth checking against mechanically, IMV. Here are some actual
examples from this document that I refer to:
* Double-check release build compiler warnings.
* make check-world.
* When adding tests files, make sure that they're added to both serial
and parallel schedules.
* When adding a new GUC, postgresql.conf.sample needs to be updated, too.
* Consider the need for a catversion bump.
* Don't assume that you haven't broken the doc build if you make even
a trivial doc change. Removing a GUC can break instances in the
release notes where they're referenced. Even grep can miss this, since
references to the GUC will have dashes rather than underscores, plus
possibly other variations.
* Do a dry run before really pushing by using --dry-run.
Of course, you ought to know that you should do this stuff anyway, but
in the real world many mistakes happen when a step is skipped over
during a routine process, perhaps caused by a seemingly insignificant
last minute change. The points are organized in a way that makes a run
down quick and easy, even when committing a trivial patch.
--
Peter Geoghegan
On 2018-Jun-28, Peter Geoghegan wrote:
On Thu, Jun 28, 2018 at 8:21 AM, Bruce Momjian <bruce@momjian.us> wrote:
Good point. I have never used it but I can see its value. I have added
it to my template.FWIW, I developed a document on committing for my own reference, with
some help from Andres.
Sounds very useful.
How about turning it into a wiki page, for everybody's benefit?
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Jun 28, 2018 at 9:52 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
FWIW, I developed a document on committing for my own reference, with
some help from Andres.Sounds very useful.
How about turning it into a wiki page, for everybody's benefit?
I'll try to do that, but I'd still recommend personalizing it. A lot
of the stuff in there is specific to my own workflow and tool
preferences, and my own personal working style. I don't really use a
template in the same way Bruce does, for example, because most of that
stuff is taken care of by my text editor having a "gitcommit" syntax.
I find it useful to have a routine checklist for things like
committing, because it frees up a little space in my head for other
things. I do the same thing when preparing for a trip.
--
Peter Geoghegan
On 6/28/18 17:14, Alvaro Herrera wrote:
2 Reported-by:
5 Author:
6 Reviewed-by:
7 Tested-by:Should these include email addresses?
One reason I include emails is that sometimes the names are spelled in
inconsistent ways or don't include ASCII characters at all. An email
address is always clear.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Jun 29, 2018 at 02:04:07PM +0200, Peter Eisentraut wrote:
On 6/28/18 17:14, Alvaro Herrera wrote:
2 Reported-by:
5 Author:
6 Reviewed-by:
7 Tested-by:Should these include email addresses?
One reason I include emails is that sometimes the names are spelled in
inconsistent ways or don't include ASCII characters at all. An email
address is always clear.
I don't know if emails are actually a good idea to include. Those tend
to change when folks change company, and a lot of people here use
company-based email addresses to discuss and work on patches.
--
Michael
On Thu, Jun 28, 2018 at 10:52:42AM -0700, Peter Geoghegan wrote:
On Thu, Jun 28, 2018 at 9:52 AM, Alvaro Herrera
I'll try to do that, but I'd still recommend personalizing it. A lot
of the stuff in there is specific to my own workflow and tool
preferences, and my own personal working style. I don't really use a
template in the same way Bruce does, for example, because most of that
stuff is taken care of by my text editor having a "gitcommit" syntax.I find it useful to have a routine checklist for things like
committing, because it frees up a little space in my head for other
things. I do the same thing when preparing for a trip.
Yes, that's a good idea. In order to run the tests, I have a dedicated
alias:
alias pgcheck='cd $HOME/postgres && make check-world -j 4 PROVE_FLAGS='\''-j 4'\''
This does not work with 9.5 and older versions though as parallel build
is broken in those cases, and also checking that parallel build is not
broken is a good routine to have in my opinion.
I would also recommend that people use PG_TEST_EXTRA='ssl ldap kerberos'
in their environment so as no tests are skipped. We have seen the SSL
test suite broken silently a couple of times since its introduction...
--
Michael
On 06/29/2018 02:19 PM, Michael Paquier wrote:
On Fri, Jun 29, 2018 at 02:04:07PM +0200, Peter Eisentraut wrote:
On 6/28/18 17:14, Alvaro Herrera wrote:
2 Reported-by:
5 Author:
6 Reviewed-by:
7 Tested-by:Should these include email addresses?
One reason I include emails is that sometimes the names are spelled
in inconsistent ways or don't include ASCII characters at all. An
email address is always clear.I don't know if emails are actually a good idea to include. Those
tend to change when folks change company, and a lot of people here
use company-based email addresses to discuss and work on patches.
Why would that be a problem? It's not a stable identifier, but it also
does not change very often. Also, those who submit patches from company
addresses do it because the patch comes from that company, and I think
it's a good idea to keep that information.
While it might not be the primary goal, I assume people will try to
process those fields by various scripts (generating stats, charts, ...).
E-mails seem to be easier to correlate than just names.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Jun 28, 2018 at 09:46:17AM -0700, Peter Geoghegan wrote:
* Don't assume that you haven't broken the doc build if you make even
a trivial doc change. Removing a GUC can break instances in the
release notes where they're referenced. Even grep can miss this, since
references to the GUC will have dashes rather than underscores, plus
possibly other variations.
Yes, I even check the docs after whitespace changes.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +On Fri, Jun 29, 2018 at 06:02:07PM -0400, Bruce Momjian wrote:
On Thu, Jun 28, 2018 at 09:46:17AM -0700, Peter Geoghegan wrote:
* Don't assume that you haven't broken the doc build if you make even
a trivial doc change. Removing a GUC can break instances in the
release notes where they're referenced. Even grep can miss this, since
references to the GUC will have dashes rather than underscores, plus
possibly other variations.Yes, I even check the docs after whitespace changes.
Could it be possible to mention as well to run pgindent before a commit?
After the indent run done for 11 beta 1, and it was just a matter of
days before some garbage diffs have accumulated in the C code, which
makes local runs more annoying when the same files are touched.
--
Michael
Greetings,
* Peter Geoghegan (pg@bowt.ie) wrote:
FWIW, I developed a document on committing for my own reference, with
some help from Andres. A lot of it is about commit message style, the
use of fields, and so on. But I've also developed a check list for
committing, knowing that there are a few classic mistakes that
committers will make from time to time despite knowing better. These
are worth checking against mechanically, IMV. Here are some actual
examples from this document that I refer to:
Good list.
* Do a dry run before really pushing by using --dry-run.
In addition to this, I'd recommend using 'git show' on the results of
the --dry-run, so that you see what you're really about to push.
Thanks!
Stephen
On 2018-Jul-02, Stephen Frost wrote:
* Do a dry run before really pushing by using --dry-run.
In addition to this, I'd recommend using 'git show' on the results of
the --dry-run, so that you see what you're really about to push.
Since commit 653530c8b196 I use this little script I borrowed from Magnus, then
page through all of it before pushing.
git push --dry-run 2>&1 | grep -v '^To' | while read line; do
if [ "$line" == "Everything up-to-date" ]; then
echo $line
else
topush=$(echo $line | awk '{print $1}')
git log --format=oneline $topush | cat
git show --format=fuller --color $topush | cat
fi
done | less -R
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Jun 28, 2018 at 10:52:42AM -0700, Peter Geoghegan wrote:
On Thu, Jun 28, 2018 at 9:52 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:FWIW, I developed a document on committing for my own reference, with
some help from Andres.
My rule has been to add to my private checklist anytime I mail or push a patch
containing a readily-checkable mistake. I go through the checklist before
mailing or pushing any patch. It has things in common with your list, plus
these:
* Validate err*() calls against https://www.postgresql.org/docs/devel/static/error-style-guide.html
* Validate *printf calls for trailing newlines
* Spellcheck the patch
* Verify long lines are not better broken
git diff origin/master | grep -E '^(\+|diff)' | sed 's/^+//' | expand -t4 | awk "length > 78 || /^diff/"
* Run pgindent on changed files; keep the changes I made necessary
* Update version numbers, if needed
CATALOG_VERSION_NO, PG_CONTROL_VERSION, XLOG_PAGE_MAGIC, PGSTAT_FILE_FORMAT_ID
* Write log message
Discussion: https://postgr.es/m/
Back-patch depth?
What should the release notes say?
Credit any reviewer.
* Check merge with master (not applicable to commits)
How about turning it into a wiki page, for everybody's benefit?
I'll try to do that, but I'd still recommend personalizing it. A lot
of the stuff in there is specific to my own workflow and tool
preferences, and my own personal working style.
I agree we won't all want the exact same checklist. Still, it wouldn't hurt
to have a wiki page of checklist entry ideas from which folks cherry-pick the
entries they like.
On Tue, Jul 10, 2018 at 9:14 PM, Noah Misch <noah@leadboat.com> wrote:
My rule has been to add to my private checklist anytime I mail or push a patch
containing a readily-checkable mistake. I go through the checklist before
mailing or pushing any patch. It has things in common with your list, plus
these:* Validate err*() calls against https://www.postgresql.org/docs/devel/static/error-style-guide.html
* Validate *printf calls for trailing newlines* Spellcheck the patch
* Verify long lines are not better broken
git diff origin/master | grep -E '^(\+|diff)' | sed 's/^+//' | expand -t4 | awk "length > 78 || /^diff/"* Run pgindent on changed files; keep the changes I made necessary
* Update version numbers, if needed
CATALOG_VERSION_NO, PG_CONTROL_VERSION, XLOG_PAGE_MAGIC, PGSTAT_FILE_FORMAT_ID
I'm going to use some of these, myself.
I'll try to do that, but I'd still recommend personalizing it. A lot
of the stuff in there is specific to my own workflow and tool
preferences, and my own personal working style.I agree we won't all want the exact same checklist. Still, it wouldn't hurt
to have a wiki page of checklist entry ideas from which folks cherry-pick the
entries they like.
You've convinced me that we should definitely have such a list. I've
put it on my TODO list.
--
Peter Geoghegan
Noah Misch <noah@leadboat.com> writes:
I agree we won't all want the exact same checklist. Still, it wouldn't hurt
to have a wiki page of checklist entry ideas from which folks cherry-pick the
entries they like.
013f320dc reminds me of something I check for religiously: look for
alternative output files for any regression test you're updating the
output of.
Actually updating said files, once you notice you need to, can be tricky
in itself. Most of the time it works to just apply the same patch to the
other variants as the delta you're observing for the output file that's
relevant to your own platform. Or you may be able to change configuration
so as to replicate the correct output for another alternative file. But
sometimes you just have to guess (and then watch the buildfarm to see if
you guessed right).
regards, tom lane
On Wed, Jul 11, 2018 at 4:35 PM, Peter Geoghegan <pg@bowt.ie> wrote:
You've convinced me that we should definitely have such a list. I've
put it on my TODO list.
I started this Wiki page:
https://wiki.postgresql.org/wiki/Committing_checklist
I've tried to avoid being too prescriptive. This is a work in progress.
--
Peter Geoghegan
On 23/07/2018 05:58, Tom Lane wrote:
013f320dc reminds me of something I check for religiously: look for
alternative output files for any regression test you're updating the
output of.Actually updating said files, once you notice you need to, can be tricky
in itself. Most of the time it works to just apply the same patch to the
other variants as the delta you're observing for the output file that's
relevant to your own platform. Or you may be able to change configuration
so as to replicate the correct output for another alternative file. But
sometimes you just have to guess (and then watch the buildfarm to see if
you guessed right).
There is a recipe for doing this using the "merge" command here:
https://wiki.postgresql.org/wiki/Regression_test_authoring#Updating_an_existing_regression_test
YMMV
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services