Обсуждение: Re: Tips on committing
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 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: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 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 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 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 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 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 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 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 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 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
Вложения
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
Вложения
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 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 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
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
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 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
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