Is "command" working right, yet?

Is "command" working right, yet?

[Bart Schaefer]

command [ -pvV ]
     The command word is taken to be the name of an external command,
     rather than a shell function or builtin.   If the POSIX_BUILTINS
     option is set, builtins will also be executed but certain special
     properties of them are suppressed. The -p flag causes a default
     path to be searched instead of that in $path. With the -v flag,
     command is similar to whence and with -V, it is equivalent to
     whence -v.

Prelimnaries:

I've created my own "true" at the head of $path to be able to distinguish
the builtin from the external command.  It just echoes EXTERNAL, like so:

burner% disable true
burner% true
EXTERNAL

First let's talk about option parsing.

burner% command -p -V true
zsh: command not found: -V
burner% command -V -p true
command: bad option: -p
burner% command -pv true
zsh: command not found: -pv

Those options can be combined in both bash and ksh.  Setting posixbuiltins
makes no difference to zsh:

burner% setopt posixbuiltins
burner% command -p -V true
zsh: command not found: -V
burner% command -V -p true  
command: bad option: -p

I think this is pretty clearly a bug.

The next question has to do with builtins and path search.  There was a
long thread about this on the austin-group (POSIX standards) list several
weeks ago and I'm still not sure what the final outcome implies.  The
issue is the circumstances in which builtins are allowed to mask commands
in $PATH. Starting over with a fresh "zsh -f" so the builtin is enabled
and NO_POSIX_BUILTINS, "command" runs the external but "command -p" runs
the builtin:

burner% command true
EXTERNAL
burner% command -p true
burner% 

The latter seems a little odd to me, but I suppose that means that when
the doc says "a default path" it means one with builtins implicitly at the
front?  However, it relates to the last question:

Using the versions available on my Ubuntu desktop, and default $PATH, all
of bash, ksh, and zsh with POSIX_BUILTINS, execute the builtin for both
"command" and "command -p".  Anybody know if that's correct?  If it is,
how does one force the use of the external command in [the forthcoming]
POSIX?  (If it involves magic syntax in $PATH, I can't assert that zsh
is going to adopt it, but I'd still like to know the answer.)

Re: Is "command" working right, yet?

[Martijn Dekker]

Bart Schaefer schreef op 03-02-16 om 00:37:
[...]
> burner% setopt posixbuiltins
> burner% command -p -V true
> zsh: command not found: -V
> burner% command -V -p true  
> command: bad option: -p
> 
> I think this is pretty clearly a bug.

Agreed, these options should combine without a problem.

> Using the versions available on my Ubuntu desktop, and default $PATH, all
> of bash, ksh, and zsh with POSIX_BUILTINS, execute the builtin for both
> "command" and "command -p".

The same for dash, pdksh/mksh and yash.

> Anybody know if that's correct?

I believe so. Normally, builtins are searched for $PATH. The spec for
'command -p'[*] says the command is executed with a default $PATH that
is guaranteed to find all of the standard utilities, but not that
builtins aren't searched first as usual. According to POSIX, builtins
are supposed to be standard commands equivalent to their external
correspondents, so in theory it shouldn't make any difference.

> If it is, how does one force the use of the external command in [the
> forthcoming] POSIX?

The only way I know of is to run it with an explicit path, such as
/bin/echo hi

To find out the path of an external command without considering
builtins, there is 'which' on most systems, but it is not POSIX. I don't
know of any current system without it, though.

For 100% cross-platform compatibility guarantee, it is not too hard to
write a shell function that finds the absolute path of a command. I
wrote this once for an installer that is compatible with the original
Bourne shell as well as POSIX:

# Output the first path of each given command, or, if given -a, all possible
# paths, in the given order, according to the system $PATH. Like BSD
'which'.
# Returns successfully if at least one path was found, unsuccessfully if
not.
# This abridged function ignores paths (as in 'which /bin/ls').
which() { (
    unset opt_allpaths flag_somenotfound
    IFS=':'  # separator in $PATH
    test "$1" = '-a' && opt_allpaths='y' && shift
    for arg do
        unset flag_foundthisone
        cmd=`basename $arg`
        for dir in $PATH; do  # for native zsh, you want ${=PATH}
            if test -f "$dir/$cmd" -a -x "$dir/$cmd"; then
                flag_foundthisone='y'
                printf '%s/%s\n' "$dir" "$cmd"
                test -z "$opt_allpaths" && break
            fi
        done
        test -n "$flag_foundthisone" || flag_somenotfound=y
    done
    test -z "$flag_somenotfound"
); }

- M.

[*]
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/command.html#tag_20_22_04

Re: Is "command" working right, yet?

[Martijn Dekker]

Op 07-02-16 om 15:24 schreef Martijn Dekker:
> Bart Schaefer schreef op 03-02-16 om 00:37:
> [...]
>> > burner% setopt posixbuiltins
>> > burner% command -p -V true
>> > zsh: command not found: -V
>> > burner% command -V -p true  
>> > command: bad option: -p
>> > 
>> > I think this is pretty clearly a bug.
> Agreed, these options should combine without a problem.

Resuming this old thread, it would be nice to get this fixed before zsh
5.3 is released. It's the only clear POSIX issue left on zsh that I know
of, and I've found a few in the past...

The culprit seems to be the simplistic option parsing code starting at
line 2664 in Src/exec.c (current git, e35dcae):

|            if ((cflags & BINF_COMMAND) && nextnode(firstnode(args))) {
|                 /* check for options to command builtin */
|                 char *next = (char *) getdata(nextnode(firstnode(args)));
|                 char *cmdopt;
|                 if (next && *next == '-' && strlen(next) == 2 &&
|                         (cmdopt = strchr("pvV", next[1])))
|                 {
|                     if (*cmdopt == 'p') {
|                         uremnode(args, firstnode(args));
|                         use_defpath = 1;
|                         if (nextnode(firstnode(args)))
|                             next = (char *)
getdata(nextnode(firstnode(args)));
|                     } else {
|                         hn = &commandbn.node;
|                         is_builtin = 1;
|                         break;
|                     }
|                 }
|                 if (!strcmp(next, "--"))
|                      uremnode(args, firstnode(args));
|             }

Sure enough, it looks if there's one option and if there is, it happily
ignores the rest. Combined options like -pV also aren't supported.

I don't understand nearly enough about the zsh codebase to know why this
routine does its own option parsing rather than calling some common
option parsing function, so I can't offer suggestions for improvement
here that aren't likely to be completely stupid.

Thanks,

- M.

Re: Is "command" working right, yet?

[Bart Schaefer]

On Sep 26, 12:31am, Martijn Dekker wrote:
}
} The culprit seems to be the simplistic option parsing code starting at
} line 2664 in Src/exec.c (current git, e35dcae):
} 
} Sure enough, it looks if there's one option and if there is, it happily
} ignores the rest. Combined options like -pV also aren't supported.

