[PATCH] Fix "private" options

[Philippe Altherr <philippe.altherr@xxxxxxxxx>]

The current implementation of the "private" builtin doesn't support the "-p" option but supports the "-m" option, which is the exact opposite of other typeset-like builtins like "local", "integer", or "float". This isn't a problem per se. However, the "zsh/param/private" module hijacks the "local" builtin in order to give it support for the "-P" option. This implies that as soon as the module is loaded, the "local" builtin loses its support for "-p" (and gains support for "-m"). The patch remedies this by adding support for "-p" and removing support for "-m".

Invoking "private" (or "local -P") with no parameter names should print all private parameters. Filtering by private parameters is however currently not supported. The patch therefore changes "private" (and "local -P") to require at least one parameter name.

The current implementation of "private" contains logic that assumes that the "+" option should trigger the printing of any listed parameter name. This is bogus; "+" is not supposed to trigger printing; under some circumstances. The patch removes this logic. This changes the behavior of "private + var", which until now printed the parameter "var" if it already existed or else created a plain scalar "var" and now is equivalent to "private var", which creates a private parameter "var" or tries to turn any already existing parameter "var" into a private one.

The patch also removes some paranoid logic for recursive calls that don't exist.

The test file V10private contains a test case that runs a modified version of the test file B02typeset where the module "zsh/param/private" has been loaded. This is to ensure that the "local" builtin, which the module hijacks, still works as expected. A problem with this test case is that when the modified test file fails, the only information we get is that the test case failed. There are no details on which test cases in the modified B02typeset test file failed and why. To remedy this, the patch removes the test case and adds the generation of the modified test file to the Makefile. The modified test file then runs like any normal test file.

- Fix "private" options

  (depends on workers/55168)

Philippe

diff --git a/.gitignore b/.gitignore
index b8922ef6f..d87312451 100644
--- a/.gitignore
+++ b/.gitignore
@@ -153,6 +153,7 @@ Src/Zle/widgets.list
 Src/Zle/zle_things.h
 Src/Zle/zle_widget.h
 
