> On 24 Jul 2019, at 22:32, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
>
> On 2019-07-23 17:30, Daniel Gustafsson wrote:
>> The reason for moving is that we print default values in usage(), and that
>> requires the value to be computed before calling usage(). We already do this
>> for resolving environment values in parseCommandLine(). If we do it in setup,
>> then we’d have to split out resolving the new_cluster.bindir into it’s own
>> function exposed to option.c, or do you have any other suggestions there?
>
> I think doing nontrivial work in order to print default values in
> usage() is bad practice, because in unfortunate cases it would even
> prevent you from calling --help. Also, in this case, it would probably
> very often exceed the typical line length of --help output and create
> some general ugliness. Writing something like "(default: same as this
> pg_upgrade)" would probably achieve just about the same.
Fair enough, those are both excellent points. I’ve shuffled the code around to
move back the check for exec_path to setup (albeit earlier than before due to
where we perform directory checking). This does mean that the directory
checking in the options parsing must learn to cope with missing directories,
which is a bit unfortunate since it’s already doing a few too many things IMHO.
To ensure dogfooding, I also removed the use of -B in ‘make check’ for
pg_upgrade, which should bump the coverage.
Also spotted a typo in a pg_upgrade file header in a file touched by this, so
included it in this thread too as a 0004.
Thanks again for reviewing, much appreciated!
cheers ./daniel
