Patch to make pgindent work cleanly

Started by Gurjeet Singhalmost 13 years ago5 messages

gurjeet@singh.im

almost 13 years ago

1 attachment(s)

Please find attached the patch for some cleanup and fix bit rot in pgindent
script.

There were a few problems with the script.

.) It failed to use the $ENV{PGENTAB} even if it was set.
.) The file it tries to download from Postgres' ftp site is no longer
present.
ftp://ftp.postgresql.org/pub/dev/indent.netbsd.patched.tgz
.) The tar command extracted the above-mentioned file to a child directory,
but did not descend into it before running make on it.
I think it expected a tarbomb, but clearly the current .tgz file on ftp
site is not a tarbomb.

I don't like the fact that it dies with a message "fetching xyz" rather
than saying "Could not fetch xyz", but this patch does not address that
since it doesn't really affect the output when script does work.

With this patch in place I could very easily run it on any arbitrary file,
which seemed like a black-magic before the patch.

src/tools/pgindent/pgindent --build <your file path here>

--
Gurjeet Singh

http://gurjeet.singh.im/

EnterprsieDB Inc.

Attachments:

pgindent.patchapplication/octet-stream; name=pgindent.patchDownload

diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent
index ce72883..4a2a9ee 100755
--- a/src/tools/pgindent/pgindent
+++ b/src/tools/pgindent/pgindent
@@ -59,7 +59,7 @@ my $filtered_typedefs_fh;
 
 sub check_indent
 {
-	system("entab < $devnull");
+	system("$entab < $devnull");
 	if ($?)
 	{
 		print STDERR
@@ -448,29 +448,28 @@ sub run_build
 
 	$ENV{PGTYPEDEFS} = abs_path('tmp_typedefs.list');
 
+	my $pg_bsd_indent_name = "pg_bsd_indent-1.2.tar.gz";
+
 	$rv =
-	  getstore("ftp://ftp.postgresql.org/pub/dev/indent.netbsd.patched.tgz",
-		"indent.netbsd.patched.tgz");
+	  getstore("ftp://ftp.postgresql.org/pub/dev/$pg_bsd_indent_name",
+		"pg_bsd_indent.tgz");
 
-	die "fetching indent.netbsd.patched.tgz" unless is_success($rv);
+	die "fetching $pg_bsd_indent_name" unless is_success($rv);
 
 	# XXX add error checking here
 
-	mkdir "bsdindent";
-	chdir "bsdindent";
-	system("tar -z -xf ../indent.netbsd.patched.tgz");
+	system("tar -z -xf pg_bsd_indent.tgz");
+	chdir "pg_bsd_indent";
 	system("make > $devnull 2>&1");
 
-	$ENV{PGINDENT} = abs_path('indent');
+	$ENV{PGINDENT} = abs_path('pg_bsd_indent');
 
 	chdir "../../entab";
-
 	system("make > $devnull 2>&1");
 
 	$ENV{PGENTAB} = abs_path('entab');
 
 	chdir $save_dir;
-
 }

Bruce Momjian

bruce@momjian.us

almost 13 years ago

In reply to: Gurjeet Singh (#1)

Re: Patch to make pgindent work cleanly

On Tue, Feb 19, 2013 at 04:50:45PM -0500, Gurjeet Singh wrote:

Please find attached the patch for some cleanup and fix bit rot in pgindent
script.

There were a few problems with the script.

.) It failed to use the $ENV{PGENTAB} even if it was set.
.) The file it tries to download from Postgres' ftp site is no longer present.
ftp://ftp.postgresql.org/pub/dev/indent.netbsd.patched.tgz
.) The tar command extracted the above-mentioned file to a child directory, but
did not descend into it before running make on it.
I think it expected a tarbomb, but clearly the current .tgz file on ftp
site is not a tarbomb.

I don't like the fact that it dies with a message "fetching xyz" rather than
saying "Could not fetch xyz", but this patch does not address that since it
doesn't really affect the output when script does work.

With this patch in place I could very easily run it on any arbitrary file,
which seemed like a black-magic before the patch.