+Test/V10private-B02typeset.ztst
 Test/*.tmp
 /.project
 .DS_Store
diff --git a/Doc/Zsh/mod_private.yo b/Doc/Zsh/mod_private.yo
index 20b0a1f3b..8d6b090bc 100644
--- a/Doc/Zsh/mod_private.yo
+++ b/Doc/Zsh/mod_private.yo
@@ -10,16 +10,20 @@ ifnzman()
 startitem()
 findex(private)
 cindex(private parameter, creating)
-item(tt(private) [ {tt(PLUS())|tt(-)}tt(AHUahlmrtux) ] \
-[ {tt(PLUS())|tt(-)}tt(EFLRZi) [ var(n) ] ] [ var(name)[tt(=)var(value)] ... ])(
+xitem(tt(private) [ {tt(PLUS())|tt(-)}tt(AHUahlrtux) ] \
+[ {tt(PLUS())|tt(-)}tt(EFLRZip) [ var(n) ] ] \
+[ tt(+) ] var(name)[tt(=)var(value)] ...)
+item(tt(private) {tt(PLUS())|tt(-)}tt(n) [ {tt(PLUS())|tt(-)}tt(Hhrtu) ] \
+[ {tt(PLUS())|tt(-)}tt(p) [ var(n) ] ] \
+[ tt(+) ] var(name)[tt(=)var(value)] ...)(
 The tt(private) builtin accepts all the same options and arguments as tt(local)
 (nmref(Shell Builtin Commands)(zshbuiltins)) except
-for the `tt(-)tt(T)' option.  Tied parameters may not be made private.
+for the `tt(-T)' option.  Tied parameters may not be made private.
 
-The `tt(-)tt(p)' option is presently disabled because the state of
-private parameters cannot reliably be reloaded.  When `tt(typeset -)tt(p)'
+Querying for private parameters is presently not supported. At least
+one parameter name must be provided. When `tt(typeset -p)'
 outputs a private parameter, it is treated as a local with the
-`tt(-)tt(h)' (hide) option enabled.
+`tt(-h)' (hide) option enabled.
 
 If used at the top level (outside a function scope), tt(private) creates a
 normal parameter in the same manner as tt(declare) or tt(typeset).  A
@@ -31,7 +35,7 @@ declared with tt(local), except having special properties noted below.
 Special parameters which expose or manipulate internal shell state, such
 as tt(ARGC), tt(argv), tt(COLUMNS), tt(LINES), tt(UID), tt(EUID), tt(IFS),
 tt(PROMPT), tt(RANDOM), tt(SECONDS), etc., cannot be made private unless
-the `tt(-)tt(h)' option is used to hide the special meaning of the
+the `tt(-h)' option is used to hide the special meaning of the
 parameter.  This may change in the future.
 )
 enditem()
diff --git a/INSTALL b/INSTALL
index da2625b43..1f75995a2 100644
--- a/INSTALL
+++ b/INSTALL
@@ -180,14 +180,14 @@ After configuring, to build zsh, execute the command:
     make
 
 It's then a good idea to check that your build is working properly:
-    make check
+    make -s check
 
 If you have trouble with a particular test, you can run it separately:
-    make TESTNUM=C02 check
+    make -s TESTNUM=C02 check
 
 The TESTNUM value can be a single test number, as above, or a letter to
 run an entire category of tests:
-    make TESTNUM=Y check
+    make -s TESTNUM=Y check
 
 See Test/README for a list of test categories.
 
diff --git a/Src/Modules/param_private.c b/Src/Modules/param_private.c
index e06cbb1de..c9f19ec1c 100644
--- a/Src/Modules/param_private.c
+++ b/Src/Modules/param_private.c
@@ -210,50 +210,47 @@ is_private(Param pm)
     return 1;
 }
 
-static int fakelevel;
+static int fakelevel = 0;
 
 /**/
 static int
 bin_private(char *nam, char **args, LinkList assigns, Options ops, int func)
 {
-    int from_typeset = 1;
-    int ofake = fakelevel;	/* paranoia in case of recursive call */
-    int hasargs = /* *args != NULL || */ (assigns && firstnode(assigns));
-    makeprivate_error = 0;
+    int errors;
 
     if (!OPT_ISSET(ops, 'P')) {
-	fakelevel = 0;
-	from_typeset = bin_typeset(nam, args, assigns, ops, func);
-	fakelevel = ofake;
-	return from_typeset;
+	return bin_typeset(nam, args, assigns, ops, func);
     } else if (OPT_ISSET(ops, 'T')) {
-	zwarn("bad option: -T");
+	zwarnnam(nam, *nam == 'l' ?
+		 "-T not allowed with -P" : "bad option: -T");
 	return 1;
-    }
-
-    if (locallevel == 0) {
+    } else if (!*args && !(assigns && firstnode(assigns))) {
+	zwarnnam(nam, *nam == 'l' ?
+		 "at least one parameter name is required with -P" :
+		 "at least one parameter name is required");
+	return 1;
+    } else if (OPT_ISSET(ops, 'p')) {
+	return bin_typeset(nam, args, assigns, ops, func);
+    } else if (locallevel == 0) {
 	if (isset(WARNCREATEGLOBAL))
 	    zwarnnam(nam, "invalid local scope, using globals");
-	return bin_typeset("private", args, assigns, ops, func);
+	return bin_typeset(nam, args, assigns, ops, func);
     }
 
-    if (!(OPT_ISSET(ops, 'm') || OPT_ISSET(ops, '+')))
-	ops->ind['g'] = 2;	/* force bin_typeset() to behave as "local" */
-    if (OPT_ISSET(ops, 'p') || OPT_ISSET(ops, 'm') ||
-	(!hasargs && OPT_ISSET(ops, '+'))) {
-	return bin_typeset("private", args, assigns, ops, func);
-    }
+    ops->ind['g'] = 2;	/* force bin_typeset() to behave as "local" */
 
     queue_signals();
     fakelevel = locallevel;
     startparamscope();
-    from_typeset = bin_typeset("private", args, assigns, ops, func);
+    errors = bin_typeset(nam, args, assigns, ops, func);
+    makeprivate_error = 0;
     scanhashtable(realparamtab, 0, 0, 0, makeprivate, 0);
+    errors |= makeprivate_error;
     endparamscope();
-    fakelevel = ofake;
+    fakelevel = 0;
     unqueue_signals();
 
-    return makeprivate_error | from_typeset;
+    return errors;
 }
 
 static void
