[PATCH] Nofork capture of stdout

[PATCH] Nofork capture of stdout

[Bart Schaefer]

[-- Attachment #1: Type: text/plain, Size: 1436 bytes --]

On Mon, Jul 17, 2023 at 8:14 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> I have not yet implemented the anonymous tempfile for ${
> capture_stdout }.  However, this equivalent construction does work
> (extra newlines for clarity):
>   ${|
>     () {
>       capture_stdout > $1
>       REPLY=$(<$1)
>     } =(</dev/null)
>   }
>
> Both $(<...) and =(<...) are non-forking shortcuts already, so this
> acts as proof of concept.

I've just discovered that =(<file) is NOT in fact non-forking.  Only
=(<<<string) is non-forking.  Any particular reason for this?  It has
the weird-feeling effect that =(<<<$(<file)) is non-forking whereas
the simpler construction is not.  In any event, =(<<<'') can be
substituted in the example above, and avoids opening /dev/null so is
preferable as well.

Which is exactly what the appended patch does:  Leverage
getoutputfile() to create a temporary name, then write it and read it
back.

Implementation question:  It seems to be OK to tokenize() a string
that has already been tokenize()d?  I tried to avoid double-tokenizing
with ${| ...} by checking whether the "|" was already tokenized as
"Bar", but there's no such hint available with ${ ... }.  It
nevertheless seems to work.

Tests are minimal because this all just falls through to the REPLY
case that's already being thoroughly tested.  Unless the
double-tokenize thing could break something.

[-- Attachment #2: nofork-stdout.txt --]
[-- Type: text/plain, Size: 3465 bytes --]

diff --git a/Doc/Zsh/expn.yo b/Doc/Zsh/expn.yo
index 7480076df..1dec2c0e6 100644
--- a/Doc/Zsh/expn.yo
+++ b/Doc/Zsh/expn.yo
@@ -1918,13 +1918,11 @@ except as noted for tt(REPLY), and var(param) should em(not) be declared
 within the command.  If var(param) names an array, array expansion rules
 apply.
 
-COMMENT(To be implemented later:
 A command enclosed in braces preceded by a dollar sign, and set off from
 the braces by whitespace, like `tt(${ )...tt( })', is replaced by its
 standard output.  Like `tt(${|)...tt(})' and unlike
 `tt($LPAR())...tt(RPAR())', the command executes in the current shell
 context with function local behaviors and does not create a subshell.
-)
 
 texinode(Arithmetic Expansion)(Brace Expansion)(Command Substitution)(Expansion)
 sect(Arithmetic Expansion)
diff --git a/Src/subst.c b/Src/subst.c
index aae178f9e..b9f67c9ea 100644
--- a/Src/subst.c
+++ b/Src/subst.c
@@ -1904,7 +1904,7 @@ paramsubst(LinkList l, LinkNode n, char **str, int qt, int pf_flags,
          * should not be part of command substitution in any case.
          * Use ${(U)${|cmd;}} as you would for ${(U)$(cmd;)}.
 	 */
-	if (*s == '|' || *s == Bar) {
+	if (*s == '|' || *s == Bar || inblank(*s)) {
 	    char *outbracep = s;
 	    char sav = *s;
 	    *s = Inbrace;
@@ -1940,6 +1940,34 @@ paramsubst(LinkList l, LinkNode n, char **str, int qt, int pf_flags,
 		    cmdarg = dupstrpfx(s+1, outbracep-s-1);
 		    rplyvar = "REPLY";
 		}
+		if (inblank(*s)) {
+		    char *dummy = NULL, *tmpfile = dupstring("=(<<<'')");
+		    /*
+		     * Admittedly a hack.  Take advantage of the enforced
+		     * locality of REPLY and the semantics of $(<file) to
+		     * construct a command that writes/reads/removes a
+		     * temporary file by using an =(<<<'') substitution.
+		     * Then fall through to the regular handling of $REPLY
+		     * to manage word splitting, expansion flags, etc.
+		     */
+		    char *fmt = "{ %s ;} >| %s; REPLY=\"$(<%s)\""; /* 28 */
+		    tokenize(tmpfile);
+		    if ((tmpfile = getoutputfile(tmpfile, &dummy))) {
+			/* Prevent shenanigans with $TMPPREFIX */
+			dummy = tmpfile;
+			tmpfile = quotestring(tmpfile, QT_BACKSLASH);
+			zsfree(dummy);
+			dummy = zhalloc(strlen(cmdarg) +
+					2 * strlen(tmpfile) +
+					28);
+			sprintf(dummy, fmt, cmdarg, tmpfile, tmpfile);
+			cmdarg = dummy;
+		    } else {
+			/* TMPPREFIX not writable? */
+			cmdoutval = lastval;
+			cmdarg = NULL;
+		    }
+		}
 		s = outbracep;
 	    }
 	}
@@ -1969,7 +1997,7 @@ paramsubst(LinkList l, LinkNode n, char **str, int qt, int pf_flags,
 
 	if (rplyvar) {
 	    if (strcmp(rplyvar, "REPLY") == 0) {
-		if ((val = ztrdup(getsparam("REPLY"))))
+		if ((val = dupstring(getsparam("REPLY"))))
 		    vunset = 0;
 		else {
 		    vunset = 1;
diff --git a/Test/D10nofork.ztst b/Test/D10nofork.ztst
index 738a45b99..5eaa12279 100644
--- a/Test/D10nofork.ztst
+++ b/Test/D10nofork.ztst
@@ -160,7 +160,7 @@ F:Why not use this error in the previous case as well?
 ?(eval):1: closing brace expected
 
   purr ${ purr STDOUT }
-0f:capture stdout
+0:capture stdout
 >STDOUT
 
 # end PS2 stack tests 
@@ -321,6 +321,10 @@ F:Fiddly here to get EOF past the test syntax
 0:here-string behavior
 >in a here string
 
+  <<<${ purr $'stdout as a here string' }
+0:another capture stdout
+>stdout as a here string
+
   print -u $ZTST_fd ${ZTST_testname}: TEST COMPLETE
 0:make sure we got to the end
 F:some tests might silently break the test harness

Re: [PATCH] Nofork capture of stdout

[Bart Schaefer]

[-- Attachment #1: Type: text/plain, Size: 827 bytes --]

On Sun, Sep 10, 2023 at 9:34 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> Which is exactly what the appended patch does:  Leverage
> getoutputfile() to create a temporary name, then write it and read it
> back.

While looking for some additional tests for this, I ran into an error
I'd never seen before:

process substitution =(<<<'') cannot be used here

This happens if there's no current job on which addfilelist() can hang
the temp name.  I don't know if there's any way to trigger it in
"normal" usage.

The appended is probably better anyway, it's slightly less of a hack
than before.  Replaces workers/52126 entirely, although the Doc hunk
is unchanged.

> Implementation question:  It seems to be OK to tokenize() a string
> that has already been tokenize()d?

Any comment on that?

[-- Attachment #2: nofork-stdout.txt --]
[-- Type: text/plain, Size: 3890 bytes --]

diff --git a/Doc/Zsh/expn.yo b/Doc/Zsh/expn.yo
index 7480076df..1dec2c0e6 100644
--- a/Doc/Zsh/expn.yo
+++ b/Doc/Zsh/expn.yo
@@ -1918,13 +1918,11 @@ except as noted for tt(REPLY), and var(param) should em(not) be declared
 within the command.  If var(param) names an array, array expansion rules
 apply.
 
-COMMENT(To be implemented later:
 A command enclosed in braces preceded by a dollar sign, and set off from
 the braces by whitespace, like `tt(${ )...tt( })', is replaced by its
 standard output.  Like `tt(${|)...tt(})' and unlike
 `tt($LPAR())...tt(RPAR())', the command executes in the current shell
 context with function local behaviors and does not create a subshell.
-)
 
 texinode(Arithmetic Expansion)(Brace Expansion)(Command Substitution)(Expansion)
 sect(Arithmetic Expansion)
diff --git a/Src/subst.c b/Src/subst.c
index aae178f9e..764213c69 100644
--- a/Src/subst.c
+++ b/Src/subst.c
@@ -1862,6 +1862,8 @@ paramsubst(LinkList l, LinkNode n, char **str, int qt, int pf_flags,
     int quoted_array_with_offset = 0;
     /* Indicates ${|...;} */
     char *rplyvar = NULL;
+    /* Indicates ${ ... ;} */
+    char *rplytmp = NULL;
 
     *s++ = '\0';
     /*
@@ -1904,7 +1906,7 @@ paramsubst(LinkList l, LinkNode n, char **str, int qt, int pf_flags,
          * should not be part of command substitution in any case.
          * Use ${(U)${|cmd;}} as you would for ${(U)$(cmd;)}.
 	 */
-	if (*s == '|' || *s == Bar) {
+	if (*s == '|' || *s == Bar || inblank(*s)) {
 	    char *outbracep = s;
 	    char sav = *s;
 	    *s = Inbrace;
@@ -1940,6 +1942,29 @@ paramsubst(LinkList l, LinkNode n, char **str, int qt, int pf_flags,
 		    cmdarg = dupstrpfx(s+1, outbracep-s-1);
 		    rplyvar = "REPLY";
 		}
+		if (inblank(*s)) {
+		    /*
+		     * Admittedly a hack.  Take advantage of the enforced
+		     * locality of REPLY and the semantics of $(<file) to
+		     * construct a command to write/read a temporary file.
+		     * Then fall through to the regular handling of $REPLY
+		     * to manage word splitting, expansion flags, etc.
+		     */
+		    char *fmt = "{ %s ;} >| %s; REPLY=\"$(<%s)\""; /* 28 */
+		    if ((rplytmp = gettempname(NULL, 1))) {
+			/* Prevent shenanigans with $TMPPREFIX */
+			char *tmpfile = quotestring(rplytmp, QT_BACKSLASH);
+			char *dummy = zhalloc(strlen(cmdarg) +
+					      2 * strlen(tmpfile) +
+					      28);
+			sprintf(dummy, fmt, cmdarg, tmpfile, tmpfile);
+			cmdarg = dummy;
+		    } else {
+			/* TMPPREFIX not writable? */
+			cmdoutval = lastval;
+			cmdarg = NULL;
+		    }
+		}
 		s = outbracep;
 	    }
 	}
@@ -1967,9 +1992,11 @@ paramsubst(LinkList l, LinkNode n, char **str, int qt, int pf_flags,
 	    }
 	}
 
+	if (rplytmp)
+	    unlink(rplytmp);
 	if (rplyvar) {
 	    if (strcmp(rplyvar, "REPLY") == 0) {
-		if ((val = ztrdup(getsparam("REPLY"))))
+		if ((val = dupstring(getsparam("REPLY"))))
 		    vunset = 0;
 		else {
 		    vunset = 1;
diff --git a/Test/D10nofork.ztst b/Test/D10nofork.ztst
index 738a45b99..3f11f80f1 100644
--- a/Test/D10nofork.ztst
+++ b/Test/D10nofork.ztst
@@ -160,7 +160,7 @@ F:Why not use this error in the previous case as well?
 ?(eval):1: closing brace expected
 
   purr ${ purr STDOUT }
-0f:capture stdout
+0:capture stdout
 >STDOUT
 
 # end PS2 stack tests 
@@ -321,6 +321,16 @@ F:Fiddly here to get EOF past the test syntax
 0:here-string behavior
 >in a here string
 
+  <<<${ purr $'stdout as a here string' }
+0:another capture stdout
+>stdout as a here string
+
+  wrap=${| REPLY="REPLY in environment assignment" } typeset -p wrap
+  wrap=${ purr "capture in environment assignment" } typeset -p wrap
+0:assignment context
+>typeset -g wrap='REPLY in environment assignment'
+>typeset -g wrap='capture in environment assignment'
+
   print -u $ZTST_fd ${ZTST_testname}: TEST COMPLETE
 0:make sure we got to the end
 F:some tests might silently break the test harness