[PATCH] getopts: don't look for +o with posix_builtins, add -p

[dana <dana@xxxxxxx>]

currently when you give getopts a spec o it will take both -o and +o as
valid options. this behaviour is not specified by posix (or supported by
most other shells that aim for posix compliance), so it should be
disabled when posix_builtins is in effect

going even further, though -- i think the more posix-like behaviour is
actually what people want like 95% of the time. even in our own
functions, we rarely want or attempt to handle the + variants. but
having to toggle posix_builtins on and off for this one thing would be
tedious and error-prone

ksh93 addresses this (posix-compliantly) by only looking for + variants
when the opt spec begins with a +. we could do this as well, but it
would mean that you could no longer use + as an actual option letter,
which daniel once pointed out is a real thing people do

oliver suggested adding an option, -p for posix i guess, instead. this
is not a 100% non-breaking change either, since making getopts take its
own options means that you can no longer have an unguarded opt spec
beginning with a -. but that's not a useful thing to do so it's probably
fine

this also opens up the possibility of adding ksh93's -a option

dana


diff --git a/Doc/Zsh/builtins.yo b/Doc/Zsh/builtins.yo
index c63d66e0c..095bc5783 100644
--- a/Doc/Zsh/builtins.yo
+++ b/Doc/Zsh/builtins.yo
@@ -988,16 +988,16 @@ tt(read -zr).
 )
 findex(getopts)
 cindex(options, processing)
-item(tt(getopts) var(optstring) var(name) [ var(arg) ... ])(
+item(tt(getopts) [ tt(-p) ] var(optstring) var(name) [ var(arg) ... ])(
 Checks the var(arg)s for legal options.  If the var(arg)s are omitted,
-use the positional parameters.  A valid option argument
+the positional parameters are used.  A valid option argument
 begins with a `tt(PLUS())' or a `tt(-)'.  An argument not beginning with
 a `tt(PLUS())' or a `tt(-)', or the argument `tt(-)tt(-)', ends the options.
 Note that a single `tt(-)' is not considered a valid option argument.
 var(optstring) contains the letters that tt(getopts)
 recognizes.  If a letter is followed by a `tt(:)', that option
-requires an argument.  The options can be
-separated from the argument by blanks.
+requires an argument.  The argument may appear either immediately
+following the option in the same word, or in the next word.
 
 Each time it is invoked, tt(getopts) places the option letter it finds
 in the shell parameter var(name), prepended with a `tt(PLUS())' when
@@ -1007,15 +1007,17 @@ is stored in tt(OPTARG).
 vindex(OPTIND, use of)
 vindex(OPTARG, use of)
 
+When the terminator `tt(-)tt(-)' is encountered, tt(OPTIND) is
+incremented such that it can be tt(shift)ed away with the options.  This
+is not true of other non-option arguments, including a single `tt(-)'.
+
 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.  (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
-stored.  The option itself is still stored in var(name) in this case.
+upon exit.  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 stored.  The option itself is still stored in var(name) in this case.
 
 A leading `tt(:)' in var(optstring) causes tt(getopts) to store the
 letter of any invalid option in tt(OPTARG), and to set var(name) to
@@ -1023,6 +1025,14 @@ letter of any invalid option in tt(OPTARG), and to set var(name) to
 missing.  Otherwise, tt(getopts) sets var(name) to `tt(?)' and prints
 an error message when an option is invalid.  The exit status is
 nonzero when there are no more options.
+
+When either the tt(POSIX_BUILTINS) option is enabled or the tt(-p)
+option is given to tt(getopts) itself, tt(getopts) behaves in a more
+POSIX-compatible manner.  Specifically, handling of
+`tt(PLUS())'-prefixed options is disabled (they are treated as
+non-options) and the value of tt(OPTIND) is calculated differently.
+Additionally, with tt(POSIX_BUILTINS) (but not with tt(-p)), the value
+of tt(OPTIND) is not kept local to the calling function.
 )
 findex(hash)
 item(tt(hash) [ tt(-Ldfmrv) ] [ var(name)[tt(=)var(value)] ] ...)(
diff --git a/Doc/Zsh/options.yo b/Doc/Zsh/options.yo
index 77dfb3fdb..3e745ec3d 100644
--- a/Doc/Zsh/options.yo
+++ b/Doc/Zsh/options.yo
@@ -2255,13 +2255,12 @@ Furthermore, functions and shell builtins are not executed after
 an tt(exec) prefix; the command to be executed must be an external
 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, 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.
+
+Other builtins may be affected as described in
+ifzman(zmanref(zshbuiltins))\
+ifnzman(noderef(Shell Builtin Commands)).
 )
 pindex(POSIX_IDENTIFIERS)
 pindex(NO_POSIX_IDENTIFIERS)
