Re: pgbench: improve --help and --version parsing - Mailing list pgsql-hackers

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.