Teach in-tree getopt_long() to move non-options to the end of argv.

Unlike the other implementations of getopt_long() I could find, the
in-tree implementation does not reorder non-options to the end of
argv.  Instead, it returns -1 as soon as the first non-option is
found, even if there are other options listed afterwards.  By
moving non-options to the end of argv, getopt_long() can parse all
specified options and return -1 when only non-options remain.
This quirk is periodically missed by hackers (e.g., 869aa40a27,
ffd398021c, and d9ddc50baf).  This commit introduces the
aforementioned non-option reordering behavior to the in-tree
getopt_long() implementation.

Special thanks to Noah Misch for his help verifying behavior on
AIX.

Reviewed-by: Kyotaro Horiguchi, Michael Paquier
Discussion: https://postgr.es/m/20230609232257.GA121461%40nathanxps13

diff --git a/src/bin/scripts/t/040_createuser.pl b/src/bin/scripts/t/040_createuser.pl
index 9ca282181d8c882e14bea852c4da00f905f45aba..3290e30c88f0b1efd5f4a7034cd5025029dc6c09 100644 (file)
--- a/src/bin/scripts/t/040_createuser.pl
+++ b/src/bin/scripts/t/040_createuser.pl
@@ -42,9 +42,8 @@ $node->issues_sql_like(
    'add a role as a member with admin option of the newly created role');
 $node->issues_sql_like(
    [
-       'createuser', '-m',
-       'regress_user3', '-m',
-       'regress user #4', 'REGRESS_USER5'
+       'createuser', 'REGRESS_USER5', '-m', 'regress_user3',
+       '-m', 'regress user #4'
    ],
    qr/statement: CREATE ROLE "REGRESS_USER5" NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS ROLE regress_user3,"regress user #4";/,
    'add a role as a member of the newly created role');
@@ -73,11 +72,14 @@ $node->issues_sql_like(
    qr/statement: CREATE ROLE regress_user11 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS IN ROLE regress_user1;/,
    '--role');
 $node->issues_sql_like(
-   [ 'createuser', '--member-of', 'regress_user1', 'regress_user12' ],
+   [ 'createuser', 'regress_user12', '--member-of', 'regress_user1' ],
    qr/statement: CREATE ROLE regress_user12 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS IN ROLE regress_user1;/,
    '--member-of');
 
 $node->command_fails([ 'createuser', 'regress_user1' ],
    'fails if role already exists');
+$node->command_fails(
+   [ 'createuser', 'regress_user1', '-m', 'regress_user2', 'regress_user3' ],
+   'fails for too many non-options');
 
 done_testing();

diff --git a/src/port/getopt_long.c b/src/port/getopt_long.c
index c9892769883a3237b128ead2318b47e06d09129c..f83de0dff97894647fff582753d71cb12ab1157d 100644 (file)
--- a/src/port/getopt_long.c
+++ b/src/port/getopt_long.c
@@ -50,8 +50,11 @@
  * This implementation does not use optreset.  Instead, we guarantee that
  * it can be restarted on a new argv array after a previous call returned -1,
  * if the caller resets optind to 1 before the first call of the new series.
- * (Internally, this means we must be sure to reset "place" to EMSG before
- * returning -1.)
+ * (Internally, this means we must be sure to reset "place" to EMSG,
+ * "nonopt_start" to -1, and "force_nonopt" to false before returning -1.)
+ *
+ * Note that this routine reorders the pointers in argv (despite the const
+ * qualifier) so that all non-options will be at the end when -1 is returned.
  */
 int
 getopt_long(int argc, char *const argv[],
@@ -60,38 +63,58 @@ getopt_long(int argc, char *const argv[],
 {
    static char *place = EMSG;  /* option letter processing */
    char       *oli;            /* option letter list index */
+   static int  nonopt_start = -1;
+   static bool force_nonopt = false;
 
    if (!*place)
    {                           /* update scanning pointer */
-       if (optind >= argc)
+       char      **args = (char **) argv;
+
+retry:
+
+       /*
+        * If we are out of arguments or only non-options remain, return -1.
+        */
+       if (optind >= argc || optind == nonopt_start)
        {
            place = EMSG;
+           nonopt_start = -1;
+           force_nonopt = false;
            return -1;
        }
 
        place = argv[optind];
 
-       if (place[0] != '-')
+       /*
+        * An argument is a non-option if it meets any of the following
+        * criteria: it follows an argument that is equivalent to the string
+        * "--", it does not start with '-', or it is equivalent to the string
+        * "-".  When we encounter a non-option, we move it to the end of argv
+        * (after shifting all remaining arguments over to make room), and
+        * then we try again with the next argument.
+        */
+       if (force_nonopt || place[0] != '-' || place[1] == '\0')
        {
-           place = EMSG;
-           return -1;
-       }
+           for (int i = optind; i < argc - 1; i++)
+               args[i] = args[i + 1];
+           args[argc - 1] = place;
 
-       place++;
+           if (nonopt_start == -1)
+               nonopt_start = argc - 1;
+           else
+               nonopt_start--;
 
-       if (!*place)
-       {
-           /* treat "-" as not being an option */
-           place = EMSG;
-           return -1;
+           goto retry;
        }
 
+       place++;
+
        if (place[0] == '-' && place[1] == '\0')
        {
            /* found "--", treat it as end of options */
            ++optind;
-           place = EMSG;
-           return -1;
+           force_nonopt = true;
+           goto retry;
        }
 
        if (place[0] == '-' && place[1])