Not exactly.  That whole thing is in a "while" loop, so as long as -p
and -v and/or -V are in separate words I think the expectation was
that it would loop around and discover the next one .. but if -p is
first, it doesn't loop, and if -v or -V is first then -p is rejected
because it's not [and can't be, see below] listed in the value of
"commandbn" [exec.c 209].

} I don't understand nearly enough about the zsh codebase to know why this
} routine does its own option parsing rather than calling some common
} option parsing function, so I can't offer suggestions for improvement

It's because "command" is a sort-of keyword instead of a builtin, so it
doesn't go through the regular builtin dispatch where the common option
parsing lives.  Originally zsh's "command" prefix didn't accept any
options, so this parsing was bolted on bit by bit as POSIX defined them.

A complication is that "command -v" is defined in terms of bin_whence(),
and "whence" has a -p option that means something different.

Re: Is "command" working right, yet?

[Martijn Dekker]

Op 26-09-16 om 04:13 schreef Bart Schaefer:
> It's because "command" is a sort-of keyword instead of a builtin,

I see.

I guess that explains another bug I've noticed: if you quote the
"command" command name to defeat a possible alias, it doesn't accept
options at all, e.g.:

% \command -V if
zsh: command not found: -V

Also, then this output:

% command -V command
command is a shell builtin

seems a bit misleading.

- Martijn

Re: Is "command" working right, yet?

[Bart Schaefer]

On Sep 26, 11:56am, Martijn Dekker wrote:
} Subject: Re: Is "command" working right, yet?
}
} I guess that explains another bug I've noticed: if you quote the
} "command" command name to defeat a possible alias, it doesn't accept
} options at all

That would be related, yes.  I wouldn't have been surprised if quoting
it resulted in "zsh: command not found: command"

} % command -V command
} command is a shell builtin
} 
} seems a bit misleading.

It says the same thing for "exec" and "noglob" and "-"; there isn't a
separate output for "precommand modifier".

