Re: [BUG] getopts OPTIND

[dana <dana@xxxxxxx>]

On 14 Apr 2021, at 08:08, Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx> wrote:
> Should the descriptions of OPTIND and/or POSIX_BUILTINS in the manual be
> extended as well?

The latter, yes; done that. The former, idk, it doesn't currently mention
anything about how it's calculated or about POSIX_BUILTINS effects so i'm
inclined to leave it alone

On 14 Apr 2021, at 08:08, Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx> wrote:
> Some more cases to test:
> .
>       t 0
>       t 1 foo
>       t 1 -- foo
>       t 1 -b
> .
> where -b doesn't take an argument.

`t 0` doesn't test anything, the loop is just skipped. `t 1 -b` tests the same
thing as `t 1 -w`, but i guess it's confusing that i picked -a -w -e -r as the
options; i've changed them to -a -b -c -d. I've also added the two foo ones
you suggested, as well as just `t 1`. (All three behaved the same as other
shells already)

dana


diff --git a/Doc/Zsh/builtins.yo b/Doc/Zsh/builtins.yo
index a7afe42cf..4b9778e40 100644
--- a/Doc/Zsh/builtins.yo
+++ b/Doc/Zsh/builtins.yo
@@ -982,7 +982,8 @@ vindex(OPTARG, use of)
 The first option to be examined may be changed by explicitly assigning
 to tt(OPTIND).  tt(OPTIND) has an initial value of tt(1), and is
 normally set to tt(1) upon entry to a shell function and restored
-upon exit (this is disabled by the tt(POSIX_BUILTINS) option).  tt(OPTARG)
+upon exit.  (The tt(POSIX_BUILTINS) option disables this, and also changes
+the way the value is calculated to match other shells).  tt(OPTARG)
 is not reset and retains its value from the most recent call to
 tt(getopts).  If either of tt(OPTIND) or tt(OPTARG) is explicitly
 unset, it remains unset, and the index or option argument is not
diff --git a/Doc/Zsh/options.yo b/Doc/Zsh/options.yo
index 714e8a1a1..ffe2d1a0d 100644
--- a/Doc/Zsh/options.yo
+++ b/Doc/Zsh/options.yo
@@ -2239,7 +2239,8 @@ command found in the path.
 
 Furthermore, the tt(getopts) builtin behaves in a POSIX-compatible
 fashion in that the associated variable tt(OPTIND) is not made
-local to functions.
+local to functions, and its value is calculated differently to match
+other shells.
 
 Moreover, the warning and special exit code from
 tt([[ -o )var(non_existent_option)tt( ]]) are suppressed.
diff --git a/Src/builtin.c b/Src/builtin.c
index 26335a2e8..13dfdf8be 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -5556,6 +5556,11 @@ bin_getopts(UNUSED(char *name), char **argv, UNUSED(Options ops), UNUSED(int fun
     /* check for legality */
     if(opch == ':' || !(p = memchr(optstr, opch, lenoptstr))) {
 	p = "?";
+	/* Keep OPTIND correct if the user doesn't return after the error */
+	if (isset(POSIXBUILTINS)) {
+	    optcind = 0;
+	    zoptind++;
+	}
 	zsfree(zoptarg);
 	setsparam(var, ztrdup(p));
 	if(quiet) {
@@ -5572,6 +5577,11 @@ bin_getopts(UNUSED(char *name), char **argv, UNUSED(Options ops), UNUSED(int fun
     if(p[1] == ':') {
 	if(optcind == lenstr) {
 	    if(!args[zoptind]) {
+		/* Fix OPTIND as above */
+		if (isset(POSIXBUILTINS)) {
+		    optcind = 0;
+		    zoptind++;
+		}
 		zsfree(zoptarg);
 		if(quiet) {
 		    setsparam(var, ztrdup(":"));
diff --git a/Test/B10getopts.ztst b/Test/B10getopts.ztst
index 72c9e209e..e50d177c7 100644
--- a/Test/B10getopts.ztst
+++ b/Test/B10getopts.ztst
@@ -96,3 +96,32 @@
   done
 0:missing option-argument (quiet mode)
 >:,x
+
+  # This function is written so it can be easily referenced against other shells
+  t() {
+    local o i=0 n=$1
+    shift
+    while [ $i -lt $n ]; do
+      i=$(( i + 1 ))
+      getopts a: o "$@" 2> /dev/null
+    done
+    printf '<%d>' "$OPTIND"
+  }
+  # Try all these the native way, then the POSIX_BUILTINS way
+  for 1 in no_posix_builtins posix_builtins; do (
+    setopt $1
+    print -rn - "$1: "
+    t 1
+    t 1 foo
+    t 1 -- foo
+    t 1 -a
+    t 1 -b
+    t 2 -a -b
+    t 4 -a -b -c -d -a
+    t 5 -a -b -c -a -b -c
+    t 5 -a -b -c -d -ax -a
+    print
+  ); done
+0:OPTIND calculation with and without POSIX_BUILTINS (workers/42248)
+>no_posix_builtins: <1><1><2><1><1><3><5><7><6>
+>posix_builtins: <1><1><2><2><2><3><6><7><7>