Re: PATCH: readonly -p with POSIX_BUILTINS

[Peter Stephenson <p.stephenson@xxxxxxxxxxx>]

On Thu, 7 May 2015 10:35:51 +0100
Peter Stephenson <p.stephenson@xxxxxxxxxxx> wrote:
> It doesn't fix the non-POSIX_BUILTINS aspect of "readonly -p" output,
> i.e. "er, this output is all a bit broken, isn't it?"  The object
> of "readonly -p" is to have a set of commands to restore the current
> state: that means it should work without complaint on a new shell (I
> don't think it can possibly be required to work once you've already run
> it as POSIX doesn't allow you to remove the readonly attribute).  I
> think that means not printing values for readonly specials.  Feel free
> to argue it shouldn't show readonly specials at all --- if you ran a
> script so generated immediately on starting a shell and it included a
> readonly special that was loaded from a module, then I think you'd be
> screwed.  I'm vaguely inclining that way myself; you don't use the -p
> option if you're just listing things for information.

This is what I've done.

pws

diff --git a/Doc/Zsh/builtins.yo b/Doc/Zsh/builtins.yo
index 5b25290..9699cf3 100644
--- a/Doc/Zsh/builtins.yo
+++ b/Doc/Zsh/builtins.yo
@@ -1785,6 +1785,11 @@ form of a typeset command and an assignment (which will be printed
 separately for arrays and associative arrays), regardless of other flags
 and options.  Note that the tt(-H) flag on parameters is respected; no
 value will be shown for these parameters.
+
+As the intention of this option is to produce output that can restore
+the current state, readonly specials (whose values cannot be
+changed) are not shown and assignments to arrays are shown before
+the tt(typeset) rendering the array readonly.
 )
 item(tt(-T) [ var(scalar)[tt(=)var(value)] var(array) [ var(sep) ] ])(
 This flag has a different meaning when used with tt(-f); see below.
diff --git a/Src/params.c b/Src/params.c
index 9eab51a..045ac1e 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -5032,78 +5032,11 @@ static const struct paramtypes pmtypes[] = {
 
 #define PMTYPES_SIZE ((int)(sizeof(pmtypes)/sizeof(struct paramtypes)))
 
-/**/
-mod_export void
-printparamnode(HashNode hn, int printflags)
+static void
+printparamvalue(Param p, int printflags)
 {
-    Param p = (Param) hn;
     char *t, **u;
 
-    if (p->node.flags & PM_UNSET) {
-	if (isset(POSIXBUILTINS) && (p->node.flags & PM_READONLY) &&
-	    (printflags & PRINT_TYPESET))
-	{
-	    /*
-	     * Special POSIX rules: show the parameter as readonly
-	     * even though it's unset, but with no value.
-	     */
-	    printflags |= PRINT_NAMEONLY;
-	}
-	else
-	    return;
-    }
-
-    if (printflags & PRINT_TYPESET)
-	printf("typeset ");
-
-    /* Print the attributes of the parameter */
-    if (printflags & (PRINT_TYPE|PRINT_TYPESET)) {
-	int doneminus = 0, i;
-	const struct paramtypes *pmptr;
-
-	for (pmptr = pmtypes, i = 0; i < PMTYPES_SIZE; i++, pmptr++) {
-	    int doprint = 0;
-	    if (pmptr->flags & PMTF_TEST_LEVEL) {
-		if (p->level)
-		    doprint = 1;
-	    } else if (p->node.flags & pmptr->binflag)
-		doprint = 1;
-
-	    if (doprint) {
-		if (printflags & PRINT_TYPESET) {
-		    if (pmptr->typeflag) {
-			if (!doneminus) {
-			    putchar('-');
-			    doneminus = 1;
-			}
-			putchar(pmptr->typeflag);
-		    }
-		} else {
-		    printf("%s ", pmptr->string);
-		}
-		if ((pmptr->flags & PMTF_USE_BASE) && p->base) {
-		    printf("%d ", p->base);
-		    doneminus = 0;
-		}
-		if ((pmptr->flags & PMTF_USE_WIDTH) && p->width) {
-		    printf("%d ", p->width);
-		    doneminus = 0;
-		}
-	    }
-	}
-	if (doneminus)
-	    putchar(' ');
-    }
-
-    if ((printflags & PRINT_NAMEONLY) ||
-	((p->node.flags & PM_HIDEVAL) && !(printflags & PRINT_INCLUDEVALUE))) {
-	zputs(p->node.nam, stdout);
-	putchar('\n');
-	return;
-    }
-
-    quotedzputs(p->node.nam, stdout);
-
     if (p->node.flags & PM_AUTOLOAD) {
 	putchar('\n');
 	return;
@@ -5112,7 +5045,7 @@ printparamnode(HashNode hn, int printflags)
 	putchar(' ');
     else if ((printflags & PRINT_TYPESET) &&
 	     (PM_TYPE(p->node.flags) == PM_ARRAY || PM_TYPE(p->node.flags) == PM_HASHED))
-	printf("\n%s=", p->node.nam);
+	printf("%s=", p->node.nam);
     else
 	putchar('=');
 
@@ -5171,3 +5104,108 @@ printparamnode(HashNode hn, int printflags)
     else
 	putchar('\n');
 }
+
+/**/
+mod_export void
+printparamnode(HashNode hn, int printflags)
+{
+    Param p = (Param) hn;
+    int array_typeset;
+
+    if (p->node.flags & PM_UNSET) {
+	if (isset(POSIXBUILTINS) && (p->node.flags & PM_READONLY) &&
+	    (printflags & PRINT_TYPESET))
+	{
+	    /*
+	     * Special POSIX rules: show the parameter as readonly
+	     * even though it's unset, but with no value.
+	     */
+	    printflags |= PRINT_NAMEONLY;
+	}
+	else
+	    return;
+    }
+
+    if (printflags & PRINT_TYPESET) {
+	if ((p->node.flags & (PM_READONLY|PM_SPECIAL)) ==
+	    (PM_READONLY|PM_SPECIAL)) {
+	    /*
+	     * It's not possible to restore the state of
+	     * these, so don't output.
+	     */
+	    return;
+	}
+	/*
+	 * Printing the value of array: this needs to be on
+	 * a separate line so more care is required.
+	 */
+	array_typeset = (PM_TYPE(p->node.flags) == PM_ARRAY ||
+			 PM_TYPE(p->node.flags) == PM_HASHED) &&
+	    !(printflags & PRINT_NAMEONLY);
+	if (array_typeset && (p->node.flags & PM_READONLY)) {
+	    /*
+	     * We need to create the array before making it
+	     * readonly.
+	     */
+	    printf("typeset -a ");
+	    zputs(p->node.nam, stdout);
+	    putchar('\n');
+	    printparamvalue(p, printflags);
+	    printflags |= PRINT_NAMEONLY;
+	}
+	printf("typeset ");
+    }
+    else
+	array_typeset = 0;
+
+    /* Print the attributes of the parameter */
+    if (printflags & (PRINT_TYPE|PRINT_TYPESET)) {
+	int doneminus = 0, i;
+	const struct paramtypes *pmptr;
+
+	for (pmptr = pmtypes, i = 0; i < PMTYPES_SIZE; i++, pmptr++) {
+	    int doprint = 0;
+	    if (pmptr->flags & PMTF_TEST_LEVEL) {
+		if (p->level)
+		    doprint = 1;
+	    } else if (p->node.flags & pmptr->binflag)
+		doprint = 1;
+
+	    if (doprint) {
+		if (printflags & PRINT_TYPESET) {
+		    if (pmptr->typeflag) {
+			if (!doneminus) {
+			    putchar('-');
+			    doneminus = 1;
+			}
+			putchar(pmptr->typeflag);
+		    }
+		} else {
+		    printf("%s ", pmptr->string);
+		}
+		if ((pmptr->flags & PMTF_USE_BASE) && p->base) {
+		    printf("%d ", p->base);
+		    doneminus = 0;
+		}
+		if ((pmptr->flags & PMTF_USE_WIDTH) && p->width) {
+		    printf("%d ", p->width);
+		    doneminus = 0;
+		}
+	    }
+	}
+	if (doneminus)
+	    putchar(' ');
+    }
+
+    if ((printflags & PRINT_NAMEONLY) ||
+	((p->node.flags & PM_HIDEVAL) && !(printflags & PRINT_INCLUDEVALUE))) {
+	zputs(p->node.nam, stdout);
+	putchar('\n');
+    } else {
+	quotedzputs(p->node.nam, stdout);
+
+	if (array_typeset)
+	    putchar('\n');
+	printparamvalue(p, printflags);
+    }
+}
diff --git a/Test/B02typeset.ztst b/Test/B02typeset.ztst
index 2edbb0b..9009619 100644
--- a/Test/B02typeset.ztst
+++ b/Test/B02typeset.ztst
@@ -487,3 +487,24 @@
 ?typeset -r pbro
 ?0
 ?(eval):10: read-only variable: pbro
+
+  readonly foo=bar novalue
+  readonly -p
+0:readonly -p output (no readonly specials)
+>typeset -r foo=bar
+>typeset -r novalue=''
+
+  local -a a1 a2
+  local -r r1=yes r2=no
+  a1=(one two) a2=(three four)
+  readonly a1
+  typeset -pm 'a[12]'
+  typeset -pm 'r[12]'
+0:readonly -p output
+>typeset -a a1
+>a1=(one two)
+>typeset -ar a1
+>typeset -a a2
+>a2=(three four)
+>typeset -r r1=yes
+>typeset -r r2=no