cd bugs

cd bugs

[Eric Blake]

POSIX requires cd to give output if a nonempty entry in CDPATH played a role, or
if 'cd -' was used.  Therefore, this line demonstrates two zsh bugs:

$ zsh -c 'emulate -R sh; cd /tmp; mkdir -p d; CDPATH=.; cd d; cd -; rmdir d'
$ bash -c 'cd /tmp; mkdir -p d; CDPATH=.; cd d; cd -; rmdir d'
/tmp/d
/tmp
$

while bash was correct in printing the absolute name of d and its parent.

When fixing this, be sure that you don't break

$ zsh -c 'emulate -R sh; mkdir foo; CDPATH=:; cd foo'

since POSIX states that particular use must remain silent (an implicit . used
from an empty CDPATH entry is different than an explicit . in CDPATH).

Another bug, mainly visible on platforms like cygwin where leading // is special
as allowed by POSIX (in particular, on my cygwin machin, //eblake exists but not
/eblake):

$ zsh -c 'emulate -R sh; CDPATH=///; cd eblake; /bin/pwd'
//eblake
$ bash -c 'CDPATH=///; cd eblake; /bin/pwd'
bash: line 0: cd: eblake: No such file or directory
/tmp
$

Since POSIX requires ///eblake to be synonymous with /eblake, and /eblake does
not exist, only bash was correct in printing an error and not changing location.

-- 
Eric Blake

Re: cd bugs

[Eric Blake]

Eric Blake <ebb9 <at> byu.net> writes:

> 
> POSIX requires cd to give output if a nonempty entry in CDPATH played a role,
> or if 'cd -' was used.  Therefore, this line demonstrates two zsh bugs:
> 
> $ zsh -c 'emulate -R sh; cd /tmp; mkdir -p d; CDPATH=.; cd d; cd -; rmdir d'
> $ bash -c 'cd /tmp; mkdir -p d; CDPATH=.; cd d; cd -; rmdir d'
> /tmp/d
> /tmp
> $
> 
> while bash was correct in printing the absolute name of d and its parent.

Another bug - POSIX requires that CDPATH be searched prior to ., regardless of
whether CDPATH contains an explicit . or an empty entry.  Therefore, this is
another example where bash is right.  But I think the zsh native behavior of
favoring . over CDPATH if CDPATH does not include . is a little more intuitive,
so this may warrant the addition of a new shell option (POSIX_CD?) that defaults
to off in zsh mode but defaults to on in sh mode for POSIX compliance.

$ zsh -c 'emulate -R sh; cd /tmp; mkdir -p a/b b; CDPATH=a; cd b; /bin/pwd'
/tmp/b
$ bash -c 'cd /tmp; mkdir -p a/b b; CDPATH=a; cd b; /bin/pwd'
/tmp/a/b
/tmp/a/b
$ cd /tmp; rm -Rf a b
$

-- 
Eric Blake

Re: cd bugs

[Peter Stephenson]

On Tue, 14 Jul 2009 22:30:33 +0000 (UTC)
Eric Blake <ebb9@byu.net> wrote:
> Another bug - POSIX requires that CDPATH be searched prior to ., regardless of
> whether CDPATH contains an explicit . or an empty entry.

Here's the option for this---obviously the other POSIX behaviour can be
associated with it, I just haven't done that here.

I also haven't bothered with optimising the case where we already tested
"." within CDPATH, it's not worth the extra code.

Index: Doc/Zsh/builtins.yo
===================================================================
RCS file: /cvsroot/zsh/zsh/Doc/Zsh/builtins.yo,v
retrieving revision 1.123
diff -u -r1.123 builtins.yo
--- Doc/Zsh/builtins.yo	2 Jul 2009 13:48:29 -0000	1.123
+++ Doc/Zsh/builtins.yo	19 Jul 2009 18:56:20 -0000
@@ -183,6 +183,9 @@
 successful.  If `tt(.)' occurs in tt(cdpath), then tt(cdpath) is searched
 strictly in order so that `tt(.)' is only tried at the appropriate point.
 
+The order of testing tt(cdpath) is modified if the option tt(POSIX_CD)
+is set, as described in the documentation for the option.
+
 If no directory is found, the option tt(CDABLE_VARS) is set, and a
 parameter named var(arg) exists whose value begins with a slash, treat its
 value as the directory.  In that case, the parameter is added to the named
Index: Doc/Zsh/options.yo
===================================================================
RCS file: /cvsroot/zsh/zsh/Doc/Zsh/options.yo,v
retrieving revision 1.84
diff -u -r1.84 options.yo
--- Doc/Zsh/options.yo	12 Jul 2009 15:10:07 -0000	1.84
+++ Doc/Zsh/options.yo	19 Jul 2009 18:56:21 -0000
@@ -115,6 +115,22 @@
 will be treated as referring to the physical parent, even if the preceding
 path segment is a symbolic link.
 )
