Horton

Thread

Commits

  1. Set exit status for pgindent if pg_bsd_indent fails

  1. pgindent exit status if a file encounters an error

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2024-06-26T10:37:31Z

    Hi All,

If pgindent encounters an error while applying pg_bsd_indent on a file, it
reports an error to stderr but does not exit with non-zero status.

$ src/tools/pgindent/pgindent .
Failure in ./src/backend/optimizer/util/relnode.c: Error@2412: Stuff
missing from end of file

$ echo $?
0

A zero status usually indicates success [1]. In that sense pgindent
shouldn't be returning 0 in this case. It has not been able to process
file/s successfully. Not returning non-zero exit status in such cases means
we can not rely on `set -e` or `git rebase` s automatic detection of
command failures. I propose to add non-zero exit status in the above case.

In the attached patch I have used exit code 3 for file processing errors.
The program exits only after reporting all such errors instead of exiting
on the first instance. That way we get to know all the errors upfront. But
I am ok if we want to exit on the first instance. That might simplify its
interaction with other exit codes.

With this change, if I run pgident in `git rebase` it stops after those
errors automatically like below
```
Executing: src/tools/pgindent/pgindent .
Failure in ./src/backend/optimizer/util/relnode.c: Error@2424: Stuff
missing from end of file

Failure in ./src/backend/optimizer/util/appendinfo.c: Error@1028: Stuff
missing from end of file

warning: execution failed: src/tools/pgindent/pgindent .
You can fix the problem, and then run

  git rebase --continue
```

I looked at pgindent README but it doesn't mention anything about exit
codes. So I believe this change is acceptable as per documentation.

With --check option pgindent reports a non-zero exit code instead of making
changes. So I feel the above change should happen at least if --check is
provided. But certainly better if we do it even without --check.

In case --check is specified and both the following happen a. pg_bsd_indent
exits with non-zero status while processing some file and b. changes are
produced while processing some other file, the program will exit with
status 2. It may be argued that instead it should exit with code 3. I am
open to both.

[1] https://en.wikipedia.org/wiki/Exit_status
-- 
Best Wishes,
Ashutosh Bapat

  2. Re: pgindent exit status if a file encounters an error

    Andrew Dunstan <andrew@dunslane.net> — 2024-06-26T11:23:39Z

    On 2024-06-26 We 6:37 AM, Ashutosh Bapat wrote:
> Hi All,
>
> If pgindent encounters an error while applying pg_bsd_indent on a 
> file, it reports an error to stderr but does not exit with non-zero 
> status.
>
> $ src/tools/pgindent/pgindent .
> Failure in ./src/backend/optimizer/util/relnode.c: Error@2412: Stuff 
> missing from end of file
>
> $ echo $?
> 0
>
> A zero status usually indicates success [1]. In that sense pgindent 
> shouldn't be returning 0 in this case. It has not been able to process 
> file/s successfully. Not returning non-zero exit status in such cases 
> means we can not rely on `set -e` or `git rebase` s automatic 
> detection of command failures. I propose to add non-zero exit status 
> in the above case.
>
> In the attached patch I have used exit code 3 for file processing 
> errors. The program exits only after reporting all such errors instead 
> of exiting on the first instance. That way we get to know all the 
> errors upfront. But I am ok if we want to exit on the first instance. 
> That might simplify its interaction with other exit codes.
>
> With this change, if I run pgident in `git rebase` it stops after 
> those errors automatically like below
> ```
> Executing: src/tools/pgindent/pgindent .
> Failure in ./src/backend/optimizer/util/relnode.c: Error@2424: Stuff 
> missing from end of file
>
> Failure in ./src/backend/optimizer/util/appendinfo.c: Error@1028: 
> Stuff missing from end of file
>
> warning: execution failed: src/tools/pgindent/pgindent .
> You can fix the problem, and then run
>
>   git rebase --continue
> ```
>
> I looked at pgindent README but it doesn't mention anything about exit 
> codes. So I believe this change is acceptable as per documentation.
>
> With --check option pgindent reports a non-zero exit code instead of 
> making changes. So I feel the above change should happen at least if 
> --check is provided. But certainly better if we do it even without 
> --check.
>
> In case --check is specified and both the following happen a. 
> pg_bsd_indent exits with non-zero status while processing some file 
> and b. changes are produced while processing some other file, the 
> program will exit with status 2. It may be argued that instead it 
> should exit with code 3. I am open to both.


