PATCH: perform expansion for precommand modifiers

PATCH: perform expansion for precommand modifiers

[Peter Stephenson]

This is something I've been meaning to get around to for a while.
"exec" and "command" have a for quite a long time now taken options,
but the parsing hadn't caught up properly so that it wasn't possible to
put the options, let alone the modifiers themselves, in a parameter.
This fixes both --- see the new tests.

It's not particularly elegant but I don't want to assume it's safe to
re-expand parts of the command line that have already been expanded, and
a bit of experimenation suggested passing in the ability to expand parts
of a list down to prefork() caused too much complexity.  The key reason
we can't simply expand everything in one go is the need to decide if we
want to do fudged assignment-style expansion, now not quite as crucial
as it was but still useful.

pws

diff --git a/Src/exec.c b/Src/exec.c
index f021a08..b273ee8 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -2642,6 +2642,27 @@ execcmd_analyse(Estate state, Execcmd_params eparams)
 }
 
 /*
+ * Transfer the first node of args to preargs, performing
+ * prefork expansion on the way if necessary.
+ */
+static void execcmd_getargs(LinkList preargs, LinkList args, int expand)
+{
+    if (!firstnode(args)) {
+	return;
+    } else if (expand) {
+	local_list0(svl);
+	init_list0(svl);
+	/* not init_list1, as we need real nodes */
+	addlinknode(&svl, uremnode(args, firstnode(args)));
+	/* Analysing commands, so vanilla options to prefork */
+	prefork(&svl, 0, NULL);
+	joinlists(preargs, &svl);
+    } else {
+        addlinknode(preargs, uremnode(args, firstnode(args)));
+    }
+}
+
+/*
  * Execute a command at the lowest level of the hierarchy.
  */
 
