Обсуждение: Three commit tips

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:

    # https://www.postgresql.org/message-id/CAGECzQQL-Dbb%2BYkid9Dhq-491MawHvi6hR_NGkhiDE%2B5zRZ6vQ%40mail.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.

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

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.

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:
>
>         #
https://www.postgresql.org/message-id/CAGECzQQL-Dbb%2BYkid9Dhq-491MawHvi6hR_NGkhiDE%2B5zRZ6vQ%40mail.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

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] 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] https://www.postgresql.org/message-id/CAFiTN-uyiUXU__VwJAimZ%2B6jQbm1s4sYi6u4fXBD%3D47xVd%3Dthg%40mail.gmail.com

Best regards,
Alexander