3.1.6-test-1: strange cd behaviour

3.1.6-test-1: strange cd behaviour

[Andrej Borsenkow]

(/tools/src  is in cdpath)

bor@itsrm2:/tools/src/zsh-3.1.6-test-1%> builtin cd zsh-3.1.6-test-1/..
bor@itsrm2:/tools/src/zsh-3.1.6-test-1%> echo $?
0
bor@itsrm2:/tools/src/zsh-3.1.6-test-1%> /bin/pwd
/tools/src/zsh-3.1.6-test-1

That is, cd does not change directory and returns success. I have some scripts
that do "cd /some/dir && rm -r ..." ahem ...

/andrej

Re: 3.1.6-test-1: strange cd behaviour

[Peter Stephenson]

"Andrej Borsenkow" wrote:
> (/tools/src  is in cdpath)
> 
> bor@itsrm2:/tools/src/zsh-3.1.6-test-1%> builtin cd zsh-3.1.6-test-1/..
> bor@itsrm2:/tools/src/zsh-3.1.6-test-1%> echo $?
> 0
> 
> That is, cd does not change directory and returns success.

I take it what you're really complaining about is that there is no
subdirectory zsh-3.1.6-test-1 at that point, i.e. 
  cd anyolddirectorynameatallatall/..
always succeeds and leaves you in `.'?

It certainly looks like a bug to me, because the unspoken assumption is
that cd will fail unless the path you've given it is valid as it stands ---
and it isn't, chdir() would fail if given the raw path.  However, I ought
to point out that bash and some versions of ksh (but not sh, which is too
dumb to massage paths, and not csh or tcsh either, and not some other
versions of ksh) silently accept this.

I wrote a patch for this, but unfortunately there's a real problem.
Consider:

% mkdir ~/tmp/d1
% cd ~/tmp/d1
% rm -rf ~/tmp/d1
% cd ..

Currently that works fine:  since tmp exists, it changes directory there.
However, if you insist on the initial directory being valid, then
(for example) statting ~/tmp/d1/.. will fail, and the cd won't work.
Furthermore, you can't even stat the relative path "..", since that's gone
too.  So I don't see a way of reconciling the two things.

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

Re: 3.1.6-test-1: strange cd behaviour

[Bart Schaefer]

On Jul 14, 10:06am, Peter Stephenson wrote:
} Subject: Re: 3.1.6-test-1: strange cd behaviour
}
} I take it what you're really complaining about is that there is no
} subdirectory zsh-3.1.6-test-1 at that point, i.e. 
}   cd anyolddirectorynameatallatall/..
} always succeeds and leaves you in `.'?
} 
} Consider:
} 
} % mkdir ~/tmp/d1
} % cd ~/tmp/d1
} % rm -rf ~/tmp/d1
} % cd ..
} 
} Currently that works fine:  since tmp exists, it changes directory there.
} However, if you insist on the initial directory being valid, then
} (for example) statting ~/tmp/d1/.. will fail, and the cd won't work.
} Furthermore, you can't even stat the relative path "..", since that's gone
} too.  So I don't see a way of reconciling the two things.

It worked (and still works) in 3.0.  What changed?

zagzig% echo $ZSH_VERSION
3.0.6-pre-5
zagzig% cd dummy/..
cd: no such file or directory: dummy/..
zagzig% cd /tmp/foo
zagzig% mkdir fred
zagzig% cd fred
zagzig% rmdir /tmp/foo/fred
zagzig% cd ..
zagzig% pwd
/tmp/foo
zagzig% cd fred/..
cd: no such file or directory: fred/..

zagzig% echo $ZSH_VERSION
3.1.6-test-1
zagzig% pwd
/usr/src/local/zsh/zsh-3.1.5-build
zagzig% cd dummy/..
zagzig% pwd
/usr/src/local/zsh/zsh-3.1.5-build
zagzig% cd /tmp/foo
zagzig% cd fred/..
zagzig% pwd
/tmp/foo
zagzig% mkdir fred
zagzig% cd fred
zagzig% rmdir /tmp/foo/fred
zagzig% cd ..
zagzig% pwd
/tmp/foo
zagzig% cd fred/..
zagzig% pwd
/tmp/foo


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

Re: 3.1.6-test-1: strange cd behaviour

[Peter Stephenson]

"Bart Schaefer" wrote:
> } Currently that works fine:  since tmp exists, it changes directory there.
> } However, if you insist on the initial directory being valid, then
> } (for example) statting ~/tmp/d1/.. will fail, and the cd won't work.
> } Furthermore, you can't even stat the relative path "..", since that's gone
> } too.  So I don't see a way of reconciling the two things.
> 
> It worked (and still works) in 3.0.  What changed?