@@ -2671,6 +2692,11 @@ execcmd_exec(Estate state, Execcmd_params eparams,
     LinkList redir = eparams->redir;
     Wordcode varspc = eparams->varspc;
     int type = eparams->type;
+    /*
+     * preargs comes from expanding the head of the args list
+     * in order to check for prefix commands.
+     */
+    LinkList preargs;
 
     doneps4 = 0;
 
@@ -2725,9 +2751,16 @@ execcmd_exec(Estate state, Execcmd_params eparams,
      * command if it contains some tokens (e.g. x=ex; ${x}port), so this *
      * only works in simple cases.  has_token() is called to make sure   *
      * this really is a simple case.                                     */
-    if (type == WC_SIMPLE || type == WC_TYPESET) {
-	while (args && nonempty(args)) {
-	    char *cmdarg = (char *) peekfirst(args);
+    if ((type == WC_SIMPLE || type == WC_TYPESET) && args) {
+	/*
+	 * preargs contains args that have been expanded by prefork.
+	 * running excargs causes the expansion, if possible and
+	 * if parsseargs is empty, otherwise does nothing.
+	 */
+	preargs = newlinklist();
+	execcmd_getargs(preargs, args, eparams->htok);
+	while (nonempty(preargs)) {
+	    char *cmdarg = (char *) peekfirst(preargs);
 	    checked = !has_token(cmdarg);
 	    if (!checked)
 		break;
@@ -2766,7 +2799,13 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 		break;
 	    }
 	    checked = 0;
-	    if ((cflags & BINF_COMMAND) && nextnode(firstnode(args))) {
+	    uremnode(preargs, firstnode(preargs));
+	    if (!firstnode(preargs)) {
+		execcmd_getargs(preargs, args, eparams->htok);
+		if (!firstnode(preargs))
+		    break;
+	    }
+	    if ((cflags & BINF_COMMAND)) {
 		/*
 		 * Check for options to "command".
 		 * If just -p, this is handled here: use the default
@@ -2776,10 +2815,11 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 		 * Otherwise, just leave marked as BINF_COMMAND
 		 * modifier with no additional action.
 		 */
-		LinkNode argnode = nextnode(firstnode(args));
-		char *argdata = (char *) getdata(argnode);
-		char *cmdopt;
+		LinkNode argnode, oldnode;
+		char *argdata, *cmdopt;
 		int has_p = 0, has_vV = 0, has_other = 0;
+		argnode = firstnode(preargs);
+		argdata = (char *) getdata(argnode);
 		while (IS_DASH(*argdata)) {
 		    /* Just to be definite, stop on single "-", too, */
 		    if (!argdata[1] ||
@@ -2812,25 +2852,30 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 			break;
 		    }
 
+		    oldnode = argnode;
 		    argnode = nextnode(argnode);
-		    if (!argnode)
-			break;
+		    if (!argnode) {
+			execcmd_getargs(preargs, args, eparams->htok);
+			if (!(argnode = nextnode(oldnode)))
+			    break;
+		    }
 		    argdata = (char *) getdata(argnode);
 		}
 		if (has_vV) {
-		    /* Leave everything alone, dispatch to whence */
+		    /*
+		     * Leave everything alone, dispatch to whence.
+		     * We need to put the name back in the list.
+		     */
+		    pushnode(preargs, "command");
 		    hn = &commandbn.node;
 		    is_builtin = 1;
 		    break;
 		} else if (has_p) {
-		    /* Use default path; absorb command and option. */
-		    uremnode(args, firstnode(args));
+		    /* Use default path */
 		    use_defpath = 1;
-		    if ((argnode = nextnode(firstnode(args))))
-			argdata = (char *) getdata(argnode);
 		}
 		/*
-		 * Else just absorb command and any trailing
+		 * Else just any trailing
 		 * end-of-options marker.  This can only occur
 		 * if we just had -p or something including more
 		 * than just -p, -v and -V, in which case we behave
@@ -2838,16 +2883,16 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 		 * isn't a good place for standard option handling.
 		 */
 		if (IS_DASH(argdata[0]) && IS_DASH(argdata[1]) && !argdata[2])
-		     uremnode(args, firstnode(args));
-	    }
-	    if ((cflags & BINF_EXEC) && nextnode(firstnode(args))) {
+		     uremnode(preargs, argnode);
+	    } else if (cflags & BINF_EXEC) {
 		/*
 		 * Check for compatibility options to exec builtin.
 		 * It would be nice to do these more generically,
 		 * but currently we don't have a mechanism for
 		 * precommand modifiers.
 		 */
-		char *next = (char *) getdata(nextnode(firstnode(args)));
+		LinkNode argnode = firstnode(preargs), oldnode;
+		char *argdata = (char *) getdata(argnode);
 		char *cmdopt, *exec_argv0 = NULL;
 		/*
 		 * Careful here: we want to make sure a final dash
@@ -2857,17 +2902,23 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 		 * people aren't likely to mix the option style
 		 * with the zsh style.
 		 */
-		while (next && IS_DASH(*next) && strlen(next) >= 2) {
-		    if (!firstnode(args)) {
+		while (argdata && IS_DASH(*argdata) && strlen(argdata) >= 2) {
+		    oldnode = argnode;
+		    argnode = nextnode(oldnode);
+		    if (!argnode) {
+			execcmd_getargs(preargs, args, eparams->htok);
+			argnode = nextnode(oldnode);
+		    }
+		    if (!argnode) {
 			zerr("exec requires a command to execute");
 			lastval = 1;
 			errflag |= ERRFLAG_ERROR;
 			goto done;
 		    }
-		    uremnode(args, firstnode(args));
-		    if (IS_DASH(next[0]) && IS_DASH(next[1]) && !next[2])
+		    uremnode(preargs, oldnode);
+		    if (IS_DASH(argdata[0]) && IS_DASH(argdata[1]) && !argdata[2])
 			break;
-		    for (cmdopt = &next[1]; *cmdopt; ++cmdopt) {
+		    for (cmdopt = &argdata[1]; *cmdopt; ++cmdopt) {
 			switch (*cmdopt) {
 			case 'a':
 			    /* argument is ARGV0 string */
@@ -2876,21 +2927,25 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 				/* position on last non-NULL character */
 				cmdopt += strlen(cmdopt+1);
 			    } else {
-				if (!firstnode(args)) {
+				if (!argnode) {
 				    zerr("exec requires a command to execute");
 				    lastval = 1;
 				    errflag |= ERRFLAG_ERROR;
 				    goto done;
 				}
-				if (!nextnode(firstnode(args))) {
+				if (!nextnode(argnode))
+				    execcmd_getargs(preargs, args,
+						    eparams->htok);
+				if (!nextnode(argnode)) {
 				    zerr("exec flag -a requires a parameter");
 				    lastval = 1;
 				    errflag |= ERRFLAG_ERROR;
 				    goto done;
 				}
-				exec_argv0 = (char *)
-				    getdata(nextnode(firstnode(args)));
-				uremnode(args, firstnode(args));
+				exec_argv0 = (char *) getdata(argnode);
+				oldnode = argnode;
+				argnode = nextnode(argnode);
+				uremnode(args, oldnode);
 			    }
 			    break;
 			case 'c':
@@ -2906,8 +2961,9 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 			    return;
 			}
 		    }
-		    if (firstnode(args) && nextnode(firstnode(args)))
-			next = (char *) getdata(nextnode(firstnode(args)));
+		    if (!argnode)
+			break;
+		    argdata = (char *) getdata(argnode);
 		}
 		if (exec_argv0) {
 		    char *str, *s;
@@ -2919,12 +2975,14 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 		    zputenv(str);
 		}
 	    }
-	    uremnode(args, firstnode(args));
 	    hn = NULL;
 	    if ((cflags & BINF_COMMAND) && unset(POSIXBUILTINS))
 		break;
+	    if (!nonempty(preargs))
+		execcmd_getargs(preargs, args, eparams->htok);
 	}
-    }
+    } else
+	preargs = NULL;
 
     /* if we get this far, it is OK to pay attention to lastval again */
     if (noerrexit == 2 && !is_shfunc)
@@ -2946,8 +3004,12 @@ execcmd_exec(Estate state, Execcmd_params eparams,
     esprefork = (magic_assign ||
 		 (isset(MAGICEQUALSUBST) && type != WC_TYPESET)) ?
 		 PREFORK_TYPESET : 0;
-    if (args && eparams->htok)
-	prefork(args, esprefork, NULL);
+    if (args) {
+	if (eparams->htok)
+	    prefork(args, esprefork, NULL);
+	if (preargs)
+	    args = joinlists(preargs, args);
+    }
 
     if (type == WC_SIMPLE || type == WC_TYPESET) {
 	int unglobbed = 0;
diff --git a/Src/linklist.c b/Src/linklist.c
index 3aa8125..85d9bb3 100644
--- a/Src/linklist.c
+++ b/Src/linklist.c
@@ -348,6 +348,35 @@ newsizedlist(int size)
 }
 
 /*
+ * Join two linked lists.  Neither may be null, though either
+ * may be empty.
+ *
+ * It is assumed the pieces come from the heap, but if not it is
+ * safe to free LinkList second.
+ */
+
+/**/
+mod_export LinkList
+joinlists(LinkList first, LinkList second)
+{
+    LinkNode moveme = firstnode(second);
+    if (moveme) {
+	if (firstnode(first)) {
+	    LinkNode anchor = lastnode(first);
+	    anchor->next = moveme;
+	    moveme->prev = anchor;
+	} else {
+	    first->list.first = moveme;
+	    moveme->prev = &first->node;
+	}
+	first->list.last = second->list.last;
+
+	second->list.first = second->list.last = NULL;
+    }
+    return first;
+}
+
+/*
  * Return the node whose data is the pointer "dat", else NULL.
  * Can be used as a boolean test.
  */
diff --git a/Test/A01grammar.ztst b/Test/A01grammar.ztst
index e4b6870..32caf5f 100644
--- a/Test/A01grammar.ztst
+++ b/Test/A01grammar.ztst
@@ -103,6 +103,13 @@
 0:`exec' with -a option, no space
 >/bin/SPLOOSH
 
+  (
+    opts=(-a /bin/WHOOOSH)
+    exec $opts /bin/sh -c 'echo $0'
+  )
+0:`exec' with -a option from expansion
+>/bin/WHOOOSH
+
   (export FOO=bar; exec -c /bin/sh -c 'echo x${FOO}x')
 0:`exec' with -c option
 >xx
@@ -123,6 +130,21 @@
 >cat is /*/cat
 >echo is a shell builtin
 
+  args=(
+  'command -pv cat'
+  'command -pv echo'
+  'command -p -V cat'
+  'command -p -V -- echo'
+  )
+  for arg in $args; do
+    ${=arg}
+  done
+0:command -p in combination, using expansion
+*>*/cat
+>echo
+>cat is /*/cat
+>echo is a shell builtin
+
   cd() { echo Not cd at all; }
   builtin cd . && unfunction cd
 0:`builtin' precommand modifier
diff --git a/Test/E01options.ztst b/Test/E01options.ztst
index 2bd4fdb..dac9430 100644
--- a/Test/E01options.ztst
+++ b/Test/E01options.ztst
@@ -804,6 +804,20 @@
 >print is a shell builtin
 ?(eval):8: command not found: print
 
+  (
+     setopt posixbuiltins
+     opts=()
+     command $opts print foo
+     opts=(-v)
+     command $opts print
+     opts=(-V)
+     command $opts print
+  )
+0:command with options from expansion
+>foo
+>print
+>print is a shell builtin
+
   # With non-special command: original value restored
   # With special builtin: new value kept
   # With special builtin preceeded by "command": original value restored.

Re: PATCH: perform expansion for precommand modifiers

[Daniel Shahaf]

Peter Stephenson wrote on Tue, Apr 25, 2017 at 10:15:20 +0100:
> This is something I've been meaning to get around to for a while.
> "exec" and "command" have a for quite a long time now taken options,
> but the parsing hadn't caught up properly so that it wasn't possible to
> put the options, let alone the modifiers themselves, in a parameter.
> This fixes both --- see the new tests.

Is this worth a NEWS entry?  (not README since the change is backwards
compatible)

diff --git a/NEWS b/NEWS
index 5479251..4b5e12a 100644
--- a/NEWS
+++ b/NEWS
@@ -4,5 +4,15 @@ CHANGES FROM PREVIOUS VERSIONS OF ZSH
 
 Note also the list of incompatibilities in the README file.
 
+Changes from 5.3.1 to 5.4
+-------------------------
+
+The 'exec' and 'command' precommand modifiers, and options to them, are
+now parsed after parameter expansion.  Previously, both the modifier
+and any options to it were only recognised if they had already been
+present in the command line before any parameter references were expanded.
+Example: 's=command; $s -V ls' now works as expected.
+
+
 Changes from 5.2 to 5.3.1
 -------------------------

Re: PATCH: perform expansion for precommand modifiers

[Bart Schaefer]

On Apr 26,  7:45pm, Daniel Shahaf wrote:
}
} Is this worth a NEWS entry?  (not README since the change is backwards
} compatible)

This also fixes this bug reported recently, to wit, quoting a precommand
modifier to protect it from alias expansion did not work correctly:

torch% alias exec='echo foo'
torch% ( exec -c printenv )
foo -c printenv
torch% ( \exec -c printenv )
zsh: command not found: -c

Now:

torch% alias exec='echo foo'
torch% ( exec -c printenv )
foo -c printenv
torch% ( \exec -c printenv )
torch% ( \exec printenv ) 
MANPATH=... and so on

Re: PATCH: perform expansion for precommand modifiers

[Peter Stephenson]

On Wed, 26 Apr 2017 14:25:08 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> This also fixes this bug reported recently, to wit, quoting a precommand
> modifier to protect it from alias expansion did not work correctly:
> 
> torch% alias exec='echo foo'
> torch% ( exec -c printenv )
> foo -c printenv
> torch% ( \exec -c printenv )
> zsh: command not found: -c
> 
> Now:
> 
> torch% alias exec='echo foo'
> torch% ( exec -c printenv )
> foo -c printenv
> torch% ( \exec -c printenv )
> torch% ( \exec printenv ) 
> MANPATH=... and so on

Here's a test.

diff --git a/Test/A01grammar.ztst b/Test/A01grammar.ztst
index 32caf5f..37311ce 100644
--- a/Test/A01grammar.ztst
+++ b/Test/A01grammar.ztst
@@ -114,6 +114,12 @@
 0:`exec' with -c option
 >xx
 
+  (\exec /bin/sh -c 'echo Test one'; print Not reached)
+  ('exec' /bin/sh -c 'echo Test two'; print Not reached)
+0:exec with quotes
+>Test one
+>Test two
+
   cat() { echo Function cat executed; }
   command cat && unfunction cat
 0:`command' precommand modifier

Re: PATCH: perform expansion for precommand modifiers

[Bart Schaefer]

On Apr 27,  9:40am, Peter Stephenson wrote:
}
} +  (\exec /bin/sh -c 'echo Test one'; print Not reached)
} +  ('exec' /bin/sh -c 'echo Test two'; print Not reached)
} +0:exec with quotes
} +>Test one
} +>Test two

