pgbench: improve --help and --version parsing
Hi,
There is a small catch in the parsing of --help and --version args by pgbench:
they are processed correctly only as the first argument. If it's not the case,
strange error message occurs:
$ pgbench -q --help
pgbench: unrecognized option '--help'
Try "pgbench --help" for more information.
The reason for this behavior is how these two arguments are handled in
src/bin/pgbench/pgbench.c:
if (argc > 1)
{
if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
{
usage();
exit(0);
}
if (strcmp(argv[1], "--version") == 0 || strcmp(argv[1], "-V") == 0)
{
puts("pgbench (PostgreSQL) " PG_VERSION);
exit(0);
}
}
All other arguments are processed with getopt_long. The proposed patch replaces
the existing way of parsing the --help and --version arguments with getopt_long,
expanding the existing switch statement.
--
Andrei Korigodski
Attachments:
pgbench-arg-parsing-v1.patchtext/x-patch; name=pgbench-arg-parsing-v1.patchDownload
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 41b756c089..d1ff85a677 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -4763,6 +4763,7 @@ main(int argc, char **argv)
{"define", required_argument, NULL, 'D'},
{"file", required_argument, NULL, 'f'},
{"fillfactor", required_argument, NULL, 'F'},
+ {"help", no_argument, NULL, '?'},
{"host", required_argument, NULL, 'h'},
{"initialize", no_argument, NULL, 'i'},
{"init-steps", required_argument, NULL, 'I'},
@@ -4783,6 +4784,7 @@ main(int argc, char **argv)
{"transactions", required_argument, NULL, 't'},
{"username", required_argument, NULL, 'U'},
{"vacuum-all", no_argument, NULL, 'v'},
+ {"version", no_argument, NULL, 'V'},
/* long-named only options */
{"unlogged-tables", no_argument, NULL, 1},
{"tablespace", required_argument, NULL, 2},
@@ -4832,20 +4834,6 @@ main(int argc, char **argv)
progname = get_progname(argv[0]);
- if (argc > 1)
- {
- if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
- {
- usage();
- exit(0);
- }
- if (strcmp(argv[1], "--version") == 0 || strcmp(argv[1], "-V") == 0)
- {
- puts("pgbench (PostgreSQL) " PG_VERSION);
- exit(0);
- }
- }
-
#ifdef WIN32
/* stderr is buffered on Win32. */
setvbuf(stderr, NULL, _IONBF, 0);
@@ -4868,7 +4856,7 @@ main(int argc, char **argv)
exit(1);
}
- while ((c = getopt_long(argc, argv, "iI:h:nvp:dqb:SNc:j:Crs:t:T:U:lf:D:F:M:P:R:L:", long_options, &optindex)) != -1)
+ while ((c = getopt_long(argc, argv, "iI:h:nvp:dqb:SNc:j:Crs:t:T:U:lf:D:F:M:P:R:L:V?", long_options, &optindex)) != -1)
{
char *script;
@@ -5097,6 +5085,14 @@ main(int argc, char **argv)
latency_limit = (int64) (limit_ms * 1000);
}
break;
+ case 'V':
+ puts("pgbench (PostgreSQL) " PG_VERSION);
+ exit(0);
+ break;
+ case '?':
+ usage();
+ exit(0);
+ break;
case 1: /* unlogged-tables */
initialization_option_set = true;
unlogged_tables = true;
Andrei Korigodski <akorigod@gmail.com> writes:
There is a small catch in the parsing of --help and --version args by pgbench:
they are processed correctly only as the first argument.
This is, in fact, how it's done in all PG apps.
regards, tom lane
On July 21, 2018 11:15:51 PM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Andrei Korigodski <akorigod@gmail.com> writes:
There is a small catch in the parsing of --help and --version args by
pgbench:
they are processed correctly only as the first argument.
This is, in fact, how it's done in all PG apps.
Think there's a fair argument that we should improve that at some point...
Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Andres Freund <andres@anarazel.de> writes:
On July 21, 2018 11:15:51 PM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote:
This is, in fact, how it's done in all PG apps.
Think there's a fair argument that we should improve that at some point...
Perhaps. Peter E. might remember why it's like that. But I'm dubious
about changing it in only one app.
regards, tom lane
On July 21, 2018 11:52:05 PM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote:
But I'm dubious
about changing it in only one app.
Agreed.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Tom Lane <tgl@sss.pgh.pa.us> writes:
Andres Freund <andres@anarazel.de> writes:
On July 21, 2018 11:15:51 PM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote:
This is, in fact, how it's done in all PG apps.
Think there's a fair argument that we should improve that at some point...
Perhaps. Peter E. might remember why it's like that.
It was done this way because then there was HAVE_GETOPT_LONG define for systems
that doesn't support getopt_long, see commit 41fde5460387 ("Polish help output.
Allow --help to work with BSD getopts.", Peter Eisentraut, 2001-01-06). Now this
define is not used by any app in src/bin, so I believe there is no need for this
workaround anymore.
By the way, this approach is already not used in pg_waldump and psql handles the
arguments more complex way to avoid the problem under discussion.
But I'm dubious about changing it in only one app.
Agreed. I have changed handling of the --help and --version options in all apps
where it exhibits the problem described, with the exception for pg_archivecleanup
where getopt is used instead of getopt_long. The separate patch will be proposed
to address it.
The patch is against current master. All tests pass.
Attachments:
help-version-args-parsing-v1.patchtext/x-patch; name=help-version-args-parsing-v1.patchDownload
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 3f203c6ca6..7a23c0b1ab 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -3130,23 +3130,9 @@ main(int argc, char *argv[])
progname = get_progname(argv[0]);
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("initdb"));
- if (argc > 1)
- {
- if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
- {
- usage(progname);
- exit(0);
- }
- if (strcmp(argv[1], "--version") == 0 || strcmp(argv[1], "-V") == 0)
- {
- puts("initdb (PostgreSQL) " PG_VERSION);
- exit(0);
- }
- }
-
/* process command-line options */
- while ((c = getopt_long(argc, argv, "dD:E:kL:nNU:WA:sST:X:g", long_options, &option_index)) != -1)
+ while ((c = getopt_long(argc, argv, "dD:E:kL:nNU:WA:sST:X:gV?", long_options, &option_index)) != -1)
{
switch (c)
{
@@ -3243,6 +3229,14 @@ main(int argc, char *argv[])
case 'g':
SetDataDirectoryCreatePerm(PG_DIR_MODE_GROUP);
break;
+ case 'V':
+ puts("initdb (PostgreSQL) " PG_VERSION);
+ exit(0);
+ break;
+ case '?':
+ usage(progname);
+ exit(0);
+ break;
default:
/* getopt_long already emitted a complaint */
fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index ef4cfc4384..03f7875514 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -2169,24 +2169,9 @@ main(int argc, char **argv)
progname = get_progname(argv[0]);
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_basebackup"));
- if (argc > 1)
- {
- if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
- {
- usage();
- exit(0);
- }
- else if (strcmp(argv[1], "-V") == 0
- || strcmp(argv[1], "--version") == 0)
- {
- puts("pg_basebackup (PostgreSQL) " PG_VERSION);
- exit(0);
- }
- }
-
atexit(cleanup_directories_atexit);
- while ((c = getopt_long(argc, argv, "CD:F:r:RS:T:X:l:nNzZ:d:c:h:p:U:s:wWkvP",
+ while ((c = getopt_long(argc, argv, "CD:F:r:RS:T:X:l:nNzZ:d:c:h:p:U:s:wWkvPV?",
long_options, &option_index)) != -1)
{
switch (c)
@@ -2328,6 +2313,14 @@ main(int argc, char **argv)
case 'P':
showprogress = true;
break;
+ case 'V':
+ puts("pg_basebackup (PostgreSQL) " PG_VERSION);
+ exit(0);
+ break;
+ case '?':
+ usage();
+ exit(0);
+ break;
case 3:
verify_checksums = false;
break;
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index 8be8d48a8a..5318454d67 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -509,22 +509,7 @@ main(int argc, char **argv)
progname = get_progname(argv[0]);
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_basebackup"));
- if (argc > 1)
- {
- if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
- {
- usage();
- exit(0);
- }
- else if (strcmp(argv[1], "-V") == 0 ||
- strcmp(argv[1], "--version") == 0)
- {
- puts("pg_receivewal (PostgreSQL) " PG_VERSION);
- exit(0);
- }
- }
-
- while ((c = getopt_long(argc, argv, "D:d:E:h:p:U:s:S:nwWvZ:",
+ while ((c = getopt_long(argc, argv, "D:d:E:h:p:U:s:S:nwWvVZ:?",
long_options, &option_index)) != -1)
{
switch (c)
@@ -584,6 +569,10 @@ main(int argc, char **argv)
case 'v':
verbose++;
break;
+ case 'V':
+ puts("pg_receivewal (PostgreSQL) " PG_VERSION);
+ exit(0);
+ break;
case 'Z':
compresslevel = atoi(optarg);
if (compresslevel < 0 || compresslevel > 9)
@@ -593,6 +582,10 @@ main(int argc, char **argv)
exit(1);
}
break;
+ case '?':
+ usage();
+ exit(0);
+ break;
/* action */
case 1:
do_create_slot = true;
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index ef85c9af4c..170e7475eb 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -711,22 +711,7 @@ main(int argc, char **argv)
progname = get_progname(argv[0]);
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_basebackup"));
- if (argc > 1)
- {
- if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
- {
- usage();
- exit(0);
- }
- else if (strcmp(argv[1], "-V") == 0 ||
- strcmp(argv[1], "--version") == 0)
- {
- puts("pg_recvlogical (PostgreSQL) " PG_VERSION);
- exit(0);
- }
- }
-
- while ((c = getopt_long(argc, argv, "E:f:F:nvd:h:p:U:wWI:o:P:s:S:",
+ while ((c = getopt_long(argc, argv, "E:f:F:nvd:h:p:U:wWI:o:P:s:S:V?",
long_options, &option_index)) != -1)
{
switch (c)
@@ -750,6 +735,14 @@ main(int argc, char **argv)
case 'v':
verbose++;
break;
+ case 'V':
+ puts("pg_recvlogical (PostgreSQL) " PG_VERSION);
+ exit(0);
+ break;
+ case '?':
+ usage();
+ exit(0);
+ break;
/* connection options */
case 'd':
dbname = pg_strdup(optarg);
diff --git a/src/bin/pg_controldata/pg_controldata.c b/src/bin/pg_controldata/pg_controldata.c
index 895a51f89d..9c8ba27764 100644
--- a/src/bin/pg_controldata/pg_controldata.c
+++ b/src/bin/pg_controldata/pg_controldata.c
@@ -88,6 +88,8 @@ main(int argc, char *argv[])
{
static struct option long_options[] = {
{"pgdata", required_argument, NULL, 'D'},
+ {"version", no_argument, NULL, 'V'},
+ {"help", no_argument, NULL, '?'},
{NULL, 0, NULL, 0}
};
@@ -110,21 +112,7 @@ main(int argc, char *argv[])
progname = get_progname(argv[0]);
- if (argc > 1)
- {
- if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
- {
- usage(progname);
- exit(0);
- }
- if (strcmp(argv[1], "--version") == 0 || strcmp(argv[1], "-V") == 0)
- {
- puts("pg_controldata (PostgreSQL) " PG_VERSION);
- exit(0);
- }
- }
-
- while ((c = getopt_long(argc, argv, "D:", long_options, NULL)) != -1)
+ while ((c = getopt_long(argc, argv, "D:V?", long_options, NULL)) != -1)
{
switch (c)
{
@@ -132,6 +120,16 @@ main(int argc, char *argv[])
DataDir = optarg;
break;
+ case 'V':
+ puts("pg_controldata (PostgreSQL) " PG_VERSION);
+ exit(0);
+ break;
+
+ case '?':
+ usage(progname);
+ exit(0);
+ break;
+
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit(1);
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index ed2396aa6c..8779cb6a66 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -2220,7 +2220,7 @@ main(int argc, char **argv)
/* process command-line options */
while (optind < argc)
{
- while ((c = getopt_long(argc, argv, "cD:e:l:m:N:o:p:P:sS:t:U:wW",
+ while ((c = getopt_long(argc, argv, "cD:e:l:m:N:o:p:P:sS:t:U:VwW?",
long_options, &option_index)) != -1)
{
switch (c)
@@ -2296,12 +2296,20 @@ main(int argc, char **argv)
/* Prepend .\ for local accounts */
register_username = psprintf(".\\%s", optarg);
break;
+ case 'V':
+ puts("pg_ctl (PostgreSQL) " PG_VERSION);
+ exit(0);
+ break;
case 'w':
do_wait = true;
break;
case 'W':
do_wait = false;
break;
+ case '?':
+ do_help();
+ exit(0);
+ break;
case 'c':
allow_core_files = true;
break;
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 86524d6598..0f961bf9a6 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -399,23 +399,9 @@ main(int argc, char **argv)
progname = get_progname(argv[0]);
- if (argc > 1)
- {
- if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
- {
- help(progname);
- exit_nicely(0);
- }
- if (strcmp(argv[1], "--version") == 0 || strcmp(argv[1], "-V") == 0)
- {
- puts("pg_dump (PostgreSQL) " PG_VERSION);
- exit_nicely(0);
- }
- }
-
InitDumpOptions(&dopt);
- while ((c = getopt_long(argc, argv, "abBcCd:E:f:F:h:j:n:N:oOp:RsS:t:T:U:vwWxZ:",
+ while ((c = getopt_long(argc, argv, "abBcCd:E:f:F:h:j:n:N:oOp:RsS:t:T:U:vVwWxZ:?",
long_options, &optindex)) != -1)
{
switch (c)
@@ -514,6 +500,11 @@ main(int argc, char **argv)
g_verbose = true;
break;
+ case 'V': /* version */
+ puts("pg_dump (PostgreSQL) " PG_VERSION);
+ exit_nicely(0);
+ break;
+
case 'w':
prompt_password = TRI_NO;
break;
@@ -535,6 +526,11 @@ main(int argc, char **argv)
}
break;
+ case '?': /* help */
+ help(progname);
+ exit_nicely(0);
+ break;
+
case 0:
/* This covers the long options. */
break;
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index eb29d318a4..7312a8c7ce 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -110,10 +110,12 @@ main(int argc, char *argv[])
{"tablespaces-only", no_argument, NULL, 't'},
{"username", required_argument, NULL, 'U'},
{"verbose", no_argument, NULL, 'v'},
+ {"version", no_argument, NULL, 'V'},
{"no-password", no_argument, NULL, 'w'},
{"password", no_argument, NULL, 'W'},
{"no-privileges", no_argument, NULL, 'x'},
{"no-acl", no_argument, NULL, 'x'},
+ {"help", no_argument, NULL, '?'},
/*
* the following options don't have an equivalent short option letter
@@ -165,20 +167,6 @@ main(int argc, char *argv[])
progname = get_progname(argv[0]);
- if (argc > 1)
- {
- if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
- {
- help();
- exit_nicely(0);
- }
- if (strcmp(argv[1], "--version") == 0 || strcmp(argv[1], "-V") == 0)
- {
- puts("pg_dumpall (PostgreSQL) " PG_VERSION);
- exit_nicely(0);
- }
- }
-
if ((ret = find_other_exec(argv[0], "pg_dump", PGDUMP_VERSIONSTR,
pg_dump_bin)) < 0)
{
@@ -205,7 +193,7 @@ main(int argc, char *argv[])
pgdumpopts = createPQExpBuffer();
- while ((c = getopt_long(argc, argv, "acd:E:f:gh:l:oOp:rsS:tU:vwWx", long_options, &optindex)) != -1)
+ while ((c = getopt_long(argc, argv, "acd:E:f:gh:l:oOp:rsS:tU:vVwWx?", long_options, &optindex)) != -1)
{
switch (c)
{
@@ -284,6 +272,11 @@ main(int argc, char *argv[])
appendPQExpBufferStr(pgdumpopts, " -v");
break;
+ case 'V':
+ puts("pg_dumpall (PostgreSQL) " PG_VERSION);
+ exit_nicely(0);
+ break;
+
case 'w':
prompt_password = TRI_NO;
appendPQExpBufferStr(pgdumpopts, " -w");
@@ -299,6 +292,11 @@ main(int argc, char *argv[])
appendPQExpBufferStr(pgdumpopts, " -x");
break;
+ case '?':
+ help();
+ exit_nicely(0);
+ break;
+
case 0:
break;
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index 501d7cea72..5735608b1b 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -106,7 +106,9 @@ main(int argc, char **argv)
{"use-list", 1, NULL, 'L'},
{"username", 1, NULL, 'U'},
{"verbose", 0, NULL, 'v'},
+ {"version", 0, NULL, 'V'},
{"single-transaction", 0, NULL, '1'},
+ {"help", 0, NULL, '?'},
/*
* the following options don't have an equivalent short option letter
@@ -136,21 +138,7 @@ main(int argc, char **argv)
progname = get_progname(argv[0]);
- if (argc > 1)
- {
- if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
- {
- usage(progname);
- exit_nicely(0);
- }
- if (strcmp(argv[1], "--version") == 0 || strcmp(argv[1], "-V") == 0)
- {
- puts("pg_restore (PostgreSQL) " PG_VERSION);
- exit_nicely(0);
- }
- }
-
- while ((c = getopt_long(argc, argv, "acCd:ef:F:h:I:j:lL:n:N:Op:P:RsS:t:T:U:vwWx1",
+ while ((c = getopt_long(argc, argv, "acCd:ef:F:h:I:j:lL:n:N:Op:P:RsS:t:T:U:vVwWx1?",
cmdopts, NULL)) != -1)
{
switch (c)
@@ -249,6 +237,11 @@ main(int argc, char **argv)
opts->verbose = 1;
break;
+ case 'V': /* version */
+ puts("pg_restore (PostgreSQL) " PG_VERSION);
+ exit_nicely(0);
+ break;
+
case 'w':
opts->promptPassword = TRI_NO;
break;
@@ -266,6 +259,11 @@ main(int argc, char **argv)
opts->exit_on_error = true;
break;
+ case '?': /* help */
+ usage(progname);
+ exit_nicely(0);
+ break;
+
case 0:
/*
diff --git a/src/bin/pg_resetwal/pg_resetwal.c b/src/bin/pg_resetwal/pg_resetwal.c
index d63a3a27f6..05114a2133 100644
--- a/src/bin/pg_resetwal/pg_resetwal.c
+++ b/src/bin/pg_resetwal/pg_resetwal.c
@@ -101,7 +101,9 @@ main(int argc, char *argv[])
{"dry-run", no_argument, NULL, 'n'},
{"next-oid", required_argument, NULL, 'o'},
{"multixact-offset", required_argument, NULL, 'O'},
+ {"version", no_argument, NULL, 'V'},
{"next-transaction-id", required_argument, NULL, 'x'},
+ {"help", no_argument, NULL, '?'},
{"wal-segsize", required_argument, NULL, 1},
{NULL, 0, NULL, 0}
};
@@ -120,22 +122,7 @@ main(int argc, char *argv[])
progname = get_progname(argv[0]);
- if (argc > 1)
- {
- if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
- {
- usage();
- exit(0);
- }
- if (strcmp(argv[1], "--version") == 0 || strcmp(argv[1], "-V") == 0)
- {
- puts("pg_resetwal (PostgreSQL) " PG_VERSION);
- exit(0);
- }
- }
-
-
- while ((c = getopt_long(argc, argv, "c:D:e:fl:m:no:O:x:", long_options, NULL)) != -1)
+ while ((c = getopt_long(argc, argv, "c:D:e:fl:m:no:O:Vx:?", long_options, NULL)) != -1)
{
switch (c)
{
@@ -293,6 +280,16 @@ main(int argc, char *argv[])
log_fname = pg_strdup(optarg);
break;
+ case 'V':
+ puts("pg_resetwal (PostgreSQL) " PG_VERSION);
+ exit(0);
+ break;
+
+ case '?':
+ usage();
+ exit(0);
+ break;
+
case 1:
set_wal_segsize = strtol(optarg, &endptr, 10) * 1024 * 1024;
if (endptr == optarg || *endptr != '\0')
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 9653106386..ad181d5b2c 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -115,27 +115,19 @@ main(int argc, char **argv)
progname = get_progname(argv[0]);
/* Process command-line arguments */
- if (argc > 1)
- {
- if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
- {
- usage(progname);
- exit(0);
- }
- if (strcmp(argv[1], "--version") == 0 || strcmp(argv[1], "-V") == 0)
- {
- puts("pg_rewind (PostgreSQL) " PG_VERSION);
- exit(0);
- }
- }
-
- while ((c = getopt_long(argc, argv, "D:nNP", long_options, &option_index)) != -1)
+ while ((c = getopt_long(argc, argv, "D:nNPV?", long_options, &option_index)) != -1)
{
switch (c)
{
case '?':
- fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
- exit(1);
+ usage(progname);
+ exit(0);
+ break;
+
+ case 'V':
+ puts("pg_rewind (PostgreSQL) " PG_VERSION);
+ exit(0);
+ break;
case 'P':
showprogress = true;
diff --git a/src/bin/pg_test_fsync/pg_test_fsync.c b/src/bin/pg_test_fsync/pg_test_fsync.c
index e6f7ef8557..50e8ad50ce 100644
--- a/src/bin/pg_test_fsync/pg_test_fsync.c
+++ b/src/bin/pg_test_fsync/pg_test_fsync.c
@@ -141,27 +141,15 @@ handle_args(int argc, char *argv[])
static struct option long_options[] = {
{"filename", required_argument, NULL, 'f'},
{"secs-per-test", required_argument, NULL, 's'},
+ {"version", no_argument, NULL, 'V'},
+ {"help", no_argument, NULL, '?'},
{NULL, 0, NULL, 0}
};
int option; /* Command line option */
int optindex = 0; /* used by getopt_long */
- if (argc > 1)
- {
- if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
- {
- printf(_("Usage: %s [-f FILENAME] [-s SECS-PER-TEST]\n"), progname);
- exit(0);
- }
- if (strcmp(argv[1], "--version") == 0 || strcmp(argv[1], "-V") == 0)
- {
- puts("pg_test_fsync (PostgreSQL) " PG_VERSION);
- exit(0);
- }
- }
-
- while ((option = getopt_long(argc, argv, "f:s:",
+ while ((option = getopt_long(argc, argv, "f:s:V?",
long_options, &optindex)) != -1)
{
switch (option)
@@ -174,6 +162,16 @@ handle_args(int argc, char *argv[])
secs_per_test = atoi(optarg);
break;
+ case 'V':
+ puts("pg_test_fsync (PostgreSQL) " PG_VERSION);
+ exit(0);
+ break;
+
+ case '?':
+ printf(_("Usage: %s [-f FILENAME] [-s SECS-PER-TEST]\n"), progname);
+ exit(0);
+ break;
+
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
progname);
diff --git a/src/bin/pg_test_timing/pg_test_timing.c b/src/bin/pg_test_timing/pg_test_timing.c
index 6e2fd1ab8c..4e24e8a599 100644
--- a/src/bin/pg_test_timing/pg_test_timing.c
+++ b/src/bin/pg_test_timing/pg_test_timing.c
@@ -42,27 +42,15 @@ handle_args(int argc, char *argv[])
{
static struct option long_options[] = {
{"duration", required_argument, NULL, 'd'},
+ {"version", no_argument, NULL, 'V'},
+ {"help", no_argument, NULL, '?'},
{NULL, 0, NULL, 0}
};
int option; /* Command line option */
int optindex = 0; /* used by getopt_long */
- if (argc > 1)
- {
- if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
- {
- printf(_("Usage: %s [-d DURATION]\n"), progname);
- exit(0);
- }
- if (strcmp(argv[1], "--version") == 0 || strcmp(argv[1], "-V") == 0)
- {
- puts("pg_test_timing (PostgreSQL) " PG_VERSION);
- exit(0);
- }
- }
-
- while ((option = getopt_long(argc, argv, "d:",
+ while ((option = getopt_long(argc, argv, "d:V?",
long_options, &optindex)) != -1)
{
switch (option)
@@ -71,6 +59,16 @@ handle_args(int argc, char *argv[])
test_duration = atoi(optarg);
break;
+ case 'V':
+ puts("pg_test_timing (PostgreSQL) " PG_VERSION);
+ exit(0);
+ break;
+
+ case '?':
+ printf(_("Usage: %s [-d DURATION]\n"), progname);
+ exit(0);
+ break;
+
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
progname);
diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c
index 9dbc9225a6..30a6831e2a 100644
--- a/src/bin/pg_upgrade/option.c
+++ b/src/bin/pg_upgrade/option.c
@@ -53,6 +53,8 @@ parseCommandLine(int argc, char *argv[])
{"retain", no_argument, NULL, 'r'},
{"jobs", required_argument, NULL, 'j'},
{"verbose", no_argument, NULL, 'v'},
+ {"version", no_argument, NULL, 'V'},
+ {"help", no_argument, NULL, '?'},
{NULL, 0, NULL, 0}
};
int option; /* Command line option */
@@ -100,7 +102,7 @@ parseCommandLine(int argc, char *argv[])
if ((log_opts.internal = fopen_priv(INTERNAL_LOG_FILE, "a")) == NULL)
pg_fatal("could not write to log file \"%s\"\n", INTERNAL_LOG_FILE);
- while ((option = getopt_long(argc, argv, "d:D:b:B:cj:ko:O:p:P:rU:v",
+ while ((option = getopt_long(argc, argv, "d:D:b:B:cj:ko:O:p:P:rU:vV?",
long_options, &optindex)) != -1)
{
switch (option)
@@ -203,6 +205,16 @@ parseCommandLine(int argc, char *argv[])
log_opts.verbose = true;
break;
+ case 'V':
+ puts("pg_upgrade (PostgreSQL) " PG_VERSION);
+ exit(0);
+ break;
+
+ case '?':
+ usage();
+ exit(0);
+ break;
+
default:
pg_fatal("Try \"%s --help\" for more information.\n",
os_info.progname);
diff --git a/src/bin/pg_verify_checksums/pg_verify_checksums.c b/src/bin/pg_verify_checksums/pg_verify_checksums.c
index 28c975446e..ca45cc126e 100644
--- a/src/bin/pg_verify_checksums/pg_verify_checksums.c
+++ b/src/bin/pg_verify_checksums/pg_verify_checksums.c
@@ -208,6 +208,8 @@ main(int argc, char *argv[])
{
static struct option long_options[] = {
{"pgdata", required_argument, NULL, 'D'},
+ {"help", no_argument, NULL, '?'},
+ {"version", no_argument, NULL, 'V'},
{NULL, 0, NULL, 0}
};
@@ -220,21 +222,7 @@ main(int argc, char *argv[])
progname = get_progname(argv[0]);
- if (argc > 1)
- {
- if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
- {
- usage();
- exit(0);
- }
- if (strcmp(argv[1], "--version") == 0 || strcmp(argv[1], "-V") == 0)
- {
- puts("pg_verify_checksums (PostgreSQL) " PG_VERSION);
- exit(0);
- }
- }
-
- while ((c = getopt_long(argc, argv, "D:r:d", long_options, &option_index)) != -1)
+ while ((c = getopt_long(argc, argv, "dD:r:V?", long_options, &option_index)) != -1)
{
switch (c)
{
@@ -252,6 +240,14 @@ main(int argc, char *argv[])
}
only_relfilenode = pstrdup(optarg);
break;
+ case 'V':
+ puts("pg_verify_checksums (PostgreSQL) " PG_VERSION);
+ exit(0);
+ break;
+ case '?':
+ usage();
+ exit(0);
+ break;
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit(1);
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 41b756c089..d1ff85a677 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -4763,6 +4763,7 @@ main(int argc, char **argv)
{"define", required_argument, NULL, 'D'},
{"file", required_argument, NULL, 'f'},
{"fillfactor", required_argument, NULL, 'F'},
+ {"help", no_argument, NULL, '?'},
{"host", required_argument, NULL, 'h'},
{"initialize", no_argument, NULL, 'i'},
{"init-steps", required_argument, NULL, 'I'},
@@ -4783,6 +4784,7 @@ main(int argc, char **argv)
{"transactions", required_argument, NULL, 't'},
{"username", required_argument, NULL, 'U'},
{"vacuum-all", no_argument, NULL, 'v'},
+ {"version", no_argument, NULL, 'V'},
/* long-named only options */
{"unlogged-tables", no_argument, NULL, 1},
{"tablespace", required_argument, NULL, 2},
@@ -4832,20 +4834,6 @@ main(int argc, char **argv)
progname = get_progname(argv[0]);
- if (argc > 1)
- {
- if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
- {
- usage();
- exit(0);
- }
- if (strcmp(argv[1], "--version") == 0 || strcmp(argv[1], "-V") == 0)
- {
- puts("pgbench (PostgreSQL) " PG_VERSION);
- exit(0);
- }
- }
-
#ifdef WIN32
/* stderr is buffered on Win32. */
setvbuf(stderr, NULL, _IONBF, 0);
@@ -4868,7 +4856,7 @@ main(int argc, char **argv)
exit(1);
}
- while ((c = getopt_long(argc, argv, "iI:h:nvp:dqb:SNc:j:Crs:t:T:U:lf:D:F:M:P:R:L:", long_options, &optindex)) != -1)
+ while ((c = getopt_long(argc, argv, "iI:h:nvp:dqb:SNc:j:Crs:t:T:U:lf:D:F:M:P:R:L:V?", long_options, &optindex)) != -1)
{
char *script;
@@ -5097,6 +5085,14 @@ main(int argc, char **argv)
latency_limit = (int64) (limit_ms * 1000);
}
break;
+ case 'V':
+ puts("pgbench (PostgreSQL) " PG_VERSION);
+ exit(0);
+ break;
+ case '?':
+ usage();
+ exit(0);
+ break;
case 1: /* unlogged-tables */
initialization_option_set = true;
unlogged_tables = true;
Agreed. I have changed handling of the --help and --version options in all apps
where it exhibits the problem described, with the exception for pg_archivecleanup
where getopt is used instead of getopt_long. The separate patch will be proposed
to address it.The patch is against current master. All tests pass.
I doubt that -V & -? are heavily tested:-) Patch works for me, though.
There seems to be other instances as well:
./src/interfaces/ecpg/preproc/ecpg.c: while ((c = getopt_long(argc, argv, "vcio:I:tD:dC:r:h", ecpg_options, NULL)) != -1)
./src/bin/scripts/clusterdb.c: while ((c = getopt_long(argc, argv, "h:p:U:wWeqd:at:v", long_options, &optindex)) != -1)
./src/bin/scripts/createdb.c: while ((c = getopt_long(argc, argv, "h:p:U:wWeO:D:T:E:l:", long_options, &optindex)) != -1)
./src/bin/scripts/dropuser.c: while ((c = getopt_long(argc, argv, "h:p:U:wWei", long_options, &optindex)) != -1)
./src/bin/scripts/pg_isready.c: while ((c = getopt_long(argc, argv, "d:h:p:qt:U:", long_options, NULL)) != -1)
./src/bin/scripts/dropdb.c: while ((c = getopt_long(argc, argv, "h:p:U:wWei", long_options, &optindex)) != -1)
./src/bin/scripts/vacuumdb.c: while ((c = getopt_long(argc, argv, "h:p:U:wWeqd:zZFat:fvj:", long_options, &optindex)) != -1)
./src/bin/scripts/createuser.c: while ((c = getopt_long(argc, argv, "h:p:U:g:wWedDsSaArRiIlLc:PE",
./src/bin/scripts/reindexdb.c: while ((c = getopt_long(argc, argv, "h:p:U:wWeqS:d:ast:i:v", long_options, &optindex)) != -1)
./src/interfaces/ecpg/preproc/ecpg.c: if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
./src/timezone/zic.c: else if (strcmp(argv[k], "--help") == 0)
./src/backend/main/main.c: if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
./src/bin/pg_archivecleanup/pg_archivecleanup.c: if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
./src/bin/pg_upgrade/option.c: if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
./src/bin/pg_ctl/pg_ctl.c: if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
./src/bin/psql/startup.c: if ((strcmp(argv[1], "-?") == 0) || (argc == 2 && (strcmp(argv[1], "--help") == 0)))
./src/bin/scripts/common.c: if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
=> implementation shared by many C "scripts".
./src/bin/pg_config/pg_config.c: if (strcmp(argv[i], "--help") == 0 || strcmp(argv[i], "-?") == 0)
./contrib/oid2name/oid2name.c: if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
./contrib/pg_standby/pg_standby.c: if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
./contrib/vacuumlo/vacuumlo.c: if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
--
Fabien.
Fabien COELHO <coelho@cri.ensmp.fr> writes:
There seems to be other instances as well
Thanks! I made the same modification of src/interfaces/ecpg/preproc/ecpg.c, but
in other cases it's either not a problem (as with src/bin/psql/startup.c or
src/timezone/zic.c) or the solution is too complex to be added to the current
patch: e.g. these tools in src/bin/scripts use handle_help_version_opts function
from src/bin/scripts/common.c which cannot be refactored so straightforward.
It's possible to remove it and modify each tool to parse the --help and
--version args for themselves but I would not include it in the same patch as
it's already not too short for a "fix" patch and these changes are better to be
discussed separately IMHO. Do you think the handle_help_version_opts function
should be removed and these args should be parsed in each src/bin/scripts app?
There are several cases where separate comparisons of argv[1] are made to detect
"--help" or "--version" before non-root user is enforced (to make it possible to
the root user to check the version) e.g. src/bin/pg_upgrade/option.c — in this
case I left this comparisons untouched while expanding the switch statement of
getopt_long, so non-root user sees the correct behavior and root still sees
"cannot be run as root" error when trying # pg_upgrade -v --help. The
alternative is to wrap these argv[...] comparisons in a for statement to
iterate through all the arguments. Another option is to enforce non-root after
getopt_long parsing but it's harder to be sure that the patch does not alter
the apps behavior unexpected way.
There are also the few apps when getopt is used instead of getopt_long, so I
decided not to fix these in the current patch because it's not so obvious. It's
possible either to replace getopt with getopt_long (and add long options, which
may be nice) or wrap --help/--version parsing in a for loop.
--
Andrei
Attachments:
help-version-args-parsing-v2.patchtext/x-patch; name=help-version-args-parsing-v2.patchDownload
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 3f203c6ca6..7a23c0b1ab 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -3130,23 +3130,9 @@ main(int argc, char *argv[])
progname = get_progname(argv[0]);
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("initdb"));
- if (argc > 1)
- {
- if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
- {
- usage(progname);
- exit(0);
- }
- if (strcmp(argv[1], "--version") == 0 || strcmp(argv[1], "-V") == 0)
- {
- puts("initdb (PostgreSQL) " PG_VERSION);
- exit(0);
- }
- }
-
/* process command-line options */
- while ((c = getopt_long(argc, argv, "dD:E:kL:nNU:WA:sST:X:g", long_options, &option_index)) != -1)
+ while ((c = getopt_long(argc, argv, "dD:E:kL:nNU:WA:sST:X:gV?", long_options, &option_index)) != -1)
{
switch (c)
{
@@ -3243,6 +3229,14 @@ main(int argc, char *argv[])
case 'g':
SetDataDirectoryCreatePerm(PG_DIR_MODE_GROUP);
break;
+ case 'V':
+ puts("initdb (PostgreSQL) " PG_VERSION);
+ exit(0);
+ break;
+ case '?':
+ usage(progname);
+ exit(0);
+ break;
default:
/* getopt_long already emitted a complaint */
fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index ef4cfc4384..03f7875514 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -2169,24 +2169,9 @@ main(int argc, char **argv)
progname = get_progname(argv[0]);
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_basebackup"));
- if (argc > 1)
- {
- if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
- {
- usage();
- exit(0);
- }
- else if (strcmp(argv[1], "-V") == 0
- || strcmp(argv[1], "--version") == 0)
- {
- puts("pg_basebackup (PostgreSQL) " PG_VERSION);
- exit(0);
- }
- }
-
atexit(cleanup_directories_atexit);
- while ((c = getopt_long(argc, argv, "CD:F:r:RS:T:X:l:nNzZ:d:c:h:p:U:s:wWkvP",
+ while ((c = getopt_long(argc, argv, "CD:F:r:RS:T:X:l:nNzZ:d:c:h:p:U:s:wWkvPV?",
long_options, &option_index)) != -1)
{
switch (c)
@@ -2328,6 +2313,14 @@ main(int argc, char **argv)
case 'P':
showprogress = true;
break;
+ case 'V':
+ puts("pg_basebackup (PostgreSQL) " PG_VERSION);
+ exit(0);
+ break;
+ case '?':
+ usage();
+ exit(0);
+ break;
case 3:
verify_checksums = false;
break;
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index 8be8d48a8a..5318454d67 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -509,22 +509,7 @@ main(int argc, char **argv)
progname = get_progname(argv[0]);
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_basebackup"));
- if (argc > 1)
- {
- if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
- {
- usage();
- exit(0);
- }
- else if (strcmp(argv[1], "-V") == 0 ||
- strcmp(argv[1], "--version") == 0)
- {
- puts("pg_receivewal (PostgreSQL) " PG_VERSION);
- exit(0);
- }
- }
-
- while ((c = getopt_long(argc, argv, "D:d:E:h:p:U:s:S:nwWvZ:",
+ while ((c = getopt_long(argc, argv, "D:d:E:h:p:U:s:S:nwWvVZ:?",
long_options, &option_index)) != -1)
{
switch (c)
@@ -584,6 +569,10 @@ main(int argc, char **argv)
case 'v':
verbose++;
break;
+ case 'V':
+ puts("pg_receivewal (PostgreSQL) " PG_VERSION);
+ exit(0);
+ break;
case 'Z':
compresslevel = atoi(optarg);
if (compresslevel < 0 || compresslevel > 9)
@@ -593,6 +582,10 @@ main(int argc, char **argv)
exit(1);
}
break;
+ case '?':
+ usage();
+ exit(0);
+ break;
/* action */
case 1:
do_create_slot = true;
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index ef85c9af4c..170e7475eb 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -711,22 +711,7 @@ main(int argc, char **argv)
progname = get_progname(argv[0]);
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_basebackup"));
- if (argc > 1)
- {
- if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
- {
- usage();
- exit(0);
- }
- else if (strcmp(argv[1], "-V") == 0 ||
- strcmp(argv[1], "--version") == 0)
- {
- puts("pg_recvlogical (PostgreSQL) " PG_VERSION);
- exit(0);
- }
- }
-
- while ((c = getopt_long(argc, argv, "E:f:F:nvd:h:p:U:wWI:o:P:s:S:",
+ while ((c = getopt_long(argc, argv, "E:f:F:nvd:h:p:U:wWI:o:P:s:S:V?",
long_options, &option_index)) != -1)
{
switch (c)
@@ -750,6 +735,14 @@ main(int argc, char **argv)
case 'v':
verbose++;
break;
+ case 'V':
+ puts("pg_recvlogical (PostgreSQL) " PG_VERSION);
+ exit(0);
+ break;
+ case '?':
+ usage();
+ exit(0);
+ break;
/* connection options */
case 'd':
dbname = pg_strdup(optarg);
diff --git a/src/bin/pg_controldata/pg_controldata.c b/src/bin/pg_controldata/pg_controldata.c
index 895a51f89d..9c8ba27764 100644
--- a/src/bin/pg_controldata/pg_controldata.c
+++ b/src/bin/pg_controldata/pg_controldata.c
@@ -88,6 +88,8 @@ main(int argc, char *argv[])
{
static struct option long_options[] = {
{"pgdata", required_argument, NULL, 'D'},
+ {"version", no_argument, NULL, 'V'},
+ {"help", no_argument, NULL, '?'},
{NULL, 0, NULL, 0}
};
@@ -110,21 +112,7 @@ main(int argc, char *argv[])
progname = get_progname(argv[0]);
- if (argc > 1)
- {
- if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
- {
- usage(progname);
- exit(0);
- }
- if (strcmp(argv[1], "--version") == 0 || strcmp(argv[1], "-V") == 0)
- {
- puts("pg_controldata (PostgreSQL) " PG_VERSION);
- exit(0);
- }
- }
-
- while ((c = getopt_long(argc, argv, "D:", long_options, NULL)) != -1)
+ while ((c = getopt_long(argc, argv, "D:V?", long_options, NULL)) != -1)
{
switch (c)
{
@@ -132,6 +120,16 @@ main(int argc, char *argv[])
DataDir = optarg;
break;
+ case 'V':
+ puts("pg_controldata (PostgreSQL) " PG_VERSION);
+ exit(0);
+ break;
+
+ case '?':
+ usage(progname);
+ exit(0);
+ break;
+
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit(1);
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index ed2396aa6c..8779cb6a66 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -2220,7 +2220,7 @@ main(int argc, char **argv)
/* process command-line options */
while (optind < argc)
{
- while ((c = getopt_long(argc, argv, "cD:e:l:m:N:o:p:P:sS:t:U:wW",
+ while ((c = getopt_long(argc, argv, "cD:e:l:m:N:o:p:P:sS:t:U:VwW?",
long_options, &option_index)) != -1)
{
switch (c)
@@ -2296,12 +2296,20 @@ main(int argc, char **argv)
/* Prepend .\ for local accounts */
register_username = psprintf(".\\%s", optarg);
break;
+ case 'V':
+ puts("pg_ctl (PostgreSQL) " PG_VERSION);
+ exit(0);
+ break;
case 'w':
do_wait = true;
break;
case 'W':
do_wait = false;
break;
+ case '?':
+ do_help();
+ exit(0);
+ break;
case 'c':
allow_core_files = true;
break;
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 86524d6598..0f961bf9a6 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -399,23 +399,9 @@ main(int argc, char **argv)
progname = get_progname(argv[0]);
- if (argc > 1)
- {
- if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
- {
- help(progname);
- exit_nicely(0);
- }
- if (strcmp(argv[1], "--version") == 0 || strcmp(argv[1], "-V") == 0)
- {
- puts("pg_dump (PostgreSQL) " PG_VERSION);
- exit_nicely(0);
- }
- }
-
InitDumpOptions(&dopt);
- while ((c = getopt_long(argc, argv, "abBcCd:E:f:F:h:j:n:N:oOp:RsS:t:T:U:vwWxZ:",
+ while ((c = getopt_long(argc, argv, "abBcCd:E:f:F:h:j:n:N:oOp:RsS:t:T:U:vVwWxZ:?",
long_options, &optindex)) != -1)
{
switch (c)
@@ -514,6 +500,11 @@ main(int argc, char **argv)
g_verbose = true;
break;
+ case 'V': /* version */
+ puts("pg_dump (PostgreSQL) " PG_VERSION);
+ exit_nicely(0);
+ break;
+
case 'w':
prompt_password = TRI_NO;
break;
@@ -535,6 +526,11 @@ main(int argc, char **argv)
}
break;
+ case '?': /* help */
+ help(progname);
+ exit_nicely(0);
+ break;
+
case 0:
/* This covers the long options. */
break;
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index eb29d318a4..7312a8c7ce 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -110,10 +110,12 @@ main(int argc, char *argv[])
{"tablespaces-only", no_argument, NULL, 't'},
{"username", required_argument, NULL, 'U'},
{"verbose", no_argument, NULL, 'v'},
+ {"version", no_argument, NULL, 'V'},
{"no-password", no_argument, NULL, 'w'},
{"password", no_argument, NULL, 'W'},
{"no-privileges", no_argument, NULL, 'x'},
{"no-acl", no_argument, NULL, 'x'},
+ {"help", no_argument, NULL, '?'},
/*
* the following options don't have an equivalent short option letter
@@ -165,20 +167,6 @@ main(int argc, char *argv[])
progname = get_progname(argv[0]);
- if (argc > 1)
- {
- if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
- {
- help();
- exit_nicely(0);
- }
- if (strcmp(argv[1], "--version") == 0 || strcmp(argv[1], "-V") == 0)
- {
- puts("pg_dumpall (PostgreSQL) " PG_VERSION);
- exit_nicely(0);
- }
- }
-
if ((ret = find_other_exec(argv[0], "pg_dump", PGDUMP_VERSIONSTR,
pg_dump_bin)) < 0)
{
@@ -205,7 +193,7 @@ main(int argc, char *argv[])
pgdumpopts = createPQExpBuffer();
- while ((c = getopt_long(argc, argv, "acd:E:f:gh:l:oOp:rsS:tU:vwWx", long_options, &optindex)) != -1)
+ while ((c = getopt_long(argc, argv, "acd:E:f:gh:l:oOp:rsS:tU:vVwWx?", long_options, &optindex)) != -1)
{
switch (c)
{
@@ -284,6 +272,11 @@ main(int argc, char *argv[])
appendPQExpBufferStr(pgdumpopts, " -v");
break;
+ case 'V':
+ puts("pg_dumpall (PostgreSQL) " PG_VERSION);
+ exit_nicely(0);
+ break;
+
case 'w':
prompt_password = TRI_NO;
appendPQExpBufferStr(pgdumpopts, " -w");
@@ -299,6 +292,11 @@ main(int argc, char *argv[])
appendPQExpBufferStr(pgdumpopts, " -x");
break;
+ case '?':
+ help();
+ exit_nicely(0);
+ break;
+
case 0:
break;
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index 501d7cea72..5735608b1b 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -106,7 +106,9 @@ main(int argc, char **argv)
{"use-list", 1, NULL, 'L'},
{"username", 1, NULL, 'U'},
{"verbose", 0, NULL, 'v'},
+ {"version", 0, NULL, 'V'},
{"single-transaction", 0, NULL, '1'},
+ {"help", 0, NULL, '?'},
/*
* the following options don't have an equivalent short option letter
@@ -136,21 +138,7 @@ main(int argc, char **argv)
progname = get_progname(argv[0]);
- if (argc > 1)
- {
- if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
- {
- usage(progname);
- exit_nicely(0);
- }
- if (strcmp(argv[1], "--version") == 0 || strcmp(argv[1], "-V") == 0)
- {
- puts("pg_restore (PostgreSQL) " PG_VERSION);
- exit_nicely(0);
- }
- }
-
- while ((c = getopt_long(argc, argv, "acCd:ef:F:h:I:j:lL:n:N:Op:P:RsS:t:T:U:vwWx1",
+ while ((c = getopt_long(argc, argv, "acCd:ef:F:h:I:j:lL:n:N:Op:P:RsS:t:T:U:vVwWx1?",
cmdopts, NULL)) != -1)
{
switch (c)
@@ -249,6 +237,11 @@ main(int argc, char **argv)
opts->verbose = 1;
break;
+ case 'V': /* version */
+ puts("pg_restore (PostgreSQL) " PG_VERSION);
+ exit_nicely(0);
+ break;
+
case 'w':
opts->promptPassword = TRI_NO;
break;
@@ -266,6 +259,11 @@ main(int argc, char **argv)
opts->exit_on_error = true;
break;
+ case '?': /* help */
+ usage(progname);
+ exit_nicely(0);
+ break;
+
case 0:
/*
diff --git a/src/bin/pg_resetwal/pg_resetwal.c b/src/bin/pg_resetwal/pg_resetwal.c
index d63a3a27f6..05114a2133 100644
--- a/src/bin/pg_resetwal/pg_resetwal.c
+++ b/src/bin/pg_resetwal/pg_resetwal.c
@@ -101,7 +101,9 @@ main(int argc, char *argv[])
{"dry-run", no_argument, NULL, 'n'},
{"next-oid", required_argument, NULL, 'o'},
{"multixact-offset", required_argument, NULL, 'O'},
+ {"version", no_argument, NULL, 'V'},
{"next-transaction-id", required_argument, NULL, 'x'},
+ {"help", no_argument, NULL, '?'},
{"wal-segsize", required_argument, NULL, 1},
{NULL, 0, NULL, 0}
};
@@ -120,22 +122,7 @@ main(int argc, char *argv[])
progname = get_progname(argv[0]);
- if (argc > 1)
- {
- if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
- {
- usage();
- exit(0);
- }
- if (strcmp(argv[1], "--version") == 0 || strcmp(argv[1], "-V") == 0)
- {
- puts("pg_resetwal (PostgreSQL) " PG_VERSION);
- exit(0);
- }
- }
-
-
- while ((c = getopt_long(argc, argv, "c:D:e:fl:m:no:O:x:", long_options, NULL)) != -1)
+ while ((c = getopt_long(argc, argv, "c:D:e:fl:m:no:O:Vx:?", long_options, NULL)) != -1)
{
switch (c)
{
@@ -293,6 +280,16 @@ main(int argc, char *argv[])
log_fname = pg_strdup(optarg);
break;
+ case 'V':
+ puts("pg_resetwal (PostgreSQL) " PG_VERSION);
+ exit(0);
+ break;
+
+ case '?':
+ usage();
+ exit(0);
+ break;
+
case 1:
set_wal_segsize = strtol(optarg, &endptr, 10) * 1024 * 1024;
if (endptr == optarg || *endptr != '\0')
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 9653106386..ad181d5b2c 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -115,27 +115,19 @@ main(int argc, char **argv)
progname = get_progname(argv[0]);
/* Process command-line arguments */
- if (argc > 1)
- {
- if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
- {
- usage(progname);
- exit(0);
- }
- if (strcmp(argv[1], "--version") == 0 || strcmp(argv[1], "-V") == 0)
- {
- puts("pg_rewind (PostgreSQL) " PG_VERSION);
- exit(0);
- }
- }
-
- while ((c = getopt_long(argc, argv, "D:nNP", long_options, &option_index)) != -1)
+ while ((c = getopt_long(argc, argv, "D:nNPV?", long_options, &option_index)) != -1)
{
switch (c)
{
case '?':
- fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
- exit(1);
+ usage(progname);
+ exit(0);
+ break;
+
+ case 'V':
+ puts("pg_rewind (PostgreSQL) " PG_VERSION);
+ exit(0);
+ break;
case 'P':
showprogress = true;
diff --git a/src/bin/pg_test_fsync/pg_test_fsync.c b/src/bin/pg_test_fsync/pg_test_fsync.c
index e6f7ef8557..50e8ad50ce 100644
--- a/src/bin/pg_test_fsync/pg_test_fsync.c
+++ b/src/bin/pg_test_fsync/pg_test_fsync.c
@@ -141,27 +141,15 @@ handle_args(int argc, char *argv[])
static struct option long_options[] = {
{"filename", required_argument, NULL, 'f'},
{"secs-per-test", required_argument, NULL, 's'},
+ {"version", no_argument, NULL, 'V'},
+ {"help", no_argument, NULL, '?'},
{NULL, 0, NULL, 0}
};
int option; /* Command line option */
int optindex = 0; /* used by getopt_long */
- if (argc > 1)
- {
- if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
- {
- printf(_("Usage: %s [-f FILENAME] [-s SECS-PER-TEST]\n"), progname);
- exit(0);
- }
- if (strcmp(argv[1], "--version") == 0 || strcmp(argv[1], "-V") == 0)
- {
- puts("pg_test_fsync (PostgreSQL) " PG_VERSION);
- exit(0);
- }
- }
-
- while ((option = getopt_long(argc, argv, "f:s:",
+ while ((option = getopt_long(argc, argv, "f:s:V?",
long_options, &optindex)) != -1)
{
switch (option)
@@ -174,6 +162,16 @@ handle_args(int argc, char *argv[])
secs_per_test = atoi(optarg);
break;
+ case 'V':
+ puts("pg_test_fsync (PostgreSQL) " PG_VERSION);
+ exit(0);
+ break;
+
+ case '?':
+ printf(_("Usage: %s [-f FILENAME] [-s SECS-PER-TEST]\n"), progname);
+ exit(0);
+ break;
+
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
progname);
diff --git a/src/bin/pg_test_timing/pg_test_timing.c b/src/bin/pg_test_timing/pg_test_timing.c
index 6e2fd1ab8c..4e24e8a599 100644
--- a/src/bin/pg_test_timing/pg_test_timing.c
+++ b/src/bin/pg_test_timing/pg_test_timing.c
@@ -42,27 +42,15 @@ handle_args(int argc, char *argv[])
{
static struct option long_options[] = {
{"duration", required_argument, NULL, 'd'},
+ {"version", no_argument, NULL, 'V'},
+ {"help", no_argument, NULL, '?'},
{NULL, 0, NULL, 0}
};
int option; /* Command line option */
int optindex = 0; /* used by getopt_long */
- if (argc > 1)
- {
- if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
- {
- printf(_("Usage: %s [-d DURATION]\n"), progname);
- exit(0);
- }
- if (strcmp(argv[1], "--version") == 0 || strcmp(argv[1], "-V") == 0)
- {
- puts("pg_test_timing (PostgreSQL) " PG_VERSION);
- exit(0);
- }
- }
-
- while ((option = getopt_long(argc, argv, "d:",
+ while ((option = getopt_long(argc, argv, "d:V?",
long_options, &optindex)) != -1)
{
switch (option)
@@ -71,6 +59,16 @@ handle_args(int argc, char *argv[])
test_duration = atoi(optarg);
break;
+ case 'V':
+ puts("pg_test_timing (PostgreSQL) " PG_VERSION);
+ exit(0);
+ break;
+
+ case '?':
+ printf(_("Usage: %s [-d DURATION]\n"), progname);
+ exit(0);
+ break;
+
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
progname);
diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c
index 9dbc9225a6..30a6831e2a 100644
--- a/src/bin/pg_upgrade/option.c
+++ b/src/bin/pg_upgrade/option.c
@@ -53,6 +53,8 @@ parseCommandLine(int argc, char *argv[])
{"retain", no_argument, NULL, 'r'},
{"jobs", required_argument, NULL, 'j'},
{"verbose", no_argument, NULL, 'v'},
+ {"version", no_argument, NULL, 'V'},
+ {"help", no_argument, NULL, '?'},
{NULL, 0, NULL, 0}
};
int option; /* Command line option */
@@ -100,7 +102,7 @@ parseCommandLine(int argc, char *argv[])
if ((log_opts.internal = fopen_priv(INTERNAL_LOG_FILE, "a")) == NULL)
pg_fatal("could not write to log file \"%s\"\n", INTERNAL_LOG_FILE);
- while ((option = getopt_long(argc, argv, "d:D:b:B:cj:ko:O:p:P:rU:v",
+ while ((option = getopt_long(argc, argv, "d:D:b:B:cj:ko:O:p:P:rU:vV?",
long_options, &optindex)) != -1)
{
switch (option)
@@ -203,6 +205,16 @@ parseCommandLine(int argc, char *argv[])
log_opts.verbose = true;
break;
+ case 'V':
+ puts("pg_upgrade (PostgreSQL) " PG_VERSION);
+ exit(0);
+ break;
+
+ case '?':
+ usage();
+ exit(0);
+ break;
+
default:
pg_fatal("Try \"%s --help\" for more information.\n",
os_info.progname);
diff --git a/src/bin/pg_verify_checksums/pg_verify_checksums.c b/src/bin/pg_verify_checksums/pg_verify_checksums.c
index 28c975446e..ca45cc126e 100644
--- a/src/bin/pg_verify_checksums/pg_verify_checksums.c
+++ b/src/bin/pg_verify_checksums/pg_verify_checksums.c
@@ -208,6 +208,8 @@ main(int argc, char *argv[])
{
static struct option long_options[] = {
{"pgdata", required_argument, NULL, 'D'},
+ {"help", no_argument, NULL, '?'},
+ {"version", no_argument, NULL, 'V'},
{NULL, 0, NULL, 0}
};
@@ -220,21 +222,7 @@ main(int argc, char *argv[])
progname = get_progname(argv[0]);
- if (argc > 1)
- {
- if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
- {
- usage();
- exit(0);
- }
- if (strcmp(argv[1], "--version") == 0 || strcmp(argv[1], "-V") == 0)
- {
- puts("pg_verify_checksums (PostgreSQL) " PG_VERSION);
- exit(0);
- }
- }
-
- while ((c = getopt_long(argc, argv, "D:r:d", long_options, &option_index)) != -1)
+ while ((c = getopt_long(argc, argv, "dD:r:V?", long_options, &option_index)) != -1)
{
switch (c)
{
@@ -252,6 +240,14 @@ main(int argc, char *argv[])
}
only_relfilenode = pstrdup(optarg);
break;
+ case 'V':
+ puts("pg_verify_checksums (PostgreSQL) " PG_VERSION);
+ exit(0);
+ break;
+ case '?':
+ usage();
+ exit(0);
+ break;
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit(1);
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 41b756c089..d1ff85a677 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -4763,6 +4763,7 @@ main(int argc, char **argv)
{"define", required_argument, NULL, 'D'},
{"file", required_argument, NULL, 'f'},
{"fillfactor", required_argument, NULL, 'F'},
+ {"help", no_argument, NULL, '?'},
{"host", required_argument, NULL, 'h'},
{"initialize", no_argument, NULL, 'i'},
{"init-steps", required_argument, NULL, 'I'},
@@ -4783,6 +4784,7 @@ main(int argc, char **argv)
{"transactions", required_argument, NULL, 't'},
{"username", required_argument, NULL, 'U'},
{"vacuum-all", no_argument, NULL, 'v'},
+ {"version", no_argument, NULL, 'V'},
/* long-named only options */
{"unlogged-tables", no_argument, NULL, 1},
{"tablespace", required_argument, NULL, 2},
@@ -4832,20 +4834,6 @@ main(int argc, char **argv)
progname = get_progname(argv[0]);
- if (argc > 1)
- {
- if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
- {
- usage();
- exit(0);
- }
- if (strcmp(argv[1], "--version") == 0 || strcmp(argv[1], "-V") == 0)
- {
- puts("pgbench (PostgreSQL) " PG_VERSION);
- exit(0);
- }
- }
-
#ifdef WIN32
/* stderr is buffered on Win32. */
setvbuf(stderr, NULL, _IONBF, 0);
@@ -4868,7 +4856,7 @@ main(int argc, char **argv)
exit(1);
}
- while ((c = getopt_long(argc, argv, "iI:h:nvp:dqb:SNc:j:Crs:t:T:U:lf:D:F:M:P:R:L:", long_options, &optindex)) != -1)
+ while ((c = getopt_long(argc, argv, "iI:h:nvp:dqb:SNc:j:Crs:t:T:U:lf:D:F:M:P:R:L:V?", long_options, &optindex)) != -1)
{
char *script;
@@ -5097,6 +5085,14 @@ main(int argc, char **argv)
latency_limit = (int64) (limit_ms * 1000);
}
break;
+ case 'V':
+ puts("pgbench (PostgreSQL) " PG_VERSION);
+ exit(0);
+ break;
+ case '?':
+ usage();
+ exit(0);
+ break;
case 1: /* unlogged-tables */
initialization_option_set = true;
unlogged_tables = true;
diff --git a/src/interfaces/ecpg/preproc/ecpg.c b/src/interfaces/ecpg/preproc/ecpg.c
index 9a94e39545..a91df891bf 100644
--- a/src/interfaces/ecpg/preproc/ecpg.c
+++ b/src/interfaces/ecpg/preproc/ecpg.c
@@ -116,6 +116,8 @@ int
main(int argc, char *const argv[])
{
static struct option ecpg_options[] = {
+ {"version", no_argument, NULL, 'V'},
+ {"help", no_argument, NULL, '?'},
{"regression", no_argument, NULL, ECPG_GETOPT_LONG_REGRESSION},
{NULL, 0, NULL, 0}
};
@@ -140,22 +142,8 @@ main(int argc, char *const argv[])
return ILLEGAL_OPTION;
}
- if (argc > 1)
- {
- if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
- {
- help(progname);
- exit(0);
- }
- if (strcmp(argv[1], "--version") == 0 || strcmp(argv[1], "-V") == 0)
- {
- printf("ecpg %s\n", PG_VERSION);
- exit(0);
- }
- }
-
output_filename = NULL;
- while ((c = getopt_long(argc, argv, "vcio:I:tD:dC:r:h", ecpg_options, NULL)) != -1)
+ while ((c = getopt_long(argc, argv, "vcio:I:tD:dC:r:hV?", ecpg_options, NULL)) != -1)
{
switch (c)
{
@@ -242,6 +230,14 @@ main(int argc, char *const argv[])
progname);
#endif
break;
+ case 'V':
+ printf("ecpg %s\n", PG_VERSION);
+ exit(0);
+ break;
+ case '?':
+ help(progname);
+ exit(0);
+ break;
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), argv[0]);
return ILLEGAL_OPTION;
Hello
My 0.02€:
Thanks! I made the same modification of src/interfaces/ecpg/preproc/ecpg.c, but
in other cases it's either not a problem (as with src/bin/psql/startup.c or
src/timezone/zic.c) or the solution is too complex to be added to the current
patch: e.g. these tools in src/bin/scripts use handle_help_version_opts function
from src/bin/scripts/common.c which cannot be refactored so straightforward.
I think this function could be simply removed: it is just calling its
third argument on help, all printing the version with the command name on
version.
It's possible to remove it and modify each tool to parse the --help and
--version args for themselves but I would not include it in the same patch as
it's already not too short for a "fix" patch and these changes are better to be
discussed separately IMHO. Do you think the handle_help_version_opts function
should be removed and these args should be parsed in each src/bin/scripts app?
Given the contents of "handle_help_version_opts", I see no issue with
managing an update in the same patch as other commands, if this is to be
done.
There are several cases where separate comparisons of argv[1] are made to detect
"--help" or "--version" before non-root user is enforced (to make it possible to
the root user to check the version) e.g. src/bin/pg_upgrade/option.c
— in this case I left this comparisons untouched while expanding the
switch statement of getopt_long, so non-root user sees the correct
behavior and root still sees "cannot be run as root" error when trying #
pg_upgrade -v --help.
This seems reasonable.
The alternative is to wrap these argv[...] comparisons in a for
statement to iterate through all the arguments. Another option is to
enforce non-root after getopt_long parsing but it's harder to be sure
that the patch does not alter the apps behavior unexpected way.
Personnaly I would not bother that a must-not-be-root command does not
process its options when called as root, but people may object on this
one.
There are also the few apps when getopt is used instead of getopt_long, so I
decided not to fix these in the current patch because it's not so obvious. It's
possible either to replace getopt with getopt_long (and add long options, which
may be nice) or wrap --help/--version parsing in a for loop.
I'd go for homogeneity.
About the patch.
Some commands take care to manage their option struct and/or switch cases
more or less in alphabetical order (with errors, eg dbname/d in pg_dumpall
is misplaced), and that you strive to be consistent with that. You missed
on some cases though: pg_test_fsync, pg_test_timing, ecpg.
For some commands (initdb, pg_basebackup, pg_receivewal...), I see that
?/V are already in the option struct but the option management is missing
from the switch, so this is also fixing minor bugs.
You have changed the behavior in some commands: eg -? in pg_rewind used to
point to --help, now it directly provide said help. I'm fine with that.
For commands which do not use getopt_long, probably the change belongs to
another patch.
--
Fabien.
On Sun, Jul 22, 2018 at 10:19:50AM -0400, Fabien COELHO wrote:
Agreed. I have changed handling of the --help and --version options in all apps
where it exhibits the problem described, with the exception for pg_archivecleanup
where getopt is used instead of getopt_long. The separate patch will be proposed
to address it.The patch is against current master. All tests pass.
I doubt that -V & -? are heavily tested:-) Patch works for me, though.
They are not, and the patch misses this area.
I don't think that it is a bad idea to improve things the way you are
doing, however you should extend program_version_ok() and
program_help_ok() in src/test/perl/TestLib.pm so as short options are
tested for two reasons:
1) We can make sure that everything is consistent and works properly
easily without testing manually your patch, which is a pain for anybody
looking at the patch.
2) Any new standalone binary added in the core tree would be able to
check natively if it handles common options correctly with TAP tests
added.
This will need tweaks for the number of tests in a couple of TAP files,
but that's worth the shot in the long term.
--
Michael
Hello Michaï¿œl,
I doubt that -V & -? are heavily tested:-) Patch works for me, though.
They are not, and the patch misses this area.
Indeed.
I don't think that it is a bad idea to improve things the way you are
For the record, this is not my patch, I'm merely reviewing it.
doing, however you should extend program_version_ok() and
program_help_ok() in src/test/perl/TestLib.pm so as short options are
tested for two reasons:
Interesting, I did not notice these functions before. I fully agree that
manual testing is a pain for such a simple change.
Do you mean something like the attached?
--
Fabien.
Attachments:
testlib-help-version-1.patchtext/plain; name=testlib-help-version-1.patchDownload
diff --git a/src/bin/pgbench/t/002_pgbench_no_server.pl b/src/bin/pgbench/t/002_pgbench_no_server.pl
index a2845a583b..24ecfec33e 100644
--- a/src/bin/pgbench/t/002_pgbench_no_server.pl
+++ b/src/bin/pgbench/t/002_pgbench_no_server.pl
@@ -56,6 +56,10 @@ sub pgbench_scripts
return;
}
+# help and version sanity checks
+program_help_ok("pgbench");
+program_version_ok("pgbench");
+
#
# Option various errors
#
@@ -205,7 +209,7 @@ pgbench(
'pgbench help');
# Version
-pgbench('-V', 0, [qr{^pgbench .PostgreSQL. }], [qr{^$}], 'pgbench version');
+pgbench('-V', 0, [qr{^pgbench \(PostgreSQL\) \d+}], [qr{^$}], 'pgbench version');
# list of builtins
pgbench(
diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index 3a184a4fad..7799157335 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -409,13 +409,25 @@ sub program_help_ok
{
local $Test::Builder::Level = $Test::Builder::Level + 1;
my ($cmd) = @_;
- my ($stdout, $stderr);
+ my ($stdout, $stdout2, $stderr, $stderr2);
print("# Running: $cmd --help\n");
my $result = IPC::Run::run [ $cmd, '--help' ], '>', \$stdout, '2>',
\$stderr;
+ my $result2 = IPC::Run::run [ $cmd, '-?' ], '>', \$stdout2, '2>',
+ \$stderr2;
ok($result, "$cmd --help exit code 0");
+ ok($result2, "$cmd -? exit code 0");
isnt($stdout, '', "$cmd --help goes to stdout");
+ is($stdout, $stdout2, "$cmd --help and -? have same output");
+ like($stdout, qr{Usage:}, "$cmd --help is about usage");
+ like($stdout, qr{\b$cmd\b}, "$cmd --help is about $cmd");
+ like($stdout, qr{--help}, "$cmd --help is about option --help");
+ like($stdout, qr{-\?}, "$cmd --help is about options -?");
+ like($stdout, qr{--version}, "$cmd --help is about options --version");
+ like($stdout, qr{-V}, "$cmd --help is about options -V");
+ # more sanity check on the output? eg Report bug to ..., options:...
is($stderr, '', "$cmd --help nothing to stderr");
+ is($stderr2, '', "$cmd -? nothing to stderr");
return;
}
@@ -423,13 +435,19 @@ sub program_version_ok
{
local $Test::Builder::Level = $Test::Builder::Level + 1;
my ($cmd) = @_;
- my ($stdout, $stderr);
+ my ($stdout, $stdout2, $stderr, $stderr2);
print("# Running: $cmd --version\n");
my $result = IPC::Run::run [ $cmd, '--version' ], '>', \$stdout, '2>',
\$stderr;
+ my $result2 = IPC::Run::run [ $cmd, '-V' ], '>', \$stdout2, '2>',
+ \$stderr2;
ok($result, "$cmd --version exit code 0");
+ ok($result2, "$cmd -V exit code 0");
isnt($stdout, '', "$cmd --version goes to stdout");
+ is($stdout, $stdout2, "$cmd --version and -V have same output");
+ like($stdout, qr{^$cmd \(PostgreSQL\) \d+(devel|\.\d+)\b}, "$cmd --version looks like a version");
is($stderr, '', "$cmd --version nothing to stderr");
+ is($stderr2, '', "$cmd -V nothing to stderr");
return;
}
On Mon, Jul 23, 2018 at 07:47:44AM -0400, Fabien COELHO wrote:
I don't think that it is a bad idea to improve things the way you are
For the record, this is not my patch, I'm merely reviewing it.
Of course, any input is welcome. It is nice to see that you took some
time to look at the patch.
doing, however you should extend program_version_ok() and
program_help_ok() in src/test/perl/TestLib.pm so as short options are
tested for two reasons:Interesting, I did not notice these functions before. I fully agree that
manual testing is a pain for such a simple change.Do you mean something like the attached?
You basically have the idea, except that the number of tests in any TAP
files calling program_help_ok and program_version_ok needs to be
updated, and that the test is too verbose :)
# Version -pgbench('-V', 0, [qr{^pgbench .PostgreSQL. }], [qr{^$}], 'pgbench version'); +pgbench('-V', 0, [qr{^pgbench \(PostgreSQL\) \d+}], [qr{^$}], 'pgbench version');
This could go away.
+ is($stdout, $stdout2, "$cmd --help and -? have same output"); + like($stdout, qr{Usage:}, "$cmd --help is about usage"); + like($stdout, qr{\b$cmd\b}, "$cmd --help is about $cmd"); + like($stdout, qr{--help}, "$cmd --help is about option --help"); + like($stdout, qr{-\?}, "$cmd --help is about options -?"); + like($stdout, qr{--version}, "$cmd --help is about options --version"); + like($stdout, qr{-V}, "$cmd --help is about options -V");
I would keep things a bit more simple by not parsing the output as you
do, and it would be possible to reduce that to one single test with a
text block as well, say using qq().
Andrei, could you update your patch in this area please? That will ease
reviews and checks. Double-checking that the documentation gets the
correct call would not hurt as well.
--
Michael
Hello Michaï¿œl,
Do you mean something like the attached?
You basically have the idea, except that the number of tests in any TAP
files calling program_help_ok and program_version_ok
Indeed. I wanted to outline the perl module part, really.
needs to be updated, and that the test is too verbose :)
# Version -pgbench('-V', 0, [qr{^pgbench .PostgreSQL. }], [qr{^$}], 'pgbench version'); +pgbench('-V', 0, [qr{^pgbench \(PostgreSQL\) \d+}], [qr{^$}], 'pgbench version');This could go away.
Indeed.
+ is($stdout, $stdout2, "$cmd --help and -? have same output"); + like($stdout, qr{Usage:}, "$cmd --help is about usage"); + like($stdout, qr{\b$cmd\b}, "$cmd --help is about $cmd"); + like($stdout, qr{--help}, "$cmd --help is about option --help"); + like($stdout, qr{-\?}, "$cmd --help is about options -?"); + like($stdout, qr{--version}, "$cmd --help is about options --version"); + like($stdout, qr{-V}, "$cmd --help is about options -V");I would keep things a bit more simple by not parsing the output as you
do, and it would be possible to reduce that to one single test with a
text block as well, say using qq().
I do not understand what you mean by "reduce that to one single test". I
cannot see how to test stdouts equalities and contents' sanity in one
single test.
--
Fabien.
On Thu, Jul 26, 2018 at 06:22:16PM -0400, Fabien COELHO wrote:
I do not understand what you mean by "reduce that to one single test". I
cannot see how to test stdouts equalities and contents' sanity in one single
test.
I was wondering about reducing the number of tests to a strict minimum.
At the end I think that I would just remove most of the tests you are
adding here, they seem rather noisy..
--
Michael
I do not understand what you mean by "reduce that to one single test". I
cannot see how to test stdouts equalities and contents' sanity in one single
test.I was wondering about reducing the number of tests to a strict minimum.
At the end I think that I would just remove most of the tests you are
adding here, they seem rather noisy..
Ok, if you remove tests, it can be done in less tests, obviously.
ISTM that some minimal sanity on the output would be useful. If some
minimal common format is assumed, one re could check that the name of the
program appears, and a few options are indeed describe. That could make
one "like" test.
--
Fabien.
On Thu, Jul 26, 2018 at 06:42:02PM -0400, Fabien COELHO wrote:
Ok, if you remove tests, it can be done in less tests, obviously.
Well, if we can find a way to reduce the number of tests but not their
coverage, that's always welcome.
ISTM that some minimal sanity on the output would be useful. If some minimal
common format is assumed, one re could check that the name of the program
appears, and a few options are indeed describe. That could make one "like"
test.
Let's see the patch author's take on the matter then.
--
Michael
Hello.
Sorry for not getting into this but I'm looking forward to this.
(I'm tired to insert '--help' to the top of argument list..)
This patch still applies to the current master but ecpg.c.
At Sun, 22 Jul 2018 16:13:20 -0400 (EDT), Fabien COELHO <coelho@cri.ensmp.fr> wrote in <alpine.DEB.2.21.1807221540080.13768@lancre>
Hello
My 0.02€:
Thanks! I made the same modification of
src/interfaces/ecpg/preproc/ecpg.c, but
in other cases it's either not a problem (as with
src/bin/psql/startup.c or
src/timezone/zic.c) or the solution is too complex to be added to the
current
patch: e.g. these tools in src/bin/scripts use
handle_help_version_opts function
from src/bin/scripts/common.c which cannot be refactored so
straightforward.I think this function could be simply removed: it is just calling its
third argument on help, all printing the version with the command name
on version.
Still harmonized version string is useful. We can remove the
function simply and add print_version_string(const char
*fixed_progname) instead.
It's possible to remove it and modify each tool to parse the --help
and
--version args for themselves but I would not include it in the same
--patch as
it's already not too short for a "fix" patch and these changes are
better to be
discussed separately IMHO. Do you think the handle_help_version_opts
function
should be removed and these args should be parsed in each
src/bin/scripts app?Given the contents of "handle_help_version_opts", I see no issue with
managing an update in the same patch as other commands, if this is to
be done.
+1.
There are several cases where separate comparisons of argv[1] are made
to detect
"--help" or "--version" before non-root user is enforced (to make it
possible to
the root user to check the version) e.g. src/bin/pg_upgrade/option.c
— in this case I left this comparisons untouched while expanding the
switch statement of getopt_long, so non-root user sees the correct
behavior and root still sees "cannot be run as root" error when trying
# pg_upgrade -v --help.This seems reasonable.
The alternative is to wrap these argv[...] comparisons in a for
statement to iterate through all the arguments. Another option is to
enforce non-root after getopt_long parsing but it's harder to be sure
that the patch does not alter the apps behavior unexpected way.Personnaly I would not bother that a must-not-be-root command does not
process its options when called as root, but people may object on this
one.
I forgot the detailed discussion for the code but looking with a
fresh eyes, the only thing we should avoid there is creating a
log file with superuser privilege. All other stuff in the getopt
loop is harmless. So just inhibiting root to make the internal
log file before the loop, then exiting if root after it would
work. Some error messages can be emitted to stderr but it doesn't
matter.
There are also the few apps when getopt is used instead of
getopt_long, so I
decided not to fix these in the current patch because it's not so
obvious. It's
possible either to replace getopt with getopt_long (and add long
options, which
may be nice) or wrap --help/--version parsing in a for loop.I'd go for homogeneity.
+1.
About the patch.
Some commands take care to manage their option struct and/or switch
cases more or less in alphabetical order (with errors, eg dbname/d in
pg_dumpall is misplaced), and that you strive to be consistent with
that. You missed
on some cases though: pg_test_fsync, pg_test_timing, ecpg.For some commands (initdb, pg_basebackup, pg_receivewal...), I see
that ?/V are already in the option struct but the option management is
missing from the switch, so this is also fixing minor bugs.You have changed the behavior in some commands: eg -? in pg_rewind
used to point to --help, now it directly provide said help. I'm fine
with that.
Mmm. This is related to getopt's specification. getopt_long
returns '?' for ambiguous match or extraneous parameter. So any
wrong parameter or even --help let the program to show only "try
--help" and we no longer have a means to see the true help
message without the change. In other words pg_rewind's (or all
our programs using -? as the synonum of --help) original behavior
can be incompatible with getopt's behavior. We should redefine
the behavior for -?, --help and unknown options respectively
basing getopts's behavior. We can identify --help using
option_index returnd from getopt_long if it were not at the
beginning of long_options:p
For commands which do not use getopt_long, probably the change belongs
to another patch.
Agreed.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center