This is a story of horror.  In both versions the directory path code
(c.f. xsymlinks()) is as clear as mud mixed with more mud in mud sauce (in
fact, I remember Zefram saying something similar).  What changed is that in
3.0 relative paths are handled differently from absolute ones, while in 3.1
the existing pwd is tacked on the front and all are handled together.  In
3.0, to make this work, ..'s behave differently if they are going up
further than the start of the current pwd.  In the case of `cd dummy/..',
it didn't and instead performed a chdir() on the whole thing, so that was
tested properly.  On the other hand, cd .. works by examining and
modifiying the current pwd, so you could cd from a non-existent directory.
However, if you have .. in your cdpath, then `cd dummy/..' will try the
path `../dummy/..', realises it's going up beyond pwd and hence allow the
dummy/.. to be rationalized away, and this works perfectly, taking you,
somewhat unexpectedly, to the parent of your current directory.

3.1 is more integrated: it takes a complete path and operates on that.
This has the effects previously reported.  Without restoring the 3.0
behaviour, the fix would be to do something clever when pwd is a prefix of
the path --- i.e., $PWD/.. is automatically turned into $PWD:h, whether the
directory exists or not, but $PWD/../dummy/.. becomes
${PWD:h}/dummy/.. which is weighed in the balance and found wanting.  This
would get all the advantages and none of the disadvantages of the 3.0
method (I hope).  The rule would be something like

- If $PWD is a prefix, rationalize away any immediately following ..'s
  (and .'s, to be on the safe side) before doing any testing.
- At that point, even if $PWD is a prefix, look at the path and see if it
  contains any /../ or finishes with /.. . If so, stat() it and check
  that it exists.  If not, return and let the chdir code handle errors.
- If everything's OK so far (i.e. no ..'s, or the directory exists)
  rationalize the rest of the path.

I may or may not get round to this myself.

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

PATCH: 3.1.6-test-1: strange cd behaviour

[Peter Stephenson]

Peter Stephenson wrote:
> This is a story of horror.
>
> Without restoring the 3.0
> behaviour, the fix would be to do something clever when pwd is a prefix of
> the path --- i.e., $PWD/.. is automatically turned into $PWD:h, whether the
> directory exists or not, but $PWD/../dummy/.. becomes
> ${PWD:h}/dummy/.. which is weighed in the balance and found wanting.  This
> would get all the advantages and none of the disadvantages of the 3.0
> method (I hope).  The rule would be something like
> 
> - If $PWD is a prefix, rationalize away any immediately following ..'s
>   (and .'s, to be on the safe side) before doing any testing.
> - At that point, even if $PWD is a prefix, look at the path and see if it
>   contains any /../ or finishes with /.. . If so, stat() it and check
>   that it exists.  If not, return and let the chdir code handle errors.
> - If everything's OK so far (i.e. no ..'s, or the directory exists)
>   rationalize the rest of the path.

This does it:  as you see it's a significant chunk of extra code with no
other aim than making sure cd foo/.. doesn't work if foo doesn't exist
without preventing you from changing back up to directories that still
exist if yours doesn't.  But life is non-optimal.  If anybody has any
better suggestions...?

Please test this, since people tend to get cross if they can't change
directories properly.

Note that this has the effect that `cd $PWD/<relative>' is tested in
exactly the same way as `cd <relative>', where <relative> is any relative
path.  So `cd $PWD/..' will also always work if $PWD doesn't exist but its
parent does, regardless (obviously) of whether you use the variable $PWD or
the full directory name.  That's the effect of the code change between 3.0
and 3.1.  (Of course, if you're somewhere else in cdpath, none of this
matters.)