This isn't testing the right thing.  The complaint was that the -c option
of exec was treated as a command name if the keyword was quoted.

Similarly for "command -p" which now that I try it is still broken:

torch% \command -p echo
zsh: command not found: -p

Re: PATCH: perform expansion for precommand modifiers

[Peter Stephenson]

On Thu, 27 Apr 2017 16:12:59 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Apr 27,  9:40am, Peter Stephenson wrote:
> }
> } +  (\exec /bin/sh -c 'echo Test one'; print Not reached)
> } +  ('exec' /bin/sh -c 'echo Test two'; print Not reached)
> } +0:exec with quotes
> } +>Test one
> } +>Test two
> 
> This isn't testing the right thing.  The complaint was that the -c option
> of exec was treated as a command name if the keyword was quoted.

I think we can fix this by adding a test for that.

> Similarly for "command -p" which now that I try it is still broken:
> 
> torch% \command -p echo
> zsh: command not found: -p

That's nothing to do with the quoting.  I assumed we had a test for -p
without -v or -V, but apparently we don't, so I didn't notice there was
an extra node to remove.

pws

diff --git a/Src/exec.c b/Src/exec.c
index 978a32d..15f663c 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -2824,7 +2824,7 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 		 * Otherwise, just leave marked as BINF_COMMAND
 		 * modifier with no additional action.
 		 */