+pindex(POSIX_CD)
+pindex(POSIXCD)
+pindex(NO_POSIX_CD)
+pindex(NOPOSIXCD)
+cindex(CDPATH, order of checking)
+item(tt(POSIX_CD))(
+Modifies the behaviour of tt(cd), tt(chdir) and tt(pushd) commands
+to make them more compatible with the POSIX standard. The behaviour with
+the option unset is described in the documentation for the tt(cd)
+builtin in
+ifzman(zmanref(zshbuiltins))\
+ifnzman(noderef(Shell Builtin Commands)).
+If the option is set, the shell does not test for directories beneath
+the local directory (`tt(.)') until after all directories in tt(cdpath)
+have been tested.
+)
 pindex(PUSHD_IGNORE_DUPS)
 pindex(NO_PUSHD_IGNORE_DUPS)
 pindex(PUSHDIGNOREDUPS)
Index: Src/builtin.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/builtin.c,v
retrieving revision 1.231
diff -u -r1.231 builtin.c
--- Src/builtin.c	15 Jul 2009 08:43:31 -0000	1.231
+++ Src/builtin.c	19 Jul 2009 18:56:21 -0000
@@ -945,14 +945,23 @@
 	return NULL;
     }
 
-    /* if cdpath is being used, check it for . */
-    if (!nocdpath)
+    /*
+     * If cdpath is being used, check it for ".".
+     * Don't bother doing this if POSIXCD is set, we don't
+     * need to know (though it doesn't actually matter).
+     */
+    if (!nocdpath && !isset(POSIXCD))
 	for (pp = cdpath; *pp; pp++)
 	    if (!(*pp)[0] || ((*pp)[0] == '.' && (*pp)[1] == '\0'))
 		hasdot = 1;
-    /* if there is no . in cdpath (or it is not being used), try the directory
-       as-is (i.e. from .) */
-    if (!hasdot) {
+    /*
+     * If 
+     * (- there is no . in cdpath
+     *  - or cdpath is not being used)
+     *  - and the POSIXCD option is not set
+     * try the directory as-is (i.e. from .)
+     */
+    if (!hasdot && !isset(POSIXCD)) {
 	if ((ret = cd_try_chdir(NULL, dest, hard)))
 	    return ret;
 	if (errno != ENOENT)
@@ -971,6 +980,15 @@
 	    if (errno != ENOENT)
 		eno = errno;
 	}
+    /*
+     * POSIX requires us to check "." after CDPATH rather than before.
+     */
+    if (isset(POSIXCD)) {
+	if ((ret = cd_try_chdir(NULL, dest, hard)))
+	    return ret;
+	if (errno != ENOENT)
+	    eno = errno;
+    }
 
     /* handle the CDABLEVARS option */
     if ((ret = cd_able_vars(dest))) {
Index: Src/options.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/options.c,v
retrieving revision 1.50
diff -u -r1.50 options.c
--- Src/options.c	10 Jul 2009 11:08:48 -0000	1.50
+++ Src/options.c	19 Jul 2009 18:56:22 -0000
@@ -200,6 +200,7 @@
 {{NULL, "pathdirs",	      OPT_EMULATE},		 PATHDIRS},
 {{NULL, "posixaliases",       OPT_EMULATE|OPT_BOURNE},	 POSIXALIASES},
 {{NULL, "posixbuiltins",      OPT_EMULATE|OPT_BOURNE},	 POSIXBUILTINS},
+{{NULL, "posixcd",            OPT_EMULATE|OPT_BOURNE},	 POSIXCD},
 {{NULL, "posixidentifiers",   OPT_EMULATE|OPT_BOURNE},	 POSIXIDENTIFIERS},
 {{NULL, "posixjobs",          OPT_EMULATE|OPT_BOURNE},	 POSIXJOBS},
 {{NULL, "printeightbit",      0},                        PRINTEIGHTBIT},
Index: Src/zsh.h
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/zsh.h,v
retrieving revision 1.160
diff -u -r1.160 zsh.h
--- Src/zsh.h	11 Jul 2009 16:43:00 -0000	1.160
+++ Src/zsh.h	19 Jul 2009 18:56:22 -0000
@@ -1966,6 +1966,7 @@
     PATHDIRS,
     POSIXALIASES,
     POSIXBUILTINS,
+    POSIXCD,
     POSIXIDENTIFIERS,
     POSIXJOBS,
     PRINTEIGHTBIT,


-- 
Peter Stephenson <p.w.stephenson@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/

Re: cd bugs

[Eric Blake]

Eric Blake <ebb9 <at> byu.net> writes:

> $ zsh -c 'emulate -R sh; CDPATH=///; cd eblake; /bin/pwd'
> //eblake
> $ bash -c 'CDPATH=///; cd eblake; /bin/pwd'
> bash: line 0: cd: eblake: No such file or directory
> /tmp
> $

Of the three examples mentioned in this thread, this one is definitely a bug,
but one that only affects platforms where // is special (which, based on the
__CYGWIN__ conditional in the source, appears to be just cygwin).

The other two items mentioned in this thread are POSIX-compliance issues, but as
was pointed out to me off-list, changing them to obey POSIX is a feature request
and not a bug fix.  Unfortunately, while I'd like to help code it up, I'm not
sure how to go about adding a new option to implement POSIX semantics in cd
handling.  Besides adding a new option, one part is simple (in cd_do_chdir, set
hasdot to 1 and fake an implicit '.' entry at the end of cdpath if the POSIX_CD
option is set), the other part is a bit more complex (cdpath has to
differentiate between an explicit "." in CDPATH, which increments doprintdir,
and an implicit '.' due to an explicit empty entry or the implicit '.' added
from the first part of the fix, whereas right now the cdpath variable has
already converted implicit '.' to explicit "." in the array).

Actually, in looking at this patch again, I'm starting to wonder if it might be
better to teach tricat that if the first argument ends in '/' and the second
argument is exactly "/", then it does not need to use the second argument; this
would certainly make it touch all code paths that do file name concatenation,
rather than trying to change down and protect every caller of tricat with
__CYGWIN__ conditionals.  In other words, there are probably also bugs with ~
expansion when $HOME is exactly / or //, as well as other potential gotchas with
// handling that I haven't even investigated here.


From: Eric Blake <ebb9@byu.net>
Date: Tue, 14 Jul 2009 21:01:03 -0600
Subject: [PATCH] Eric Blake: 27149: fix // handling in cd for cygwin

---
 ChangeLog     |    5 +++++
 Src/builtin.c |   14 ++++++++------
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 4255d6f..b9d6dbd 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2009-07-14  Eric Blake  <ebb9@byu.net>
+
+	* Eric Blake: 27149: Src/builtin.c: Fix // handling in cd for
+	cygwin.
+
 2009-07-14  Peter Stephenson  <pws@csr.com>

 	* Andy Spencer: 27148: Completion/Linux/Command/_modutils:
diff --git a/Src/builtin.c b/Src/builtin.c
index 62c6e3c..56cc916 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -1029,17 +1029,19 @@ cd_try_chdir(char *pfix, char *dest, int hard)

     /* handle directory prefix */
     if (pfix && *pfix) {
-	if (*pfix == '/')
+	if (*pfix == '/') {
 #ifdef __CYGWIN__
 /* NB: Don't turn "/"+"bin" into "//"+"bin" by mistake!  "//bin" may *
  * not be what user really wants (probably wants "/bin"), but        *
  * "//bin" could be valid too (see fixdir())!  This is primarily for *
- * handling CDPATH correctly.                                        */
-	    buf = tricat(pfix, ( pfix[1] == '\0' ? "" : "/" ), dest);
+ * handling CDPATH correctly.  Likewise for "//"+"bin" not becoming  *
+ * "///bin" (aka "/bin").                                            */
+	    int root = pfix[1] == '\0' || (pfix[1] == '/' && pfix[2] == '\0');
+	    buf = tricat(pfix, ( root ? "" : "/" ), dest);
 #else
 	    buf = tricat(pfix, "/", dest);
 #endif
-	else {
+	} else {
 	    int pfl = strlen(pfix);
 	    dlen = strlen(pwd);

@@ -1200,8 +1202,8 @@ fixdir(char *src)
 	/* compress multiple /es into single */
 	if (*src == '/') {
 #ifdef __CYGWIN__
-	    /* allow leading // under cygwin */
-	    if (src == s0 && src[1] == '/')
+	    /* allow leading // under cygwin, but /// still becomes / */
+	    if (src == s0 && src[1] == '/' && src[2] != '/')
 		*dest++ = *src++;
 #endif
 	    *dest++ = *src++;
-- 
1.6.3.3.334.g916e1

Re: cd bugs

[Peter Stephenson]

On Wed, 15 Jul 2009 03:28:53 +0000 (UTC)
Eric Blake <ebb9@byu.net> wrote:
> Eric Blake <ebb9 <at> byu.net> writes:
> 
> > $ zsh -c 'emulate -R sh; CDPATH=///; cd eblake; /bin/pwd'
> > //eblake
> > $ bash -c 'CDPATH=///; cd eblake; /bin/pwd'
> > bash: line 0: cd: eblake: No such file or directory
> > /tmp
> > $
> 
> Of the three examples mentioned in this thread, this one is definitely a bug,
> but one that only affects platforms where // is special (which, based on the
> __CYGWIN__ conditional in the source, appears to be just cygwin).

Thanks, I've committed it.

> Actually, in looking at this patch again, I'm starting to wonder if it
> might be better to teach tricat that if the first argument ends in '/'
> and the second argument is exactly "/", then it does not need to use the
> second argument; this would certainly make it touch all code paths that
> do file name concatenation, rather than trying to change down and protect
> every caller of tricat with __CYGWIN__ conditionals.  In other words,
> there are probably also bugs with ~ expansion when $HOME is exactly / or
> //, as well as other potential gotchas with // handling that I haven't
> even investigated here.

It would probably be better to introduce a pathtricat(), since tricat()
isn't necessarily tied to paths; but it's possible similar issues occur
with dupstring() or ztrdup(), too.  It's certainly a danger this could
happen elsewhere.

-- 
Peter Stephenson <pws@csr.com>            Software Engineer
Tel: +44 (0)1223 692070                   Cambridge Silicon Radio Limited
Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, UK


'member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom'

Re: cd bugs

[Peter Stephenson]

On Tue, 14 Jul 2009 21:59:14 +0000 (UTC)
Eric Blake <ebb9@byu.net> wrote:
> POSIX requires cd to give output if a nonempty entry in CDPATH played a
> role, or if 'cd -' was used.  Therefore, this line demonstrates two zsh
> bugs:
>
> $ zsh -c 'emulate -R sh; cd /tmp; mkdir -p d; CDPATH=.; cd d; cd -; rmdir d'
> $ bash -c 'cd /tmp; mkdir -p d; CDPATH=.; cd d; cd -; rmdir d'
> /tmp/d
> /tmp
> $
>
> while bash was correct in printing the absolute name of d and its parent.
>
> When fixing this, be sure that you don't break
>
> $ zsh -c 'emulate -R sh; mkdir foo; CDPATH=:; cd foo'
>
> since POSIX states that particular use must remain silent (an implicit . used
> from an empty CDPATH entry is different than an explicit . in CDPATH).

I think this should make the POSIX_CD option behave appropriately.

Index: Doc/Zsh/options.yo
===================================================================
RCS file: /cvsroot/zsh/zsh/Doc/Zsh/options.yo,v
retrieving revision 1.85
diff -u -r1.85 options.yo
--- Doc/Zsh/options.yo	19 Jul 2009 19:08:49 -0000	1.85
+++ Doc/Zsh/options.yo	21 Jul 2009 09:20:37 -0000
@@ -130,6 +130,14 @@
 If the option is set, the shell does not test for directories beneath
 the local directory (`tt(.)') until after all directories in tt(cdpath)
 have been tested.
+
+Also, if the option is set, the conditions under which the shell
+prints the new directory after changing to it are modified.  It is
+no longer restricted to interactive shells (although printing of
+the directory stack with tt(pushd) is still limited to interactive
+shells); and any use of a component of tt(CDPATH), including a `tt(.)' but
+excluding an empty component that is otherwise treated as `tt(.)', causes
+the directory to be printed.
 )
 pindex(PUSHD_IGNORE_DUPS)
 pindex(NO_PUSHD_IGNORE_DUPS)
Index: Src/builtin.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/builtin.c,v
retrieving revision 1.232
diff -u -r1.232 builtin.c
--- Src/builtin.c	19 Jul 2009 19:08:50 -0000	1.232
+++ Src/builtin.c	21 Jul 2009 09:20:37 -0000
@@ -972,8 +972,19 @@
     if (!nocdpath)
 	for (pp = cdpath; *pp; pp++) {
 	    if ((ret = cd_try_chdir(*pp, dest, hard))) {
-		if (strcmp(*pp, ".")) {
-		    doprintdir++;
+		if (isset(POSIXCD)) {
+		    /*
+		     * For POSIX we need to print the directory
+		     * any time CDPATH was used, except in the
+		     * special case of an empty segment being
+		     * treated as a ".".
+		     */
+		    if (**pp)
+			doprintdir++;
+		}  else {
+		    if (strcmp(*pp, ".")) {
+			doprintdir++;
+		    }
 		}
 		return ret;
 	    }
@@ -1146,8 +1157,8 @@
     pwd = new_pwd;
     set_pwd_env();
 
-    if (isset(INTERACTIVE)) {
-	if (func != BIN_CD) {
+    if (isset(INTERACTIVE) || isset(POSIXCD)) {
+	if (func != BIN_CD && isset(INTERACTIVE)) {
             if (unset(PUSHDSILENT) && !quiet)
 	        printdirstack();
         } else if (doprintdir) {


-- 
Peter Stephenson <pws@csr.com>            Software Engineer
Tel: +44 (0)1223 692070                   Cambridge Silicon Radio Limited
Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, UK


'member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom'