Re: PATCH: add "--config-file=" option to pg_rewind

Am 10.03.22 um 14:43 schrieb Michael Banck:
> Heya,
> 
> some minor comments, I didn't look at the added test and I did not test
> the patch at all, but (as part of the Debian/Ubuntu packaging team) I
> think this patch is really important:

Much appreciated!

[...]

> I think we usually just say "data directory"; pgdata was previously only
> used as part of the option name, not like this in the text.

Fixed.

[...]

> target-pgdata is the name of the option, but maybe it is useful to spell
> out "target data directory" here, even if that means we spill over to
> two lines (which we might have to do anyway, that new line is pretty
> long).

Fixed.

> 
>> @@ -121,6 +123,7 @@ main(int argc, char **argv)
>>          {"no-sync", no_argument, NULL, 'N'},
>>          {"progress", no_argument, NULL, 'P'},
>>          {"debug", no_argument, NULL, 3},
>> +        {"config-file", required_argument, NULL, 5},
> 
> Not sure what our policy is, but the way the numbered options are 1, 2,
> 4, 3, 5 after this is weird; this was already off before this patch
> though.

That one has been triggering my inner Monk too ;-)

Fixed!


[...]

> You removed an empty line here. Maybe rather prepend this with a comment
> on what's going on, and have en empty line before and afterwards.

> Kinde same here.

It does smell a bit like "stating the obvious", but alas! Both fixed.

Cheers,
-- 
Gunnar "Nick" Bluth

Eimermacherweg 106
D-48159 Münster

Mobil +49 172 8853339
Email: gunnar.bluth@pro-open.de
__________________________________________________________________________
"Ceterum censeo SystemD esse delendam" - Cato