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.
