Hi, Michael

Thanks for reviewing the patch.

<#secure method=pgpmime mode=sign>
On Wed, 22 Jul 2026 at 10:47, Michael Paquier <[email protected]> wrote:
> On Tue, Jul 21, 2026 at 06:16:27PM +0800, Japin Li wrote:
>> +  place = EMSG;
>> +  optind++;
>> +
>>    if (optstring[0] == ':')
>>        return BADARG;
>>  
>> @@ -153,9 +156,6 @@ retry:
>>                "%s: option requires an argument -- %s\n",
>>                argv[0], place);
>>  
>> -  place = EMSG;
>> -  optind++;
>> -
>>    if (has_arg == required_argument)
>>        return BADCH;
>>    optarg = NULL;
>
> This also means that for long options we have an error message would
> now use an empty string instead of an option name all the time.  That
> looks incorrect.

Right.  I was mistaken about the option name.  Fixed in v2-0002.

>
> You are claiming that this makes the short option path more
> consistent.  That's true based on what your patch does, but it also
> looks to me like the short option area is already wrong in setting
> EMSG before we issue the error string "option requires an argument".
> So, to me, you are making the situation worse in some cases while
> claiming that it improves the situation in some other cases.
>
> Our implementation of getopt_long() is only used on Windows for MSVC,
> as far as I know.  I'd also suggest to post one or more examples of
> how this changes the implementation behavior.  You should be able to
> force your way through easily so as the Postgres getopt_long()
> implementation is used with a quick hack, just to prove your point,
> saving you the pain of deploying a WIN32 host..  (Like use a
> pg_getopt_long() and patch one of the binaries, whatever you prefer.)

Here's a mini‑demo for getopt_long() in v2‑0001. Without v2‑0002, the output is:

    $ ./getopt_long_test --log-filename
    option './getopt_long_test' requires an argument
    unrecognized option './getopt_long_test'
    unrecognized option './getopt_long_test'
    unrecognized option './getopt_long_test'
    unrecognized option './getopt_long_test'
    $ ./getopt_long_test -f
    option '-f' requires an argument

With v2‑0002 applied, the output is:

    $ ./getopt_long_test --log-filename
    option '--log-filename' requires an argument
    $ ./getopt_long_test -f
    option '-f' requires an argument

> --
> Michael

-- 
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.


Reply via email to