Make --help output fit within 80 columns per line
Hi,
As discussed in [1]/messages/by-id/3fe4af5a0a81fc6a2ec01cb484c0a487@oss.nttdata.com, outputs of --help for some commands fits into 80
columns
per line, while others do not.
Since it seems preferable to have consistent line break policy and some
people
use 80-column terminal, wouldn't it be better to make all commands in 80
columns per line?
Attached patch which does this for src/bin commands.
If this is the way to go, I'll do same things for contrib commands.
[1]: /messages/by-id/3fe4af5a0a81fc6a2ec01cb484c0a487@oss.nttdata.com
/messages/by-id/3fe4af5a0a81fc6a2ec01cb484c0a487@oss.nttdata.com
--
Regards,
--
Atsushi Torikoshi
NTT DATA CORPORATION
Attachments:
v1-0001-Make-help-output-fit-within-80-columns-per-line.patchtext/x-diff; name=v1-0001-Make-help-output-fit-within-80-columns-per-line.patchDownload
From 4505bbcf0efe199e34159696d64bd9f8cc60fb37 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <torikoshia@oss.nttdata.com>
Date: Wed, 5 Jul 2023 10:14:58 +0900
Subject: [PATCH v1] Make --help output fit within 80 columns per line
Outputs of --help for some commands fits into 80 columns per line,
while others do not.
For the consistency and for 80-column terminal, this patch makes them
fit within 80 columns per line.
---
src/bin/initdb/initdb.c | 6 ++++--
src/bin/pg_amcheck/pg_amcheck.c | 21 ++++++++++++-------
src/bin/pg_archivecleanup/pg_archivecleanup.c | 2 +-
src/bin/pg_basebackup/pg_receivewal.c | 12 +++++++----
src/bin/pg_basebackup/pg_recvlogical.c | 18 ++++++++++------
src/bin/pg_checksums/pg_checksums.c | 3 ++-
src/bin/pg_dump/pg_dump.c | 3 ++-
src/bin/pg_dump/pg_dumpall.c | 3 ++-
src/bin/pg_dump/pg_restore.c | 6 ++++--
src/bin/pg_upgrade/option.c | 9 +++++---
src/bin/pgbench/pgbench.c | 6 ++++--
src/bin/psql/help.c | 9 +++++---
src/bin/scripts/createdb.c | 3 ++-
src/bin/scripts/pg_isready.c | 3 ++-
src/bin/scripts/vacuumdb.c | 21 ++++++++++++-------
15 files changed, 83 insertions(+), 42 deletions(-)
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index fc1fb363e7..ccdd093e24 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -2429,8 +2429,10 @@ usage(const char *progname)
printf(_(" %s [OPTION]... [DATADIR]\n"), progname);
printf(_("\nOptions:\n"));
printf(_(" -A, --auth=METHOD default authentication method for local connections\n"));
- printf(_(" --auth-host=METHOD default authentication method for local TCP/IP connections\n"));
- printf(_(" --auth-local=METHOD default authentication method for local-socket connections\n"));
+ printf(_(" --auth-host=METHOD default authentication method for local TCP/IP\n"
+ " connections\n"));
+ printf(_(" --auth-local=METHOD default authentication method for local-socket\n"
+ " connections\n"));
printf(_(" [-D, --pgdata=]DATADIR location for this database cluster\n"));
printf(_(" -E, --encoding=ENCODING set default encoding for new databases\n"));
printf(_(" -g, --allow-group-access allow group read/execute on data directory\n"));
diff --git a/src/bin/pg_amcheck/pg_amcheck.c b/src/bin/pg_amcheck/pg_amcheck.c
index 68f8180c19..754a2723ce 100644
--- a/src/bin/pg_amcheck/pg_amcheck.c
+++ b/src/bin/pg_amcheck/pg_amcheck.c
@@ -1150,17 +1150,23 @@ help(const char *progname)
printf(_(" -S, --exclude-schema=PATTERN do NOT check matching schema(s)\n"));
printf(_(" -t, --table=PATTERN check matching table(s)\n"));
printf(_(" -T, --exclude-table=PATTERN do NOT check matching table(s)\n"));
- printf(_(" --no-dependent-indexes do NOT expand list of relations to include indexes\n"));
- printf(_(" --no-dependent-toast do NOT expand list of relations to include TOAST tables\n"));
+ printf(_(" --no-dependent-indexes do NOT expand list of relations to include\n"
+ " indexes\n"));
+ printf(_(" --no-dependent-toast do NOT expand list of relations to include\n"
+ " TOAST tables\n"));
printf(_(" --no-strict-names do NOT require patterns to match objects\n"));
printf(_("\nTable checking options:\n"));
printf(_(" --exclude-toast-pointers do NOT follow relation TOAST pointers\n"));
printf(_(" --on-error-stop stop checking at end of first corrupt page\n"));
- printf(_(" --skip=OPTION do NOT check \"all-frozen\" or \"all-visible\" blocks\n"));
- printf(_(" --startblock=BLOCK begin checking table(s) at the given block number\n"));
- printf(_(" --endblock=BLOCK check table(s) only up to the given block number\n"));
+ printf(_(" --skip=OPTION do NOT check \"all-frozen\" or \"all-visible\"\n"
+ " blocks\n"));
+ printf(_(" --startblock=BLOCK begin checking table(s) at the given block\n"
+ " number\n"));
+ printf(_(" --endblock=BLOCK check table(s) only up to the given block\n"
+ " number\n"));
printf(_("\nB-tree index checking options:\n"));
- printf(_(" --heapallindexed check that all heap tuples are found within indexes\n"));
+ printf(_(" --heapallindexed check that all heap tuples are found within\n"
+ " indexes\n"));
printf(_(" --parent-check check index parent/child relationships\n"));
printf(_(" --rootdescend search from root page to refind tuples\n"));
printf(_("\nConnection options:\n"));
@@ -1172,7 +1178,8 @@ help(const char *progname)
printf(_(" --maintenance-db=DBNAME alternate maintenance database\n"));
printf(_("\nOther options:\n"));
printf(_(" -e, --echo show the commands being sent to the server\n"));
- printf(_(" -j, --jobs=NUM use this many concurrent connections to the server\n"));
+ printf(_(" -j, --jobs=NUM use this many concurrent connections to the\n"
+ " server\n"));
printf(_(" -P, --progress show progress information\n"));
printf(_(" -v, --verbose write a lot of output\n"));
printf(_(" -V, --version output version information, then exit\n"));
diff --git a/src/bin/pg_archivecleanup/pg_archivecleanup.c b/src/bin/pg_archivecleanup/pg_archivecleanup.c
index fc0dca9856..52b29e5ca7 100644
--- a/src/bin/pg_archivecleanup/pg_archivecleanup.c
+++ b/src/bin/pg_archivecleanup/pg_archivecleanup.c
@@ -267,7 +267,7 @@ usage(void)
printf(_("\n"
"Or for use as a standalone archive cleaner:\n"
"e.g.\n"
- " pg_archivecleanup /mnt/server/archiverdir 000000010000000000000010.00000020.backup\n"));
+ " pg_archivecleanup archiverdir 000000010000000000000010.00000020.backup\n"));
printf(_("\nReport bugs to <%s>.\n"), PACKAGE_BUGREPORT);
printf(_("%s home page: <%s>\n"), PACKAGE_NAME, PACKAGE_URL);
}
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index d0a4079d50..39a40d525e 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -81,11 +81,13 @@ usage(void)
printf(_("\nOptions:\n"));
printf(_(" -D, --directory=DIR receive write-ahead log files into this directory\n"));
printf(_(" -E, --endpos=LSN exit after receiving the specified LSN\n"));
- printf(_(" --if-not-exists do not error if slot already exists when creating a slot\n"));
+ printf(_(" --if-not-exists do not error if slot already exists when creating a\n"
+ " slot\n"));
printf(_(" -n, --no-loop do not loop on connection lost\n"));
printf(_(" --no-sync do not wait for changes to be written safely to disk\n"));
printf(_(" -s, --status-interval=SECS\n"
- " time between status packets sent to server (default: %d)\n"), (standby_message_timeout / 1000));
+ " time between status packets sent to server\n"
+ " (default: %d)\n"), (standby_message_timeout / 1000));
printf(_(" -S, --slot=SLOTNAME replication slot to use\n"));
printf(_(" --synchronous flush write-ahead log immediately after writing\n"));
printf(_(" -v, --verbose output verbose messages\n"));
@@ -101,8 +103,10 @@ usage(void)
printf(_(" -w, --no-password never prompt for password\n"));
printf(_(" -W, --password force password prompt (should happen automatically)\n"));
printf(_("\nOptional actions:\n"));
- printf(_(" --create-slot create a new replication slot (for the slot's name see --slot)\n"));
- printf(_(" --drop-slot drop the replication slot (for the slot's name see --slot)\n"));
+ printf(_(" --create-slot create a new replication slot (for the slot's name see\n"
+ " --slot)\n"));
+ printf(_(" --drop-slot drop the replication slot (for the slot's name see\n"
+ " --slot)\n"));
printf(_("\nReport bugs to <%s>.\n"), PACKAGE_BUGREPORT);
printf(_("%s home page: <%s>\n"), PACKAGE_NAME, PACKAGE_URL);
}
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index f3c7937a1d..33e1110da6 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -76,15 +76,19 @@ usage(void)
printf(_("Usage:\n"));
printf(_(" %s [OPTION]...\n"), progname);
printf(_("\nAction to be performed:\n"));
- printf(_(" --create-slot create a new replication slot (for the slot's name see --slot)\n"));
- printf(_(" --drop-slot drop the replication slot (for the slot's name see --slot)\n"));
- printf(_(" --start start streaming in a replication slot (for the slot's name see --slot)\n"));
+ printf(_(" --create-slot create a new replication slot (for the slot's name see\n"
+ " --slot)\n"));
+ printf(_(" --drop-slot drop the replication slot (for the slot's name see\n"
+ " --slot)\n"));
+ printf(_(" --start start streaming in a replication slot (for the slot's\n"
+ " name see --slot)\n"));
printf(_("\nOptions:\n"));
printf(_(" -E, --endpos=LSN exit after receiving the specified LSN\n"));
printf(_(" -f, --file=FILE receive log into this file, - for stdout\n"));
printf(_(" -F --fsync-interval=SECS\n"
" time between fsyncs to the output file (default: %d)\n"), (fsync_interval / 1000));
- printf(_(" --if-not-exists do not error if slot already exists when creating a slot\n"));
+ printf(_(" --if-not-exists do not error if slot already exists when creating a\n"
+ " slot\n"));
printf(_(" -I, --startpos=LSN where in an existing slot should the streaming start\n"));
printf(_(" -n, --no-loop do not loop on connection lost\n"));
printf(_(" -o, --option=NAME[=VALUE]\n"
@@ -92,9 +96,11 @@ usage(void)
" output plugin\n"));
printf(_(" -P, --plugin=PLUGIN use output plugin PLUGIN (default: %s)\n"), plugin);
printf(_(" -s, --status-interval=SECS\n"
- " time between status packets sent to server (default: %d)\n"), (standby_message_timeout / 1000));
+ " time between status packets sent to server\n"
+ " (default: %d)\n"), (standby_message_timeout / 1000));
printf(_(" -S, --slot=SLOTNAME name of the logical replication slot\n"));
- printf(_(" -t, --two-phase enable decoding of prepared transactions when creating a slot\n"));
+ printf(_(" -t, --two-phase enable decoding of prepared transactions when creating\n"
+ " a slot\n"));
printf(_(" -v, --verbose output verbose messages\n"));
printf(_(" -V, --version output version information, then exit\n"));
printf(_(" -?, --help show this help, then exit\n"));
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 19eb67e485..2b42ab7e0a 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -76,7 +76,8 @@ static pg_time_t last_progress_report = 0;
static void
usage(void)
{
- printf(_("%s enables, disables, or verifies data checksums in a PostgreSQL database cluster.\n\n"), progname);
+ printf(_("%s enables, disables, or verifies data checksums in a PostgreSQL\n"
+ "database cluster.\n\n"), progname);
printf(_("Usage:\n"));
printf(_(" %s [OPTION]... [DATADIR]\n"), progname);
printf(_("\nOptions:\n"));
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 5dab1ba9ea..44156ee85e 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -1067,7 +1067,8 @@ help(const char *progname)
printf(_(" -Z, --compress=METHOD[:DETAIL]\n"
" compress as specified\n"));
printf(_(" --lock-wait-timeout=TIMEOUT fail after waiting TIMEOUT for a table lock\n"));
- printf(_(" --no-sync do not wait for changes to be written safely to disk\n"));
+ printf(_(" --no-sync do not wait for changes to be written safely to\n"
+ " disk\n"));
printf(_(" -?, --help show this help, then exit\n"));
printf(_("\nOptions controlling the output content:\n"));
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index 3627b69e2a..76653d6ec8 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -661,7 +661,8 @@ help(void)
printf(_(" --no-role-passwords do not dump passwords for roles\n"));
printf(_(" --no-security-labels do not dump security label assignments\n"));
printf(_(" --no-subscriptions do not dump subscriptions\n"));
- printf(_(" --no-sync do not wait for changes to be written safely to disk\n"));
+ printf(_(" --no-sync do not wait for changes to be written safely to\n"
+ " disk\n"));
printf(_(" --no-table-access-method do not dump table access methods\n"));
printf(_(" --no-tablespaces do not dump tablespace assignments\n"));
printf(_(" --no-toast-compression do not dump TOAST compression methods\n"));
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index 049a100634..fbbb9f93f8 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -459,7 +459,8 @@ usage(const char *progname)
printf(_(" -S, --superuser=NAME superuser user name to use for disabling triggers\n"));
printf(_(" -t, --table=NAME restore named relation (table, view, etc.)\n"));
printf(_(" -T, --trigger=NAME restore named trigger\n"));
- printf(_(" -x, --no-privileges skip restoration of access privileges (grant/revoke)\n"));
+ printf(_(" -x, --no-privileges skip restoration of access privileges\n"
+ " (grant/revoke)\n"));
printf(_(" -1, --single-transaction restore as a single transaction\n"));
printf(_(" --disable-triggers disable triggers during data-only restore\n"));
printf(_(" --enable-row-security enable row security\n"));
@@ -472,7 +473,8 @@ usage(const char *progname)
printf(_(" --no-subscriptions do not restore subscriptions\n"));
printf(_(" --no-table-access-method do not restore table access methods\n"));
printf(_(" --no-tablespaces do not restore tablespace assignments\n"));
- printf(_(" --section=SECTION restore named section (pre-data, data, or post-data)\n"));
+ printf(_(" --section=SECTION restore named section (pre-data, data, or\n"
+ " post-data)\n"));
printf(_(" --strict-names require table and/or schema include patterns to\n"
" match at least one entity each\n"));
printf(_(" --use-set-session-authorization\n"
diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c
index 640361009e..58c86309a1 100644
--- a/src/bin/pg_upgrade/option.c
+++ b/src/bin/pg_upgrade/option.c
@@ -275,9 +275,11 @@ usage(void)
printf(_(" -c, --check check clusters only, don't change any data\n"));
printf(_(" -d, --old-datadir=DATADIR old cluster data directory\n"));
printf(_(" -D, --new-datadir=DATADIR new cluster data directory\n"));
- printf(_(" -j, --jobs=NUM number of simultaneous processes or threads to use\n"));
+ printf(_(" -j, --jobs=NUM number of simultaneous processes or threads to\n"
+ " use\n"));
printf(_(" -k, --link link instead of copying files to new cluster\n"));
- printf(_(" -N, --no-sync do not wait for changes to be written safely to disk\n"));
+ printf(_(" -N, --no-sync do not wait for changes to be written safely to\n"
+ " disk\n"));
printf(_(" -o, --old-options=OPTIONS old cluster options to pass to the server\n"));
printf(_(" -O, --new-options=OPTIONS new cluster options to pass to the server\n"));
printf(_(" -p, --old-port=PORT old cluster port number (default %d)\n"), old_cluster.port);
@@ -303,7 +305,8 @@ usage(void)
" the \"bin\" directory for the new version (-B BINDIR)\n"));
printf(_("\n"
"For example:\n"
- " pg_upgrade -d oldCluster/data -D newCluster/data -b oldCluster/bin -B newCluster/bin\n"
+ " pg_upgrade -d oldCluster/data -D newCluster/data\n"
+ " -b oldCluster/bin -B newCluster/bin\n"
"or\n"));
#ifndef WIN32
printf(_(" $ export PGDATAOLD=oldCluster/data\n"
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 1d1670d4c2..611ae77967 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -888,8 +888,10 @@ usage(void)
" --index-tablespace=TABLESPACE\n"
" create indexes in the specified tablespace\n"
" --partition-method=(range|hash)\n"
- " partition pgbench_accounts with this method (default: range)\n"
- " --partitions=NUM partition pgbench_accounts into NUM parts (default: 0)\n"
+ " partition pgbench_accounts with this method\n"
+ " (default: range)\n"
+ " --partitions=NUM partition pgbench_accounts into NUM parts\n"
+ " (default: 0)\n"
" --tablespace=TABLESPACE create tables in the specified tablespace\n"
" --unlogged-tables create tables as unlogged tables\n"
"\nOptions to select what to run:\n"
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index 0ff595e7ee..48ba9e5617 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -118,19 +118,22 @@ usage(unsigned short int pager)
HELP0(" -H, --html HTML table output mode\n");
HELP0(" -P, --pset=VAR[=ARG] set printing option VAR to ARG (see \\pset command)\n");
HELP0(" -R, --record-separator=STRING\n"
- " record separator for unaligned output (default: newline)\n");
+ " record separator for unaligned output\n"
+ " (default: newline)\n");
HELP0(" -t, --tuples-only print rows only\n");
HELP0(" -T, --table-attr=TEXT set HTML table tag attributes (e.g., width, border)\n");
HELP0(" -x, --expanded turn on expanded table output\n");
HELP0(" -z, --field-separator-zero\n"
" set field separator for unaligned output to zero byte\n");
HELP0(" -0, --record-separator-zero\n"
- " set record separator for unaligned output to zero byte\n");
+ " set record separator for unaligned output to zero\n"
+ " byte\n");
HELP0("\nConnection options:\n");
/* Display default host */
env = getenv("PGHOST");
- HELPN(" -h, --host=HOSTNAME database server host or socket directory (default: \"%s\")\n",
+ HELPN(" -h, --host=HOSTNAME database server host or socket directory\n"
+ " (default: \"%s\")\n",
env ? env : _("local socket"));
/* Display default port */
env = getenv("PGPORT");
diff --git a/src/bin/scripts/createdb.c b/src/bin/scripts/createdb.c
index 9ca86a3e53..fa1f6d95e5 100644
--- a/src/bin/scripts/createdb.c
+++ b/src/bin/scripts/createdb.c
@@ -297,7 +297,8 @@ help(const char *progname)
printf(_(" --icu-locale=LOCALE ICU locale setting for the database\n"));
printf(_(" --icu-rules=RULES ICU rules setting for the database\n"));
printf(_(" --locale-provider={libc|icu}\n"
- " locale provider for the database's default collation\n"));
+ " locale provider for the database's default\n"
+ " collation\n"));
printf(_(" -O, --owner=OWNER database user to own the new database\n"));
printf(_(" -S, --strategy=STRATEGY database creation strategy wal_log or file_copy\n"));
printf(_(" -T, --template=TEMPLATE template database to copy\n"));
diff --git a/src/bin/scripts/pg_isready.c b/src/bin/scripts/pg_isready.c
index 64bbffb0b2..a5e617ba1b 100644
--- a/src/bin/scripts/pg_isready.c
+++ b/src/bin/scripts/pg_isready.c
@@ -235,7 +235,8 @@ help(const char *progname)
printf(_("\nConnection options:\n"));
printf(_(" -h, --host=HOSTNAME database server host or socket directory\n"));
printf(_(" -p, --port=PORT database server port\n"));
- printf(_(" -t, --timeout=SECS seconds to wait when attempting connection, 0 disables (default: %s)\n"), DEFAULT_CONNECT_TIMEOUT);
+ printf(_(" -t, --timeout=SECS seconds to wait when attempting connection,\n"
+ " 0 disables (default: %s)\n"), DEFAULT_CONNECT_TIMEOUT);
printf(_(" -U, --username=USERNAME user name to connect as\n"));
printf(_("\nReport bugs to <%s>.\n"), PACKAGE_BUGREPORT);
printf(_("%s home page: <%s>\n"), PACKAGE_NAME, PACKAGE_URL);
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index 4b17a07089..7fffdad17b 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -1134,19 +1134,26 @@ help(const char *progname)
printf(_(" -e, --echo show the commands being sent to the server\n"));
printf(_(" -f, --full do full vacuuming\n"));
printf(_(" -F, --freeze freeze row transaction information\n"));
- printf(_(" --force-index-cleanup always remove index entries that point to dead tuples\n"));
+ printf(_(" --force-index-cleanup always remove index entries that point to dead\n"
+ " tuples\n"));
printf(_(" -j, --jobs=NUM use this many concurrent connections to vacuum\n"));
printf(_(" --min-mxid-age=MXID_AGE minimum multixact ID age of tables to vacuum\n"));
printf(_(" --min-xid-age=XID_AGE minimum transaction ID age of tables to vacuum\n"));
- printf(_(" --no-index-cleanup don't remove index entries that point to dead tuples\n"));
+ printf(_(" --no-index-cleanup don't remove index entries that point to dead\n"
+ " tuples\n"));
printf(_(" --no-process-main skip the main relation\n"));
- printf(_(" --no-process-toast skip the TOAST table associated with the table to vacuum\n"));
- printf(_(" --no-truncate don't truncate empty pages at the end of the table\n"));
+ printf(_(" --no-process-toast skip the TOAST table associated with the table\n"
+ " to vacuum\n"));
+ printf(_(" --no-truncate don't truncate empty pages at the end of the\n"
+ " table\n"));
printf(_(" -n, --schema=PATTERN vacuum tables in the specified schema(s) only\n"));
- printf(_(" -N, --exclude-schema=PATTERN do not vacuum tables in the specified schema(s)\n"));
- printf(_(" -P, --parallel=PARALLEL_WORKERS use this many background workers for vacuum, if available\n"));
+ printf(_(" -N, --exclude-schema=PATTERN do not vacuum tables in the specified\n"
+ " schema(s)\n"));
+ printf(_(" -P, --parallel=PARALLEL_WORKERS use this many background workers for vacuum,\n"
+ " if available\n"));
printf(_(" -q, --quiet don't write any messages\n"));
- printf(_(" --skip-locked skip relations that cannot be immediately locked\n"));
+ printf(_(" --skip-locked skip relations that cannot be immediately\n"
+ " locked\n"));
printf(_(" -t, --table='TABLE[(COLUMNS)]' vacuum specific table(s) only\n"));
printf(_(" -v, --verbose write a lot of output\n"));
printf(_(" -V, --version output version information, then exit\n"));
--
2.39.2
On 2023-07-05 10:47, torikoshia wrote:
Hi,
As discussed in [1], outputs of --help for some commands fits into 80
columns
per line, while others do not.Since it seems preferable to have consistent line break policy and some
people
use 80-column terminal, wouldn't it be better to make all commands in
80
columns per line?Attached patch which does this for src/bin commands.
If this is the way to go, I'll do same things for contrib commands.
[1]
/messages/by-id/3fe4af5a0a81fc6a2ec01cb484c0a487@oss.nttdata.com
Thanks for making the patches! I have some comments to v1 patch.
(1)
Why don't you add test for the purpose? It could be overkill...
I though the following function is the best place.
diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm
b/src/test/perl/PostgreSQL/Test/Utils.pm
index 617caa022f..1bdb81ac56 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -843,6 +843,10 @@ sub program_help_ok
ok($result, "$cmd --help exit code 0");
isnt($stdout, '', "$cmd --help goes to stdout");
is($stderr, '', "$cmd --help nothing to stderr");
+ foreach my $line (split /\n/, $stdout)
+ {
+ ok(length($line) <= 80, "$cmd --help output fit within
80 columns per line");
+ }
return;
}(2)
Is there any reason that only src/bin commands are targeted? I found
that
we also need to fix vacuumlo with the above test. I think it's better to
fix it because it's a contrib module.
$ vacuumlo --help | while IFS='' read line; do echo $((`echo $line | wc
-m` - 1)) $line; done | sort -n -r | head -n 2
84 -n, --dry-run don't remove large objects, just show
what would be done
74 -l, --limit=LIMIT commit after removing each LIMIT large
objects
(3)
Is to delete '/mnt/server' intended? I though it better to leave it as
is since archive_cleanup_command example uses the absolute path.
- " pg_archivecleanup /mnt/server/archiverdir
000000010000000000000010.00000020.backup\n"));
+ " pg_archivecleanup archiverdir
000000010000000000000010.00000020.backup\n"));I will confirmed that the --help text are not changed and only
the line breaks are changed. But, currently the above change
break it.
(4)
I found that some binaries, for example ecpg, are not tested with
program_help_ok(). Is it better to add tests in the patch?
BTW, I check the difference with the following commands
# files include "--help"
$ find -name "*.c" | xargs -I {} sh -c 'if [ `grep -e --help {} | wc -l`
-gt 0 ]; then echo {}; fi'
# programs which is tested with program_help_ok
$ find -name "*.pl" | xargs -I {} sh -c 'grep -e program_help_ok {}'
Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION
On 2023-08-21 13:08, Masahiro Ikeda wrote:
Thanks for your review!
(1)
Why don't you add test for the purpose? It could be overkill...
I though the following function is the best place.diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm index 617caa022f..1bdb81ac56 100644 --- a/src/test/perl/PostgreSQL/Test/Utils.pm +++ b/src/test/perl/PostgreSQL/Test/Utils.pm @@ -843,6 +843,10 @@ sub program_help_ok ok($result, "$cmd --help exit code 0"); isnt($stdout, '', "$cmd --help goes to stdout"); is($stderr, '', "$cmd --help nothing to stderr"); + foreach my $line (split /\n/, $stdout) + { + ok(length($line) <= 80, "$cmd --help output fit within 80 columns per line"); + } return; }
Agreed.
(2)
Is there any reason that only src/bin commands are targeted? I found
that
we also need to fix vacuumlo with the above test. I think it's better
to
fix it because it's a contrib module.$ vacuumlo --help | while IFS='' read line; do echo $((`echo $line |
wc -m` - 1)) $line; done | sort -n -r | head -n 2
84 -n, --dry-run don't remove large objects, just show
what would be done
74 -l, --limit=LIMIT commit after removing each LIMIT large
objects
This is because I wasn't sure making all --help outputs fit into 80
columns per line is right thing to do as described below:
| If this is the way to go, I'll do same things for contrib commands.
If there are no objection, I'm going to make other commands fit within
80 columns per line including (4).
(3)
Is to delete '/mnt/server' intended? I though it better to leave it as
is since archive_cleanup_command example uses the absolute path.- " pg_archivecleanup /mnt/server/archiverdir 000000010000000000000010.00000020.backup\n")); + " pg_archivecleanup archiverdir 000000010000000000000010.00000020.backup\n"));I will confirmed that the --help text are not changed and only
the line breaks are changed. But, currently the above change
break it.
Yes, it is intended as described in the thread.
/messages/by-id/20230615.152036.1556630042388070221.horikyota.ntt@gmail.com
| We could shorten it by removing the "/mnt/server" portion, but
I'm not sure if it's worth doing.
However, I feel it is acceptable to make an exception and exceed 80
characters for this line.
(4)
I found that some binaries, for example ecpg, are not tested with
program_help_ok(). Is it better to add tests in the patch?
Agreed.
BTW, I check the difference with the following commands
# files include "--help"
$ find -name "*.c" | xargs -I {} sh -c 'if [ `grep -e --help {} | wc
-l` -gt 0 ]; then echo {}; fi'# programs which is tested with program_help_ok
$ find -name "*.pl" | xargs -I {} sh -c 'grep -e program_help_ok {}'
Thanks for sharing your procedure!
--
Regards,
--
Atsushi Torikoshi
NTT DATA Group Corporation
Hi,
On 2023-08-22 22:57, torikoshia wrote:
On 2023-08-21 13:08, Masahiro Ikeda wrote:
(2)
Is there any reason that only src/bin commands are targeted? I found
that
we also need to fix vacuumlo with the above test. I think it's better
to
fix it because it's a contrib module.$ vacuumlo --help | while IFS='' read line; do echo $((`echo $line |
wc -m` - 1)) $line; done | sort -n -r | head -n 2
84 -n, --dry-run don't remove large objects, just show
what would be done
74 -l, --limit=LIMIT commit after removing each LIMIT large
objectsThis is because I wasn't sure making all --help outputs fit into 80
columns per line is right thing to do as described below:| If this is the way to go, I'll do same things for contrib commands.
If there are no objection, I'm going to make other commands fit within
80 columns per line including (4).
OK. Sorry, I missed the sentence above.
I'd like to hear what others comment too.
(3)
Is to delete '/mnt/server' intended? I though it better to leave it
as
is since archive_cleanup_command example uses the absolute path.- " pg_archivecleanup /mnt/server/archiverdir 000000010000000000000010.00000020.backup\n")); + " pg_archivecleanup archiverdir 000000010000000000000010.00000020.backup\n"));I will confirmed that the --help text are not changed and only
the line breaks are changed. But, currently the above change
break it.Yes, it is intended as described in the thread.
/messages/by-id/20230615.152036.1556630042388070221.horikyota.ntt@gmail.com
| We could shorten it by removing the "/mnt/server" portion, but
I'm not sure if it's worth doing.However, I feel it is acceptable to make an exception and exceed 80
characters for this line.
Thanks for sharing the thread. I understood.
It seems that the directory name should be consistent with the example
of archive_cleanup_command. However, it does not seem appropriate to
make archive_cleanup_command to use a relative path.
```
pg_archivecleanup --help
(snip)
e.g.
archive_cleanup_command = 'pg_archivecleanup /mnt/server/archiverdir
%r'
Or for use as a standalone archive cleaner:
e.g.
pg_archivecleanup /mnt/server/archiverdir
000000010000000000000010.00000020.backup
```
IMHO, is simply breaking the line acceptable?
```
Or for use as a standalone archive cleaner:
e.g.
pg_archivecleanup /mnt/server/archiverdir
000000010000000000000010.00000020.backup
```
Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION
On Mon, Aug 21, 2023 at 1:09 PM Masahiro Ikeda
<ikedamsh@oss.nttdata.com> wrote:
(1)
Why don't you add test for the purpose? It could be overkill...
I though the following function is the best place.
Added the test.
BTW, psql --help outputs the content of PGHOST, which caused a failure
in the test:
```
-h, --host=HOSTNAME database server host or socket directory
(default:
"/var/folders/m7/9snkd5b54cx_b4lxkl9ljlcc0000gn/T/LobrmSUf7t")
```
It may be overkill, added a logic for removing the content of PGHOST.
On 2023-08-23 09:45, Masahiro Ikeda wrote:
Hi,
On 2023-08-22 22:57, torikoshia wrote:
On 2023-08-21 13:08, Masahiro Ikeda wrote:
(2)
Is there any reason that only src/bin commands are targeted? I found
that
we also need to fix vacuumlo with the above test. I think it's better
to
fix it because it's a contrib module.$ vacuumlo --help | while IFS='' read line; do echo $((`echo $line |
wc -m` - 1)) $line; done | sort -n -r | head -n 2
84 -n, --dry-run don't remove large objects, just show
what would be done
74 -l, --limit=LIMIT commit after removing each LIMIT large
objectsThis is because I wasn't sure making all --help outputs fit into 80
columns per line is right thing to do as described below:| If this is the way to go, I'll do same things for contrib commands.
If there are no objection, I'm going to make other commands fit within
80 columns per line including (4).OK. Sorry, I missed the sentence above.
I'd like to hear what others comment too.
Although there are no comments, attached patch modifies vaccumlo.
(3)
Is to delete '/mnt/server' intended? I though it better to leave it
as
is since archive_cleanup_command example uses the absolute path.- " pg_archivecleanup /mnt/server/archiverdir 000000010000000000000010.00000020.backup\n")); + " pg_archivecleanup archiverdir 000000010000000000000010.00000020.backup\n"));I will confirmed that the --help text are not changed and only
the line breaks are changed. But, currently the above change
break it.Yes, it is intended as described in the thread.
/messages/by-id/20230615.152036.1556630042388070221.horikyota.ntt@gmail.com
| We could shorten it by removing the "/mnt/server" portion, but
I'm not sure if it's worth doing.However, I feel it is acceptable to make an exception and exceed 80
characters for this line.Thanks for sharing the thread. I understood.
It seems that the directory name should be consistent with the example
of archive_cleanup_command. However, it does not seem appropriate to
make archive_cleanup_command to use a relative path.```
pg_archivecleanup --help
(snip)
e.g.
archive_cleanup_command = 'pg_archivecleanup /mnt/server/archiverdir
%r'Or for use as a standalone archive cleaner:
e.g.
pg_archivecleanup /mnt/server/archiverdir
000000010000000000000010.00000020.backup
```IMHO, is simply breaking the line acceptable?
Agreed.
(4)
I found that some binaries, for example ecpg, are not tested with
program_help_ok(). Is it better to add tests in the patch?
Added program_help_ok() to ecpg and pgbench.
Although pg_regress and zic are not tested using program_help_ok, but
left as they are because they are not commands that users execute
directly.
--
Regards,
--
Atsushi Torikoshi
NTT DATA Group Corporation
Attachments:
v2-0001-Make-help-output-fit-within-80-columns-per-line.patchtext/x-diff; name=v2-0001-Make-help-output-fit-within-80-columns-per-line.patchDownload
From 4728cab3e39f7994b8b6dd41787cac90b5cb64f6 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <torikoshia@oss.nttdata.com>
Date: Thu, 31 Aug 2023 16:22:39 +0900
Subject: [PATCH v2] Make --help output fit within 80 columns per line
Outputs of --help output of some commands fit into 80 columns per line,
while others do not.
For the consistency and for 80-column terminal, this patch makes them
fit within 80 columns per line.
Also this patch adds a test which checks if --help outputs fit within
80 columns per line.
---
contrib/vacuumlo/vacuumlo.c | 3 ++-
src/bin/initdb/initdb.c | 6 ++++--
src/bin/pg_amcheck/pg_amcheck.c | 21 ++++++++++++-------
src/bin/pg_archivecleanup/pg_archivecleanup.c | 3 ++-
src/bin/pg_basebackup/pg_receivewal.c | 12 +++++++----
src/bin/pg_basebackup/pg_recvlogical.c | 18 ++++++++++------
src/bin/pg_checksums/pg_checksums.c | 3 ++-
src/bin/pg_dump/pg_dump.c | 3 ++-
src/bin/pg_dump/pg_dumpall.c | 3 ++-
src/bin/pg_dump/pg_restore.c | 6 ++++--
src/bin/pg_upgrade/option.c | 9 +++++---
src/bin/pgbench/pgbench.c | 6 ++++--
src/bin/pgbench/t/002_pgbench_no_server.pl | 1 +
src/bin/psql/help.c | 9 +++++---
src/bin/scripts/createdb.c | 3 ++-
src/bin/scripts/pg_isready.c | 3 ++-
src/bin/scripts/vacuumdb.c | 21 ++++++++++++-------
src/interfaces/ecpg/Makefile | 1 +
src/interfaces/ecpg/t/001_basic.pl | 14 +++++++++++++
src/test/perl/PostgreSQL/Test/Utils.pm | 10 +++++++++
20 files changed, 112 insertions(+), 43 deletions(-)
create mode 100644 src/interfaces/ecpg/t/001_basic.pl
diff --git a/contrib/vacuumlo/vacuumlo.c b/contrib/vacuumlo/vacuumlo.c
index 8941262731..39c1238802 100644
--- a/contrib/vacuumlo/vacuumlo.c
+++ b/contrib/vacuumlo/vacuumlo.c
@@ -417,7 +417,8 @@ usage(const char *progname)
printf("Usage:\n %s [OPTION]... DBNAME...\n\n", progname);
printf("Options:\n");
printf(" -l, --limit=LIMIT commit after removing each LIMIT large objects\n");
- printf(" -n, --dry-run don't remove large objects, just show what would be done\n");
+ printf(" -n, --dry-run don't remove large objects, just show what would be\n"
+ " done\n");
printf(" -v, --verbose write a lot of progress messages\n");
printf(" -V, --version output version information, then exit\n");
printf(" -?, --help show this help, then exit\n");
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 905b979947..42e9e19265 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -2434,8 +2434,10 @@ usage(const char *progname)
printf(_(" %s [OPTION]... [DATADIR]\n"), progname);
printf(_("\nOptions:\n"));
printf(_(" -A, --auth=METHOD default authentication method for local connections\n"));
- printf(_(" --auth-host=METHOD default authentication method for local TCP/IP connections\n"));
- printf(_(" --auth-local=METHOD default authentication method for local-socket connections\n"));
+ printf(_(" --auth-host=METHOD default authentication method for local TCP/IP\n"
+ " connections\n"));
+ printf(_(" --auth-local=METHOD default authentication method for local-socket\n"
+ " connections\n"));
printf(_(" [-D, --pgdata=]DATADIR location for this database cluster\n"));
printf(_(" -E, --encoding=ENCODING set default encoding for new databases\n"));
printf(_(" -g, --allow-group-access allow group read/execute on data directory\n"));
diff --git a/src/bin/pg_amcheck/pg_amcheck.c b/src/bin/pg_amcheck/pg_amcheck.c
index 57df14bc1e..606383ae0b 100644
--- a/src/bin/pg_amcheck/pg_amcheck.c
+++ b/src/bin/pg_amcheck/pg_amcheck.c
@@ -1150,17 +1150,23 @@ help(const char *progname)
printf(_(" -S, --exclude-schema=PATTERN do NOT check matching schema(s)\n"));
printf(_(" -t, --table=PATTERN check matching table(s)\n"));
printf(_(" -T, --exclude-table=PATTERN do NOT check matching table(s)\n"));
- printf(_(" --no-dependent-indexes do NOT expand list of relations to include indexes\n"));
- printf(_(" --no-dependent-toast do NOT expand list of relations to include TOAST tables\n"));
+ printf(_(" --no-dependent-indexes do NOT expand list of relations to include\n"
+ " indexes\n"));
+ printf(_(" --no-dependent-toast do NOT expand list of relations to include\n"
+ " TOAST tables\n"));
printf(_(" --no-strict-names do NOT require patterns to match objects\n"));
printf(_("\nTable checking options:\n"));
printf(_(" --exclude-toast-pointers do NOT follow relation TOAST pointers\n"));
printf(_(" --on-error-stop stop checking at end of first corrupt page\n"));
- printf(_(" --skip=OPTION do NOT check \"all-frozen\" or \"all-visible\" blocks\n"));
- printf(_(" --startblock=BLOCK begin checking table(s) at the given block number\n"));
- printf(_(" --endblock=BLOCK check table(s) only up to the given block number\n"));
+ printf(_(" --skip=OPTION do NOT check \"all-frozen\" or \"all-visible\"\n"
+ " blocks\n"));
+ printf(_(" --startblock=BLOCK begin checking table(s) at the given block\n"
+ " number\n"));
+ printf(_(" --endblock=BLOCK check table(s) only up to the given block\n"
+ " number\n"));
printf(_("\nB-tree index checking options:\n"));
- printf(_(" --heapallindexed check that all heap tuples are found within indexes\n"));
+ printf(_(" --heapallindexed check that all heap tuples are found within\n"
+ " indexes\n"));
printf(_(" --parent-check check index parent/child relationships\n"));
printf(_(" --rootdescend search from root page to refind tuples\n"));
printf(_("\nConnection options:\n"));
@@ -1172,7 +1178,8 @@ help(const char *progname)
printf(_(" --maintenance-db=DBNAME alternate maintenance database\n"));
printf(_("\nOther options:\n"));
printf(_(" -e, --echo show the commands being sent to the server\n"));
- printf(_(" -j, --jobs=NUM use this many concurrent connections to the server\n"));
+ printf(_(" -j, --jobs=NUM use this many concurrent connections to the\n"
+ " server\n"));
printf(_(" -P, --progress show progress information\n"));
printf(_(" -v, --verbose write a lot of output\n"));
printf(_(" -V, --version output version information, then exit\n"));
diff --git a/src/bin/pg_archivecleanup/pg_archivecleanup.c b/src/bin/pg_archivecleanup/pg_archivecleanup.c
index 2c3b301f3b..6f62d4661f 100644
--- a/src/bin/pg_archivecleanup/pg_archivecleanup.c
+++ b/src/bin/pg_archivecleanup/pg_archivecleanup.c
@@ -276,7 +276,8 @@ usage(void)
printf(_("\n"
"Or for use as a standalone archive cleaner:\n"
"e.g.\n"
- " pg_archivecleanup /mnt/server/archiverdir 000000010000000000000010.00000020.backup\n"));
+ " pg_archivecleanup /mnt/server/archiverdir\n"
+ " 000000010000000000000010.00000020.backup\n"));
printf(_("\nReport bugs to <%s>.\n"), PACKAGE_BUGREPORT);
printf(_("%s home page: <%s>\n"), PACKAGE_NAME, PACKAGE_URL);
}
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index d0a4079d50..39a40d525e 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -81,11 +81,13 @@ usage(void)
printf(_("\nOptions:\n"));
printf(_(" -D, --directory=DIR receive write-ahead log files into this directory\n"));
printf(_(" -E, --endpos=LSN exit after receiving the specified LSN\n"));
- printf(_(" --if-not-exists do not error if slot already exists when creating a slot\n"));
+ printf(_(" --if-not-exists do not error if slot already exists when creating a\n"
+ " slot\n"));
printf(_(" -n, --no-loop do not loop on connection lost\n"));
printf(_(" --no-sync do not wait for changes to be written safely to disk\n"));
printf(_(" -s, --status-interval=SECS\n"
- " time between status packets sent to server (default: %d)\n"), (standby_message_timeout / 1000));
+ " time between status packets sent to server\n"
+ " (default: %d)\n"), (standby_message_timeout / 1000));
printf(_(" -S, --slot=SLOTNAME replication slot to use\n"));
printf(_(" --synchronous flush write-ahead log immediately after writing\n"));
printf(_(" -v, --verbose output verbose messages\n"));
@@ -101,8 +103,10 @@ usage(void)
printf(_(" -w, --no-password never prompt for password\n"));
printf(_(" -W, --password force password prompt (should happen automatically)\n"));
printf(_("\nOptional actions:\n"));
- printf(_(" --create-slot create a new replication slot (for the slot's name see --slot)\n"));
- printf(_(" --drop-slot drop the replication slot (for the slot's name see --slot)\n"));
+ printf(_(" --create-slot create a new replication slot (for the slot's name see\n"
+ " --slot)\n"));
+ printf(_(" --drop-slot drop the replication slot (for the slot's name see\n"
+ " --slot)\n"));
printf(_("\nReport bugs to <%s>.\n"), PACKAGE_BUGREPORT);
printf(_("%s home page: <%s>\n"), PACKAGE_NAME, PACKAGE_URL);
}
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index 9fe4ac8126..e2f3d19fca 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -86,15 +86,19 @@ usage(void)
printf(_("Usage:\n"));
printf(_(" %s [OPTION]...\n"), progname);
printf(_("\nAction to be performed:\n"));
- printf(_(" --create-slot create a new replication slot (for the slot's name see --slot)\n"));
- printf(_(" --drop-slot drop the replication slot (for the slot's name see --slot)\n"));
- printf(_(" --start start streaming in a replication slot (for the slot's name see --slot)\n"));
+ printf(_(" --create-slot create a new replication slot (for the slot's name see\n"
+ " --slot)\n"));
+ printf(_(" --drop-slot drop the replication slot (for the slot's name see\n"
+ " --slot)\n"));
+ printf(_(" --start start streaming in a replication slot (for the slot's\n"
+ " name see --slot)\n"));
printf(_("\nOptions:\n"));
printf(_(" -E, --endpos=LSN exit after receiving the specified LSN\n"));
printf(_(" -f, --file=FILE receive log into this file, - for stdout\n"));
printf(_(" -F --fsync-interval=SECS\n"
" time between fsyncs to the output file (default: %d)\n"), (fsync_interval / 1000));
- printf(_(" --if-not-exists do not error if slot already exists when creating a slot\n"));
+ printf(_(" --if-not-exists do not error if slot already exists when creating a\n"
+ " slot\n"));
printf(_(" -I, --startpos=LSN where in an existing slot should the streaming start\n"));
printf(_(" -n, --no-loop do not loop on connection lost\n"));
printf(_(" -o, --option=NAME[=VALUE]\n"
@@ -102,9 +106,11 @@ usage(void)
" output plugin\n"));
printf(_(" -P, --plugin=PLUGIN use output plugin PLUGIN (default: %s)\n"), plugin);
printf(_(" -s, --status-interval=SECS\n"
- " time between status packets sent to server (default: %d)\n"), (standby_message_timeout / 1000));
+ " time between status packets sent to server\n"
+ " (default: %d)\n"), (standby_message_timeout / 1000));
printf(_(" -S, --slot=SLOTNAME name of the logical replication slot\n"));
- printf(_(" -t, --two-phase enable decoding of prepared transactions when creating a slot\n"));
+ printf(_(" -t, --two-phase enable decoding of prepared transactions when creating\n"
+ " a slot\n"));
printf(_(" -v, --verbose output verbose messages\n"));
printf(_(" -V, --version output version information, then exit\n"));
printf(_(" -?, --help show this help, then exit\n"));
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 19eb67e485..2b42ab7e0a 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -76,7 +76,8 @@ static pg_time_t last_progress_report = 0;
static void
usage(void)
{
- printf(_("%s enables, disables, or verifies data checksums in a PostgreSQL database cluster.\n\n"), progname);
+ printf(_("%s enables, disables, or verifies data checksums in a PostgreSQL\n"
+ "database cluster.\n\n"), progname);
printf(_("Usage:\n"));
printf(_(" %s [OPTION]... [DATADIR]\n"), progname);
printf(_("\nOptions:\n"));
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 65f64c282d..bf72f2c436 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -1067,7 +1067,8 @@ help(const char *progname)
printf(_(" -Z, --compress=METHOD[:DETAIL]\n"
" compress as specified\n"));
printf(_(" --lock-wait-timeout=TIMEOUT fail after waiting TIMEOUT for a table lock\n"));
- printf(_(" --no-sync do not wait for changes to be written safely to disk\n"));
+ printf(_(" --no-sync do not wait for changes to be written safely to\n"
+ " disk\n"));
printf(_(" -?, --help show this help, then exit\n"));
printf(_("\nOptions controlling the output content:\n"));
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index e2a9733d34..683056852c 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -661,7 +661,8 @@ help(void)
printf(_(" --no-role-passwords do not dump passwords for roles\n"));
printf(_(" --no-security-labels do not dump security label assignments\n"));
printf(_(" --no-subscriptions do not dump subscriptions\n"));
- printf(_(" --no-sync do not wait for changes to be written safely to disk\n"));
+ printf(_(" --no-sync do not wait for changes to be written safely to\n"
+ " disk\n"));
printf(_(" --no-table-access-method do not dump table access methods\n"));
printf(_(" --no-tablespaces do not dump tablespace assignments\n"));
printf(_(" --no-toast-compression do not dump TOAST compression methods\n"));
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index 049a100634..fbbb9f93f8 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -459,7 +459,8 @@ usage(const char *progname)
printf(_(" -S, --superuser=NAME superuser user name to use for disabling triggers\n"));
printf(_(" -t, --table=NAME restore named relation (table, view, etc.)\n"));
printf(_(" -T, --trigger=NAME restore named trigger\n"));
- printf(_(" -x, --no-privileges skip restoration of access privileges (grant/revoke)\n"));
+ printf(_(" -x, --no-privileges skip restoration of access privileges\n"
+ " (grant/revoke)\n"));
printf(_(" -1, --single-transaction restore as a single transaction\n"));
printf(_(" --disable-triggers disable triggers during data-only restore\n"));
printf(_(" --enable-row-security enable row security\n"));
@@ -472,7 +473,8 @@ usage(const char *progname)
printf(_(" --no-subscriptions do not restore subscriptions\n"));
printf(_(" --no-table-access-method do not restore table access methods\n"));
printf(_(" --no-tablespaces do not restore tablespace assignments\n"));
- printf(_(" --section=SECTION restore named section (pre-data, data, or post-data)\n"));
+ printf(_(" --section=SECTION restore named section (pre-data, data, or\n"
+ " post-data)\n"));
printf(_(" --strict-names require table and/or schema include patterns to\n"
" match at least one entity each\n"));
printf(_(" --use-set-session-authorization\n"
diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c
index 640361009e..58c86309a1 100644
--- a/src/bin/pg_upgrade/option.c
+++ b/src/bin/pg_upgrade/option.c
@@ -275,9 +275,11 @@ usage(void)
printf(_(" -c, --check check clusters only, don't change any data\n"));
printf(_(" -d, --old-datadir=DATADIR old cluster data directory\n"));
printf(_(" -D, --new-datadir=DATADIR new cluster data directory\n"));
- printf(_(" -j, --jobs=NUM number of simultaneous processes or threads to use\n"));
+ printf(_(" -j, --jobs=NUM number of simultaneous processes or threads to\n"
+ " use\n"));
printf(_(" -k, --link link instead of copying files to new cluster\n"));
- printf(_(" -N, --no-sync do not wait for changes to be written safely to disk\n"));
+ printf(_(" -N, --no-sync do not wait for changes to be written safely to\n"
+ " disk\n"));
printf(_(" -o, --old-options=OPTIONS old cluster options to pass to the server\n"));
printf(_(" -O, --new-options=OPTIONS new cluster options to pass to the server\n"));
printf(_(" -p, --old-port=PORT old cluster port number (default %d)\n"), old_cluster.port);
@@ -303,7 +305,8 @@ usage(void)
" the \"bin\" directory for the new version (-B BINDIR)\n"));
printf(_("\n"
"For example:\n"
- " pg_upgrade -d oldCluster/data -D newCluster/data -b oldCluster/bin -B newCluster/bin\n"
+ " pg_upgrade -d oldCluster/data -D newCluster/data\n"
+ " -b oldCluster/bin -B newCluster/bin\n"
"or\n"));
#ifndef WIN32
printf(_(" $ export PGDATAOLD=oldCluster/data\n"
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 713e8a06bb..b7ef127758 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -882,8 +882,10 @@ usage(void)
" --index-tablespace=TABLESPACE\n"
" create indexes in the specified tablespace\n"
" --partition-method=(range|hash)\n"
- " partition pgbench_accounts with this method (default: range)\n"
- " --partitions=NUM partition pgbench_accounts into NUM parts (default: 0)\n"
+ " partition pgbench_accounts with this method\n"
+ " (default: range)\n"
+ " --partitions=NUM partition pgbench_accounts into NUM parts\n"
+ " (default: 0)\n"
" --tablespace=TABLESPACE create tables in the specified tablespace\n"
" --unlogged-tables create tables as unlogged tables\n"
"\nOptions to select what to run:\n"
diff --git a/src/bin/pgbench/t/002_pgbench_no_server.pl b/src/bin/pgbench/t/002_pgbench_no_server.pl
index ffc8c772ae..932e80e465 100644
--- a/src/bin/pgbench/t/002_pgbench_no_server.pl
+++ b/src/bin/pgbench/t/002_pgbench_no_server.pl
@@ -234,6 +234,7 @@ for my $o (@options)
}
# Help
+program_help_ok('pgbench');
pgbench(
'--help', 0,
[
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index 38c165a627..b335cea8be 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -118,19 +118,22 @@ usage(unsigned short int pager)
HELP0(" -H, --html HTML table output mode\n");
HELP0(" -P, --pset=VAR[=ARG] set printing option VAR to ARG (see \\pset command)\n");
HELP0(" -R, --record-separator=STRING\n"
- " record separator for unaligned output (default: newline)\n");
+ " record separator for unaligned output\n"
+ " (default: newline)\n");
HELP0(" -t, --tuples-only print rows only\n");
HELP0(" -T, --table-attr=TEXT set HTML table tag attributes (e.g., width, border)\n");
HELP0(" -x, --expanded turn on expanded table output\n");
HELP0(" -z, --field-separator-zero\n"
" set field separator for unaligned output to zero byte\n");
HELP0(" -0, --record-separator-zero\n"
- " set record separator for unaligned output to zero byte\n");
+ " set record separator for unaligned output to zero\n"
+ " byte\n");
HELP0("\nConnection options:\n");
/* Display default host */
env = getenv("PGHOST");
- HELPN(" -h, --host=HOSTNAME database server host or socket directory (default: \"%s\")\n",
+ HELPN(" -h, --host=HOSTNAME database server host or socket directory\n"
+ " (default: \"%s\")\n",
env ? env : _("local socket"));
/* Display default port */
env = getenv("PGPORT");
diff --git a/src/bin/scripts/createdb.c b/src/bin/scripts/createdb.c
index 9ca86a3e53..fa1f6d95e5 100644
--- a/src/bin/scripts/createdb.c
+++ b/src/bin/scripts/createdb.c
@@ -297,7 +297,8 @@ help(const char *progname)
printf(_(" --icu-locale=LOCALE ICU locale setting for the database\n"));
printf(_(" --icu-rules=RULES ICU rules setting for the database\n"));
printf(_(" --locale-provider={libc|icu}\n"
- " locale provider for the database's default collation\n"));
+ " locale provider for the database's default\n"
+ " collation\n"));
printf(_(" -O, --owner=OWNER database user to own the new database\n"));
printf(_(" -S, --strategy=STRATEGY database creation strategy wal_log or file_copy\n"));
printf(_(" -T, --template=TEMPLATE template database to copy\n"));
diff --git a/src/bin/scripts/pg_isready.c b/src/bin/scripts/pg_isready.c
index 64bbffb0b2..a5e617ba1b 100644
--- a/src/bin/scripts/pg_isready.c
+++ b/src/bin/scripts/pg_isready.c
@@ -235,7 +235,8 @@ help(const char *progname)
printf(_("\nConnection options:\n"));
printf(_(" -h, --host=HOSTNAME database server host or socket directory\n"));
printf(_(" -p, --port=PORT database server port\n"));
- printf(_(" -t, --timeout=SECS seconds to wait when attempting connection, 0 disables (default: %s)\n"), DEFAULT_CONNECT_TIMEOUT);
+ printf(_(" -t, --timeout=SECS seconds to wait when attempting connection,\n"
+ " 0 disables (default: %s)\n"), DEFAULT_CONNECT_TIMEOUT);
printf(_(" -U, --username=USERNAME user name to connect as\n"));
printf(_("\nReport bugs to <%s>.\n"), PACKAGE_BUGREPORT);
printf(_("%s home page: <%s>\n"), PACKAGE_NAME, PACKAGE_URL);
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index f03d5b1c6c..275b8ccea1 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -1134,19 +1134,26 @@ help(const char *progname)
printf(_(" -e, --echo show the commands being sent to the server\n"));
printf(_(" -f, --full do full vacuuming\n"));
printf(_(" -F, --freeze freeze row transaction information\n"));
- printf(_(" --force-index-cleanup always remove index entries that point to dead tuples\n"));
+ printf(_(" --force-index-cleanup always remove index entries that point to dead\n"
+ " tuples\n"));
printf(_(" -j, --jobs=NUM use this many concurrent connections to vacuum\n"));
printf(_(" --min-mxid-age=MXID_AGE minimum multixact ID age of tables to vacuum\n"));
printf(_(" --min-xid-age=XID_AGE minimum transaction ID age of tables to vacuum\n"));
- printf(_(" --no-index-cleanup don't remove index entries that point to dead tuples\n"));
+ printf(_(" --no-index-cleanup don't remove index entries that point to dead\n"
+ " tuples\n"));
printf(_(" --no-process-main skip the main relation\n"));
- printf(_(" --no-process-toast skip the TOAST table associated with the table to vacuum\n"));
- printf(_(" --no-truncate don't truncate empty pages at the end of the table\n"));
+ printf(_(" --no-process-toast skip the TOAST table associated with the table\n"
+ " to vacuum\n"));
+ printf(_(" --no-truncate don't truncate empty pages at the end of the\n"
+ " table\n"));
printf(_(" -n, --schema=PATTERN vacuum tables in the specified schema(s) only\n"));
- printf(_(" -N, --exclude-schema=PATTERN do not vacuum tables in the specified schema(s)\n"));
- printf(_(" -P, --parallel=PARALLEL_WORKERS use this many background workers for vacuum, if available\n"));
+ printf(_(" -N, --exclude-schema=PATTERN do not vacuum tables in the specified\n"
+ " schema(s)\n"));
+ printf(_(" -P, --parallel=PARALLEL_WORKERS use this many background workers for vacuum,\n"
+ " if available\n"));
printf(_(" -q, --quiet don't write any messages\n"));
- printf(_(" --skip-locked skip relations that cannot be immediately locked\n"));
+ printf(_(" --skip-locked skip relations that cannot be immediately\n"
+ " locked\n"));
printf(_(" -t, --table='TABLE[(COLUMNS)]' vacuum specific table(s) only\n"));
printf(_(" -v, --verbose write a lot of output\n"));
printf(_(" -V, --version output version information, then exit\n"));
diff --git a/src/interfaces/ecpg/Makefile b/src/interfaces/ecpg/Makefile
index e4bbf7b8a8..43ef654e50 100644
--- a/src/interfaces/ecpg/Makefile
+++ b/src/interfaces/ecpg/Makefile
@@ -29,3 +29,4 @@ clean distclean maintainer-clean:
checktcp: | temp-install
check checktcp installcheck:
$(MAKE) -C test $@
+ $(prove_check)
diff --git a/src/interfaces/ecpg/t/001_basic.pl b/src/interfaces/ecpg/t/001_basic.pl
new file mode 100644
index 0000000000..7d54a0da73
--- /dev/null
+++ b/src/interfaces/ecpg/t/001_basic.pl
@@ -0,0 +1,14 @@
+
+# Copyright (c) 2021-2023, PostgreSQL Global Development Group
+
+use strict;
+use warnings;
+
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+program_help_ok('ecpg');
+program_version_ok('ecpg');
+program_options_handling_ok('ecpg');
+
+done_testing();
diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
index 617caa022f..9a721e7bcd 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -843,6 +843,16 @@ sub program_help_ok
ok($result, "$cmd --help exit code 0");
isnt($stdout, '', "$cmd --help goes to stdout");
is($stderr, '', "$cmd --help nothing to stderr");
+ foreach my $line (split /\n/, $stdout)
+ {
+ if (length($line) > 80)
+ {
+ # Ignore embedded PGHOST, which can be long length
+ $line =~ s/$ENV{PGHOST}//;
+ }
+ ok(length($line) <= 80,
+ "$cmd --help outputs fit within 80 columns per line");
+ }
return;
}
--
2.39.2
I like this work a lot. It's good to give developers easy to verify
guidance about formatting the --help messages.
However, I think instead of just adding a bunch of line breaks to
satisfy the test, we should really try to make the lines shorter by
rewording. Otherwise, chances are we are making the usability worse for
many users, because it's more likely that part of the help will scroll
off the screen. For example, in many cases, we could replace "do not"
by "don't", or we could decrease the indentation of the second column by
a few spaces, or just reword altogether.
Also, it would be very useful if the TAP test function could print out
the violating lines if a test fails. (Similar to how is() and like()
print the failing values.) Maybe start with that, and then it's easier
to play with different wording variants to try to make it fit better.
On 2023-09-12 15:27, Peter Eisentraut wrote:
I like this work a lot. It's good to give developers easy to verify
guidance about formatting the --help messages.However, I think instead of just adding a bunch of line breaks to
satisfy the test, we should really try to make the lines shorter by
rewording. Otherwise, chances are we are making the usability worse
for many users, because it's more likely that part of the help will
scroll off the screen. For example, in many cases, we could replace
"do not" by "don't", or we could decrease the indentation of the
second column by a few spaces, or just reword altogether.Also, it would be very useful if the TAP test function could print out
the violating lines if a test fails. (Similar to how is() and like()
print the failing values.) Maybe start with that, and then it's
easier to play with different wording variants to try to make it fit
better.
Thanks for the review!
I'll try to fix the patch according to your comments.
--
Regards,
--
Atsushi Torikoshi
NTT DATA Group Corporation
On Tue, Jul 4, 2023 at 9:47 PM torikoshia <torikoshia@oss.nttdata.com>
wrote:
Since it seems preferable to have consistent line break policy and some
people use 80-column terminal, wouldn't it be better to make all commands
in 80
columns per line?
All this seems an awful lot of work to support this mythical 80-column
terminal user. It's 2023, perhaps it's time to widen the default assumption
past 80 characters?
Cheers,
Greg
On 2023-09-12 15:27, Peter Eisentraut wrote:
Also, it would be very useful if the TAP test function could print out
the violating lines if a test fails. (Similar to how is() and like()
print the failing values.)
Attached patch for this.
Below are the the outputs when test failed:
```
$ cd contrib/vacuumlo
$ make check
...(snip)...
t/001_basic.pl .. 1/?
# Failed test ' -n, --dry-run don't remove large
objects, just show what would be done'
# at
/home/atorik/postgres/contrib/vacuumlo/../../src/test/perl/PostgreSQL/Test/Utils.pm
line 850.
# Looks like you failed 1 test of 21.
# Failed test 'vacuumlo --help outputs fit within 80 columns per line'
# at t/001_basic.pl line 10.
# Looks like you failed 1 test of 9.
t/001_basic.pl .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/9 subtests
Test Summary Report
-------------------
t/001_basic.pl (Wstat: 256 (exited 1) Tests: 9 Failed: 1)
Failed test: 4
Non-zero exit status: 1
Files=1, Tests=9, 0 wallclock secs ( 0.01 usr 0.01 sys + 0.04 cusr
0.01 csys = 0.07 CPU)
Result: FAIL
```
```
$ cat tmp_check/log/regress_log_001_basic
# Running: vacuumlo --help
[23:11:10.378](0.230s) ok 1 - vacuumlo --help exit code 0
[23:11:10.379](0.001s) ok 2 - vacuumlo --help goes to stdout
[23:11:10.379](0.000s) ok 3 - vacuumlo --help nothing to stderr
[23:11:10.380](0.000s) # Subtest: vacuumlo --help outputs fit within 80
columns per line
[23:11:10.380](0.001s) ok 1 - vacuumlo removes unreferenced large
objects from databases.
[23:11:10.380](0.000s) ok 2 -
[23:11:10.381](0.000s) ok 3 - Usage:
[23:11:10.381](0.000s) ok 4 - vacuumlo [OPTION]... DBNAME...
[23:11:10.381](0.000s) ok 5 -
[23:11:10.381](0.000s) ok 6 - Options:
[23:11:10.381](0.000s) ok 7 - -l, --limit=LIMIT commit
after removing each LIMIT large objects
[23:11:10.382](0.000s) ok 20 - Report bugs to
<pgsql-bugs@lists.postgresql.org>.
[23:11:10.382](0.000s) ok 21 - PostgreSQL home page:
<https://www.postgresql.org/>
[23:11:10.382](0.000s) 1..21
[23:11:10.382](0.000s) # Looks like you failed 1 test of 21.
[23:11:10.382](0.000s) not ok 4 - vacuumlo --help outputs fit within 80
columns per line
[23:11:10.382](0.000s)
[23:11:10.382](0.000s) # Failed test 'vacuumlo --help outputs fit
within 80 columns per line'
# at t/001_basic.pl line 10.
# Running: vacuumlo --version
[23:11:10.388](0.005s) ok 5 - vacuumlo --version exit code 0
[23:11:10.388](0.000s) ok 6 - vacuumlo --version goes to stdout
[23:11:10.388](0.000s) ok 7 - vacuumlo --version nothing to stderr
# Running: vacuumlo --not-a-valid-option
[23:11:10.391](0.003s) ok 8 - vacuumlo with invalid option nonzero exit
code
[23:11:10.391](0.000s) ok 9 - vacuumlo with invalid option prints error
message
[23:11:10.391](0.000s) 1..9
[23:11:10.391](0.000s) # Looks like you failed 1 test of 9.
```
I feel using subtest in Test::More improves readability.
On 2023-09-14 02:46, Greg Sabino Mullane wrote:
All this seems an awful lot of work to support this mythical 80-column
terminal user.
It's 2023, perhaps it's time to widen the default assumption past 80
characters?
That may be a good idea.
However, from what I have seen some basic commands like `ls` in my Linux
environments, the man command has over 100 characters per line, while
the output of the --help option seems to be within 80 characters per
line.
Also, the current PostgreSQL commands follow the "no more than 80
characters per line".
I do not intend to adhere to this rule(my terminals are usually bigger
than 80 chars per line), but wouldn't it be a not bad direction to use
80 characters for all commands?
Thoughts?
--
Regards,
--
Atsushi Torikoshi
NTT DATA Group Corporation
Attachments:
v1-0001-Added-a-test-for-checking-the-length-of-help-outp.patchtext/x-diff; name=v1-0001-Added-a-test-for-checking-the-length-of-help-outp.patchDownload
From 3dbfdb79680a0c1b68d4f742ae408810a1ee999d Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <torikoshia@oss.nttdata.com>
Date: Fri, 15 Sep 2023 23:29:23 +0900
Subject: [PATCH v1] Added a test for checking the length of --help output
---
src/test/perl/PostgreSQL/Test/Utils.pm | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
index 617caa022f..989f369ae7 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -843,6 +843,13 @@ sub program_help_ok
ok($result, "$cmd --help exit code 0");
isnt($stdout, '', "$cmd --help goes to stdout");
is($stderr, '', "$cmd --help nothing to stderr");
+
+ subtest "$cmd --help outputs fit within 80 columns per line" => sub {
+ foreach my $line (split /\n/, $stdout)
+ {
+ ok(length($line) <= 80, "$line");
+ }
+};
return;
}
--
2.39.2
On Fri, Sep 15, 2023 at 11:11 AM torikoshia <torikoshia@oss.nttdata.com>
wrote:
I do not intend to adhere to this rule(my terminals are usually bigger
than 80 chars per line), but wouldn't it be a not bad direction to use
80 characters for all commands?
Well, that's the question du jour, isn't it? The 80 character limit is
based on punch cards, and really has no place in modern systems. While gnu
systems are stuck in the past, many other ones have moved on to more
sensible defaults:
$ wget --help | wc -L
110
$ gcloud --help | wc -L
122
$ yum --help | wc -L
122
git is an interesting one, as they force things through a pager for their
help, but if you look at their raw help text files, they have plenty of
times they go past 80 when needed:
$ wc -L git/Documentation/git-*.txt | sort -g | tail -20
109 git-filter-branch.txt
109 git-rebase.txt
116 git-diff-index.txt
116 git-http-fetch.txt
117 git-restore.txt
122 git-checkout.txt
122 git-ls-tree.txt
129 git-init-db.txt
131 git-push.txt
132 git-update-ref.txt
142 git-maintenance.txt
144 git-interpret-trailers.txt
146 git-cat-file.txt
148 git-repack.txt
161 git-config.txt
162 git-notes.txt
205 git-stash.txt
251 git-submodule.txt
So in summary, I think 80 is a decent soft limit, but let's not stress out
about some lines going over that, and make a hard limit of perhaps 120.
See also: https://hilton.org.uk/blog/source-code-line-length
Cheers,
Greg
P.S. I know this won't change anything right away, but it will get the
conversation started, so we can escape the inertia of punch cards / VT100
terminals someday. :)
On 31.08.23 09:47, torikoshia wrote:
BTW, psql --help outputs the content of PGHOST, which caused a failure
in the test:```
-h, --host=HOSTNAME database server host or socket directory
(default:
"/var/folders/m7/9snkd5b54cx_b4lxkl9ljlcc0000gn/T/LobrmSUf7t")
```It may be overkill, added a logic for removing the content of PGHOST.
I wonder, should we remove this? We display the
environment-variable-based defaults in psql --help, but not for any
other programs. This is potentially misleading.
On Tue, Sep 19, 2023 at 3:23 AM Greg Sabino Mullane <htamfids@gmail.com>
wrote:
Thanks for your investigation!
On Fri, Sep 15, 2023 at 11:11 AM torikoshia
<torikoshia@oss.nttdata.com> wrote:I do not intend to adhere to this rule(my terminals are usually bigger
than 80 chars per line), but wouldn't it be a not bad direction to use
80 characters for all commands?Well, that's the question du jour, isn't it? The 80 character limit is
based on punch cards, and really has no place in modern systems. While
gnu systems are stuck in the past, many other ones have moved on to
more sensible defaults:$ wget --help | wc -L
110$ gcloud --help | wc -L
122$ yum --help | wc -L
122git is an interesting one, as they force things through a pager for
their help, but if you look at their raw help text files, they have
plenty of times they go past 80 when needed:$ wc -L git/Documentation/git-*.txt | sort -g | tail -20
109 git-filter-branch.txt
109 git-rebase.txt
116 git-diff-index.txt
116 git-http-fetch.txt
117 git-restore.txt
122 git-checkout.txt
122 git-ls-tree.txt
129 git-init-db.txt
131 git-push.txt
132 git-update-ref.txt
142 git-maintenance.txt
144 git-interpret-trailers.txt
146 git-cat-file.txt
148 git-repack.txt
161 git-config.txt
162 git-notes.txt
205 git-stash.txt
251 git-submodule.txtSo in summary, I think 80 is a decent soft limit, but let's not stress
out about some lines going over that, and make a hard limit of perhaps
120.
+1. It may be a good compromise.
For enforcing the hard limit, is it better to add a regression test like
patch 0001?
On 2023-09-21 16:45, Peter Eisentraut wrote:
On 31.08.23 09:47, torikoshia wrote:
BTW, psql --help outputs the content of PGHOST, which caused a failure
in the test:```
-h, --host=HOSTNAME database server host or socket directory
(default:
"/var/folders/m7/9snkd5b54cx_b4lxkl9ljlcc0000gn/T/LobrmSUf7t")
```It may be overkill, added a logic for removing the content of PGHOST.
I wonder, should we remove this? We display the
environment-variable-based defaults in psql --help, but not for any
other programs. This is potentially misleading.
Agreed. It seems inconsistent with other commands.
Patch 0002 removed environment-variable-based defaults in psql --help.
--
Regards,
--
Atsushi Torikoshi
NTT DATA Group Corporation
Attachments:
v1-0001-Added-a-test-for-checking-the-line-length-of-help.patchtext/x-diff; name=v1-0001-Added-a-test-for-checking-the-line-length-of-help.patchDownload
From b0ae0826374ce86c95ee36637913b65f865d6e0b Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <torikoshia@oss.nttdata.com>
Date: Mon, 25 Sep 2023 10:40:36 +0900
Subject: [PATCH v1 1/2] Added a test for checking the line length of --help
output.
---
src/test/perl/PostgreSQL/Test/Utils.pm | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
index d66fe1cf45..291b5dcbbb 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -884,6 +884,14 @@ sub program_help_ok
ok($result, "$cmd --help exit code 0");
isnt($stdout, '', "$cmd --help goes to stdout");
is($stderr, '', "$cmd --help nothing to stderr");
+
+ # We set the hard limit on the length of line to 120
+ subtest "$cmd --help outputs fit within 120 columns per line" => sub {
+ foreach my $line (split /\n/, $stdout)
+ {
+ ok(length($line) <= 120, "$line");
+ }
+};
return;
}
--
2.39.2
v1-0002-Squashed-commit-of-the-following.patchtext/x-diff; name=v1-0002-Squashed-commit-of-the-following.patchDownload
From 207c4059b42e975bc788452838099da825972d15 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <torikoshia@oss.nttdata.com>
Date: Mon, 25 Sep 2023 10:52:08 +0900
Subject: [PATCH v1 2/2] Removed environment-variable-based defaults in psql --help
---
src/bin/psql/help.c | 33 ++++-----------------------------
1 file changed, 4 insertions(+), 29 deletions(-)
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index 12280c0e54..3b2d59e2ee 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -50,22 +50,10 @@
void
usage(unsigned short int pager)
{
- const char *env;
- const char *user;
- char *errstr;
PQExpBufferData buf;
int nlcount;
FILE *output;
- /* Find default user, in case we need it. */
- user = getenv("PGUSER");
- if (!user)
- {
- user = get_user_name(&errstr);
- if (!user)
- pg_fatal("%s", errstr);
- }
-
/*
* To avoid counting the output lines manually, build the output in "buf"
* and then count them.
@@ -77,13 +65,8 @@ usage(unsigned short int pager)
HELP0(" psql [OPTION]... [DBNAME [USERNAME]]\n\n");
HELP0("General options:\n");
- /* Display default database */
- env = getenv("PGDATABASE");
- if (!env)
- env = user;
HELP0(" -c, --command=COMMAND run only single command (SQL or internal) and exit\n");
- HELPN(" -d, --dbname=DBNAME database name to connect to (default: \"%s\")\n",
- env);
+ HELP0(" -d, --dbname=DBNAME database name to connect to\n");
HELP0(" -f, --file=FILENAME execute commands from file, then exit\n");
HELP0(" -l, --list list available databases, then exit\n");
HELP0(" -v, --set=, --variable=NAME=VALUE\n"
@@ -128,17 +111,9 @@ usage(unsigned short int pager)
" set record separator for unaligned output to zero byte\n");
HELP0("\nConnection options:\n");
- /* Display default host */
- env = getenv("PGHOST");
- HELPN(" -h, --host=HOSTNAME database server host or socket directory (default: \"%s\")\n",
- env ? env : _("local socket"));
- /* Display default port */
- env = getenv("PGPORT");
- HELPN(" -p, --port=PORT database server port (default: \"%s\")\n",
- env ? env : DEF_PGPORT_STR);
- /* Display default user */
- HELPN(" -U, --username=USERNAME database user name (default: \"%s\")\n",
- user);
+ HELP0(" -h, --host=HOSTNAME database server host or socket directory\n");
+ HELP0(" -p, --port=PORT database server port\n");
+ HELP0(" -U, --username=USERNAME database user name\n");
HELP0(" -w, --no-password never prompt for password\n");
HELP0(" -W, --password force password prompt (should happen automatically)\n");
--
2.39.2
On 2023-09-25 15:27, torikoshia wrote:
On Tue, Sep 19, 2023 at 3:23 AM Greg Sabino Mullane
<htamfids@gmail.com> wrote:
Thanks for your investigation!On Fri, Sep 15, 2023 at 11:11 AM torikoshia
<torikoshia@oss.nttdata.com> wrote:I do not intend to adhere to this rule(my terminals are usually
bigger
than 80 chars per line), but wouldn't it be a not bad direction to
use
80 characters for all commands?Well, that's the question du jour, isn't it? The 80 character limit is
based on punch cards, and really has no place in modern systems. While
gnu systems are stuck in the past, many other ones have moved on to
more sensible defaults:$ wget --help | wc -L
110$ gcloud --help | wc -L
122$ yum --help | wc -L
122git is an interesting one, as they force things through a pager for
their help, but if you look at their raw help text files, they have
plenty of times they go past 80 when needed:$ wc -L git/Documentation/git-*.txt | sort -g | tail -20
109 git-filter-branch.txt
109 git-rebase.txt
116 git-diff-index.txt
116 git-http-fetch.txt
117 git-restore.txt
122 git-checkout.txt
122 git-ls-tree.txt
129 git-init-db.txt
131 git-push.txt
132 git-update-ref.txt
142 git-maintenance.txt
144 git-interpret-trailers.txt
146 git-cat-file.txt
148 git-repack.txt
161 git-config.txt
162 git-notes.txt
205 git-stash.txt
251 git-submodule.txtSo in summary, I think 80 is a decent soft limit, but let's not stress
out about some lines going over that, and make a hard limit of perhaps
120.+1. It may be a good compromise.
For enforcing the hard limit, is it better to add a regression test
like patch 0001?
Ugh, regression tests failed and it appears to be due to reasons related
to meson.
I'm going to investigate it.
--
Regards,
--
Atsushi Torikoshi
NTT DATA Group Corporation
On 25.09.23 08:27, torikoshia wrote:
So in summary, I think 80 is a decent soft limit, but let's not stress
out about some lines going over that, and make a hard limit of perhaps
120.+1. It may be a good compromise.
For enforcing the hard limit, is it better to add a regression test like
patch 0001?
Agreed. It seems inconsistent with other commands.
Patch 0002 removed environment-variable-based defaults in psql --help.
I have committed 0002 and a different implementation of 0001. I set the
maximum line length to 95, which is the current maximum in use.
I'm open to discussing other line lengths, but
1) If we make it longer, then we should also adjust the existing
wrapping so that we don't have a mix of lines wrapped at 80 and some
significantly longer lines.
2) There are some general readability guidelines that suggest like 66 or
72 characters per line. If you take that and add the option name itself
and some indentation, then around 90 does seem like a sensible limit.
3) The examples from other tools posted earlier don't convince me. Some
of their --help outputs look like junk and poorly taken care of.
So I think nudging people to aim for 80..95 seems like a good target
right now. But I'm not against adjustments.
On 2023-09-25 15:27, torikoshia wrote:
Ugh, regression tests failed and it appears to be due to reasons
related to meson.
I'm going to investigate it.
ISTM
On 2023-10-06 19:49, Peter Eisentraut wrote:
On 25.09.23 08:27, torikoshia wrote:
So in summary, I think 80 is a decent soft limit, but let's not
stress out about some lines going over that, and make a hard limit of
perhaps 120.+1. It may be a good compromise.
For enforcing the hard limit, is it better to add a regression test
like patch 0001?Agreed. It seems inconsistent with other commands.
Patch 0002 removed environment-variable-based defaults in psql --help.I have committed 0002 and a different implementation of 0001. I set
the maximum line length to 95, which is the current maximum in use.
Thanks!
BTW as far as I investigated, the original 0002 patch failed because
current meson doesn't accept subtest outputs.
As I commented below thread a few days ago, they once modified to
accept subtest outputs, but not anymore.
https://github.com/mesonbuild/meson/issues/10032
I'm open to discussing other line lengths, but
1) If we make it longer, then we should also adjust the existing
wrapping so that we don't have a mix of lines wrapped at 80 and some
significantly longer lines.2) There are some general readability guidelines that suggest like 66
or 72 characters per line. If you take that and add the option name
itself and some indentation, then around 90 does seem like a sensible
limit.3) The examples from other tools posted earlier don't convince me.
Some of their --help outputs look like junk and poorly taken care of.So I think nudging people to aim for 80..95 seems like a good target
right now. But I'm not against adjustments.
--
Regards,
--
Atsushi Torikoshi
NTT DATA Group Corporation