src/tools/pgindent/pgindent --build <your file path here>

I have applied this patch. However, I modified the tarball name to
reference $INDENT_VERSION, rather than hard-coding "1.2".

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Gurjeet Singh

gurjeet@singh.im

almost 13 years ago

In reply to: Bruce Momjian (#2)

Re: Patch to make pgindent work cleanly

On Fri, Apr 12, 2013 at 11:44 AM, Bruce Momjian <bruce@momjian.us> wrote:

On Tue, Feb 19, 2013 at 04:50:45PM -0500, Gurjeet Singh wrote:

Please find attached the patch for some cleanup and fix bit rot in

pgindent

script.

There were a few problems with the script.

.) It failed to use the $ENV{PGENTAB} even if it was set.
.) The file it tries to download from Postgres' ftp site is no longer

present.

ftp://ftp.postgresql.org/pub/dev/indent.netbsd.patched.tgz
.) The tar command extracted the above-mentioned file to a child

directory, but

did not descend into it before running make on it.
I think it expected a tarbomb, but clearly the current .tgz file on

ftp

site is not a tarbomb.

I don't like the fact that it dies with a message "fetching xyz" rather

than

saying "Could not fetch xyz", but this patch does not address that since

it

doesn't really affect the output when script does work.

With this patch in place I could very easily run it on any arbitrary

file,

which seemed like a black-magic before the patch.

src/tools/pgindent/pgindent --build <your file path here>

I have applied this patch. However, I modified the tarball name to
reference $INDENT_VERSION, rather than hard-coding "1.2".

Thanks!

Can you also improve the output when it dies upon failure to fetch
something? Currently the only error message it emits is "fetching xyz", and
leaves the user confused as to what really the problem was. The only
indication of a problem might be the exit code, but I'm not sure of that,
and that doesn't help the interactive user running it on terminal.

--
Gurjeet Singh

http://gurjeet.singh.im/

EnterpriseDB Inc.

Bruce Momjian

bruce@momjian.us

almost 13 years ago

In reply to: Gurjeet Singh (#3)

1 attachment(s)

Re: Patch to make pgindent work cleanly

On Fri, Apr 12, 2013 at 01:34:49PM -0400, Gurjeet Singh wrote:

Can you also improve the output when it dies upon failure to fetch something?
Currently the only error message it emits is "fetching xyz", and leaves the
user confused as to what really the problem was. The only indication of a
problem might be the exit code, but I'm not sure of that, and that doesn't
help the interactive user running it on terminal.

Good point. I have reviewed all the error messages and improved them
with the attached, applied patch, e.g.:

cannot locate typedefs file "xyz" at /usr/local/bin/pgindent line 121.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Attachments:

pgindent.difftext/x-diff; charset=us-asciiDownload

diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent
index 2e9d443..73237ca 100755
--- a/src/tools/pgindent/pgindent
+++ b/src/tools/pgindent/pgindent
@@ -30,7 +30,7 @@ my %options = (
 	"excludes=s"  => \$excludes,
 	"indent=s"    => \$indent,
 	"build"       => \$build,);
-GetOptions(%options) || die "bad command line";
+GetOptions(%options) || die "bad command line argument";
 
 run_build($code_base) if ($build);
 
@@ -118,10 +118,11 @@ sub load_typedefs
 		  if (-f "$tdtry/src/tools/pgindent/typedefs.list");
 		$tdtry = "$tdtry/..";
 	}
-	die "no typedefs file" unless $typedefs_file && -f $typedefs_file;
+	die "cannot locate typedefs file \"$typedefs_file\""
+		unless $typedefs_file && -f $typedefs_file;
 
 	open(my $typedefs_fh, '<', $typedefs_file)
-	  || die "opening $typedefs_file: $!";
+	  || die "cannot open typedefs file \"$typedefs_file\": $!";
 	my @typedefs = <$typedefs_fh>;
 	close($typedefs_fh);
 