Re: Is "command" working right, yet?

[Peter Stephenson]

On Sun, 25 Sep 2016 20:13:28 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> A complication is that "command -v" is defined in terms of bin_whence(),
> and "whence" has a -p option that means something different.

That also means path search.

I don't understand what any of

command -p -v true
command -v -p true

could possibly mean other than "whence -vp -- true".  The "-p" can't mean
execute along the path in this case, because the "-v" or "-V" clearly
marks this as a query.

Surely it's just a case of finding all the valid options (p, v, V)  and
seeing if there's anything other than "p" present?

The real minefield appears to be what to do with invalid options.

pws

Re: Is "command" working right, yet?

[Martijn Dekker]

Op 27-09-16 om 11:08 schreef Peter Stephenson:
> On Sun, 25 Sep 2016 20:13:28 -0700
> Bart Schaefer <schaefer@brasslantern.com> wrote:
>> A complication is that "command -v" is defined in terms of bin_whence(),
>> and "whence" has a -p option that means something different.
> 
> That also means path search.

Nope.

'command -p' means: ignore the PATH environment variable and do the
search as normal, but (if it's a path search) use the system default
path as output by the 'getconf PATH' command. That means the -p option
has no effect for builtins.

POSIX provides this as one way to make shell scripts a bit more robust;
'command -p grep' guarantees you get either the system's external 'grep'
or a builtin version provided by the shell, and not some other 'grep'
that the user (or someone who hacked them) may have installed in some
location that comes before the real 'grep' in $PATH and that may, for
all anyone knows, delete all their files.

Reference:
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/command.html#tag_20_22_04

This is why it's important that '-p' should combine properly with '-v'
and '-V'. The command 'command -pv grep', provided there is no builtin
for it, should output the location of the system default 'grep' as
opposed to whatever 'grep' may take precedence over it in $PATH.

By the way, 'command -p' *nearly* works properly (i.e. as described
above) in zsh, apart from combining with other options. See line 664 and
on in exec.c, line 2030 and on in configure.ac, and line 77 in config.h.in.

The problem is configure.ac only tries non-standard 'getconf' commands
(i.e. 'getconf _CS_PATH' and 'getconf CS_PATH'); the command to use on
BSD/OSX is 'getconf PATH', which works on Linux as well. I don't know if
the other two should be kept, but 'getconf PATH' should at least be added.

diff --git a/configure.ac b/configure.ac
index 0e0bd53..e0eb320 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2035,6 +2035,8 @@ AC_CACHE_VAL(zsh_cv_cs_path,
   zsh_cv_cs_path=`getconf _CS_PATH`
 elif getconf CS_PATH >/dev/null 2>&1; then
   zsh_cv_cs_path=`getconf CS_PATH`
+elif getconf PATH >/dev/null 2>&1; then
+  zsh_cv_cs_path=`getconf PATH`
 else
   zsh_cv_cs_path="/bin:/usr/bin"
 fi])


Thanks,

- M.

Re: Is "command" working right, yet?

[Peter Stephenson]

On Tue, 27 Sep 2016 13:15:23 +0100
Martijn Dekker <martijn@inlv.org> wrote:
> 'command -p' means: ignore the PATH environment variable and do the
> search as normal, but (if it's a path search) use the system default
> path as output by the 'getconf PATH' command. That means the -p option
> has no effect for builtins.

OK, I think that means you want something like the following, where
command -p in combination with -v or -V uses the default path to do the
search and print the result.

I've committed your tweak to configure.ac.

pws

diff --git a/Src/builtin.c b/Src/builtin.c
index 248f929..2b2275a 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -3643,7 +3643,7 @@ bin_whence(char *nam, char **argv, Options ops, int func)
 		returnval = 1;
 	    }
 	    popheap();