diff --git a/NEWS b/NEWS
index 108762bbd..dbf7bef97 100644
--- a/NEWS
+++ b/NEWS
@@ -86,6 +86,9 @@ to use outside of completion contexts.
 
 The zsh/mathfunc module now provides isnan() and isinf() functions.
 
+The getopts builtin learnt a -p option to make it behave like the
+POSIX_BUILTINS option has been temporarily enabled.
+
 Changes since 5.8.1
 -------------------
 
diff --git a/README b/README
index 8d686a77c..1036663a4 100644
--- a/README
+++ b/README
@@ -149,6 +149,12 @@ Also, as a consequence of the zparseopts builtin now using standard
 argument parsing for its own options, long-option specs must be guarded
 using -- or similar.
 
+The getopts builtin no longer interprets arguments beginning with a +
+as potential options during parsing when the POSIX_BUILTINS option is
+enabled.  Additionally, as a consequence of learning the related -p
+option, an opt spec beginning with a hyphen must be guarded by - or --.
+(Note that the effect of a hyphen in the opt spec is unspecified.)
+
 Incompatibilities between 5.8.1 and 5.9
 ---------------------------------------
 
diff --git a/Src/builtin.c b/Src/builtin.c
index 4b11aed60..266fcf924 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -76,7 +76,7 @@ static struct builtin builtins[] =
     BUILTIN("float", BINF_PLUSOPTS | BINF_MAGICEQUALS | BINF_PSPECIAL | BINF_ASSIGN, (HandlerFunc)bin_typeset, 0, -1, 0, "E:%F:%HL:%R:%Z:%ghlp:%rtux", "E"),
     BUILTIN("functions", BINF_PLUSOPTS, bin_functions, 0, -1, 0, "ckmMstTuUWx:z", NULL),
     BUILTIN("getln", 0, bin_read, 0, -1, 0, "ecnAlE", "zr"),
-    BUILTIN("getopts", 0, bin_getopts, 2, -1, 0, NULL, NULL),
+    BUILTIN("getopts", 0, bin_getopts, 2, -1, 0, "p", NULL),
     BUILTIN("hash", BINF_MAGICEQUALS, bin_hash, 0, -1, 0, "Ldfmrv", NULL),
 
 #ifdef ZSH_HASH_DEBUG
@@ -5669,13 +5669,18 @@ int optcind;
 
 /**/
 int