--- Src/builtin.c.cd	Tue Jul 13 14:31:03 1999
+++ Src/builtin.c	Wed Jul 14 14:26:38 1999
@@ -1045,11 +1045,41 @@
 static void
 fixdir(char *src)
 {
-    char *dest = src;
-    char *d0 = dest;
-#ifdef __CYGWIN__
+    char *dest = src, *d0 = dest, *chks = src, *ptrp;
+#ifdef __CYGWIN
     char *s0 = src;
 #endif
+    int donecheck = 0, len;
+
+    /*
+     * Normally, we should not rationalize away path segments foo/.. if
+     * the directory foo does not exist.  However, if foo was part of
+     * pwd then we should allow it, because we know the current
+     * directory was once valid, although may have been deleted, and `cd
+     * ..' should always work as long as the parent directory exists.
+     * Hence we find an initial portion of the path consisting of pwd
+     * followed by any number of .. or . segments, and don't check
+     * that the original path exists.  After this point (given by
+     * chks), if there are any remaining ..'s in the path we
+     * check that the directory with the remaining portion
+     * unrationalized really exists, and return if it doesn't.
+     *
+     * Bug:  this is ridiculously heavy handed.
+     */
+    len = strlen(pwd);
+    if (!strncmp(src, pwd, len) && src[len] == '/') {
+	chks = src + len;
+	while (*chks == '/')
+	    chks++;
+	while (*chks == '.' &&
+	       (!chks[1] || chks[1] == '/' ||
+		(chks[1] == '.' && (!chks[2] || chks[2] == '/')))) {
+	    while (*chks == '.')
+		chks++;
+	    while (*chks == '/')
+		chks++;
+	}
+    }
 
 /*** if have RFS superroot directory ***/
 #ifdef HAVE_SUPERROOT
@@ -1085,6 +1115,31 @@
 	}
 	if (src[0] == '.' && src[1] == '.' &&
 	  (src[2] == '\0' || src[2] == '/')) {
+	    if (!donecheck && src >= chks) {
+		/*
+		 * We need to check the original path exists, to catch
+		 * problems like 'cd nonexistent/..'.  We use dest
+		 * as far as we've got, plus the rest of src.
+		 * We only need to do this once, as any later ..'s
+		 * will automatically be tested here.
+		 */
+		struct stat st;
+		char *testdir;
+		if (dest == src)
+		    testdir = dupstring(d0);
+		else {
+		    *dest = '\0';
+		    testdir = dyncat(d0, src);
+		}
+		for (chks = src, ptrp = testdir + (dest - d0); *chks;
+		     chks++, ptrp++)
+		    *ptrp = (*chks == Meta) ? (*++chks ^ 32) : *chks;
+		if (stat(testdir, &st) < 0 || !S_ISDIR(st.st_mode)) {
+		    strcpy(d0, testdir);
+		    return;
+		}
+		donecheck = 1;
+	    }
 	    if (dest > d0 + 1) {
 		/* remove a foo/.. combination */
 		for (dest--; dest > d0 + 1 && dest[-1] != '/'; dest--);

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

RE: PATCH: 3.1.6-test-1: strange cd behaviour

[Andrej Borsenkow]

>
> This does it:  as you see it's a significant chunk of extra code with no
> other aim than making sure cd foo/.. doesn't work if foo doesn't exist
> without preventing you from changing back up to directories that still
> exist if yours doesn't.  But life is non-optimal.  If anybody has any
> better suggestions...?
>

Yes. Completely remove this. If current dir no more exists, the error message
here is more of a feature - it indicates, that somethig went wrong. User always
has ``cd'' or even ``cd /'' fallback. I'm afraid, in this case zsh oversmarts
itself.  And after all, with all symlinks around, you cannot even be sure, that
$PWD is prefix of current dir :-( (Our /bin/sh resolves links while /bin/ksh
does not).

/andrej

Re: PATCH: 3.1.6-test-1: strange cd behaviour

[Peter Stephenson]

"Andrej Borsenkow" wrote:
> Yes. Completely remove this. If current dir no more exists, the error message
> here is more of a feature - it indicates, that somethig went wrong. User alwa
> ys
> has ``cd'' or even ``cd /'' fallback. I'm afraid, in this case zsh oversmarts
> itself.  And after all, with all symlinks around, you cannot even be sure, th
> at
> $PWD is prefix of current dir :-( (Our /bin/sh resolves links while /bin/ksh
> does not).

Actually, that's not completely removing it, that's including about half
the code, just not the stuff that allows the PWD to be backtracked
automatically.  (Unless you are suggesting going back to not testing for a
non-existent directory at all?  But then you don't get the error message
with `cd ..'.)

Furthermore, whether PWD is a prefix of the *physical* current directory is
a different issue entirely, depending on the option CHASELINKS.  Without
that set, zsh will always try to turn $PWD/.. into ${PWD:h}, wherever that
lands you up.  The issue here is whether to check for the existence of
$PWD/.. in the former case, as soon as it sees the .. .

Which opens another kettle of fish: if the .. is not at the end, then
/foo/bar/../rod might not be the same physical path as /foo/rod.  So if you
are in /foo/bar, and that is a symbolic link to somewhere not under foo,
and you try to change to /foo/rod by `cd ../rod', then without the bit of
laxity I added allowing backing up $PWD regardless of existence, cd ../rod
will fail, which I don't think is on, since if CHASELINKS is not set this
is just the sort of behaviour you expect to work --- note that this works
in ksh, too.  (Although with AUTOCD `../rod' on its own already fails,
because it tests for a physical directory, since cancd() doesn't call
fixdir() --- anyone want that fixed?)

It's now true that if you do `cd bar/../rod' and bar is a symlink, then
this won't work because of the test I added --- it stat's bar/../rod which
looks for rod in the physical parent of the directory to which bar points.
Maybe this is already going too far when CHASELINKS is unset.  (I could
change it to stat every occurrence of <dir>/.. , which was my first thought,
and which should get round this.)

So the options seem to be: as it was or as it is now, the halfway house
breaks too much when the current directory is a link.  Zsh has been
oversmart with directories all its life.

Any more suggestions?  I could maybe make it more stringent when CHASELINKS
is not set --- but that's increasing the complexity still further.  I would
be perfectly happy to back off the patch altogether, too.

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

Re: PATCH: 3.1.6-test-1: strange cd behaviour

[Peter Stephenson]

Peter Stephenson wrote:
> (Although with AUTOCD `../rod' on its own already fails,
> because it tests for a physical directory, since cancd() doesn't call
> fixdir() --- anyone want that fixed?)
> 
> It's now true that if you do `cd bar/../rod' and bar is a symlink, then
> this won't work because of the test I added --- it stat's bar/../rod which
> looks for rod in the physical parent of the directory to which bar points.
> Maybe this is already going too far when CHASELINKS is unset.  (I could
> change it to stat every occurrence of <dir>/.. , which was my first thought,
> and which should get round this.)

Fixing the first of these bugs seems to be easy, just an extra call to
fixdir() with pwd tacked on front if necessary, so I did it.

Looking at the second one, I realised my response to Andrej's suggestion
was excessive.  By only testing the directories which really need to be
there --- e.g. in the case of foo/bar/../rod/.., that would be foo/bar,
then foo/rod when we wipe out the bar/.. bit --- we can both have a
reliable test for real directories, and also not bother whether or not
we're in pwd, since we're always looking only at the already modified
directory string which has to exist.

So this code is very much simpler than what I proposed before, and also has
the feature Andrej suggested, that `cd ..' might as well fail if the
directory doesn't exist, since ... well, since the directory doesn't
exist.  You can do ${PWD:h} to get to the parent.  Most of the patch is
removing what I just added.

--- Src/builtin.c.cd2	Thu Jul 15 17:21:44 1999
+++ Src/builtin.c	Thu Jul 15 17:40:15 1999
@@ -1042,44 +1042,13 @@
  * combinations, are removed and the path is unmetafied.   */
 
 /**/
-static void
+void
 fixdir(char *src)
 {
-    char *dest = src, *d0 = dest, *chks = src, *ptrp;
+    char *dest = src, *d0 = dest;
 #ifdef __CYGWIN
     char *s0 = src;
 #endif
-    int donecheck = 0, len;
-
-    /*
-     * Normally, we should not rationalize away path segments foo/.. if
-     * the directory foo does not exist.  However, if foo was part of
-     * pwd then we should allow it, because we know the current
-     * directory was once valid, although may have been deleted, and `cd
-     * ..' should always work as long as the parent directory exists.
-     * Hence we find an initial portion of the path consisting of pwd
-     * followed by any number of .. or . segments, and don't check
-     * that the original path exists.  After this point (given by
-     * chks), if there are any remaining ..'s in the path we
-     * check that the directory with the remaining portion
-     * unrationalized really exists, and return if it doesn't.
-     *
-     * Bug:  this is ridiculously heavy handed.
-     */
-    len = strlen(pwd);
-    if (!strncmp(src, pwd, len) && src[len] == '/') {
-	chks = src + len;
-	while (*chks == '/')
-	    chks++;
-	while (*chks == '.' &&
-	       (!chks[1] || chks[1] == '/' ||
-		(chks[1] == '.' && (!chks[2] || chks[2] == '/')))) {
-	    while (*chks == '.')
-		chks++;
-	    while (*chks == '/')
-		chks++;
-	}
-    }
 
 /*** if have RFS superroot directory ***/
 #ifdef HAVE_SUPERROOT
@@ -1115,33 +1084,21 @@
 	}
 	if (src[0] == '.' && src[1] == '.' &&
 	  (src[2] == '\0' || src[2] == '/')) {
-	    if (!donecheck && src >= chks) {
+	    if (dest > d0 + 1) {
 		/*
-		 * We need to check the original path exists, to catch
-		 * problems like 'cd nonexistent/..'.  We use dest
-		 * as far as we've got, plus the rest of src.
-		 * We only need to do this once, as any later ..'s
-		 * will automatically be tested here.
+		 * remove a foo/.. combination:
+		 * first check foo exists, else return
 		 */
 		struct stat st;
-		char *testdir;
-		if (dest == src)
-		    testdir = dupstring(d0);
-		else {
-		    *dest = '\0';
-		    testdir = dyncat(d0, src);
-		}
-		for (chks = src, ptrp = testdir + (dest - d0); *chks;
-		     chks++, ptrp++)
-		    *ptrp = (*chks == Meta) ? (*++chks ^ 32) : *chks;
-		if (stat(testdir, &st) < 0 || !S_ISDIR(st.st_mode)) {
-		    strcpy(d0, testdir);
+		*dest = '\0';
+		if (stat(d0, &st) < 0 || !S_ISDIR(st.st_mode)) {
+		    char *ptrd, *ptrs;
+		    if (dest == src)
+			*dest = '.';
+		    for (ptrs = src, ptrd = dest; *ptrs; ptrs++, ptrd++)
+			*ptrd = (*ptrs == Meta) ? (*++ptrs ^ 32) : *ptrs;
 		    return;
 		}
-		donecheck = 1;
-	    }
-	    if (dest > d0 + 1) {
-		/* remove a foo/.. combination */
 		for (dest--; dest > d0 + 1 && dest[-1] != '/'; dest--);
 		if (dest[-1] != '/')
 		    dest--;
--- Src/exec.c.cd2	Thu Jul  8 17:10:17 1999
+++ Src/exec.c	Thu Jul 15 17:40:01 1999
@@ -3144,9 +3144,16 @@
 cancd2(char *s)
 {
     struct stat buf;
-    char *us = unmeta(s);
+    char *us = unmeta(s), *us2 = NULL;
 
+    if (*us != '/')
+	us = us2 = tricat(unmeta(pwd), "/", us);
+    else
+	us = dupstring(us);
+    fixdir(us);
     return !(access(us, X_OK) || stat(us, &buf) || !S_ISDIR(buf.st_mode));
+    if (us2)
+	zsfree(us2);
 }
 
 /**/

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

Re: 3.1.6-test-1: strange cd behaviour

[Bart Schaefer]

On Jul 14, 11:15am, Peter Stephenson wrote:
} Subject: Re: 3.1.6-test-1: strange cd behaviour
}
} > It worked (and still works) in 3.0.  What changed?
} 
} 3.0 relative paths are handled differently from absolute ones, while in 3.1
} the existing pwd is tacked on the front and all are handled together.  In
} 3.0, to make this work, ..'s behave differently if they are going up
} further than the start of the current pwd.  In the case of `cd dummy/..',
} it didn't and instead performed a chdir() on the whole thing, so that was
} tested properly.  On the other hand, cd .. works by examining and
} modifiying the current pwd, so you could cd from a non-existent directory.

Even with chaselinks set?  That would seem to me to be a bug.  And indeed
(in raw 3.0.5, but 3.0.6-pre-5 and 3.1.6-test-1 are the same):

zagzig[24] mkdir /tmp/bar /tmp/foo
zagzig[25] mkdir /tmp/bar/subdir
zagzig[26] cd /tmp/foo
zagzig[27] ln -s /tmp/bar/subdir .
zagzig[28] cd subdir
zagzig[29] setopt chaselinks
zagzig[30] cd ..
zagzig[31] pwd
/tmp/foo

} 3.1 is more integrated: it takes a complete path and operates on that.
} This has the effects previously reported.  Without restoring the 3.0
} behaviour, the fix would be
} 
} - If $PWD is a prefix, rationalize away any immediately following ..'s
}   (and .'s, to be on the safe side) before doing any testing.

So my first suggestion is that no rationalization should happen when
chaselinks is set.

} - At that point, even if $PWD is a prefix, look at the path and see if it
}   contains any /../ or finishes with /.. . If so, stat() it and check
}   that it exists.  If not, return and let the chdir code handle errors.

When chaselinks is NOT set, this approach mail fail due to permissions on
intermediate path elements when in fact it would succeed if the path were
rationalized first.  On the other hand, I suppose that if you want it to
fail because of nonexistent directories you probably want it to fail for
permissions as well.

However, it's also that case that you can't test the entire path once
and then assume it's OK to rationalize it, because stat() will follow
intermediate symlinks whereas rationalization will not.  (You pointed
this out yourself in the message excerpted below.)

As a third point, the correct way to test every component would in at
least some case be to actually chdir() there, not to stat() it.

} - If everything's OK so far (i.e. no ..'s, or the directory exists)
}   rationalize the rest of the path.

On Jul 15,  2:23pm, Peter Stephenson wrote:
} Subject: Re: PATCH: 3.1.6-test-1: strange cd behaviour
}
} "Andrej Borsenkow" wrote:
} > Yes. Completely remove this. If current dir no more exists, the
} > error message here is more of a feature - it indicates, that
} > somethig went wrong.
} 
} Actually, that's not completely removing it, that's including about half
} the code, just not the stuff that allows the PWD to be backtracked
} automatically.  (Unless you are suggesting going back to not testing for a
} non-existent directory at all?  But then you don't get the error message
} with `cd ..'.)

Here's what I propose, in more detail:

(1) When chaselinks is set, don't rationalize, simply attempt the cd and
either get the new directory if it succeeds or issue an error if it fails.
I'm going to peer at 3.0.6-pre; hopefully it isn't too hard to add this
one bit.

(2) When chaselinks is not set, either:
(2a) keep the pre-7139 behavior, or
(2b) add a "chasedots" option or some such that means zsh should act as
though chaselinks is set iff the path contains ".." anywhere.

} Furthermore, whether PWD is a prefix of the *physical* current directory is
} a different issue entirely, depending on the option CHASELINKS.  Without
} that set, zsh will always try to turn $PWD/.. into ${PWD:h}, wherever that
} lands you up.