-	} else if ((cnam = findcmd(*argv, 1))) {
+	} else if ((cnam = findcmd(*argv, 1, func == BIN_COMMAND))) {
 	    /* Found external command. */
 	    if (wd) {
 		printf("%s: command\n", *argv);
diff --git a/Src/exec.c b/Src/exec.c
index c27c41c..c79a278 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -207,7 +207,7 @@ static int (*execfuncs[WC_COUNT-WC_CURSH]) _((Estate, int)) = {
 
 /* structure for command builtin for when it is used with -v or -V */
 static struct builtin commandbn =
-    BUILTIN(0, 0, bin_whence, 0, -1, BIN_COMMAND, "vV", NULL);
+    BUILTIN("command", 0, bin_whence, 0, -1, BIN_COMMAND, "pvV", NULL);
 
 /* parse string into a list */
 
@@ -575,6 +575,41 @@ commandnotfound(char *arg0, LinkList args)
     return doshfunc(shf, args, 1);
 }
 
+/*
+ * Search the default path for cmd.
+ * pbuf of length plen is the buffer to use.
+ * Return NULL if not found.
+ */
+
+static char *
+search_defpath(char *cmd, char *pbuf, int plen)
+{
+    char *ps = DEFAULT_PATH, *pe = NULL, *s;
+
+    for (ps = DEFAULT_PATH; ps; ps = pe ? pe+1 : NULL) {
+	pe = strchr(ps, ':');
+	if (*ps == '/') {
+	    s = pbuf;
+	    if (pe) {
+		if (pe - ps >= plen)
+		    continue;
+		struncpy(&s, ps, pe-ps);
+	    } else {
+		if (strlen(ps) >= plen)
+		    continue;
+		strucpy(&s, ps);
+	    }
+	    *s++ = '/';
+	    if ((s - pbuf) + strlen(cmd) >= plen)
+		continue;
+	    strucpy(&s, cmd);
+	    if (iscom(pbuf))
+		return pbuf;
+	}
+    }
+    return NULL;
+}
+
 /* execute an external command */
 
 /**/
@@ -663,27 +698,10 @@ execute(LinkList args, int flags, int defpath)
 
     /* for command -p, search the default path */
     if (defpath) {
-	char *s, pbuf[PATH_MAX];
-	char *dptr, *pe, *ps = DEFAULT_PATH;
-
-	for(;ps;ps = pe ? pe+1 : NULL) {
-	    pe = strchr(ps, ':');
-	    if (*ps == '/') {
-		s = pbuf;
-		if (pe)
-		    struncpy(&s, ps, pe-ps);
-		else
-		    strucpy(&s, ps);
-		*s++ = '/';
-		if ((s - pbuf) + strlen(arg0) >= PATH_MAX)
-		    continue;
-		strucpy(&s, arg0);
-		if (iscom(pbuf))
-		    break;
-	    }
-	}
+	char pbuf[PATH_MAX];
+	char *dptr;
 
-	if (!ps) {
+	if (!search_defpath(arg0, pbuf, PATH_MAX)) {
 	    if (commandnotfound(arg0, args) == 0)
 		_exit(0);
 	    zerr("command not found: %s", arg0);
@@ -760,17 +778,26 @@ execute(LinkList args, int flags, int defpath)
 /*
  * Get the full pathname of an external command.
  * If the second argument is zero, return the first argument if found;
- * if non-zero, return the path using heap memory.  (RET_IF_COM(X), above).
+ * if non-zero, return the path using heap memory.  (RET_IF_COM(X),
+ * above).
+ * If the third argument is non-zero, use the system default path
+ * instead of the current path.
  */
 
 /**/
 mod_export char *
-findcmd(char *arg0, int docopy)
+findcmd(char *arg0, int docopy, int default_path)
 {
     char **pp;
     char *z, *s, buf[MAXCMDLEN];
     Cmdnam cn;
 
+    if (default_path)
+    {
+	if (search_defpath(arg0, buf, MAXCMDLEN))
+	    return docopy ? dupstring(buf) : arg0;
+	return NULL;
+    }
     cn = (Cmdnam) cmdnamtab->getnode(cmdnamtab, arg0);
     if (!cn && isset(HASHCMDS) && !isrelative(arg0))
 	cn = hashcmd(arg0, path);
@@ -2662,24 +2689,76 @@ execcmd(Estate state, int input, int output, int how, int last1)
 	    }
 	    checked = 0;
 	    if ((cflags & BINF_COMMAND) && nextnode(firstnode(args))) {
-		/* check for options to command builtin */
-		char *next = (char *) getdata(nextnode(firstnode(args)));
+		/*
+		 * Check for options to "command".
+		 * If just -p, this is handled here: use the default
+		 * path to execute.
+		 * If -v or -V, possibly with -p, dispatch to bin_whence
+		 * but with flag to indicate special handling of -p.
+		 * Otherwise, just leave marked as BINF_COMMAND
+		 * modifier with no additional action.
+		 */
+		LinkNode argnode = nextnode(firstnode(args));
+		char *argdata = (char *) getdata(argnode);
 		char *cmdopt;
-		if (next && *next == '-' && strlen(next) == 2 &&
-		        (cmdopt = strchr("pvV", next[1])))
-		{
-		    if (*cmdopt == 'p') {
-			uremnode(args, firstnode(args));
-			use_defpath = 1;
-			if (nextnode(firstnode(args)))
-			    next = (char *) getdata(nextnode(firstnode(args)));
-		    } else {
-			hn = &commandbn.node;
-			is_builtin = 1;
+		int has_p = 0, has_vV = 0, has_other = 0;
+		while (*argdata == '-') {
+		    /* Just to be definite, stop on single "-", too, */
+		    if (!argdata[1] || (argdata[1] == '-' && !argdata[2]))
+			break;
+		    for (cmdopt = argdata+1; *cmdopt; cmdopt++) {
+			switch (*cmdopt) {
+			case 'p':
+			    /*
+			     * If we've got this multiple times (command
+			     * -p -p) we'll treat the second -p as a
+			     * command because we only remove one below.
+			     * Don't think that's a big issue, and it's
+			     * also traditional behaviour.
+			     */
+			    has_p = 1;
+			    break;
+			case 'v':
+			case 'V':
+			    has_vV = 1;
+			    break;
+			default:
+			    has_other = 1;
+			    break;
+			}
+		    }
+		    if (has_other) {
+			/* Don't know how to handle this, so don't */
+			has_p = has_vV = 0;
 			break;
 		    }
+
+		    argnode = nextnode(argnode);
+		    if (!argnode)
+			break;
+		    argdata = (char *) getdata(argnode);
 		}
-		if (!strcmp(next, "--"))
+		if (has_vV) {
+		    /* Leave everything alone, dispatch to whence */
+		    hn = &commandbn.node;
+		    is_builtin = 1;
+		    break;
+		} else if (has_p) {
+		    /* Use default path; absorb command and option. */
+		    uremnode(args, firstnode(args));
+		    use_defpath = 1;
+		    if ((argnode = nextnode(firstnode(args))))
+			argdata = (char *) getdata(argnode);
+		}
+		/*
+		 * Else just absorb command and 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
+		 * as if this is command [non-option-stuff].  This
+		 * isn't a good place for standard option handling.
+		 */
+		if (!strcmp(argdata, "--"))
 		     uremnode(args, firstnode(args));
 	    }
 	    if ((cflags & BINF_EXEC) && nextnode(firstnode(args))) {
diff --git a/Src/subst.c b/Src/subst.c
index 92fde45..ecd7487 100644
--- a/Src/subst.c
+++ b/Src/subst.c
@@ -627,7 +627,7 @@ equalsubstr(char *str, int assign, int nomatch)
     cmdstr = dupstrpfx(str, pp-str);
     untokenize(cmdstr);
     remnulargs(cmdstr);
-    if (!(cnam = findcmd(cmdstr, 1))) {
+    if (!(cnam = findcmd(cmdstr, 1, 0))) {
 	if (nomatch)
 	    zerr("%s not found", cmdstr);
 	return NULL;

Re: Is "command" working right, yet?

[Martijn Dekker]

Op 28-09-16 om 11:30 schreef Peter Stephenson:
> On Tue, 27 Sep 2016 13:15:23 +0100
> Martijn Dekker <martijn@inlv.org> wrote:
>> > 'command -p' means: ignore the PATH environment variable and do the
>> > search as normal, but (if it's a path search) use the system default
>> > path as output by the 'getconf PATH' command. That means the -p option
>> > has no effect for builtins.
> OK, I think that means you want something like the following, where
> command -p in combination with -v or -V uses the default path to do the
> search and print the result.

Nearly. The options combine now, but builtins don't take precedence. As
explained above, I'd expect "command -pv echo" to output simply "echo"
and not "/bin/echo", because it's a builtin. Also 'command -pv :' should
output ':'.

Ref.:
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/command.html#tag_20_22
(note: the wordage under -p, "Perform the command search using a default
value for PATH [...]", does not mean "make it a path search even if
there's a matching builtin"; it just means "ignore $PATH and use the
system default path instead, if applicable".)

Thanks,

- M.

Re: Is "command" working right, yet?

[Peter Stephenson]

On Wed, 28 Sep 2016 19:37:46 +0100
Martijn Dekker <martijn@inlv.org> wrote:
> The options combine now, but builtins don't take precedence. As
> explained above, I'd expect "command -pv echo" to output simply "echo"
> and not "/bin/echo", because it's a builtin. Also 'command -pv :' should
> output ':'.

OK, I've simply added that as a special case otherwise bin_whence() is
going to get too tortuous, plus some tests.

pws

diff --git a/Src/builtin.c b/Src/builtin.c
index 248f929..c78fd9b 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -3643,7 +3643,15 @@ bin_whence(char *nam, char **argv, Options ops, int func)
 		returnval = 1;
 	    }
 	    popheap();
-	} else if ((cnam = findcmd(*argv, 1))) {
+	} else if (func == BIN_COMMAND &&
+		   (hn = builtintab->getnode(builtintab, *argv))) {
+	    /*
+	     * Special case for "command -p[vV]" which needs to
+	     * show a builtin in preference to an external command.
+	     */
+	    builtintab->printnode(hn, printflags);
+	    informed = 1;
+	} else if ((cnam = findcmd(*argv, 1, func == BIN_COMMAND))) {
 	    /* Found external command. */
 	    if (wd) {
 		printf("%s: command\n", *argv);
diff --git a/Src/exec.c b/Src/exec.c
index c27c41c..c79a278 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -207,7 +207,7 @@ static int (*execfuncs[WC_COUNT-WC_CURSH]) _((Estate, int)) = {
 
 /* structure for command builtin for when it is used with -v or -V */
 static struct builtin commandbn =
-    BUILTIN(0, 0, bin_whence, 0, -1, BIN_COMMAND, "vV", NULL);
+    BUILTIN("command", 0, bin_whence, 0, -1, BIN_COMMAND, "pvV", NULL);
 
 /* parse string into a list */
 
@@ -575,6 +575,41 @@ commandnotfound(char *arg0, LinkList args)
     return doshfunc(shf, args, 1);
 }
 
+/*
+ * Search the default path for cmd.
+ * pbuf of length plen is the buffer to use.
+ * Return NULL if not found.
+ */
+
+static char *
+search_defpath(char *cmd, char *pbuf, int plen)
+{
+    char *ps = DEFAULT_PATH, *pe = NULL, *s;
+
+    for (ps = DEFAULT_PATH; ps; ps = pe ? pe+1 : NULL) {
+	pe = strchr(ps, ':');
+	if (*ps == '/') {
+	    s = pbuf;
+	    if (pe) {
+		if (pe - ps >= plen)
+		    continue;
+		struncpy(&s, ps, pe-ps);
+	    } else {
+		if (strlen(ps) >= plen)
+		    continue;
+		strucpy(&s, ps);
+	    }
+	    *s++ = '/';
+	    if ((s - pbuf) + strlen(cmd) >= plen)
+		continue;
+	    strucpy(&s, cmd);
+	    if (iscom(pbuf))
+		return pbuf;
+	}
+    }
+    return NULL;
+}
+
 /* execute an external command */
 
 /**/
@@ -663,27 +698,10 @@ execute(LinkList args, int flags, int defpath)
 
     /* for command -p, search the default path */
     if (defpath) {
-	char *s, pbuf[PATH_MAX];
-	char *dptr, *pe, *ps = DEFAULT_PATH;
-
-	for(;ps;ps = pe ? pe+1 : NULL) {
-	    pe = strchr(ps, ':');
-	    if (*ps == '/') {
-		s = pbuf;
-		if (pe)
-		    struncpy(&s, ps, pe-ps);
-		else
-		    strucpy(&s, ps);
-		*s++ = '/';
-		if ((s - pbuf) + strlen(arg0) >= PATH_MAX)
-		    continue;
-		strucpy(&s, arg0);
-		if (iscom(pbuf))
-		    break;
-	    }
-	}
+	char pbuf[PATH_MAX];
+	char *dptr;
 
-	if (!ps) {
+	if (!search_defpath(arg0, pbuf, PATH_MAX)) {
 	    if (commandnotfound(arg0, args) == 0)
 		_exit(0);
 	    zerr("command not found: %s", arg0);
@@ -760,17 +778,26 @@ execute(LinkList args, int flags, int defpath)
 /*
  * Get the full pathname of an external command.
  * If the second argument is zero, return the first argument if found;
- * if non-zero, return the path using heap memory.  (RET_IF_COM(X), above).
+ * if non-zero, return the path using heap memory.  (RET_IF_COM(X),
+ * above).
+ * If the third argument is non-zero, use the system default path
+ * instead of the current path.
  */
 
 /**/
 mod_export char *
-findcmd(char *arg0, int docopy)
+findcmd(char *arg0, int docopy, int default_path)
 {
     char **pp;
     char *z, *s, buf[MAXCMDLEN];
     Cmdnam cn;
 
+    if (default_path)
+    {
+	if (search_defpath(arg0, buf, MAXCMDLEN))
+	    return docopy ? dupstring(buf) : arg0;
+	return NULL;
+    }
     cn = (Cmdnam) cmdnamtab->getnode(cmdnamtab, arg0);
     if (!cn && isset(HASHCMDS) && !isrelative(arg0))
 	cn = hashcmd(arg0, path);
@@ -2662,24 +2689,76 @@ execcmd(Estate state, int input, int output, int how, int last1)
 	    }
 	    checked = 0;
 	    if ((cflags & BINF_COMMAND) && nextnode(firstnode(args))) {
-		/* check for options to command builtin */
-		char *next = (char *) getdata(nextnode(firstnode(args)));
+		/*
+		 * Check for options to "command".
+		 * If just -p, this is handled here: use the default
+		 * path to execute.
+		 * If -v or -V, possibly with -p, dispatch to bin_whence
+		 * but with flag to indicate special handling of -p.
+		 * Otherwise, just leave marked as BINF_COMMAND
+		 * modifier with no additional action.
+		 */
+		LinkNode argnode = nextnode(firstnode(args));
+		char *argdata = (char *) getdata(argnode);
 		char *cmdopt;
-		if (next && *next == '-' && strlen(next) == 2 &&
-		        (cmdopt = strchr("pvV", next[1])))
-		{
-		    if (*cmdopt == 'p') {
-			uremnode(args, firstnode(args));
-			use_defpath = 1;
-			if (nextnode(firstnode(args)))
-			    next = (char *) getdata(nextnode(firstnode(args)));
-		    } else {
-			hn = &commandbn.node;
-			is_builtin = 1;
+		int has_p = 0, has_vV = 0, has_other = 0;
+		while (*argdata == '-') {
+		    /* Just to be definite, stop on single "-", too, */
+		    if (!argdata[1] || (argdata[1] == '-' && !argdata[2]))
+			break;
+		    for (cmdopt = argdata+1; *cmdopt; cmdopt++) {
+			switch (*cmdopt) {
+			case 'p':
+			    /*
+			     * If we've got this multiple times (command
+			     * -p -p) we'll treat the second -p as a
+			     * command because we only remove one below.
+			     * Don't think that's a big issue, and it's
+			     * also traditional behaviour.
+			     */
+			    has_p = 1;
+			    break;
+			case 'v':
+			case 'V':
+			    has_vV = 1;
+			    break;
+			default:
+			    has_other = 1;
+			    break;
+			}
+		    }
+		    if (has_other) {
+			/* Don't know how to handle this, so don't */
+			has_p = has_vV = 0;
 			break;
 		    }
+
+		    argnode = nextnode(argnode);
+		    if (!argnode)
+			break;
+		    argdata = (char *) getdata(argnode);
 		}
-		if (!strcmp(next, "--"))
+		if (has_vV) {
+		    /* Leave everything alone, dispatch to whence */
+		    hn = &commandbn.node;
+		    is_builtin = 1;
+		    break;
+		} else if (has_p) {
+		    /* Use default path; absorb command and option. */
+		    uremnode(args, firstnode(args));
+		    use_defpath = 1;
+		    if ((argnode = nextnode(firstnode(args))))
+			argdata = (char *) getdata(argnode);
+		}
+		/*
+		 * Else just absorb command and 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
+		 * as if this is command [non-option-stuff].  This
+		 * isn't a good place for standard option handling.
+		 */
+		if (!strcmp(argdata, "--"))
 		     uremnode(args, firstnode(args));
 	    }
 	    if ((cflags & BINF_EXEC) && nextnode(firstnode(args))) {
diff --git a/Src/subst.c b/Src/subst.c
index 92fde45..ecd7487 100644
--- a/Src/subst.c
+++ b/Src/subst.c
@@ -627,7 +627,7 @@ equalsubstr(char *str, int assign, int nomatch)
     cmdstr = dupstrpfx(str, pp-str);
     untokenize(cmdstr);
     remnulargs(cmdstr);
-    if (!(cnam = findcmd(cmdstr, 1))) {
+    if (!(cnam = findcmd(cmdstr, 1, 0))) {
 	if (nomatch)
 	    zerr("%s not found", cmdstr);
 	return NULL;
diff --git a/Test/A01grammar.ztst b/Test/A01grammar.ztst
index 394480c..46a58e9 100644
--- a/Test/A01grammar.ztst
+++ b/Test/A01grammar.ztst
@@ -113,6 +113,16 @@
 <External command cat executed
 >External command cat executed
 
+  command -pv cat
+  command -pv echo
+  command -p -V cat
+  command -p -V -- echo
+0:command -p in combination
+*>*/cat
+>echo
+>cat is /*/cat
+>echo is a shell builtin
+
   cd() { echo Not cd at all; }
   builtin cd . && unfunction cd
 0:`builtin' precommand modifier

Re: Is "command" working right, yet?

[Martijn Dekker]

Op 29-09-16 om 10:55 schreef Peter Stephenson:
> On Wed, 28 Sep 2016 19:37:46 +0100
> Martijn Dekker <martijn@inlv.org> wrote:
>> The options combine now, but builtins don't take precedence. As
>> explained above, I'd expect "command -pv echo" to output simply "echo"
>> and not "/bin/echo", because it's a builtin. Also 'command -pv :' should
>> output ':'.
> 
> OK, I've simply added that as a special case otherwise bin_whence() is
> going to get too tortuous, plus some tests.

Another issue crept in: 'command -v' and 'command -V' now always act as
if the -p option is given, i.e. they always search the default system
PATH for external commands rather than the current one.

- Martijn

Re: Is "command" working right, yet?

[Peter Stephenson]

On Tue, 11 Oct 2016 15:40:33 +0200
Martijn Dekker <martijn@inlv.org> wrote:
> Another issue crept in: 'command -v' and 'command -V' now always act as
> if the -p option is given, i.e. they always search the default system
> PATH for external commands rather than the current one.

They are assuming -p is set, yes.

pws

diff --git a/Src/builtin.c b/Src/builtin.c
index a274ff7..8b8b217 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -3663,7 +3663,7 @@ bin_whence(char *nam, char **argv, Options ops, int func)
 		returnval = 1;
 	    }
 	    popheap();
-	} else if (func == BIN_COMMAND &&
+	} else if (func == BIN_COMMAND && OPT_ISSET(ops,'p') &&
 		   (hn = builtintab->getnode(builtintab, *argv))) {
 	    /*
 	     * Special case for "command -p[vV]" which needs to
@@ -3671,7 +3671,9 @@ bin_whence(char *nam, char **argv, Options ops, int func)
 	     */
 	    builtintab->printnode(hn, printflags);
 	    informed = 1;
-	} else if ((cnam = findcmd(*argv, 1, func == BIN_COMMAND))) {
+	} else if ((cnam = findcmd(*argv, 1,
+				   func == BIN_COMMAND &&
+				   OPT_ISSET(ops,'p')))) {
 	    /* Found external command. */
 	    if (wd) {
 		printf("%s: command\n", *argv);

Re: Is "command" working right, yet?

[Martijn Dekker]

And another one (sorry): options aren't parsed at all if they're the
result of an expansion.

% defpath=-p
% command $defpath true
zsh: command not found: -p

- Martijn

Re: Is "command" working right, yet?

[Peter Stephenson]

On Tue, 11 Oct 2016 18:20:55 +0200
Martijn Dekker <martijn@inlv.org> wrote:
> And another one (sorry): options aren't parsed at all if they're the
> result of an expansion.
> 
> % defpath=-p
> % command $defpath true
> zsh: command not found: -p

I've got some ideas about this that could be generally beneficial, but
they're quite intrusive, so this is probably going to get punted till
after the release.

The case of

command $(print -- -p) true

looks less tractable, however, without more rearrangement than I think
is workable.  It doesn't seem a particularly serious problem.

pws

Re: Is "command" working right, yet?

[Peter Stephenson]

On Thu, 20 Oct 2016 13:44:20 +0100
Peter Stephenson <p.stephenson@samsung.com> wrote:
> The case of
> 
> command $(print -- -p) true
> 
> looks less tractable, however, without more rearrangement than I think
> is workable.  It doesn't seem a particularly serious problem.

I beg your pardon, I got that wrong.  The only substitution left till
after the fork is globbing, which is certainly not useful.  So ignore
the above.

pws