-bin_getopts(UNUSED(char *name), char **argv, UNUSED(Options ops), UNUSED(int func))
+bin_getopts(UNUSED(char *name), char **argv, Options ops, UNUSED(int func))
 {
-    int lenstr, lenoptstr, quiet, lenoptbuf;
+    int lenstr, lenoptstr, quiet, lenoptbuf, posix;
     char *optstr = unmetafy(*argv++, &lenoptstr), *var = *argv++;
     char **args = (*argv) ? argv : pparams;
     char *str, optbuf[2] = " ", *p, opch;
 
+    // note that resetting + restoring OPTIND happens in doshfunc(), so using -p
+    // or enabling POSIX_BUILTINS inside a function that calls getopts is not
+    // exactly the same as enabling POSIX_BUILTINS before the function is called
+    posix = isset(POSIXBUILTINS) || OPT_ISSET(ops, 'p');
+
     /* zoptind keeps count of the current argument number.  The *
      * user can set it to zero to start a new option parse.     */
     if (zoptind < 1) {
@@ -5703,7 +5708,7 @@ bin_getopts(UNUSED(char *name), char **argv, UNUSED(Options ops), UNUSED(int fun
 	str = unmetafy(dupstring(args[zoptind - 1]), &lenstr);
     }
     if(!optcind) {
-	if(lenstr < 2 || (*str != '-' && *str != '+'))
+	if (lenstr < 2 || (*str != '-' && (posix || *str != '+')))
 	    return 1;
 	if(lenstr == 2 && str[0] == '-' && str[1] == '-') {
 	    zoptind++;
@@ -5723,7 +5728,7 @@ bin_getopts(UNUSED(char *name), char **argv, UNUSED(Options ops), UNUSED(int fun
     if(opch == ':' || !(p = memchr(optstr, opch, lenoptstr))) {
 	p = "?";
 	/* Keep OPTIND correct if the user doesn't return after the error */
-	if (isset(POSIXBUILTINS)) {
+	if (posix) {
 	    optcind = 0;
 	    zoptind++;
 	}
@@ -5744,7 +5749,7 @@ bin_getopts(UNUSED(char *name), char **argv, UNUSED(Options ops), UNUSED(int fun
 	if(optcind == lenstr) {
 	    if(!args[zoptind]) {
 		/* Fix OPTIND as above */
-		if (isset(POSIXBUILTINS)) {
+		if (posix) {
 		    optcind = 0;
 		    zoptind++;
 		}
diff --git a/Test/B10getopts.ztst b/Test/B10getopts.ztst
index e50d177c7..5ababebcc 100644
--- a/Test/B10getopts.ztst
+++ b/Test/B10getopts.ztst
@@ -125,3 +125,64 @@
 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>
+
+  for 1 in no_posix_builtins posix_builtins; do (
+    setopt $1
+    print -rn - $1:
+    set -- -a -b +b -c
+    while getopts :abc opt; do
+      case $opt in
+        a|b|+b|c) print -rn - " $opt" ;;
+        ?)        print -rn - " ?$OPTARG" ;;
+      esac
+    done
+    print
+  ); done
+0:POSIX_BUILTINS disables '+' variant handling
+>no_posix_builtins: a b +b c
+>posix_builtins: a b
+
+  (
+    setopt no_posix_builtins
+    for 1 in -a +a -x +x; do
+      () { local opt; getopts :abc opt; print -r - $opt } $1
+    done
+    for 1 in -a +a -x +x; do
+      () { local opt; getopts -p :abc opt; print -r - $opt } $1
+    done
+  )
+0:-p works like POSIX_BUILTINS
+>a
+>+a
+>?
+>?
+>a
+>
+>?
+>
+
+  # not enough arguments
+  () { getopts }
+  () { getopts '' }
+  () { getopts x }
+  () { getopts - x }
+  () { getopts -p - x }
+  () { getopts -p -- x }
+  # invalid option to getopts
+  () { getopts -x x }
+  () { getopts -x x y }
+  () { getopts -x - x y }
+  # guarded spec
+  () { getopts - -x y }
+  # argv on command line
+  () { getopts x y a b c }
+-:option parsing
+?(anon):getopts: not enough arguments
+?(anon):getopts: not enough arguments
+?(anon):getopts: not enough arguments
+?(anon):getopts: not enough arguments
+?(anon):getopts: not enough arguments
+?(anon):getopts: not enough arguments
+?(anon):getopts: bad option: -x
+?(anon):getopts: bad option: -x
+?(anon):getopts: bad option: -x