@@ -649,7 +646,7 @@ printprivatenode(HashNode hn, int printflags)
 
 static struct builtin bintab[] = {
     /* Copied from BUILTIN("local"), "P" added */
-    BUILTIN("private", BINF_PLUSOPTS | BINF_MAGICEQUALS | BINF_PSPECIAL | BINF_ASSIGN, (HandlerFunc)bin_private, 0, -1, 0, "AE:%F:%HL:%PR:%TUZ:%ahi:%lnmrtux", "P")
+    BUILTIN("private", BINF_PLUSOPTS | BINF_MAGICEQUALS | BINF_PSPECIAL | BINF_ASSIGN, (HandlerFunc)bin_private, 0, -1, 0, "AE:%F:%HL:%PR:%TUZ:%ahi:%lnp:%rtux", "P")
 };
 
 static struct features module_features = {
diff --git a/Test/B02typeset.ztst b/Test/B02typeset.ztst
index 6bc53c68a..ece825156 100644
--- a/Test/B02typeset.ztst
+++ b/Test/B02typeset.ztst
@@ -29,8 +29,12 @@
 # that code, as that would affect the scoping.
 
 %prep
-  ## Do not remove the next line, it's used by V10private.ztst
-  # test_zsh_param_private
+  load-private-module() {
+    if ! zmodload zsh/param/private 2>/dev/null; then
+      ZTST_unimplemented="can't load the zsh/param/private module for testing"
+    fi
+  }
+  # load-private-module # Needed for V10private-B02typeset.ztst
 
   mkdir typeset.tmp && cd typeset.tmp
 
@@ -149,10 +153,22 @@
 >array local array
 >scalar a r r a y
 
+ declare -p scalar
+ declare -m scalar
  declare +m scalar
-0:declare previously lacked -m/+m options
+0:declare supports -p and -m/+m options
+>typeset -g scalar=scalar
+>scalar=scalar
 >scalar
 
+ local -p scalar
+ local -m scalar
+ local +m scalar
+1:local supports -p but not -m/+m options
+>typeset -g scalar=scalar
+?(eval):local:2: bad option: -m
+?(eval):local:3: bad option: +m
+
  scope10
  print $outer
 0:Global export
diff --git a/Test/Makefile.in b/Test/Makefile.in
index 09f37bf53..12a85c0ff 100644
--- a/Test/Makefile.in
+++ b/Test/Makefile.in
@@ -40,9 +40,23 @@ INSTALL         = @INSTALL@
 
 # ========== DEPENDENCIES FOR TESTING ==========
 
+V10private-B02typeset.ztst: B02typeset.ztst
+	TEXT="# load-private-module #"; \
+	if ! grep -q "$$TEXT" $<; then \
+	  echo "$<: Failed to generate $@, text \"$$TEXT\" is missing" >&2; \
+	  exit 1; \
+	fi; \
+	{ \
+	  echo '######################################################################'; \
+	  echo '# DO NOT EDIT - Generated from $< by make'; \
+	  echo '######################################################################'; \
+	  echo ''; \
+	  sed -e "s,$$TEXT.*,load-private-module," < $<; \
+	} > $@
+
 # If ZTST_handler is not empty, run that instead of runtests.zsh.
 # It's assumed to be found in the source directory.
-check test:
+check test: V10private-B02typeset.ztst
 	if test -n "$(DLLD)"; then \
 	  cd $(dir_top) && DESTDIR= \
 	  $(MAKE) MODDIR=`pwd`/$(subdir)/Modules install.modules > /dev/null; \
@@ -68,7 +82,7 @@ check test:
 @CLEAN_MK@
 
 mostlyclean-here:
-	rm -rf Modules .zcompdump *.tmp
+	rm -rf Modules .zcompdump *.tmp V10private-B02typeset.ztst
 
 distclean-here:
 	rm -f Makefile
diff --git a/Test/README b/Test/README
index b9d393d7c..0eac9daf8 100644
--- a/Test/README
+++ b/Test/README
@@ -14,24 +14,26 @@ scripts names:
  Y: completion
  Z: separate systems and user contributions
 
-You will need to run these by using `make test' in the Test subdirectory of
-the build area for your system (which may or may not be the same as the
-Test subdirectory of the source tree), or the directory above.  You can get
-more information about the tests being performed with
-  ZTST_verbose=1 make check
-(`test' is equivalent to `check') or change 1 to 2 for even more detail.
+You will need to run these by using `make -s check' in the Test
+subdirectory of the build area for your system (which may or may not
+be the same as the Test subdirectory of the source tree), or the
+directory above.  The `-s' option tells `make' not to print the
+commands it runs.  You can get more information about the tests being
+performed with
+  ZTST_verbose=1 make -s check
+or change 1 to 2 for even more detail.
 
 A test file is usually aborted on the first error.  To continue to the
 end, run with