Yeah, I think this is reasonable but we should adjust the status setting 
a few lines lower to


    $status ||= 2;


cheers


andrew


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

  3. Re: pgindent exit status if a file encounters an error

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2024-06-26T13:28:12Z

    Hi Andrew,
Thanks for the quick review.

On Wed, Jun 26, 2024 at 4:53 PM Andrew Dunstan <andrew@dunslane.net> wrote:

>
> With --check option pgindent reports a non-zero exit code instead of
> making changes. So I feel the above change should happen at least if
> --check is provided. But certainly better if we do it even without --check.
>
> In case --check is specified and both the following happen a.
> pg_bsd_indent exits with non-zero status while processing some file and b.
> changes are produced while processing some other file, the program will
> exit with status 2. It may be argued that instead it should exit with code
> 3. I am open to both.
>
>
> Yeah, I think this is reasonable but we should adjust the status setting a
> few lines lower to
>
>
>    $status ||= 2;
>

So you are suggesting that status 3 is preferred over status 2 when both
are applicable. I am fine with that.

Here's what the behaviour looks like: (notice echo $? after running
pgindent)

1. Running without --check, if pgindent encounters file processing errors,
exit code is 3.
2. Running with --check, exit code depends upon whether pgindent encounters
a file with processing error first or a file that undergoes a change.
   a. If it encounters a file that would undergo a change first, exit
status is 2
   b. If it encounters a file with processing error first, exit status is 3
3. If --check is specified and no file undergoes a change, but there are
file processing errors, it will exit with code 3.

The variation in the second case based on the order of files processed
looks fine to me. What do you say?

The usage help mentions exit code 2 specifically while describing --check
option but it doesn't mention exit code 1. Neither does the README. So I
don't think we need to document exit code 3 anywhere. Please let me know if
you think otherwise.

-- 
Best Wishes,
Ashutosh Bapat

  4. Re: pgindent exit status if a file encounters an error

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-06-26T15:24:38Z

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> writes:
> The usage help mentions exit code 2 specifically while describing --check
> option but it doesn't mention exit code 1. Neither does the README. So I
> don't think we need to document exit code 3 anywhere. Please let me know if
> you think otherwise.

I think we should have at least a code comment summarizing the
possible exit codes, along the lines of

# Exit codes:
#   0 -- all OK
#   1 -- could not invoke pgindent, nothing done
#   2 -- --check mode and at least one file requires changes
#   3 -- pgindent failed on at least one file

			regards, tom lane

  5. Re: pgindent exit status if a file encounters an error

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2024-06-28T12:35:35Z

    On Wed, Jun 26, 2024 at 8:54 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> writes:
> > The usage help mentions exit code 2 specifically while describing --check
> > option but it doesn't mention exit code 1. Neither does the README. So I
> > don't think we need to document exit code 3 anywhere. Please let me know
> if
> > you think otherwise.
>
> I think we should have at least a code comment summarizing the
> possible exit codes, along the lines of
>
> # Exit codes:
> #   0 -- all OK
> #   1 -- could not invoke pgindent, nothing done
> #   2 -- --check mode and at least one file requires changes
> #   3 -- pgindent failed on at least one file
>

Thanks. Here's a patch with these lines.

In an offline chat Andrew mentioned that he expects more changes in the
patch and he would take care of those. Will review and test his patch.

-- 
Best Wishes,
Ashutosh Bapat

  6. Re: pgindent exit status if a file encounters an error

    Bruce Momjian <bruce@momjian.us> — 2024-10-16T23:26:10Z

    Uh, where are we on this?

---------------------------------------------------------------------------

On Fri, Jun 28, 2024 at 06:05:35PM +0530, Ashutosh Bapat wrote:
> 
> 
> On Wed, Jun 26, 2024 at 8:54 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
>     Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> writes:
>     > The usage help mentions exit code 2 specifically while describing --check
>     > option but it doesn't mention exit code 1. Neither does the README. So I
>     > don't think we need to document exit code 3 anywhere. Please let me know
>     if
>     > you think otherwise.
> 
>     I think we should have at least a code comment summarizing the
>     possible exit codes, along the lines of
> 
>     # Exit codes:
>     #   0 -- all OK
>     #   1 -- could not invoke pgindent, nothing done
>     #   2 -- --check mode and at least one file requires changes
>     #   3 -- pgindent failed on at least one file
> 
> 
> Thanks. Here's a patch with these lines.
>  
> In an offline chat Andrew mentioned that he expects more changes in the patch
> and he would take care of those. Will review and test his patch.
> 
> --
> Best Wishes,
> Ashutosh Bapat

