* cd bugs @ 2009-07-14 21:59 Eric Blake 2009-07-14 22:30 ` Eric Blake ` (2 more replies) 0 siblings, 3 replies; 6+ messages in thread From: Eric Blake @ 2009-07-14 21:59 UTC (permalink / raw) To: zsh-workers 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: cd bugs 2009-07-14 21:59 cd bugs Eric Blake @ 2009-07-14 22:30 ` Eric Blake 2009-07-19 19:00 ` Peter Stephenson 2009-07-15 3:28 ` Eric Blake 2009-07-21 9:22 ` Peter Stephenson 2 siblings, 1 reply; 6+ messages in thread From: Eric Blake @ 2009-07-14 22:30 UTC (permalink / raw) To: zsh-workers 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: cd bugs 2009-07-14 22:30 ` Eric Blake @ 2009-07-19 19:00 ` Peter Stephenson 0 siblings, 0 replies; 6+ messages in thread From: Peter Stephenson @ 2009-07-19 19:00 UTC (permalink / raw) To: zsh-workers 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/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: cd bugs 2009-07-14 21:59 cd bugs Eric Blake 2009-07-14 22:30 ` Eric Blake @ 2009-07-15 3:28 ` Eric Blake 2009-07-15 8:45 ` Peter Stephenson 2009-07-21 9:22 ` Peter Stephenson 2 siblings, 1 reply; 6+ messages in thread From: Eric Blake @ 2009-07-15 3:28 UTC (permalink / raw) To: zsh-workers 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: cd bugs 2009-07-15 3:28 ` Eric Blake @ 2009-07-15 8:45 ` Peter Stephenson 0 siblings, 0 replies; 6+ messages in thread From: Peter Stephenson @ 2009-07-15 8:45 UTC (permalink / raw) To: zsh-workers 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' ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: cd bugs 2009-07-14 21:59 cd bugs Eric Blake 2009-07-14 22:30 ` Eric Blake 2009-07-15 3:28 ` Eric Blake @ 2009-07-21 9:22 ` Peter Stephenson 2 siblings, 0 replies; 6+ messages in thread From: Peter Stephenson @ 2009-07-21 9:22 UTC (permalink / raw) To: zsh-workers 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' ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-07-21 9:30 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-07-14 21:59 cd bugs Eric Blake 2009-07-14 22:30 ` Eric Blake 2009-07-19 19:00 ` Peter Stephenson 2009-07-15 3:28 ` Eric Blake 2009-07-15 8:45 ` Peter Stephenson 2009-07-21 9:22 ` Peter Stephenson
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/zsh/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).