Обсуждение: Add --check option to pgindent

Not sold on the name, but --check is a combination of --silent-diff and
--show-diff. I envision --check mostly being used in CI environments.
I recently came across a situation where this behavior would have been
useful. Without --check, you're left to capture the output of
--show-diff and exit 2 if the output isn't empty by yourself.

--
Tristan Partin
Neon (https://neon.tech)

> On 12 Dec 2023, at 01:09, Tristan Partin <tristan@neon.tech> wrote:
>
> Not sold on the name, but --check is a combination of --silent-diff and --show-diff. I envision --check mostly being
usedin CI environments. I recently came across a situation where this behavior would have been useful. Without --check,
you'releft to capture the output of --show-diff and exit 2 if the output isn't empty by yourself. 

I haven’t studied the patch but I can see that being useful.

./daniel

> On 12 Dec 2023, at 01:09, Tristan Partin <tristan@neon.tech> wrote:
>
> Not sold on the name, but --check is a combination of --silent-diff and --show-diff. I envision --check mostly being
usedin CI environments. I recently came across a situation where this behavior would have been useful. Without --check,
you'releft to capture the output of --show-diff and exit 2 if the output isn't empty by yourself. 

I wonder if we should model this around the semantics of git diff to keep it
similar to other CI jobs which often use git diff?  git diff --check means "are
there conflicts or issues" which isn't really comparable to here, git diff
--exit-code however is pretty much exactly what this is trying to accomplish.

That would make pgindent --show-diff --exit-code exit with 1 if there were
diffs and 0 if there are no diffs.

--
Daniel Gustafsson

Hi,

On Tue, Dec 12, 2023 at 11:18:59AM +0100, Daniel Gustafsson wrote:
> > On 12 Dec 2023, at 01:09, Tristan Partin <tristan@neon.tech> wrote:
> > 
> > Not sold on the name, but --check is a combination of --silent-diff
> > and --show-diff. I envision --check mostly being used in CI
> > environments. I recently came across a situation where this behavior
> > would have been useful. Without --check, you're left to capture the
> > output of --show-diff and exit 2 if the output isn't empty by
> > yourself.
> 
> I wonder if we should model this around the semantics of git diff to
> keep it similar to other CI jobs which often use git diff?  git diff
> --check means "are there conflicts or issues" which isn't really
> comparable to here, git diff --exit-code however is pretty much
> exactly what this is trying to accomplish.
> 
> That would make pgindent --show-diff --exit-code exit with 1 if there
> were diffs and 0 if there are no diffs.

To be honest, I find that rather convoluted; contrary to "git diff", I
believe the primary action of pgident is not to show diffs, so I find
the proposed --check option to be entirely reasonable from a UX
perspective.

On the other hand, tying a "does this need re-indenting?" question to a
"--show-diff --exit-code" option combination is not very obvious (to me,
at least).


Cheers,

Michael

On Tue, Dec 12, 2023 at 11:18:59AM +0100, Daniel Gustafsson wrote:

> > On 12 Dec 2023, at 01:09, Tristan Partin <tristan@neon.tech> wrote:

> > 

> > Not sold on the name, but --check is a combination of --silent-diff

> > and --show-diff. I envision --check mostly being used in CI

> > environments. I recently came across a situation where this behavior

> > would have been useful. Without --check, you're left to capture the

> > output of --show-diff and exit 2 if the output isn't empty by

> > yourself.

> 

> I wonder if we should model this around the semantics of git diff to

> keep it similar to other CI jobs which often use git diff?  git diff

> --check means "are there conflicts or issues" which isn't really

> comparable to here, git diff --exit-code however is pretty much

> exactly what this is trying to accomplish.

> 

> That would make pgindent --show-diff --exit-code exit with 1 if there

> were diffs and 0 if there are no diffs.

To be honest, I find that rather convoluted; contrary to "git diff", I

believe the primary action of pgident is not to show diffs, so I find

the proposed --check option to be entirely reasonable from a UX

perspective.

On the other hand, tying a "does this need re-indenting?" question to a

"--show-diff --exit-code" option combination is not very obvious (to me,

at least).

"Euler Taveira" <euler@eulerto.com> writes:
> When you add exceptions, it starts to complicate the UI.

Indeed.  It seems like --silent-diff was poorly defined and poorly
named, and we need to rethink that option along the way to adding
this behavior.  The idea that --show-diff and --silent-diff can
be used together is just inherently confusing, because they sound
like opposites.

            regards, tom lane

On Tue Dec 12, 2023 at 5:47 AM CST, Euler Taveira wrote:
> On Tue, Dec 12, 2023, at 7:28 AM, Michael Banck wrote:
> > On Tue, Dec 12, 2023 at 11:18:59AM +0100, Daniel Gustafsson wrote:
> > > > On 12 Dec 2023, at 01:09, Tristan Partin <tristan@neon.tech> wrote:
> > > >
> > > > Not sold on the name, but --check is a combination of --silent-diff
> > > > and --show-diff. I envision --check mostly being used in CI
> > > > environments. I recently came across a situation where this behavior
> > > > would have been useful. Without --check, you're left to capture the
> > > > output of --show-diff and exit 2 if the output isn't empty by
> > > > yourself.
> > >
> > > I wonder if we should model this around the semantics of git diff to
> > > keep it similar to other CI jobs which often use git diff?  git diff
> > > --check means "are there conflicts or issues" which isn't really
> > > comparable to here, git diff --exit-code however is pretty much
> > > exactly what this is trying to accomplish.
> > >
> > > That would make pgindent --show-diff --exit-code exit with 1 if there
> > > were diffs and 0 if there are no diffs.
> >
> > To be honest, I find that rather convoluted; contrary to "git diff", I
> > believe the primary action of pgident is not to show diffs, so I find
> > the proposed --check option to be entirely reasonable from a UX
> > perspective.
> >
> > On the other hand, tying a "does this need re-indenting?" question to a
> > "--show-diff --exit-code" option combination is not very obvious (to me,
> > at least).
>
> Multiple options to accomplish a use case might not be obvious. I'm wondering
> if we can combine it into a unique option.
>
> --show-diff             show the changes that would be made
> --silent-diff           exit with status 2 if any changes would be made
> + --check                 combination of --show-diff and --silent-diff
>
> I mean
>
> --diff=show,silent,check
>
> When you add exceptions, it starts to complicate the UI.
>
> usage("Cannot have both --silent-diff and --show-diff")
>    if $silent_diff && $show_diff;
>
> +usage("Cannot have both --check and --show-diff")
> +  if $check && $show_diff;
> +
> +usage("Cannot have both --check and --silent-diff")
> +  if $check && $silent_diff;
> +

I definitely agree. I just didn't want to remove options, but if people
are ok with that, I will just change the interface.

I like the git-diff semantics or have --diff and --check, similar to how
Python's black[0] is.

[0]: https://github.com/psf/black

--
Tristan Partin
Neon (https://neon.tech)

On 2023-Dec-12, Tom Lane wrote:

> "Euler Taveira" <euler@eulerto.com> writes:
> > When you add exceptions, it starts to complicate the UI.
> 
> Indeed.  It seems like --silent-diff was poorly defined and poorly
> named, and we need to rethink that option along the way to adding
> this behavior.  The idea that --show-diff and --silent-diff can
> be used together is just inherently confusing, because they sound
> like opposites.

Maybe it's enough to rename --silent-diff to --check.  You can do
"--show-diff --check" and get both the error and the diff printed; or
just "--check" and it'll throw an error without further ado; or
"--show-diff" and it will both apply the diff and print it.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"I must say, I am absolutely impressed with what pgsql's implementation of
VALUES allows me to do. It's kind of ridiculous how much "work" goes away in
my code.  Too bad I can't do this at work (Oracle 8/9)."       (Tom Allison)
           http://archives.postgresql.org/pgsql-general/2007-06/msg00016.php

On 2023-12-12 Tu 10:30, Alvaro Herrera wrote:
> On 2023-Dec-12, Tom Lane wrote:
>
>> "Euler Taveira" <euler@eulerto.com> writes:
>>> When you add exceptions, it starts to complicate the UI.
>> Indeed.  It seems like --silent-diff was poorly defined and poorly
>> named, and we need to rethink that option along the way to adding
>> this behavior.  The idea that --show-diff and --silent-diff can
>> be used together is just inherently confusing, because they sound
>> like opposites
> Maybe it's enough to rename --silent-diff to --check.  You can do
> "--show-diff --check" and get both the error and the diff printed; or
> just "--check" and it'll throw an error without further ado; or
> "--show-diff" and it will both apply the diff and print it.
>

That seems reasonable. These features were fairly substantially debated 
when we put them in, but I'm fine with some tweaking. But note: 
--show-diff doesn't apply the diff, it's intentionally non-destructive.


cheers


andrew

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

On Wed Dec 13, 2023 at 2:46 PM CST, Andrew Dunstan wrote:
>
> On 2023-12-12 Tu 10:30, Alvaro Herrera wrote:
> > On 2023-Dec-12, Tom Lane wrote:
> >
> >> "Euler Taveira" <euler@eulerto.com> writes:
> >>> When you add exceptions, it starts to complicate the UI.
> >> Indeed.  It seems like --silent-diff was poorly defined and poorly
> >> named, and we need to rethink that option along the way to adding
> >> this behavior.  The idea that --show-diff and --silent-diff can
> >> be used together is just inherently confusing, because they sound
> >> like opposites
> > Maybe it's enough to rename --silent-diff to --check.  You can do
> > "--show-diff --check" and get both the error and the diff printed; or
> > just "--check" and it'll throw an error without further ado; or
> > "--show-diff" and it will both apply the diff and print it.
> >
>
> That seems reasonable. These features were fairly substantially debated
> when we put them in, but I'm fine with some tweaking. But note:
> --show-diff doesn't apply the diff, it's intentionally non-destructive.

Here is a new patch:

- Renames --silent-diff to --check
- Renames --show-diff to --diff
- Allows one to use --check and --diff in the same command

I am not tied to the second patch if people like --show-diff better than
--diff.

Weirdly enough, my email client doesn't show this as part of the
original thread, but I will respond here anyway.

--
Tristan Partin
Neon (https://neon.tech)

This part of the first patch seems incorrect, i.e. same condition in
the if as elsif

-       if ($silent_diff)
+       if ($check)
+       {
+           print show_diff($source, $source_filename);
+           exit 2;
+       }
+       elsif ($check)
        {
            exit 2;
        }

On Thu, 14 Dec 2023 at 17:54, Tristan Partin <tristan@neon.tech> wrote:
>
> On Wed Dec 13, 2023 at 2:46 PM CST, Andrew Dunstan wrote:
> >
> > On 2023-12-12 Tu 10:30, Alvaro Herrera wrote:
> > > On 2023-Dec-12, Tom Lane wrote:
> > >
> > >> "Euler Taveira" <euler@eulerto.com> writes:
> > >>> When you add exceptions, it starts to complicate the UI.
> > >> Indeed.  It seems like --silent-diff was poorly defined and poorly
> > >> named, and we need to rethink that option along the way to adding
> > >> this behavior.  The idea that --show-diff and --silent-diff can
> > >> be used together is just inherently confusing, because they sound
> > >> like opposites
> > > Maybe it's enough to rename --silent-diff to --check.  You can do
> > > "--show-diff --check" and get both the error and the diff printed; or
> > > just "--check" and it'll throw an error without further ado; or
> > > "--show-diff" and it will both apply the diff and print it.
> > >
> >
> > That seems reasonable. These features were fairly substantially debated
> > when we put them in, but I'm fine with some tweaking. But note:
> > --show-diff doesn't apply the diff, it's intentionally non-destructive.
>
> Here is a new patch:
>
> - Renames --silent-diff to --check
> - Renames --show-diff to --diff
> - Allows one to use --check and --diff in the same command
>
> I am not tied to the second patch if people like --show-diff better than
> --diff.
>
> Weirdly enough, my email client doesn't show this as part of the
> original thread, but I will respond here anyway.
>
> --
> Tristan Partin
> Neon (https://neon.tech)

On Fri Dec 15, 2023 at 8:18 AM CST, Jelte Fennema-Nio wrote:
> This part of the first patch seems incorrect, i.e. same condition in
> the if as elsif
>
> -       if ($silent_diff)
> +       if ($check)
> +       {
> +           print show_diff($source, $source_filename);
> +           exit 2;
> +       }
> +       elsif ($check)
>         {
>             exit 2;
>         }

Weird how I was able to screw that up so bad :). Thanks! Here is a v3.
--
Tristan Partin
Neon (https://neon.tech)

> On 15 Dec 2023, at 16:43, Tristan Partin <tristan@neon.tech> wrote:

> Here is a v3.

I think this is pretty much ready to go, the attached v4 squashes the changes
and fixes the man-page which also needed an update.  The referenced Wiki page
will need an edit or two after this goes in, but that's easy enough.

--
Daniel Gustafsson

> On 15 Dec 2023, at 16:43, Tristan Partin <tristan@neon.tech> wrote:

> Here is a v3.

I think this is pretty much ready to go, the attached v4 squashes the changes

and fixes the man-page which also needed an update.  The referenced Wiki page

will need an edit or two after this goes in, but that's easy enough.

On Mon Dec 18, 2023 at 6:41 AM CST, Daniel Gustafsson wrote:
> > On 15 Dec 2023, at 16:43, Tristan Partin <tristan@neon.tech> wrote:
>
> > Here is a v3.
>
> I think this is pretty much ready to go, the attached v4 squashes the changes
> and fixes the man-page which also needed an update.  The referenced Wiki page
> will need an edit or two after this goes in, but that's easy enough.

I have never edited the Wiki before. How can I do that? More than happy
to do it.

--
Tristan Partin
Neon (https://neon.tech)

On Mon Dec 18, 2023 at 7:56 AM CST, Euler Taveira wrote:
> On Mon, Dec 18, 2023, at 9:41 AM, Daniel Gustafsson wrote:
> > > On 15 Dec 2023, at 16:43, Tristan Partin <tristan@neon.tech> wrote:
> >
> > > Here is a v3.
> >
> > I think this is pretty much ready to go, the attached v4 squashes the changes
> > and fixes the man-page which also needed an update.  The referenced Wiki page
> > will need an edit or two after this goes in, but that's easy enough.
>
> ... and pgbuildfarm client [1] needs to be changed too.
>
>
> [1] https://github.com/PGBuildFarm/client-code/blob/main/PGBuild/Modules/CheckIndent.pm#L55-L90

Thanks Euler. I am on it.

--
Tristan Partin
Neon (https://neon.tech)

On Mon, 18 Dec 2023 at 16:45, Tristan Partin <tristan@neon.tech> wrote:
>
> On Mon Dec 18, 2023 at 6:41 AM CST, Daniel Gustafsson wrote:
> > > On 15 Dec 2023, at 16:43, Tristan Partin <tristan@neon.tech> wrote:
> >
> > > Here is a v3.
> >
> > I think this is pretty much ready to go, the attached v4 squashes the changes
> > and fixes the man-page which also needed an update.  The referenced Wiki page
> > will need an edit or two after this goes in, but that's easy enough.
>
> I have never edited the Wiki before. How can I do that? More than happy
> to do it.

As mentioned at the bottom of the main page of the wiki:

  NOTE: due to recent spam activity "editor" privileges are granted
manually for the time being.
  If you just created a new community account or if your current
account used to have "editor" privileges ask on either the PostgreSQL
-www Mailinglist or the PostgreSQL IRC Channel (direct your request to
'pginfra', multiple individuals in the channel highlight on that
string) for help. Please include your community account name in those
requests.

After that, it's just a case of loggin in on the wiki (link top right
corner, which uses the community account) and then just go on to your
prefered page, click edit, and do your modifications. Don't forget to
save the modifications - I'm not sure whether the wiki editor's state
is locally persisted.

Kind regards,

Matthias van de Meent
Neon (https://neon.tech)

On Mon, 18 Dec 2023 at 13:42, Daniel Gustafsson <daniel@yesql.se> wrote:
> I think this is pretty much ready to go, the attached v4 squashes the changes
> and fixes the man-page which also needed an update.  The referenced Wiki page
> will need an edit or two after this goes in, but that's easy enough.

One thing I'm wondering: When both --check and --diff are passed,
should pgindent still early exit with 2 on the first incorrectly
formatted file? Or should it show diffs for all failing files? I'm
leaning towards the latter making more sense.

Related (but not required for this patch): For my pre-commit hook I
would very much like it if there was an option to have pgindent write
the changes to disk, but still exit non-zero, e.g. a --write flag that
could be combined with --check just like --diff and --check can be
combined with this patch. Currently my pre-commit hook need two
separate calls to pgindent to both abort the commit and write the
required changes to disk.

On Mon, 18 Dec 2023 at 17:14, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> One thing I'm wondering: When both --check and --diff are passed,
> should pgindent still early exit with 2 on the first incorrectly
> formatted file? Or should it show diffs for all failing files? I'm
> leaning towards the latter making more sense.

To be clear, in the latter case it would still exit with 2 at the end
of the script, but only after showing diffs for all files.

On Mon Dec 18, 2023 at 10:14 AM CST, Jelte Fennema-Nio wrote:
> On Mon, 18 Dec 2023 at 13:42, Daniel Gustafsson <daniel@yesql.se> wrote:
> > I think this is pretty much ready to go, the attached v4 squashes the changes
> > and fixes the man-page which also needed an update.  The referenced Wiki page
> > will need an edit or two after this goes in, but that's easy enough.
>
> One thing I'm wondering: When both --check and --diff are passed,
> should pgindent still early exit with 2 on the first incorrectly
> formatted file? Or should it show diffs for all failing files? I'm
> leaning towards the latter making more sense.

Makes sense. Let me iterate on the squashed version of the patch.

> Related (but not required for this patch): For my pre-commit hook I
> would very much like it if there was an option to have pgindent write
> the changes to disk, but still exit non-zero, e.g. a --write flag that
> could be combined with --check just like --diff and --check can be
> combined with this patch. Currently my pre-commit hook need two
> separate calls to pgindent to both abort the commit and write the
> required changes to disk.

I could propose something. It would help if I had an example of such
a tool that already exists.

--
Tristan Partin
Neon (https://neon.tech)

On Mon, 18 Dec 2023 at 17:50, Tristan Partin <tristan@neon.tech> wrote:
> I could propose something. It would help if I had an example of such
> a tool that already exists.

Basically the same behaviour as what you're trying to add now for
--check, only instead of printing the diff it would actually change
the files just like running pgindent without a --check flag does. i.e.
allow pgindent --check to not run in "dry-run" mode
My pre-commit hook looks like this currently (removed boring cruft around it):

  if ! src/tools/pgindent/pgindent --check $files > /dev/null; then
    exit 0
  fi
  echo "Running pgindent on changed files"
  src/tools/pgindent/pgindent $files
  echo "Commit abandoned. Rerun git commit to adopt pgindent changes"
  exit 1

But I would like it to look like:

  if src/tools/pgindent/pgindent --check --write $files > /dev/null; then
    exit 0
  fi
  echo "Commit abandoned. Rerun git commit to adopt pgindent changes"
  exit 1

On Mon Dec 18, 2023 at 10:50 AM CST, Tristan Partin wrote:
> On Mon Dec 18, 2023 at 10:14 AM CST, Jelte Fennema-Nio wrote:
> > On Mon, 18 Dec 2023 at 13:42, Daniel Gustafsson <daniel@yesql.se> wrote:
> > > I think this is pretty much ready to go, the attached v4 squashes the changes
> > > and fixes the man-page which also needed an update.  The referenced Wiki page
> > > will need an edit or two after this goes in, but that's easy enough.
> >
> > One thing I'm wondering: When both --check and --diff are passed,
> > should pgindent still early exit with 2 on the first incorrectly
> > formatted file? Or should it show diffs for all failing files? I'm
> > leaning towards the latter making more sense.
>
> Makes sense. Let me iterate on the squashed version of the patch.

Here is an additional patch which implements the behavior you described.
The first patch is just Daniel's squashed version of my patches.

--
Tristan Partin
Neon (https://neon.tech)

On Mon Dec 18, 2023 at 11:21 AM CST, Jelte Fennema-Nio wrote:
> On Mon, 18 Dec 2023 at 17:50, Tristan Partin <tristan@neon.tech> wrote:
> > I could propose something. It would help if I had an example of such
> > a tool that already exists.
>
> Basically the same behaviour as what you're trying to add now for
> --check, only instead of printing the diff it would actually change
> the files just like running pgindent without a --check flag does. i.e.
> allow pgindent --check to not run in "dry-run" mode
> My pre-commit hook looks like this currently (removed boring cruft around it):
>
>   if ! src/tools/pgindent/pgindent --check $files > /dev/null; then
>     exit 0
>   fi
>   echo "Running pgindent on changed files"
>   src/tools/pgindent/pgindent $files
>   echo "Commit abandoned. Rerun git commit to adopt pgindent changes"
>   exit 1
>
> But I would like it to look like:
>
>   if src/tools/pgindent/pgindent --check --write $files > /dev/null; then
>     exit 0
>   fi
>   echo "Commit abandoned. Rerun git commit to adopt pgindent changes"
>   exit 1

To me, the two options seem at odds, like you either check or write. How
would you feel about just capturing the diffs that are printed and
patch(1)-ing them?

--
Tristan Partin
Neon (https://neon.tech)

On 2023-12-18 Mo 11:14, Jelte Fennema-Nio wrote:
> On Mon, 18 Dec 2023 at 13:42, Daniel Gustafsson <daniel@yesql.se> wrote:
>> I think this is pretty much ready to go, the attached v4 squashes the changes
>> and fixes the man-page which also needed an update.  The referenced Wiki page
>> will need an edit or two after this goes in, but that's easy enough.
> One thing I'm wondering: When both --check and --diff are passed,
> should pgindent still early exit with 2 on the first incorrectly
> formatted file? Or should it show diffs for all failing files? I'm
> leaning towards the latter making more sense.


It should show them all.


>
> Related (but not required for this patch): For my pre-commit hook I
> would very much like it if there was an option to have pgindent write
> the changes to disk, but still exit non-zero, e.g. a --write flag that
> could be combined with --check just like --diff and --check can be
> combined with this patch. Currently my pre-commit hook need two
> separate calls to pgindent to both abort the commit and write the
> required changes to disk.


Not sold on this. I don't want pgindent applied automatically to my 
uncommitted changes, but I do want a reminder that I forgot to run it. 
In any case, as you say it's a different topic.


cheers


andrew

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

> On 19 Dec 2023, at 14:51, Andrew Dunstan <andrew@dunslane.net> wrote:
>
> On 2023-12-18 Mo 11:14, Jelte Fennema-Nio wrote:
>> On Mon, 18 Dec 2023 at 13:42, Daniel Gustafsson <daniel@yesql.se> wrote:
>>> I think this is pretty much ready to go, the attached v4 squashes the changes
>>> and fixes the man-page which also needed an update.  The referenced Wiki page
>>> will need an edit or two after this goes in, but that's easy enough.
>> One thing I'm wondering: When both --check and --diff are passed,
>> should pgindent still early exit with 2 on the first incorrectly
>> formatted file? Or should it show diffs for all failing files? I'm
>> leaning towards the latter making more sense.
>
> It should show them all.

Agreed.

>> Related (but not required for this patch): For my pre-commit hook I
>> would very much like it if there was an option to have pgindent write
>> the changes to disk, but still exit non-zero, e.g. a --write flag that
>> could be combined with --check just like --diff and --check can be
>> combined with this patch. Currently my pre-commit hook need two
>> separate calls to pgindent to both abort the commit and write the
>> required changes to disk.
>
> Not sold on this. I don't want pgindent applied automatically to my uncommitted changes, but I do want a reminder
thatI forgot to run it. In any case, as you say it's a different topic. 

I think we risk making the tool confusing if we add too many options which can
all be combined to suit every need.  The posted v5 seems like a good compromise
I reckon.

Andrew: When applying this, how do we synchronize with the buildfarm to avoid
false negatives due to the BF using the wrong options?

--
Daniel Gustafsson

On Mon Dec 18, 2023 at 3:18 PM CST, Tristan Partin wrote:
> On Mon Dec 18, 2023 at 10:50 AM CST, Tristan Partin wrote:
> > On Mon Dec 18, 2023 at 10:14 AM CST, Jelte Fennema-Nio wrote:
> > > On Mon, 18 Dec 2023 at 13:42, Daniel Gustafsson <daniel@yesql.se> wrote:
> > > > I think this is pretty much ready to go, the attached v4 squashes the changes
> > > > and fixes the man-page which also needed an update.  The referenced Wiki page
> > > > will need an edit or two after this goes in, but that's easy enough.
> > >
> > > One thing I'm wondering: When both --check and --diff are passed,
> > > should pgindent still early exit with 2 on the first incorrectly
> > > formatted file? Or should it show diffs for all failing files? I'm
> > > leaning towards the latter making more sense.
> >
> > Makes sense. Let me iterate on the squashed version of the patch.
>
> Here is an additional patch which implements the behavior you described.
> The first patch is just Daniel's squashed version of my patches.

Assuming all this is good, I now have access to edit the Wiki. The PR
for buildfarm client code is up, and hopefully that PR is correct.

--
Tristan Partin
Neon (https://neon.tech)

On Mon, 18 Dec 2023 at 22:18, Tristan Partin <tristan@neon.tech> wrote:
> Here is an additional patch which implements the behavior you described.
> The first patch is just Daniel's squashed version of my patches.

I think we'd still want the early exit behaviour when only --check is
provided. No need to spend time checking more files if you're not
doing anything else.

On Mon, 18 Dec 2023 at 22:34, Tristan Partin <tristan@neon.tech> wrote:
> To me, the two options seem at odds, like you either check or write. How
> would you feel about just capturing the diffs that are printed and
> patch(1)-ing them?

I tried capturing the diffs and patching them, but simply piping the
pgindent output to patch(1) didn't work because the pipe loses the
exit code of pgindent. -o pipefail would help with this, but it's not
available in all shells. Also then it's suddenly unclear if pgident
failed or if patch failed.

Attached are two additional patches (in addition to your unchanged
previous ones). One which adds the --write flag and one which early
exits with --check when neither --write or --diff are provided. The
additional code I added for the --write flag is really minimal IMO.

On Tue, 19 Dec 2023 at 14:51, Andrew Dunstan <andrew@dunslane.net> wrote:
> Not sold on this. I don't want pgindent applied automatically to my
> uncommitted changes, but I do want a reminder that I forgot to run it.
> In any case, as you say it's a different topic.

To be clear, with this patch just passing --check won't apply the
changes automatically. Only when passing both --write and --check will
it write the files.

To clarify my commit workflow is as follows:
git add -p # interactively select all the changes that I want in my commit
git commit
# pre-commit hook fails because of pgindent and "fixes" my files in
place but doesn't add them to the commit yet
git add -p
# I look at all the changes that pgindent make to see if they are
sensible and accept them, if not I find some other way to fix them
git commit
# now commit succeeded

On Tue, 19 Dec 2023 at 14:58, Daniel Gustafsson <daniel@yesql.se> wrote:
> I think we risk making the tool confusing if we add too many options which can
> all be combined to suit every need.  The posted v5 seems like a good compromise
> I reckon.

I personally think these options are completely independent. So it's
more confusing to me that they cannot be combined. I updated the help
message in 0003 as well, to describe them as completely independent:
    --diff                  show the changes that need to be made
    --check                 exit with status 2 if any changes need to be made
    --write                 rewrites files that need changes (default
if neither --check/--diff/--write are provided)


PS. prettier (javascript formatter) allows both --check and --write at
the same time to do exactly this
PPS. the help message didn't contain anything about pgindent writing
files by default (i.e. when --check or --diff are not provided)
PPPS. I attached my new pre-commit hook for reference.

On Tue Dec 19, 2023 at 10:36 AM CST, Jelte Fennema-Nio wrote:
> On Mon, 18 Dec 2023 at 22:18, Tristan Partin <tristan@neon.tech> wrote:
> > Here is an additional patch which implements the behavior you described.
> > The first patch is just Daniel's squashed version of my patches.
>
> I think we'd still want the early exit behaviour when only --check is
> provided. No need to spend time checking more files if you're not
> doing anything else.

Good point. Patch looks good.

> On Mon, 18 Dec 2023 at 22:34, Tristan Partin <tristan@neon.tech> wrote:
> > To me, the two options seem at odds, like you either check or write. How
> > would you feel about just capturing the diffs that are printed and
> > patch(1)-ing them?
>
> I tried capturing the diffs and patching them, but simply piping the
> pgindent output to patch(1) didn't work because the pipe loses the
> exit code of pgindent. -o pipefail would help with this, but it's not
> available in all shells. Also then it's suddenly unclear if pgident
> failed or if patch failed.

I was envisioning something along the lines of:

    pgindent --check --diff > patches.txt
    status=$?
    patch <patches.txt # no idea if this works, or if you need a for loop with manual parsing
    exit $status

--
Tristan Partin
Neon (https://neon.tech)

On 2023-12-19 Tu 08:57, Daniel Gustafsson wrote:
>> On 19 Dec 2023, at 14:51, Andrew Dunstan <andrew@dunslane.net> wrote:
>>
>> On 2023-12-18 Mo 11:14, Jelte Fennema-Nio wrote:
>>> On Mon, 18 Dec 2023 at 13:42, Daniel Gustafsson <daniel@yesql.se> wrote:
>>>> I think this is pretty much ready to go, the attached v4 squashes the changes
>>>> and fixes the man-page which also needed an update.  The referenced Wiki page
>>>> will need an edit or two after this goes in, but that's easy enough.
>>> One thing I'm wondering: When both --check and --diff are passed,
>>> should pgindent still early exit with 2 on the first incorrectly
>>> formatted file? Or should it show diffs for all failing files? I'm
>>> leaning towards the latter making more sense.
>> It should show them all.
> Agreed.
>
>>> Related (but not required for this patch): For my pre-commit hook I
>>> would very much like it if there was an option to have pgindent write
>>> the changes to disk, but still exit non-zero, e.g. a --write flag that
>>> could be combined with --check just like --diff and --check can be
>>> combined with this patch. Currently my pre-commit hook need two
>>> separate calls to pgindent to both abort the commit and write the
>>> required changes to disk.
>> Not sold on this. I don't want pgindent applied automatically to my uncommitted changes, but I do want a reminder
thatI forgot to run it. In any case, as you say it's a different topic.
 
> I think we risk making the tool confusing if we add too many options which can
> all be combined to suit every need.  The posted v5 seems like a good compromise
> I reckon.
>
> Andrew: When applying this, how do we synchronize with the buildfarm to avoid
> false negatives due to the BF using the wrong options?


The only buildfarm animal involved here is koel, which I run, so the 
simplest way will be for me to commit the core patch and adjust the 
buildfarm code at the same time.


cheers


andrew

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

On 2023-12-20 We 11:05, Andrew Dunstan wrote:
>
> On 2023-12-19 Tu 08:57, Daniel Gustafsson wrote:
>>   The posted v5 seems like a good compromise
>> I reckon.
>>
>> Andrew: When applying this, how do we synchronize with the buildfarm 
>> to avoid
>> false negatives due to the BF using the wrong options?
>
>
> The only buildfarm animal involved here is koel, which I run, so the 
> simplest way will be for me to commit the core patch and adjust the 
> buildfarm code at the same time.
>
>

V5 seems to be the consensus. I went with that, but added in logic to 
exit the loop early for --check if we're not also doing --diff.


cheers


andrew


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

On Tue, 19 Dec 2023 at 17:54, Tristan Partin <tristan@neon.tech> wrote:
> I was envisioning something along the lines of:
>
>         pgindent --check --diff > patches.txt
>         status=$?
>         patch <patches.txt # no idea if this works, or if you need a for loop with manual parsing
>         exit $status

Okay, I got a working version. And I updated the pre-commit hook on
the wiki accordingly.