public inbox for [email protected]
help / color / mirror / Atom feedFrom: Nathan Bossart <[email protected]>
To: Kyotaro Horiguchi <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Subject: Re: add non-option reordering to in-tree getopt_long
Date: Fri, 16 Jun 2023 11:28:47 -0700
Message-ID: <20230616182847.GA71216@nathanxps13> (raw)
In-Reply-To: <[email protected]>
References: <20230616000959.GA922959@nathanxps13>
<[email protected]>
<20230616045828.GA972663@nathanxps13>
<[email protected]>
On Fri, Jun 16, 2023 at 04:51:38PM +0900, Kyotaro Horiguchi wrote:
> (Honestly, the rearrangement code looks somewhat tricky to grasp..)
Yeah, I think there's still some room for improvement here.
> It doesn't work properly if '--' is involved.
>
> For example, consider the following options (even though they don't
> work for the command).
>
> psql -t -T hoho -a hoge -- -1 hage hige huge
>
> After the function returns -1, the argv array looks like this. The
> "[..]" indicates the position of optind.
>
> psql -t -T hoho -a -- [-1] hage hige huge hoge
>
> This is clearly incorrect since "hoge" gets moved to the end. The
> rearrangement logic needs to take into account the '--'. For your
> information, the glibc version yields the following result for the
> same options.
>
> psql -t -T hoho -a -- [hoge] -1 hage hige huge
Ah, so it effectively retains the non-option ordering, even if there is a
'--'. I think this behavior is worth keeping. I attempted to fix this in
the attached patch.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
[text/x-diff] v4-0001-Teach-in-tree-getopt_long-to-move-non-options-to-.patch (4.1K, ../20230616182847.GA71216@nathanxps13/2-v4-0001-Teach-in-tree-getopt_long-to-move-non-options-to-.patch)
download | inline diff:
From 51e1033374c464ed90484f641d31b0ab705672f2 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Fri, 9 Jun 2023 15:51:58 -0700
Subject: [PATCH v4 1/1] Teach in-tree getopt_long() to move non-options to the
end of argv.
---
src/bin/scripts/t/040_createuser.pl | 10 +++---
src/port/getopt_long.c | 50 +++++++++++++++++++++--------
2 files changed, 42 insertions(+), 18 deletions(-)
diff --git a/src/bin/scripts/t/040_createuser.pl b/src/bin/scripts/t/040_createuser.pl
index 9ca282181d..ba40ab11a3 100644
--- a/src/bin/scripts/t/040_createuser.pl
+++ b/src/bin/scripts/t/040_createuser.pl
@@ -42,9 +42,9 @@ $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 +73,13 @@ $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 c989276988..a4bdc6c8f0 100644
--- a/src/port/getopt_long.c
+++ b/src/port/getopt_long.c
@@ -50,8 +50,9 @@
* 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.)
+ *
+ * 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 +61,59 @@ 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 */
+retry:
if (optind >= argc)
{
place = EMSG;
+ nonopt_start = -1;
+ force_nonopt = false;
return -1;
}
place = argv[optind];
- if (place[0] != '-')
+ /* non-options, including '-' and anything after '--' */
+ if (place[0] != '-' || place[1] == '\0' || force_nonopt)
{
- place = EMSG;
- return -1;
- }
+ char **args = (char **) argv;
- place++;
+ /*
+ * If only non-options remain, return -1. Else, move the
+ * non-option to the end and try again.
+ */
+ if (optind == nonopt_start)
+ {
+ place = EMSG;
+ nonopt_start = -1;
+ force_nonopt = false;
+ return -1;
+ }
- if (!*place)
- {
- /* treat "-" as not being an option */
- place = EMSG;
- return -1;
+ for (int i = optind; i < argc - 1; i++)
+ args[i] = args[i + 1];
+ args[argc - 1] = place;
+
+ if (nonopt_start == -1)
+ nonopt_start = argc - 1;
+ else
+ nonopt_start--;
+
+ 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])
--
2.25.1
view thread (8+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: add non-option reordering to in-tree getopt_long
In-Reply-To: <20230616182847.GA71216@nathanxps13>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox