PATCH: 3.1.4: autocd to directory in $PATH

PATCH: 3.1.4: autocd to directory in $PATH

[Peter Stephenson]

Here's the bare bones patch for the bug that if a directory is both in
$cdpath and $path you can't autocd to a subdirectory of it because
that's hashed as a command.

It turns out that there's no major problem if a command to be executed
is in the $path first as a directory, then as a command: the command
is hashed incorrectly (as `hash' shows), but the exec is retried and
works.  Also, which/whence does a more sophisticated search and finds
the true command.

(What's happening is zsh tries to exec the hashed command, and if the
exec fails, only then does the command get searched for.  I toyed for
a moment with the idea of doing without command hashing, but there are
almost certainly too many ramifications.)

Anyway, at the moment it looks to me like nothing other than the full
stat() horror would be a really worthwhile improvement on the
following.

*** Src/exec.c.hc	Mon Sep 28 10:37:43 1998
--- Src/exec.c	Mon Sep 28 11:37:19 1998
***************
*** 524,529 ****
--- 524,542 ----
  
  /**/
  int
+ isreallycom(Cmdnam cn)
+ {
+     char fullnam[MAXCMDLEN];
+     struct stat statbuf;
+ 
+     strcpy(fullnam, cn->u.name ? *(cn->u.name) : "");
+     strcat(fullnam, "/");
+     strcat(fullnam, cn->nam);
+     return iscom(fullnam);
+ }
+ 
+ /**/
+ int
  isrelative(char *s)
  {
      if (*s != '/')
***************
*** 1489,1494 ****
--- 1502,1509 ----
  	    char *cmdarg = (char *) peekfirst(args);
  
  	    hn = cmdnamtab->getnode(cmdnamtab, cmdarg);
+ 	    if (hn && !isreallycom((Cmdnam)hn))
+ 		hn = NULL;
  	    if (!hn && isset(HASHCMDS) && strcmp(cmdarg, "..")) {
  		for (s = cmdarg; *s && *s != '/'; s++);
  		if (!*s)

-- 
Peter Stephenson <pws@ibmth.df.unipi.it>       Tel: +39 050 844536
WWW:  http://www.ifh.de/~pws/
Gruppo Teorico, Dipartimento di Fisica
Piazza Torricelli 2, 56100 Pisa, Italy

Re: PATCH: 3.1.4: autocd to directory in $PATH

[Bart Schaefer]

On Oct 3,  5:30pm, Peter Stephenson wrote:
} Subject: PATCH: 3.1.4: autocd to directory in $PATH
}
} at the moment it looks to me like nothing other than the full
} stat() horror would be a really worthwhile improvement on
[...]
} ***************
} *** 1489,1494 ****
} --- 1502,1509 ----
}   	    char *cmdarg = (char *) peekfirst(args);
}   
}   	    hn = cmdnamtab->getnode(cmdnamtab, cmdarg);
} + 	    if (hn && !isreallycom((Cmdnam)hn))
} + 		hn = NULL;
}   	    if (!hn && isset(HASHCMDS) && strcmp(cmdarg, "..")) {
}   		for (s = cmdarg; *s && *s != '/'; s++);
}   		if (!*s)
		    hn = (HashNode) hashcmd(cmdarg, pathchecked);

(One more line of context added for clarity.)

I haven't actually walked through this with a debugger, but it looks to
me as though hn = NULL at that point is going to trigger the call to
hashcmd(cmdarg, pathchecked), which in turn is going to call iscom() a
second time; so you end up stat()ing the directory twice.  Perhaps

 	    if (hn && !isreallycom((Cmdnam)hn))
 		hn = NULL;
 	    else if (!hn && isset(HASHCMDS) && strcmp(cmdarg, "..")) {
            ^^^^
would be better?

Also, what about removing the improperly hashed directory from cmdnamtab
once you've discovered that it isn't a command?  That would (I think) get
any real command with the same name put into the table the next time that
name was used, so that you'll never needlessly stat() again.

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

PATCH: 3.1.4: autocd to directory in $PATH (2)

[Peter Stephenson]

"Bart Schaefer" wrote:
> I haven't actually walked through this with a debugger, but it looks to
> me as though hn = NULL at that point is going to trigger the call to
> hashcmd(cmdarg, pathchecked), which in turn is going to call iscom() a
> second time; so you end up stat()ing the directory twice.

It does call hashcmd() again, but that's right... consider what can
happen otherwise:

% ./zsh -f
% mkdir ~/foo1 ~/foo2
% path=(~/foo1 ~/foo2 $path)
% cdpath=(~/foo1 .)      
% mkdir ~/foo1/file1
% echo "#\!/bin/sh\necho This is \$0" >~/foo2/file1
% chmod +x ~/foo2/file1
% setopt autocd
% file1                         # problem here:  cd's to ~/foo1/file1
~/foo1/file1
% pwd
/home/user2/pws/foo1/file1
% which file1
/home/user2/pws/foo2/file1
% file1                         # correct this time:  call ~/foo2/file1
This is /home/user2/pws/foo2/file1


The directory does get stat'ed twice, but it's hard to avoid without
more fiddling around with special cases.  The next fix does at least
mean it's restricted to twice altogether.

> Also, what about removing the improperly hashed directory from cmdnamtab
> once you've discovered that it isn't a command?  That would (I think) get
> any real command with the same name put into the table the next time that
> name was used, so that you'll never needlessly stat() again.

I meant to do that, there were only vague historical reasons why I
didn't.  Here's another replacement patch, this time for 4404.  I've
expunged an uncessary statbuf declaration, too.

*** Src/exec.c.hc	Mon Sep 28 10:37:43 1998
--- Src/exec.c	Mon Oct 12 14:10:37 1998
***************
*** 524,529 ****
--- 524,541 ----
  
  /**/
  int
+ isreallycom(Cmdnam cn)
+ {
+     char fullnam[MAXCMDLEN];
+ 
+     strcpy(fullnam, cn->u.name ? *(cn->u.name) : "");
+     strcat(fullnam, "/");
+     strcat(fullnam, cn->nam);
+     return iscom(fullnam);
+ }
+ 
+ /**/
+ int
  isrelative(char *s)
  {
      if (*s != '/')
***************
*** 1489,1494 ****
--- 1501,1511 ----
  	    char *cmdarg = (char *) peekfirst(args);
  
  	    hn = cmdnamtab->getnode(cmdnamtab, cmdarg);
+ 	    if (hn && !isreallycom((Cmdnam)hn)) {
+ 		cmdnamtab->removenode(cmdnamtab, cmdarg);
+ 		cmdnamtab->freenode(hn);
+ 		hn = NULL;
+ 	    }
  	    if (!hn && isset(HASHCMDS) && strcmp(cmdarg, "..")) {
  		for (s = cmdarg; *s && *s != '/'; s++);
  		if (!*s)

-- 
Peter Stephenson <pws@ibmth.df.unipi.it>       Tel: +39 050 844536
WWW:  http://www.ifh.de/~pws/
Dipartimento di Fisica, Via Buonarotti 2, 56100 Pisa, Italy

PATCH: Re: PATCH: 3.1.4: autocd to directory in $PATH (2)

[Bart Schaefer]

On Oct 12,  2:18pm, Peter Stephenson wrote:
} Subject: PATCH: 3.1.4: autocd to directory in $PATH (2)
}
} The directory does get stat'ed twice, but it's hard to avoid without
} more fiddling around with special cases.  The next fix does at least
} mean it's restricted to twice altogether.

It bothers me that this happens on every command execution.

Here's a patch, which must be applied on top of Peter's patch, that
restricts the two calls to stat to cases where an autocd might occur.
Specifically, this means:
    * autocd is set
    * the shell is reading commands from stdin
    * there are no I/O redirections to perform
    * there are no command arguments to pass
[I simply moved computation of these four conditions to the top of the
function, then tested the result both in the original context and also
before calling isreallycom().]

Thus there should be no performance penalty (compared to previous zsh
versions) for the great majority of commands, nor for any command if
you don't set autocd.

I did have two remarks about isreallycom():

} + isreallycom(Cmdnam cn)
} + {
} +     char fullnam[MAXCMDLEN];

Other places in the code use PATH_MAX size buffer for the same purpose.
Which is better?

There also seems to be inconsistency as to:

} +     strcpy(fullnam, cn->u.name ? *(cn->u.name) : "");
} +     strcat(fullnam, "/");
} +     strcat(fullnam, cn->nam);

as opposed to:
                    z = buf;
                    strucpy(&z, *pp);
                    *z++ = '/';
                    strcpy(z, arg0);

My patch doesn't change any of that stuff, I'm just pointing it out.

Index: Src/exec.c
===================================================================
*** exec.c	1998/10/14 18:19:05	1.4
--- exec.c	1998/10/14 18:22:09
***************
*** 1495,1500 ****
--- 1495,1504 ----
  
      if (type == SIMPLE && !nullexec) {
  	char *s;
+ 	char trycd = (isset(AUTOCD) && isset(SHINSTDIN)
+ 		      && empty(cmd->redir) && !empty(args)
+ 		      && !nextnode(firstnode(args))
+ 		      && *(char *)peekfirst(args));
  
  	DPUTS(empty(args), "BUG: empty(args) in exec.c");
  	if (!hn) {
***************
*** 1502,1508 ****
  	    char *cmdarg = (char *) peekfirst(args);
  
  	    hn = cmdnamtab->getnode(cmdnamtab, cmdarg);
! 	    if (hn && !isreallycom((Cmdnam)hn)) {
  		cmdnamtab->removenode(cmdnamtab, cmdarg);
  		cmdnamtab->freenode(hn);
  		hn = NULL;
--- 1506,1512 ----
  	    char *cmdarg = (char *) peekfirst(args);
  
  	    hn = cmdnamtab->getnode(cmdnamtab, cmdarg);
! 	    if (hn && trycd && !isreallycom((Cmdnam)hn)) {
  		cmdnamtab->removenode(cmdnamtab, cmdarg);
  		cmdnamtab->freenode(hn);
  		hn = NULL;
***************
*** 1516,1524 ****
  
  	/* If no command found yet, see if it  *
  	 * is a directory we should AUTOCD to. */
! 	if (!hn && isset(AUTOCD) && isset(SHINSTDIN) && empty(cmd->redir)
! 	    && !nextnode(firstnode(args)) && *(char *)peekfirst(args)
! 	    && (s = cancd(peekfirst(args)))) {
  	    peekfirst(args) = (void *) s;
  	    pushnode(args, dupstring("cd"));
  	    if ((hn = builtintab->getnode(builtintab, "cd")))
--- 1520,1526 ----
  
  	/* If no command found yet, see if it  *
  	 * is a directory we should AUTOCD to. */
! 	if (!hn && trycd && (s = cancd(peekfirst(args)))) {
  	    peekfirst(args) = (void *) s;
  	    pushnode(args, dupstring("cd"));
  	    if ((hn = builtintab->getnode(builtintab, "cd")))


-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com