-  ZTST_continue=1 make check
+  ZTST_continue=1 make -s check
 This can usefully be combined with ZTST_verbose.  The test is always
 aborted on a syntax error as in that case it is not obvoius how to
 continue.
 
 Individual or groups of tests can be performed with
-  make TESTNUM=C02 check
+  make -s TESTNUM=C02 check
 or
-  make TESTNUM=C check
+  make -s TESTNUM=C check
 to perform just the test beginning C02, or all tests beginning C,
 respectively.
 
diff --git a/Test/V10private.ztst b/Test/V10private.ztst
index f3ad7b6b9..7624a1d7d 100644
--- a/Test/V10private.ztst
+++ b/Test/V10private.ztst
@@ -4,10 +4,6 @@
 
  if ! zmodload zsh/param/private 2>/dev/null; then
    ZTST_unimplemented="can't load the zsh/param/private module for testing"
- else
-   # Do not use .tmp here, ztst.zsh will remove it too soon (see %cleanup)
-   mkdir private.TMP
-   sed -e 's,# test_zsh_param_private,zmodload zsh/param/private,' < $ZTST_srcdir/B02typeset.ztst > private.TMP/B02
  fi
 
  setopt TYPESET_TO_UNSET
@@ -17,6 +13,35 @@
  (zmodload -u zsh/param/private && zmodload zsh/param/private)
 0:unload and reload the module without crashing
 
+ () {
+   private     VAR1=ok
+   local -P    VAR2=ok
+   private  -T VAR=KO var 2>&1
+   local -P -T VAR=KO var 2>&1
+   private  -p 2>&1
+   local -P -p 2>&1
+   private  -m 2>&1
+   local -P -m 2>&1
+   private  +m 2>&1
+   local -P +m 2>&1
+   private  +  2>&1
+   local -P +  2>&1
+   typeset -p  VAR1 VAR2
+ }
+0:private/local option compatibility
+>(anon):private:3: bad option: -T
+>(anon):local:4: -T not allowed with -P
+>(anon):private:5: at least one parameter name is required
+>(anon):local:6: at least one parameter name is required with -P
+>(anon):private:7: bad option: -m
+>(anon):local:8: bad option: -m
+>(anon):private:9: bad option: +m
+>(anon):local:10: bad option: +m
+>(anon):private:11: at least one parameter name is required
+>(anon):local:12: at least one parameter name is required with -P
+>typeset -h VAR1=ok
+>typeset -h VAR2=ok
+
  typeset scalar_test=toplevel
  () {
   print $scalar_test
@@ -118,14 +143,16 @@
  }
  outer () {
   local -PA hash_test=(in function)
-  private + hash_test
+  typeset -p hash_test
+  print ${(t)hash_test} ${(kv)hash_test}
   inner
  }
  outer
  print ${(kv)hash_test}
 0:private hides value from surrounding scope in nested scope
 >typeset -a hash_test=( top level )
->hash_test=( [in]=function )
+>typeset -Ah hash_test=( [in]=function )
+>association-local-hide-special in function
 >typeset -g -a hash_test=( top level )
 >array-local top level
 >top level
@@ -292,15 +319,6 @@ F:future revision will create a global with this assignment
  () { private -h SECONDS }
 0:private parameter may hide a special parameter
 
- if (( UID )); then
-   ZTST_verbose=0 $ZTST_exe +Z -f $ZTST_srcdir/ztst.zsh private.TMP/B02
- else
-   ZTST_skip="cannot re-run typeset tests when tests run as superuser"
- fi
-0:typeset still works with zsh/param/private module loaded
-*>*
-*>*
-
  typeset top=TOP
  () {
   local -P -n test=top
@@ -605,8 +623,3 @@ F:Better if caught in checkclobberparam() but exec.c doesn't know scope
  }
 1:function may not access private declared in nofork
 *?*: q: can't modify read-only parameter
-
-%clean
-
-  unsetopt TYPESET_TO_UNSET
-  rm -r private.TMP