Re: [PATCH] Fix getopt_long() argument handling and add tests

Hi all,

On Thu, Sep 10, 2026 at 11:46 AM Nathan Bossart
<nathandbossart(at)gmail(dot)com> wrote:
>
> On Sat, Aug 29, 2026 at 12:57:00PM -0400, Sehrope Sarkuni wrote:
> > While reworking the pg_waldump TAP test [1] I ran "pg_waldump --stats
> > --limit 5" and it failed on Windows CI with 'could not locate WAL file
> > "5"'. For a long option with optional_argument and no "=", the port
> > getopt_long() increments optind in the missing-argument branch and
> > again at the end of the long option path, so the next option is
> > skipped and its value becomes a positional argument. The same branch
> > returns BADARG when optstring starts with a colon although nothing is
> > missing.
> >
> > [...]
> >
> > 0003 fixes a second problem the tests turned up. The port moves each
> > non-option to the end of argv as soon as it sees it, so by the time a
> > following option looks for its argument, a non-option that came before
> > it on the command line is already sitting after it and gets taken:
> > "pg_amcheck mydb --jobs" parses as --jobs=mydb rather than reporting
> > the missing argument. The system getopt_long() reports the missing
> > argument. The fix bounds the argument search by the start of the
> > moved block. 0004 adds the test cases for it, kept separate in case
> > 0003 is backpatched without the test module.
>
> Nice finds. I've attached what I have staged for commit. I've tried to
> simplify the diffs a bit, and I also got rid of the new test module in
> favor of adding cases to existing test files. I don't think we need to
> bother back-patching 0001; the bug went undiscovered for ~23 years. I can
> try back-patching 0002 to v17, though.
>

I tested both v2 patches on the latest master and was able to
reproduce both reported issues with the in-tree getopt_long()
implementation before applying the patches with the following
observations:

--stats --limit 5 incorrectly skipped --limit and left 5 as a
positional argument.
mydb --jobs incorrectly consumed mydb as the argument to --jobs.

After applying both patches, both cases behaved correctly. And I ran
the affected TAP tests and everything got passed. Also I reviewed the
changes in src/port/getopt_long.c and the corresponding test
additions. I did not find any issues with the v2 patches.
The patch looks good to me.

Regards,
Solai