PATCH: perform expansion for precommand modifiers

[Peter Stephenson <p.stephenson@xxxxxxxxxxx>]

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.