I just tried this in pre-7139 3.1.6-test-1, and found that zsh *always*
turns $PWD/.. into $PWD:h, completely independent of chaselinks.

zagzig% pwd       
/tmp/foo/subdir
zagzig% setopt chaselinks
zagzig% echo $PWD
/tmp/foo/subdir
zagzig% pwd
/tmp/bar/subdir
zagzig% echo $PWD
/tmp/foo/subdir
zagzig% cd $PWD/..
zagzig% pwd
/tmp/foo

} Which opens another kettle of fish: if the .. is not at the end, then
} /foo/bar/../rod might not be the same physical path as /foo/rod.

Yes, that's the same point I made above about stat().

} (Although with AUTOCD `../rod' on its own already fails,
} because it tests for a physical directory, since cancd() doesn't call
} fixdir() --- anyone want that fixed?)

Only if you add the "chasedots" option.

} Maybe this is already going too far when CHASELINKS is unset.  (I could
} change it to stat every occurrence of <dir>/.. , which was my first
} thought, and which should get round this.)

This is definitely going too far.

} I would be perfectly happy to back off the patch altogether, too.

That plus (1) above would make me comfortable.

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

PATCH: 3.1.6-test-1: remorselessly strange cd behaviour

[Peter Stephenson]

"Bart Schaefer" wrote:
> On Jul 14, 11:15am, Peter Stephenson wrote:
> } On the other hand, cd .. works by examining and
> } modifiying the current pwd, so you could cd from a non-existent directory.
> 
> Even with chaselinks set?  That would seem to me to be a bug.

This will be fixed by removing fixdir() when chaselinks is set.

> } - If $PWD is a prefix, rationalize away any immediately following ..'s
> }   (and .'s, to be on the safe side) before doing any testing.
> 
> So my first suggestion is that no rationalization should happen when
> chaselinks is set.

This seems easy (! first thing that was), since zsh will always explicitly
tidy up pwd when chaselinks is set (except as noted below), so the .. side
effect was fixdir()'s only contribution.

> } - At that point, even if $PWD is a prefix, look at the path and see if it
> }   contains any /../ or finishes with /.. . If so, stat() it and check
> }   that it exists.  If not, return and let the chdir code handle errors.
> 
> When chaselinks is NOT set, this approach mail fail due to permissions on
> intermediate path elements when in fact it would succeed if the path were
> rationalized first.  On the other hand, I suppose that if you want it to
> fail because of nonexistent directories you probably want it to fail for
> permissions as well.

Maybe it's not as bad as you fear: the only thing that happens is that the
physical path is used for the cd, rather than deleting directories from the
path it can't handle; the cd will still succeed if it was ever going to,
but you get the links resolved, since it didn't know what to do with them.

> However, it's also that case that you can't test the entire path once
> and then assume it's OK to rationalize it, because stat() will follow
> intermediate symlinks whereas rationalization will not.  (You pointed
> this out yourself in the message excerpted below.)

Yes, I think this is now fixed.

> As a third point, the correct way to test every component would in at
> least some case be to actually chdir() there, not to stat() it.

Probably, but this opens more avenues of complexity.  Plus it's presumably
OK to remove `foo/..' from the path (when not chasing links) just so long
as foo is a directory, even if you can't cd into it.

> Here's what I propose, in more detail:
> 
> (1) When chaselinks is set, don't rationalize, simply attempt the cd and
> either get the new directory if it succeeds or issue an error if it fails.
> I'm going to peer at 3.0.6-pre; hopefully it isn't too hard to add this
> one bit.

This seems OK.

> (2) When chaselinks is not set, either:
> (2a) keep the pre-7139 behavior, or
> (2b) add a "chasedots" option or some such that means zsh should act as
> though chaselinks is set iff the path contains ".." anywhere.

I don't think the old behaviour is quite up to scratch, so I've added
chasedots.  But it has slightly obscure features, too:  suppose your home
directory path is a link (typically because of an automounter), and you do
a `cd ..' from a link below $HOME, which keeps you in your hierarchy under
$HOME.  Then the home directory will be resolved to its physical directory
at the same time as the .. is resolved to the physical directory.  But this
is forced on us:  if we don't resolve the links, we simply don't know where
we've changed to --- at least, without some directory-equivalence mechanism
like Emacs, which I'm not proposing.

> I just tried this in pre-7139 3.1.6-test-1, and found that zsh *always*
> turns $PWD/.. into $PWD:h, completely independent of chaselinks.

That's now gone when chaselinks is set.  I've mentioned this explicitly in
the manual.

> } (Although with AUTOCD `../rod' on its own already fails,
> } because it tests for a physical directory, since cancd() doesn't call
> } fixdir() --- anyone want that fixed?)
> 
> Only if you add the "chasedots" option.

