Three commit tips

Started by Bruce Momjianabout 2 years ago5 messages
#1Bruce Momjian
bruce@momjian.us

I have three suggestions on committing that I thought would be helpful
to the hacker audience.

First, I have been hesitant to ascribe others as patch authors if I
heavily modified a doc patch because I didn't want them blamed for any
mistakes I made. However, I also want to give them credit, so I decided
I would annotate commits with "partial", e.g.:

Author: Andy Jackson (partial)

Second, I have found the git diff option --word-diff=color to be very
helpful and I have started using it, especially for doc patches where
the old text is in red and the new text is in green. Posting patches in
that format is probably not helpful though.

Third, I have come up with the following shell script to test for proper
pgindentation, which I run automatically before commit:

# /messages/by-id/CAGECzQQL-Dbb+Ykid9Dhq-491MawHvi6hR_NGkhiDE+5zRZ6vQ@mail.gmail.com
src/tools/pgindent/pgindent $(git diff --name-only --diff-filter=ACMR) > /tmp/$$

if [ \( "$(wc -l < /tmp/$$)" -eq 1 -a "$(expr match "$(cat /tmp/$$)" "No files to process\>")" -eq 0 \) -o \
"$(wc -l < /tmp/$$)" -gt 1 ]
then echo "pgindent failure in master branch, exiting." 1>&2
cat /tmp/$$
exit 1
fi

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

Only you can decide what is important to you.

#2Nathan Bossart
nathandbossart@gmail.com
In reply to: Bruce Momjian (#1)
Re: Three commit tips

On Thu, Nov 02, 2023 at 11:22:38AM -0400, Bruce Momjian wrote:

First, I have been hesitant to ascribe others as patch authors if I
heavily modified a doc patch because I didn't want them blamed for any
mistakes I made. However, I also want to give them credit, so I decided
I would annotate commits with "partial", e.g.:

Author: Andy Jackson (partial)

I tend to use "Co-authored-by" for this purpose.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

#3Bruce Momjian
bruce@momjian.us
In reply to: Nathan Bossart (#2)
Re: Three commit tips

On Thu, Nov 2, 2023 at 11:07:19AM -0500, Nathan Bossart wrote:

On Thu, Nov 02, 2023 at 11:22:38AM -0400, Bruce Momjian wrote:

First, I have been hesitant to ascribe others as patch authors if I
heavily modified a doc patch because I didn't want them blamed for any
mistakes I made. However, I also want to give them credit, so I decided
I would annotate commits with "partial", e.g.:

Author: Andy Jackson (partial)

I tend to use "Co-authored-by" for this purpose.

Very good idea. I will use that instead.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

Only you can decide what is important to you.

#4Ashutosh Bapat
ashutosh.bapat.oss@gmail.com
In reply to: Bruce Momjian (#1)
Re: Three commit tips

On Thu, Nov 2, 2023 at 8:52 PM Bruce Momjian <bruce@momjian.us> wrote:

Third, I have come up with the following shell script to test for proper
pgindentation, which I run automatically before commit:

# /messages/by-id/CAGECzQQL-Dbb+Ykid9Dhq-491MawHvi6hR_NGkhiDE+5zRZ6vQ@mail.gmail.com
src/tools/pgindent/pgindent $(git diff --name-only --diff-filter=ACMR) > /tmp/$$

if [ \( "$(wc -l < /tmp/$$)" -eq 1 -a "$(expr match "$(cat /tmp/$$)" "No files to process\>")" -eq 0 \) -o \
"$(wc -l < /tmp/$$)" -gt 1 ]
then echo "pgindent failure in master branch, exiting." 1>&2
cat /tmp/$$
exit 1
fi

Looks useful. Git supports pre-push hook:
https://git-scm.com/book/en/v2/Customizing-Git-Git-Hooks, a sample
script showing how to access the commits being pushed and other
arguments at https://github.com/git/git/blob/87c86dd14abe8db7d00b0df5661ef8cf147a72a3/templates/hooks--pre-push.sample.
I have not used it. But it seems that your script can be used to
implement the pre-push hook.

--
Best Wishes,
Ashutosh Bapat

#5Alexander Lakhin
exclusion@gmail.com
In reply to: Bruce Momjian (#1)
1 attachment(s)
Re: Three commit tips

02.11.2023 18:22, Bruce Momjian wrote:

Third, I have come up with the following shell script to test for proper
pgindentation, which I run automatically before commit:

I would also suggest using a script like attached to check a patch for
new unicums (usually some of them are typos or inconsistencies introduced
by the patch).
The script itself can be improved, without a doubt, but it still can be
useful as-is. For example, for a couple of arbitrarily chosen today's
patches [1]/messages/by-id/CAFiTN-uyiUXU__VwJAimZ+6jQbm1s4sYi6u4fXBD=47xVd=thg@mail.gmail.com it shows:

.../postgresql.git$ check-patch-for-unicums.sh .../v4-0001-Make-all-SLRU-buffer-sizes-configurable.patch
New unicums:
cotents:        ./doc/src/sgml/config.sgml:        Specifies the amount of memory to use to cache the cotents of
====

.../postgresql.git$ check-patch-for-unicums.sh .../v4-0003-Partition-wise-slru-locks.patch
New unicums:
bank_tranche_id: ./src/include/access/slru.h: int bank_tranche_id, SyncRequestHandler sync_handler);
CommitTSSLRU:   ./src/backend/storage/lmgr/lwlock.c: "CommitTSSLRU",
CommitTsSLRULock: ./src/backend/storage/lmgr/lwlocknames.txt:#38 was CommitTsSLRULock
ControlLock:    ./src/backend/replication/slot.c:        * flag while holding the ControlLock as otherwise a concurrent
ctllock:        ./src/backend/access/transam/slru.c: * ctllock: LWLock to use to control access to the shared control
structure.
cur_lru_count:  ./src/backend/access/transam/slru.c:             * Notice that this next line forcibly advances
cur_lru_count to a
...

[1]: /messages/by-id/CAFiTN-uyiUXU__VwJAimZ+6jQbm1s4sYi6u4fXBD=47xVd=thg@mail.gmail.com

Best regards,
Alexander

Attachments:

check-patch-for-unicums.shapplication/x-shellscript; name=check-patch-for-unicums.shDownload