@@ -143,7 +144,7 @@ sub process_exclude
 {
 	if ($excludes && @files)
 	{
-		open(my $eh, '<', $excludes) || die "opening $excludes";
+		open(my $eh, '<', $excludes) || die "cannot open exclude file \"$excludes\"";
 		while (my $line = <$eh>)
 		{
 			chomp $line;
@@ -162,7 +163,7 @@ sub read_source
 	my $source;
 
 	open(my $src_fd, '<', $source_filename)
-	  || die "opening $source_filename: $!";
+	  || die "cannot open file \"$source_filename\": $!";
 	local ($/) = undef;
 	$source = <$src_fd>;
 	close($src_fd);
@@ -177,7 +178,7 @@ sub write_source
 	my $source_filename = shift;
 
 	open(my $src_fh, '>', $source_filename)
-	  || die "opening $source_filename: $!";
+	  || die "cannot open file \"$source_filename\": $!";
 	print $src_fh $source;
 	close($src_fh);
 }
@@ -436,25 +437,25 @@ sub run_build
 		$code_base = "$code_base/..";
 	}
 
-	die "no src/tools/pgindent directory in $code_base"
+	die "cannot locate src/tools/pgindent directory in \"$code_base\""
 	  unless -d "$code_base/src/tools/pgindent";
 
 	chdir "$code_base/src/tools/pgindent";
 
-	my $rv = getstore("http://buildfarm.postgresql.org/cgi-bin/typedefs.pl",
-		"tmp_typedefs.list");
+	my $typedefs_list_url = "http://buildfarm.postgresql.org/cgi-bin/typedefs.pl";
 
-	die "fetching typedefs.list" unless is_success($rv);
+	my $rv = getstore($typedefs_list_url, "tmp_typedefs.list");
+
+	die "cannot fetch typedefs list from $typedefs_list_url" unless is_success($rv);
 
 	$ENV{PGTYPEDEFS} = abs_path('tmp_typedefs.list');
 
-	my $pg_bsd_indent_name = "pg_bsd_indent-" . $INDENT_VERSION . ".tar.gz";
+	my $pg_bsd_indent_url = "ftp://ftp.postgresql.org/pub/dev/pg_bsd_indent-" . 
+				 $INDENT_VERSION . ".tar.gz";
 
-	$rv =
-	  getstore("ftp://ftp.postgresql.org/pub/dev/$pg_bsd_indent_name",
-		"pg_bsd_indent.tgz");
+	$rv = getstore($pg_bsd_indent_url, "pg_bsd_indent.tgz");
 
-	die "fetching $pg_bsd_indent_name" unless is_success($rv);
+	die "cannot fetch BSD indent tarfile from $pg_bsd_indent_url" unless is_success($rv);
 
 	# XXX add error checking here
 
@@ -484,7 +485,7 @@ sub build_clean
 		$code_base = "$code_base/..";
 	}
 
-	die "no src/tools/pgindent directory in $code_base"
+	die "cannot locate src/tools/pgindent directory in \"$code_base\""
 	  unless -d "$code_base/src/tools/pgindent";
 
 	chdir "$code_base";

Gurjeet Singh

gurjeet@singh.im

almost 13 years ago

In reply to: Bruce Momjian (#4)

Re: Patch to make pgindent work cleanly

On Fri, Apr 12, 2013 at 3:26 PM, Bruce Momjian <bruce@momjian.us> wrote:

On Fri, Apr 12, 2013 at 01:34:49PM -0400, Gurjeet Singh wrote:

Can you also improve the output when it dies upon failure to fetch

something?

Currently the only error message it emits is "fetching xyz", and leaves

the

user confused as to what really the problem was. The only indication of a
problem might be the exit code, but I'm not sure of that, and that

doesn't

help the interactive user running it on terminal.

Good point. I have reviewed all the error messages and improved them
with the attached, applied patch, e.g.:

cannot locate typedefs file "xyz" at /usr/local/bin/pgindent line
121.

Thanks!

--
Gurjeet Singh

http://gurjeet.singh.im/

EnterpriseDB Inc.