Обсуждение: The pgperltidy diffs in HEAD
I routinely run pgperltidy src/ when hacking on things, and am greeted with lots of diffs like how pgindent runs used to be. Are there objections to applying the diffs we've accumulated so far with a .git-blame-ignore-revs update alongside it? Are there reasons not that I am missing? Attached is the current output from pgperltidy, I haven't looked over it in detail but I am happy to take that on assuming it's not objected to. -- Daniel Gustafsson
Вложения
On 2025-Nov-25, Daniel Gustafsson wrote: > I routinely run pgperltidy src/ when hacking on things, and am greeted with > lots of diffs like how pgindent runs used to be. Are there objections to > applying the diffs we've accumulated so far with a .git-blame-ignore-revs > update alongside it? Are there reasons not that I am missing? None here. I tend to run pgperltidy on individual files so this is not normally a problem for me, but I kinda dislike that our steady status is not clean. > Attached is the current output from pgperltidy, I haven't looked over it in > detail but I am happy to take that on assuming it's not objected to. Hmm, I wonder if you ran this with our documented version of perltidy. I have vague memories of pgperltidy leaving the generate-lwlocknames.pl script the way it is now, for example. But then, maybe the one who used the wrong perltidy version is me. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
=?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de> writes:
> On 2025-Nov-25, Daniel Gustafsson wrote:
>> I routinely run pgperltidy src/ when hacking on things, and am greeted with
>> lots of diffs like how pgindent runs used to be. Are there objections to
>> applying the diffs we've accumulated so far with a .git-blame-ignore-revs
>> update alongside it? Are there reasons not that I am missing?
> None here. I tend to run pgperltidy on individual files so this is not
> normally a problem for me, but I kinda dislike that our steady status is
> not clean.
While I've not got any great objection to running pgperltidy now,
it seems like it'd be better if committers were all on the same page
about this. My understanding of the current policy is that we'll
keep the tree pgindent-clean on the fly, but worry about pgperltidy
only once a year or so. Is there consensus for tightening that up?
> Hmm, I wonder if you ran this with our documented version of perltidy.
This sort of thing is why I'm hesitant. We didn't really dare expect
committers to ensure pgindent cleanliness until we had that tool
fully integrated in our tree, so that there was one true (and readily
available) version to use. perltidy still fails that test AFAIK;
you have to go looking for the agreed-on version.
regards, tom lane
> On 25 Nov 2025, at 17:00, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de> writes: >> On 2025-Nov-25, Daniel Gustafsson wrote: >>> I routinely run pgperltidy src/ when hacking on things, and am greeted with >>> lots of diffs like how pgindent runs used to be. Are there objections to >>> applying the diffs we've accumulated so far with a .git-blame-ignore-revs >>> update alongside it? Are there reasons not that I am missing? > >> None here. I tend to run pgperltidy on individual files so this is not >> normally a problem for me, but I kinda dislike that our steady status is >> not clean. > > While I've not got any great objection to running pgperltidy now, > it seems like it'd be better if committers were all on the same page > about this. My understanding of the current policy is that we'll > keep the tree pgindent-clean on the fly, but worry about pgperltidy > only once a year or so. Is there consensus for tightening that up? I don't think there is, but I wouldn't mind if that was the case given how nice it is to have a pgindent clean tree at all times. >> Hmm, I wonder if you ran this with our documented version of perltidy. > > This sort of thing is why I'm hesitant. We didn't really dare expect > committers to ensure pgindent cleanliness until we had that tool > fully integrated in our tree, so that there was one true (and readily > available) version to use. perltidy still fails that test AFAIK; > you have to go looking for the agreed-on version. ..and since I managed to run it with the wrong version for the attached diff that argument certainly does have merit (I now gave up on having two version installed for $reasons and will settle on the postgres-mandated version for all things). Maybe this is best left alone for now and made into a topic for a committer meeting at pgconf.dev? -- Daniel Gustafsson
Daniel Gustafsson <daniel@yesql.se> writes:
> On 25 Nov 2025, at 17:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de> writes:
>>> Hmm, I wonder if you ran this with our documented version of perltidy.
> ..and since I managed to run it with the wrong version for the attached diff that
> argument certainly does have merit (I now gave up on having two version
> installed for $reasons and will settle on the postgres-mandated version for all
> things).
pgindent has long had a check that you're running the correct version
of bsdindent, but I just realized that pgperltidy makes no such check
for perltidy. Should we install one?
regards, tom lane
On 2025-Nov-25, Tom Lane wrote: > pgindent has long had a check that you're running the correct version > of bsdindent, but I just realized that pgperltidy makes no such check > for perltidy. Should we install one? Yeah, we should definitely have that. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
> On 25 Nov 2025, at 18:53, Álvaro Herrera <alvherre@kurilemu.de> wrote: > > On 2025-Nov-25, Tom Lane wrote: > >> pgindent has long had a check that you're running the correct version >> of bsdindent, but I just realized that pgperltidy makes no such check >> for perltidy. Should we install one? > > Yeah, we should definitely have that. Agreed. Perhaps something like the attached would work? -- Daniel Gustafsson
Вложения
On 2025-Nov-25, Daniel Gustafsson wrote: > Agreed. Perhaps something like the attached would work? Hmm, I got this src/tools/pgindent/pgperltidy: 10: [: 0: unexpected operator $ ls -l /bin/sh lrwxrwxrwx 1 root root 4 feb 4 2025 /bin/sh -> dash* (Seems to work ok silently with bash.) -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Digital and video cameras have this adjustment and film cameras don't for the same reason dogs and cats lick themselves: because they can." (Ken Rockwell)
> On 25 Nov 2025, at 20:31, Álvaro Herrera <alvherre@kurilemu.de> wrote:
>
> On 2025-Nov-25, Daniel Gustafsson wrote:
>
>> Agreed. Perhaps something like the attached would work?
>
> Hmm, I got this
>
> src/tools/pgindent/pgperltidy: 10: [: 0: unexpected operator
>
> $ ls -l /bin/sh
> lrwxrwxrwx 1 root root 4 feb 4 2025 /bin/sh -> dash*
>
> (Seems to work ok silently with bash.)
If you replace the if statement with the one below using test, does that make
it work?
if test $? = 1; then
--
Daniel Gustafsson
Daniel Gustafsson <daniel@yesql.se> writes:
>> On 2025-Nov-25, Tom Lane wrote:
>>> pgindent has long had a check that you're running the correct version
>>> of bsdindent, but I just realized that pgperltidy makes no such check
>>> for perltidy. Should we install one?
> Agreed. Perhaps something like the attached would work?
WFM.
regards, tom lane
On 2025-11-25 Tu 2:36 PM, Daniel Gustafsson wrote: >> On 25 Nov 2025, at 20:31, Álvaro Herrera <alvherre@kurilemu.de> wrote: >> >> On 2025-Nov-25, Daniel Gustafsson wrote: >> >>> Agreed. Perhaps something like the attached would work? >> Hmm, I got this >> >> src/tools/pgindent/pgperltidy: 10: [: 0: unexpected operator >> >> $ ls -l /bin/sh >> lrwxrwxrwx 1 root root 4 feb 4 2025 /bin/sh -> dash* >> >> (Seems to work ok silently with bash.) > If you replace the if statement with the one below using test, does that make > it work? > > if test $? = 1; then > Looks to me like your original patch had == where it should have had = Your formulation above corrects that. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Andrew Dunstan <andrew@dunslane.net> writes:
> On 2025-11-25 Tu 2:36 PM, Daniel Gustafsson wrote:
>>> On 25 Nov 2025, at 20:31, Álvaro Herrera <alvherre@kurilemu.de> wrote:
>>>
>>> On 2025-Nov-25, Daniel Gustafsson wrote:
>>>
>>>> Agreed. Perhaps something like the attached would work?
>>> Hmm, I got this
>>>
>>> src/tools/pgindent/pgperltidy: 10: [: 0: unexpected operator
>>>
>>> $ ls -l /bin/sh
>>> lrwxrwxrwx 1 root root 4 feb 4 2025 /bin/sh -> dash*
>>>
>>> (Seems to work ok silently with bash.)
>> If you replace the if statement with the one below using test, does that make
>> it work?
>>
>> if test $? = 1; then
>>
>
> Looks to me like your original patch had == where it should have had =
>
> Your formulation above corrects that.
Even simpler, and avoiding having to move the `set -e` after the check:
PERLTIDY_VERSION=20230309
if ! $PERLTIDY -v | grep -q $PERLTIDY_VERSION; then
echo "error: pgperltidy requires perltidy v$PERLTIDY_VERSION"
exit 1
fi
- ilmari
On Tue, Nov 25, 2025 at 2:03 PM Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> wrote: > Even simpler, and avoiding having to move the `set -e` after the check: +1; you beat me to it by a couple seconds :) --Jacob
> On 25 Nov 2025, at 23:03, Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> wrote: > Even simpler, and avoiding having to move the `set -e` after the check: > > PERLTIDY_VERSION=20230309 > if ! $PERLTIDY -v | grep -q $PERLTIDY_VERSION; then > echo "error: pgperltidy requires perltidy v$PERLTIDY_VERSION" > exit 1 > fi Ah, even better, thanks! -- Daniel Gustafsson
Вложения
Daniel Gustafsson <daniel@yesql.se> writes:
> [2. text/x-diff; v2_perltidyversion.diff]
> diff --git a/doc/src/sgml/func/func-string.sgml b/doc/src/sgml/func/func-string.sgml
> index 7ad1436e5f8..646b5d6d8c4 100644
> --- a/doc/src/sgml/func/func-string.sgml
> +++ b/doc/src/sgml/func/func-string.sgml
> @@ -173,7 +173,7 @@
This seems unrelated to the rest of the patch patch.
> diff --git a/src/tools/pgindent/pgperltidy b/src/tools/pgindent/pgperltidy
> index 6af27d21d55..6fac758665a 100755
> --- a/src/tools/pgindent/pgperltidy
> +++ b/src/tools/pgindent/pgperltidy
> @@ -7,6 +7,12 @@ set -e
> # set this to override default perltidy program:
> PERLTIDY=${PERLTIDY:-perltidy}
>
> +PERLTIDY_VERSION=20230309
> +if ! $PERLTIDY -v | grep -q $PERLTIDY_VERSION; then
> + echo "error: pgperltidy requires perltidy v$PERLTIDY_VERSION"
I just realised, this message should really go to stderr, i.e. have >&2
on the end.
> + exit 1
> +fi
> +
> . src/tools/perlcheck/find_perl_files
>
> find_perl_files "$@" | xargs $PERLTIDY --profile=src/tools/pgindent/perltidyrc
- ilmari
> On 26 Nov 2025, at 12:11, Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> wrote:
> This seems unrelated to the rest of the patch patch.
Yeah, I was working on a docs-patch in the same tree and only realized this
morning that I had accidentally included that part =)
>> diff --git a/src/tools/pgindent/pgperltidy b/src/tools/pgindent/pgperltidy
>> index 6af27d21d55..6fac758665a 100755
>> --- a/src/tools/pgindent/pgperltidy
>> +++ b/src/tools/pgindent/pgperltidy
>> @@ -7,6 +7,12 @@ set -e
>> # set this to override default perltidy program:
>> PERLTIDY=${PERLTIDY:-perltidy}
>>
>> +PERLTIDY_VERSION=20230309
>> +if ! $PERLTIDY -v | grep -q $PERLTIDY_VERSION; then
>> + echo "error: pgperltidy requires perltidy v$PERLTIDY_VERSION"
>
> I just realised, this message should really go to stderr, i.e. have >&2
> on the end.
Good point, I'll change that before committing and I'll also reword it to use
the same error as pgindent which has this:
"You do not appear to have $indent version $INDENT_VERSION installed on your system.\n";
Consistency is good.
--
Daniel Gustafsson
> On 26 Nov 2025, at 12:16, Daniel Gustafsson <daniel@yesql.se> wrote: > I'll change that before committing and I'll also reword it to use > the same error as pgindent More specifically, the attached is what I have staged for commit. -- Daniel Gustafsson
Вложения
Daniel Gustafsson <daniel@yesql.se> writes: >> On 26 Nov 2025, at 12:16, Daniel Gustafsson <daniel@yesql.se> wrote: > >> I'll change that before committing and I'll also reword it to use >> the same error as pgindent > > More specifically, the attached is what I have staged for commit. LGTM > -- > Daniel Gustafsson - ilmari