> From 2a61830f0a2b6559d04bef75c6aa224e30ed1609 Mon Sep 17 00:00:00 2001
> From: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>
> Date: Wed, 26 Jun 2024 15:46:53 +0530
> Subject: [PATCH 1/2] pgindent exit status on error
> 
> When pg_bsd_indent exits with a non-zero status or reports an error,
> make pgindent exit with non-zero status 3. The program does not exit on
> the first instance of the error. Instead it continues to process
> remaining files as long as some other exit condition is encountered, in
> which case exit code 3 is reported.
> 
> This helps to detect errors automatically when pgindent is run in shells
> which interpret non-zero exit status as failure.
> 
> Author: Ashutosh Bapat
> Reviewed by: Andrew Dunstan
> Discussion: https://www.postgresql.org/message-id/CAExHW5sPRSiFeLdP-u1Fa5ba7YS2f0gvLjmKOobopKadJwQ_GQ@mail.gmail.com
> ---
>  src/tools/pgindent/pgindent | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent
> index 48d83bc434..03b319c9c3 100755
> --- a/src/tools/pgindent/pgindent
> +++ b/src/tools/pgindent/pgindent
> @@ -1,5 +1,12 @@
>  #!/usr/bin/perl
>  
> +# Program to maintain uniform layout style in our C and Perl code.
> +# Exit codes:
> +#   0 -- all OK
> +#   1 -- could not invoke pgindent, nothing done
> +#   2 -- --check mode and at least one file requires changes
> +#   3 -- pgindent failed on at least one file
> +
>  # Copyright (c) 2021-2024, PostgreSQL Global Development Group
>  
>  use strict;
> @@ -409,6 +416,7 @@ foreach my $source_filename (@files)
>  	if ($source eq "")
>  	{
>  		print STDERR "Failure in $source_filename: " . $error_message . "\n";
> +		$status = 3;
>  		next;
>  	}
>  
> @@ -429,7 +437,7 @@ foreach my $source_filename (@files)
>  
>  			if ($check)
>  			{
> -				$status = 2;
> +				$status ||= 2;
>  				last unless $diff;
>  			}
>  		}
> -- 
> 2.34.1
> 


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

  When a patient asks the doctor, "Am I going to die?", he means 
  "Am I going to die soon?"

  7. Re: pgindent exit status if a file encounters an error

    Andrew Dunstan <andrew@dunslane.net> — 2024-11-26T22:57:20Z

    On 2024-10-16 We 7:26 PM, Bruce Momjian wrote:
> Uh, where are we on this?


It slipped under my radar - I have put it back :-)


cheers


andrew

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

  8. Re: pgindent exit status if a file encounters an error

    Andrew Dunstan <andrew@dunslane.net> — 2025-01-08T16:05:18Z

    On 2024-06-28 Fr 8:35 AM, Ashutosh Bapat wrote:
>
>
> On Wed, Jun 26, 2024 at 8:54 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
>     Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> writes:
>     > The usage help mentions exit code 2 specifically while
>     describing --check
>     > option but it doesn't mention exit code 1. Neither does the
>     README. So I
>     > don't think we need to document exit code 3 anywhere. Please let
>     me know if
>     > you think otherwise.
>
>     I think we should have at least a code comment summarizing the
>     possible exit codes, along the lines of
>
>     # Exit codes:
>     #   0 -- all OK
>     #   1 -- could not invoke pgindent, nothing done
>     #   2 -- --check mode and at least one file requires changes
>     #   3 -- pgindent failed on at least one file
>
>
> Thanks. Here's a patch with these lines.
> In an offline chat Andrew mentioned that he expects more changes in 
> the patch and he would take care of those. Will review and test his patch.
>
>

I forget now what I was intending here, so I have committed this with 
minor tweaks.


cheers


andrew

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

  9. Re: pgindent exit status if a file encounters an error

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2025-01-09T03:38:44Z

    On Wed, Jan 8, 2025 at 9:35 PM Andrew Dunstan <andrew@dunslane.net> wrote:
>
>
>
> I forget now what I was intending here, so I have committed this with minor tweaks.
>
>
Thanks Andrew for taking care of this.

-- 
Best Wishes,
Ashutosh Bapat