-		LinkNode argnode, oldnode;
+		LinkNode argnode, oldnode, pnode = NULL;
 		char *argdata, *cmdopt;
 		int has_p = 0, has_vV = 0, has_other = 0;
 		argnode = firstnode(preargs);
@@ -2845,6 +2845,7 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 			     * also traditional behaviour.
 			     */
 			    has_p = 1;
+			    pnode = argnode;
 			    break;
 			case 'v':
 			case 'V':
@@ -2882,6 +2883,12 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 		} else if (has_p) {
 		    /* Use default path */
 		    use_defpath = 1;
+		    /*
+		     * We don't need this node as we're not treating
+		     * "command" as a builtin this time.
+		     */
+		    if (pnode)
+			uremnode(preargs, pnode);
 		}
 		/*
 		 * Else just any trailing
diff --git a/Test/A01grammar.ztst b/Test/A01grammar.ztst
index 37311ce..9625a15 100644
--- a/Test/A01grammar.ztst
+++ b/Test/A01grammar.ztst
@@ -116,9 +116,11 @@
 
   (\exec /bin/sh -c 'echo Test one'; print Not reached)
   ('exec' /bin/sh -c 'echo Test two'; print Not reached)
-0:exec with quotes
+  (\exec -c /bin/sh -c 'echo Test three'; print Not reached)
+0:precommand modifiers with quotes
 >Test one
 >Test two
+>Test three
 
   cat() { echo Function cat executed; }
   command cat && unfunction cat
@@ -126,6 +128,14 @@
 <External command cat executed
 >External command cat executed
 
+  (command -p echo this is output)
+  (\command -p echo this is more output)
+  ('command' -p echo this is yet more output)
+0: command -p without -v or -V
+>this is output
+>this is more output
+>this is yet more output
+
   command -pv cat
   command -pv echo
   command -p -V cat

Re: PATCH: perform expansion for precommand modifiers

[Daniel Shahaf]

Peter Stephenson wrote on Fri, Apr 28, 2017 at 10:03:38 +0100:
> Bart Schaefer <schaefer@brasslantern.com> wrote:
> > On Apr 27,  9:40am, Peter Stephenson wrote:
> > This isn't testing the right thing.  The complaint was that the -c option
> > of exec was treated as a command name if the keyword was quoted.
> 
> I think we can fix this by adding a test for that.
> 
> > Similarly for "command -p" which now that I try it is still broken:
> > 
> > torch% \command -p echo
> > zsh: command not found: -p
> 
> That's nothing to do with the quoting.  I assumed we had a test for -p
> without -v or -V, but apparently we don't, so I didn't notice there was
> an extra node to remove.

Updated docs, replaced 41013.

diff --git a/NEWS b/NEWS
index 5479251..568b160 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,17 @@ CHANGES FROM PREVIOUS VERSIONS OF ZSH
 
 Note also the list of incompatibilities in the README file.
 
+Changes from 5.3.1 to 5.4
+-------------------------
+
+The 'exec' and 'command' precommand modifiers, and options to them, are
+now parsed after parameter expansion.  Previously, both the modifier and
+any options to it were parsed between alias expansion and parameter
+expansion (see zshexpn(1)), so they could neither be quoted nor be the
+result of parameter expansion.  Examples: 's=command; $s -V ls' and
+'\command -V ls' now work as expected.
+
+
 Changes from 5.2 to 5.3.1
 -------------------------