Hmm, the old behaviour just seems to me plain wrong.  ../rod on its own
with AUTOCD should act the same as testing whether `cd ../rod' would work,
and if so doing it, which failed in this case.

> } I would be perfectly happy to back off the patch altogether, too.
> 
> That plus (1) above would make me comfortable.

I'm not sure it should make you comfortable.

Anyway, the horrors uncovered this time include:

- With the previous patches, a `foo/..' which failed because foo wasn't
  statable wouldn't have rationalised the .. away if for some reason
  the chdir with the .. worked.  Now it does it to the physical directory.
  This shouldn't happen very often.

- I forgot to null-terminate the string passed back in such cases.

- I forgot that fixdir() did it's own unmetafication.

- xsymlink() refused to do anything if CHASELINKS wasn't set, yet
  in only one case was it called without CHASELINKS, and in one other
  CHASELINKS had to be set temporarily.  I've rationalized it so that it
  alwasy chases links and doesn't get called if that's not needed.

- xsymlinks() always got called with the flag 1, so I've hard-wired that.

- xsymlink had the strange feature that if it didn't find symlinks, but did
  remove ..'s, it gave you back the original buffer, potentially leaving
  you a pwd with ..'s in it.  In the old days, fixdir() would have fixed
  this (but using the logical, not the physical, path), but as it now
  doesn't, I've made xsymlink always return the the rationalized path.

