Message-ID: <8a58ec86-8a0d-1023-87f8-2aa47a860698@dunslane.net>
Date: Thu, 26 Jan 2023 11:54:11 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: run pgindent on a regular basis / scripted manner
Content-Language: en-US
To: Jelte Fennema <postgres@jeltef.nl>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Andres Freund <andres@anarazel.de>,
 Peter Geoghegan <pg@bowt.ie>, Bruce Momjian <bruce@momjian.us>,
 Magnus Hagander <magnus@hagander.net>,
 Alvaro Herrera <alvherre@2ndquadrant.com>, Stephen Frost
 <sfrost@snowman.net>, Noah Misch <noah@leadboat.com>,
 Jesse Zhang <sbjesse@gmail.com>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>
References: <1097305.1674334785@sss.pgh.pa.us>
 <CAH2-WzkHHcPRMj3G1b_dUfY0aR4W4OAHQ9_Z6sDGvL8Ew+aRrA@mail.gmail.com>
 <20230121224312.qmwpagrebidr6hi7@awork3.anarazel.de>
 <CAH2-Wzm0niYBPcN6eX3PzHABfj0dE3fwifo5bdturvMp=9FGqw@mail.gmail.com>
 <20230122094957.x542ojaqgi4lalq4@awork3.anarazel.de>
 <CAGECzQRYjHdNQ-Bq2yrBb-8s3Pjx7pfvYPDdNv9tcvDcscDhbg@mail.gmail.com>
 <20230122223806.3sgew6qswhsczae7@awork3.anarazel.de>
 <1495023.1674430107@sss.pgh.pa.us>
 <20230123001518.6hxyiczhn4kadvmf@awork3.anarazel.de>
 <1502504.1674433722@sss.pgh.pa.us>
 <20230123011002.fzcaa3krql3mqsfn@awork3.anarazel.de>
 <1509591.1674437376@sss.pgh.pa.us>
 <CAGECzQQG=VLe71VokP9SPMOVc1P9jLPnU0-13bf-fjASMNdo9g@mail.gmail.com>
 <b7e3bb37-412b-0eff-2528-63ee264f0078@dunslane.net>
 <CAGECzQQmM=hOpV683gdSFj4+VVArY1XpZ+kD8YctBNQpPsMQ+Q@mail.gmail.com>
 <f8006b81-2646-bfa8-d770-501e4bf31241@dunslane.net>
 <CAGECzQRfkuw-A4xvPMm8=uJ=T5dQ35qBcU-wPYE68E0Ciu-cfg@mail.gmail.com>
From: Andrew Dunstan <andrew@dunslane.net>
In-Reply-To: 
 <CAGECzQRfkuw-A4xvPMm8=uJ=T5dQ35qBcU-wPYE68E0Ciu-cfg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/8a58ec86-8a0d-1023-87f8-2aa47a860698%40dunslane.net>
Precedence: bulk


On 2023-01-26 Th 11:16, Jelte Fennema wrote:
> On Thu, 26 Jan 2023 at 15:40, Andrew Dunstan <andrew@dunslane.net> wrote:
>> I didn't really like your hook, as it forces a reindent, and many people
>> won't want that (for reasons given elsewhere in this thread).
> I'm not sure what you mean by "forces a reindent". Like I explained
> you can simply run "git commit" again to ignore the changes and
> commit anyway. As long as the files are indented on your filesystem
> the hook doesn't care if you actually included the indentation changes
> in the changes that you're currently committing.


Your hook does this:


+git diff --cached --name-only --diff-filter=ACMR | grep '\.[ch]$' |\
+    xargs src/tools/pgindent/pgindent --silent-diff \
+    || {
+        echo ERROR: Aborting commit because pgindent was not run
+        git diff --cached --name-only --diff-filter=ACMR | grep
'\.[ch]$' | xargs src/tools/pgindent/pgindent
+        exit 1
+    }


At this stage the files are now indented, so if it failed and you run
`git commit` again it will commit with the indention changes.


>
> So to be completely clear you can do the following with my hook:
> git commit # runs pgindent and fails
> git commit # commits changes anyway
> git commit -am 'Run pgindent' # commit indentation changes separately
>
> Or what I usually do:
> git commit # runs pgindent and fails
> git add --patch # choose relevant changes to add to commit
> git commit # commit the changes
> git checkout -- . # undo irrelevant changes on filesystem
>
> Honestly PGAUTOINDENT=no seems stricter, since the only
> way to bypass the failure is now to run manually run pgindent
> or git commit with the --no-verify flag.
>
>> files=$(git diff --cached --name-only --diff-filter=ACMR)
>> src/tools/pgindent/pgindent $files
> That seems like it would fail if there's any files or directories with
> spaces in them. Maybe this isn't something we care about though.


We don't have any, and the filenames git produces are relative to the
git root. I don't think this is an issue.


>
>> # no need to filter files - pgindent ignores everything that isn't a
>> # .c or .h file
> If the first argument is a non .c or .h file, then pgindent interprets
> it as the typedefs file. So it's definitely important to filter non .c
> and .h files out. Because now if you commit a single
> non .c or .h file this hook messes up the indentation in all of
> your files. You can reproduce by running:
> src/tools/pgindent/pgindent README


I have a patch at [1] to remove this misfeature.


>
>> # only do this on master
>> test  "$branch" = "master" || return 0
> I would definitely want a way to disable this check. As a normal
> submitter I never work directly on master.


Sure, that's your choice. My intended audience here is committers, who
of course do work on master.


cheers


andrew


[1]  https://postgr.es/m/21bb8573-9e56-812b-84cf-1e4f3c4c2a7b@dunslane.net

--
Andrew Dunstan
EDB: https://www.enterprisedb.com