--- Doc/Zsh/options.yo.cd3	Wed Jun 23 15:20:33 1999
+++ Doc/Zsh/options.yo	Fri Jul 16 11:48:47 1999
@@ -205,11 +205,30 @@
 slash, try to expand the expression as if it were preceded by a `tt(~)' (see
 noderef(Filename Expansion)).
 )
+pindex(CHASE_DOTS)
+cindex(cd, with .. in argument)
+item(tt(CHASE_DOTS))(
+When changing to a directory containing a path segment `tt(..)' which would
+otherwise be treated as cancelling the previous segment in the path (in
+other words, `tt(foo/..)' would be removed from the path, or if `tt(..)' is
+the first part of the path, the last part of tt($PWD) would be deleted),
+instead resolve the path to the physical directory.  This option is
+overridden by tt(CHASE_LINKS).
+
+For example, suppose tt(/foo/bar) is a link to the directory tt(/alt/rod).
+Without this option set, `tt(cd /foo/bar/..)' changes to tt(/foo); with it
+set, it changes to tt(/alt).  The same applies if the current directory
+is tt(/foo/bar) and `tt(cd ..)' is used.  Note that all other symbolic
+links in the path will also be resolved.
+)
 pindex(CHASE_LINKS)
 cindex(links, symbolic)
 cindex(symbolic links)
 item(tt(CHASE_LINKS) (tt(-w)))(
 Resolve symbolic links to their true values when changing directory.
+This also has the effect of tt(CHASE_DOTS), i.e. a `tt(..)' path segment
+will be treated as referring to the physical parent, even if the preceeding
+path segment is a symbolic link.
 )
 pindex(CLOBBER)
 cindex(clobbering, of files)
--- Src/builtin.c.cd3	Thu Jul 15 17:40:15 1999
+++ Src/builtin.c	Fri Jul 16 12:00:02 1999
@@ -658,6 +658,9 @@
     }
 }
 
+/* set if we are resolving links to their true paths */
+static int chasinglinks;
+
 /* The main pwd changing function.  The real work is done by other     *
  * functions.  cd_get_dest() does the initial argument processing;     *
  * cd_do_chdir() actually changes directory, if possible; cd_new_pwd() *
@@ -670,7 +673,6 @@
 {
     LinkNode dir;
     struct stat st1, st2;
-    int chaselinks;
 
     if (isset(RESTRICTED)) {
 	zwarnnam(nam, "restricted", NULL, 0);
@@ -694,7 +696,7 @@
 	for (s = *argv; *++s; ops[STOUC(*s)] = 1);
     }
   brk:
-    chaselinks = ops['P'] || (isset(CHASELINKS) && !ops['L']);
+    chasinglinks = ops['P'] || (isset(CHASELINKS) && !ops['L']);
     PERMALLOC {
 	pushnode(dirstack, ztrdup(pwd));
 	if (!(dir = cd_get_dest(nam, argv, ops, func))) {
@@ -702,7 +704,7 @@
 	    LASTALLOC_RETURN 1;
 	}
     } LASTALLOC;
-    cd_new_pwd(func, dir, chaselinks);
+    cd_new_pwd(func, dir);
 
     if (stat(unmeta(pwd), &st1) < 0) {
 	zsfree(pwd);
@@ -710,7 +712,7 @@
     } else if (stat(".", &st2) < 0)
 	chdir(unmeta(pwd));
     else if (st1.st_ino != st2.st_ino || st1.st_dev != st2.st_dev) {
-	if (chaselinks) {
+	if (chasinglinks) {
 	    zsfree(pwd);
 	    pwd = metafy(zgetcwd(), -1, META_DUP);
 	} else {
@@ -915,40 +917,49 @@
 cd_try_chdir(char *pfix, char *dest, int hard)
 {
     char *buf;
+    int dlen, dochaselinks = 0;
 
     /* handle directory prefix */
     if (pfix && *pfix) {
 	if (*pfix == '/')
 	    buf = tricat(pfix, "/", dest);
 	else {
-	    int pwl = strlen(pwd);
 	    int pfl = strlen(pfix);
+	    dlen = strlen(pwd);
 
-	    buf = zalloc(pwl + pfl + strlen(dest) + 3);
+	    buf = zalloc(dlen + pfl + strlen(dest) + 3);
 	    strcpy(buf, pwd);
-	    buf[pwl] = '/';
-	    strcpy(buf + pwl + 1, pfix);
-	    buf[pwl + 1 + pfl] = '/';
-	    strcpy(buf + pwl + pfl + 2, dest);
+	    buf[dlen] = '/';
+	    strcpy(buf + dlen + 1, pfix);
+	    buf[dlen + 1 + pfl] = '/';
+	    strcpy(buf + dlen + pfl + 2, dest);
 	}
     } else if (*dest == '/')
 	buf = ztrdup(dest);
     else {
-	int pwl = strlen(pwd);
+	dlen = strlen(pwd);
 
-	buf = zalloc(pwl + strlen(dest) + 2);
+	buf = zalloc(dlen + strlen(dest) + 2);
 	strcpy(buf, pwd);
-	buf[pwl] = '/';
-	strcpy(buf + pwl + 1, dest);
+	buf[dlen] = '/';
+	strcpy(buf + dlen + 1, dest);
     }
 
-    /* Normalise path.  See the definition of fixdir() for what this means. */
-    fixdir(buf);
+    /* Normalise path.  See the definition of fixdir() for what this means.
+     * We do not do this if we are chasing links.
+     */
+    if (!chasinglinks)
+	dochaselinks = fixdir(buf);
+    else
+	unmetafy(buf, &dlen);
 
     if (lchdir(buf, NULL, hard)) {
-	zsfree(buf);
+	free(buf);
 	return NULL;
     }
+    /* the chdir succeeded, so decide if we should force links to be chased */
+    if (dochaselinks)
+	chasinglinks = 1;
     return metafy(buf, -1, META_NOALLOC);
 }
 
@@ -956,7 +967,7 @@
 
 /**/
 static void
-cd_new_pwd(int func, LinkNode dir, int chaselinks)
+cd_new_pwd(int func, LinkNode dir)
 {
     List l;
     char *new_pwd, *s;
@@ -972,7 +983,7 @@
     } else if (func == BIN_CD && unset(AUTOPUSHD))
 	zsfree(getlinknode(dirstack));
 
-    if (chaselinks) {
+    if (chasinglinks) {
 	s = new_pwd;
 	new_pwd = findpwd(s);
 	zsfree(s);
@@ -1039,16 +1050,20 @@
 }
 
 /* Normalise a path.  Segments consisting of ., and foo/.. *
- * combinations, are removed and the path is unmetafied.   */
+ * combinations, are removed and the path is unmetafied.
+ * Returns 1 if we found a ../ path which should force links to
+ * be chased, 0 otherwise.
+ */
 
 /**/
-void
+int
 fixdir(char *src)
 {
     char *dest = src, *d0 = dest;
 #ifdef __CYGWIN
     char *s0 = src;
 #endif
+    int ret = 0;
 
 /*** if have RFS superroot directory ***/
 #ifdef HAVE_SUPERROOT
@@ -1080,32 +1095,40 @@
 	    while (dest > d0 + 1 && dest[-1] == '/')
 		dest--;
 	    *dest = '\0';
-	    return;
+	    return ret;
 	}
 	if (src[0] == '.' && src[1] == '.' &&
-	  (src[2] == '\0' || src[2] == '/')) {
-	    if (dest > d0 + 1) {
-		/*
-		 * remove a foo/.. combination:
-		 * first check foo exists, else return
-		 */
-		struct stat st;
-		*dest = '\0';
-		if (stat(d0, &st) < 0 || !S_ISDIR(st.st_mode)) {
-		    char *ptrd, *ptrs;
-		    if (dest == src)
-			*dest = '.';
-		    for (ptrs = src, ptrd = dest; *ptrs; ptrs++, ptrd++)
-			*ptrd = (*ptrs == Meta) ? (*++ptrs ^ 32) : *ptrs;
-		    return;
+	    (src[2] == '\0' || src[2] == '/')) {
+	    if (isset(CHASEDOTS)) {
+		ret = 1;
+		/* and treat as normal path segment */
+	    } else {
+		if (dest > d0 + 1) {
+		    /*
+		     * remove a foo/.. combination:
+		     * first check foo exists, else return.
+		     */
+		    struct stat st;
+		    *dest = '\0';
+		    if (stat(d0, &st) < 0 || !S_ISDIR(st.st_mode)) {
+			char *ptrd, *ptrs;
+			if (dest == src)
+			    *dest = '.';
+			for (ptrs = src, ptrd = dest; *ptrs; ptrs++, ptrd++)
+			    *ptrd = (*ptrs == Meta) ? (*++ptrs ^ 32) : *ptrs;
+			*ptrd = '\0';
+			return 1;
+		    }
+		    for (dest--; dest > d0 + 1 && dest[-1] != '/'; dest--);
+		    if (dest[-1] != '/')
+			dest--;
 		}
-		for (dest--; dest > d0 + 1 && dest[-1] != '/'; dest--);
-		if (dest[-1] != '/')
-		    dest--;
+		src++;
+		while (*++src == '/');
+		continue;
 	    }
-	    src++;
-	    while (*++src == '/');
-	} else if (src[0] == '.' && (src[1] == '/' || src[1] == '\0')) {
+	}
+	if (src[0] == '.' && (src[1] == '/' || src[1] == '\0')) {
 	    /* skip a . section */
 	    while (*++src == '/');
 	} else {
--- Src/exec.c.cd3	Thu Jul 15 17:40:01 1999
+++ Src/exec.c	Fri Jul 16 11:22:02 1999
@@ -3144,16 +3144,24 @@
 cancd2(char *s)
 {
     struct stat buf;
-    char *us = unmeta(s), *us2 = NULL;
+    char *us, *us2 = NULL;
 
-    if (*us != '/')
-	us = us2 = tricat(unmeta(pwd), "/", us);
-    else
-	us = dupstring(us);
-    fixdir(us);
+    /*
+     * If CHASEDOTS and CHASELINKS are not set, we want to rationalize the
+     * path by removing foo/.. combinations in the logical rather than
+     * the physical path.  If either is set, we test the physical path.
+     */
+    if (!isset(CHASEDOTS) && !isset(CHASELINKS)) {
+	if (*s != '/')
+	    us = tricat(pwd[1] ? pwd : "", "/", s);
+	else
+	    us = ztrdup(s);
+	fixdir(us2 = us);
+    } else
+	us = unmeta(s);
     return !(access(us, X_OK) || stat(us, &buf) || !S_ISDIR(buf.st_mode));
     if (us2)
-	zsfree(us2);
+	free(us2);
 }
 
 /**/
--- Src/options.c.cd3	Fri Jun 18 10:38:57 1999
+++ Src/options.c	Fri Jul 16 09:47:19 1999
@@ -91,6 +91,7 @@
 {NULL, "braceccl",	      0,			 BRACECCL},
 {NULL, "bsdecho",	      OPT_EMULATE|OPT_SH,	 BSDECHO},
 {NULL, "cdablevars",	      0,			 CDABLEVARS},
+{NULL, "chasedots",	      0,			 CHASEDOTS},
 {NULL, "chaselinks",	      0,			 CHASELINKS},
 {NULL, "clobber",	      OPT_ALL,			 CLOBBER},
 {NULL, "completealiases",     0,			 COMPLETEALIASES},
--- Src/utils.c.cd3	Tue Jul 13 14:20:12 1999
+++ Src/utils.c	Fri Jul 16 11:19:42 1999
@@ -315,7 +315,7 @@
 
 /**/
 static int
-xsymlinks(char *s, int flag)
+xsymlinks(char *s)
 {
     char **pp, **opp;
     char xbuf2[PATH_MAX*2], xbuf3[PATH_MAX*2];
@@ -338,15 +338,9 @@
 	    *p = '\0';
 	    continue;
 	}
-	if (unset(CHASELINKS)) {
-	    strcat(xbuf, "/");
-	    strcat(xbuf, *pp);
-	    zsfree(*pp);
-	    continue;
-	}
 	sprintf(xbuf2, "%s/%s", xbuf, *pp);
 	t0 = readlink(unmeta(xbuf2), xbuf3, PATH_MAX);
-	if (t0 == -1 || !flag) {
+	if (t0 == -1) {
 	    strcat(xbuf, "/");
 	    strcat(xbuf, *pp);
 	    zsfree(*pp);
@@ -355,9 +349,9 @@
 	    metafy(xbuf3, t0, META_NOALLOC);
 	    if (*xbuf3 == '/') {
 		strcpy(xbuf, "");
-		xsymlinks(xbuf3 + 1, flag);
+		xsymlinks(xbuf3 + 1);
 	    } else
-		xsymlinks(xbuf3, flag);
+		xsymlinks(xbuf3);
 	    zsfree(*pp);
 	}
     }
@@ -365,19 +359,19 @@
     return ret;
 }
 
-/* expand symlinks in s, and remove other weird things */
+/*
+ * expand symlinks in s, and remove other weird things:
+ * note that this always expands symlinks.
+ */
 
 /**/
 char *
 xsymlink(char *s)
 {
-    if (unset(CHASELINKS))
-	return ztrdup(s);
     if (*s != '/')
 	return NULL;
     *xbuf = '\0';
-    if (!xsymlinks(s + 1, 1))
-	return ztrdup(s);
+    xsymlinks(s + 1);
     if (!*xbuf)
 	return ztrdup("/");
     return ztrdup(xbuf);
@@ -387,15 +381,10 @@
 void
 print_if_link(char *s)
 {
-    int chase;
-
     if (*s == '/') {
-	chase = opts[CHASELINKS];
-	opts[CHASELINKS] = 1;
 	*xbuf = '\0';
-	if (xsymlinks(s + 1, 1))
+	if (xsymlinks(s + 1))
 	    printf(" -> "), zputs(*xbuf ? xbuf : "/", stdout);
-	opts[CHASELINKS] = chase;
     }
 }
 
@@ -586,7 +575,8 @@
 	/* Retrieve an entry from the password table/database for this user. */
 	struct passwd *pw;
 	if ((pw = getpwnam(name))) {
-	    char *dir = xsymlink(pw->pw_dir);
+	    char *dir = isset(CHASELINKS) ? xsymlink(pw->pw_dir)
+		: ztrdup(pw->pw_dir);
 	    adduserdir(name, dir, ND_USERNAME, 1);
 	    str = dupstring(dir);
 	    zsfree(dir);
--- Src/zsh.h.cd3	Fri Jul 16 09:46:47 1999
+++ Src/zsh.h	Fri Jul 16 09:46:52 1999
@@ -1171,6 +1171,7 @@
     BRACECCL,
     BSDECHO,
     CDABLEVARS,
+    CHASEDOTS,
     CHASELINKS,
     CLOBBER,
     COMPLETEALIASES,

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