zsh-workers
 help / color / mirror / code / Atom feed
* cd does not update $PWD in a timely fashion.
@ 2020-06-26 17:24 Han Boetes
  2020-06-26 17:32 ` Matthew Martin
  0 siblings, 1 reply; 5+ messages in thread
From: Han Boetes @ 2020-06-26 17:24 UTC (permalink / raw)
  To: zsh-workers

[-- Attachment #1: Type: text/plain, Size: 823 bytes --]

I had this program changing the name of the current working directory and
noticed cd .; echo $PWD didn't pick up the changes.

The following code replicates the problem.

% set -x;mkdir foo; cd foo; mv ~/foo ~/bar; echo $PWD;pwd; readlink -f .;
cd .; echo $PWD ;pwd; readlink -f .; cd .; echo $PWD ;pwd; readlink -f .
+/bin/zsh:8> mkdir foo
+/bin/zsh:8> cd foo
+/bin/zsh:8> mv -i /home/han/foo /home/han/bar
+/bin/zsh:8> echo /home/han/foo
/home/han/foo
+/bin/zsh:8> pwd
/home/han/foo
+/bin/zsh:8> readlink -f .
/home/han/bar
+/bin/zsh:8> cd .
+/bin/zsh:8> echo /home/han/foo
/home/han/foo
+/bin/zsh:8> pwd
/home/han/bar
+/bin/zsh:8> readlink -f .
/home/han/bar
+/bin/zsh:8> cd .
+/bin/zsh:8> echo /home/han/bar
/home/han/bar
+/bin/zsh:8> pwd
/home/han/bar
+/bin/zsh:8> readlink -f .
/home/han/bar


With kind regards,
Han

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: cd does not update $PWD in a timely fashion.
  2020-06-26 17:24 cd does not update $PWD in a timely fashion Han Boetes
@ 2020-06-26 17:32 ` Matthew Martin
  2020-06-27  7:25   ` Matthew Martin
  0 siblings, 1 reply; 5+ messages in thread
From: Matthew Martin @ 2020-06-26 17:32 UTC (permalink / raw)
  To: zsh-workers

On Fri, Jun 26, 2020 at 07:24:21PM +0200, Han Boetes wrote:
> I had this program changing the name of the current working directory and
> noticed cd .; echo $PWD didn't pick up the changes.

Was first discussed in IRC, so I had a head start on the patch.

bin_cd may update pwd after calling cd_new_pwd which calls set_pwd_env.
Since bin_cd is the only caller of cd_new_pwd, I think it's safe to move
that code into cd_new_pwd prior to the oldpwd, pwd, new_pwd shift. This
should also call the chpwd hook with the right value.

Seems to fix the issue locally. I know Daniel will rightfully ask for
a new test, so I'll work on that tonight.


diff --git a/Src/builtin.c b/Src/builtin.c
index 770930579..ff84ce936 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -841,7 +841,6 @@ int
 bin_cd(char *nam, char **argv, Options ops, int func)
 {
     LinkNode dir;
-    struct stat st1, st2;
 
     if (isset(RESTRICTED)) {
 	zwarnnam(nam, "restricted");
@@ -860,23 +859,6 @@ bin_cd(char *nam, char **argv, Options ops, int func)
     }
     cd_new_pwd(func, dir, OPT_ISSET(ops, 'q'));
 
-    if (stat(unmeta(pwd), &st1) < 0) {
-	setjobpwd();
-	zsfree(pwd);
-	pwd = NULL;
-	pwd = metafy(zgetcwd(), -1, META_DUP);
-    } else if (stat(".", &st2) < 0) {
-	if (chdir(unmeta(pwd)) < 0)
-	    zwarn("unable to chdir(%s): %e", pwd, errno);
-    } else if (st1.st_ino != st2.st_ino || st1.st_dev != st2.st_dev) {
-	if (chasinglinks) {
-	    setjobpwd();
-	    zsfree(pwd);
-	    pwd = NULL;
-	    pwd = metafy(zgetcwd(), -1, META_DUP);
-	} else if (chdir(unmeta(pwd)) < 0)
-	    zwarn("unable to chdir(%s): %e", pwd, errno);
-    }
     unqueue_signals();
     return 0;
 }
@@ -1210,6 +1192,7 @@ static void
 cd_new_pwd(int func, LinkNode dir, int quiet)
 {
     char *new_pwd, *s;
+    struct stat st1, st2;
     int dirstacksize;
 
     if (func == BIN_PUSHD)
@@ -1239,6 +1222,22 @@ cd_new_pwd(int func, LinkNode dir, int quiet)
 	}
     }
 
+    if (stat(unmeta(new_pwd), &st1) < 0) {
+	zsfree(new_pwd);
+	new_pwd = NULL;
+	new_pwd = metafy(zgetcwd(), -1, META_DUP);
+    } else if (stat(".", &st2) < 0) {
+	if (chdir(unmeta(new_pwd)) < 0)
+	    zwarn("unable to chdir(%s): %e", new_pwd, errno);
+    } else if (st1.st_ino != st2.st_ino || st1.st_dev != st2.st_dev) {
+	if (chasinglinks) {
+	    zsfree(new_pwd);
+	    new_pwd = NULL;
+	    new_pwd = metafy(zgetcwd(), -1, META_DUP);
+	} else if (chdir(unmeta(new_pwd)) < 0)
+	    zwarn("unable to chdir(%s): %e", new_pwd, errno);
+    }
+
     /* shift around the pwd variables, to make oldpwd and pwd relate to the
        current (i.e. new) pwd */
     zsfree(oldpwd);

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: cd does not update $PWD in a timely fashion.
  2020-06-26 17:32 ` Matthew Martin
@ 2020-06-27  7:25   ` Matthew Martin
  2020-06-27 20:42     ` Daniel Shahaf
  0 siblings, 1 reply; 5+ messages in thread
From: Matthew Martin @ 2020-06-27  7:25 UTC (permalink / raw)
  To: zsh-workers

On Fri, Jun 26, 2020 at 12:32:51PM -0500, Matthew Martin wrote:
> On Fri, Jun 26, 2020 at 07:24:21PM +0200, Han Boetes wrote:
> > I had this program changing the name of the current working directory and
> > noticed cd .; echo $PWD didn't pick up the changes.
> 
> Was first discussed in IRC, so I had a head start on the patch.
> 
> bin_cd may update pwd after calling cd_new_pwd which calls set_pwd_env.
> Since bin_cd is the only caller of cd_new_pwd, I think it's safe to move
> that code into cd_new_pwd prior to the oldpwd, pwd, new_pwd shift. This
> should also call the chpwd hook with the right value.
> 
> Seems to fix the issue locally. I know Daniel will rightfully ask for
> a new test, so I'll work on that tonight.

And with the test


diff --git a/Src/builtin.c b/Src/builtin.c
index 770930579..ff84ce936 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -841,7 +841,6 @@ int
 bin_cd(char *nam, char **argv, Options ops, int func)
 {
     LinkNode dir;
-    struct stat st1, st2;
 
     if (isset(RESTRICTED)) {
 	zwarnnam(nam, "restricted");
@@ -860,23 +859,6 @@ bin_cd(char *nam, char **argv, Options ops, int func)
     }
     cd_new_pwd(func, dir, OPT_ISSET(ops, 'q'));
 
-    if (stat(unmeta(pwd), &st1) < 0) {
-	setjobpwd();
-	zsfree(pwd);
-	pwd = NULL;
-	pwd = metafy(zgetcwd(), -1, META_DUP);
-    } else if (stat(".", &st2) < 0) {
-	if (chdir(unmeta(pwd)) < 0)
-	    zwarn("unable to chdir(%s): %e", pwd, errno);
-    } else if (st1.st_ino != st2.st_ino || st1.st_dev != st2.st_dev) {
-	if (chasinglinks) {
-	    setjobpwd();
-	    zsfree(pwd);
-	    pwd = NULL;
-	    pwd = metafy(zgetcwd(), -1, META_DUP);
-	} else if (chdir(unmeta(pwd)) < 0)
-	    zwarn("unable to chdir(%s): %e", pwd, errno);
-    }
     unqueue_signals();
     return 0;
 }
@@ -1210,6 +1192,7 @@ static void
 cd_new_pwd(int func, LinkNode dir, int quiet)
 {
     char *new_pwd, *s;
+    struct stat st1, st2;
     int dirstacksize;
 
     if (func == BIN_PUSHD)
@@ -1239,6 +1222,22 @@ cd_new_pwd(int func, LinkNode dir, int quiet)
 	}
     }
 
+    if (stat(unmeta(new_pwd), &st1) < 0) {
+	zsfree(new_pwd);
+	new_pwd = NULL;
+	new_pwd = metafy(zgetcwd(), -1, META_DUP);
+    } else if (stat(".", &st2) < 0) {
+	if (chdir(unmeta(new_pwd)) < 0)
+	    zwarn("unable to chdir(%s): %e", new_pwd, errno);
+    } else if (st1.st_ino != st2.st_ino || st1.st_dev != st2.st_dev) {
+	if (chasinglinks) {
+	    zsfree(new_pwd);
+	    new_pwd = NULL;
+	    new_pwd = metafy(zgetcwd(), -1, META_DUP);
+	} else if (chdir(unmeta(new_pwd)) < 0)
+	    zwarn("unable to chdir(%s): %e", new_pwd, errno);
+    }
+
     /* shift around the pwd variables, to make oldpwd and pwd relate to the
        current (i.e. new) pwd */
     zsfree(oldpwd);
diff --git a/Test/B01cd.ztst b/Test/B01cd.ztst
index 21e751dcb..cd0b79fb2 100644
--- a/Test/B01cd.ztst
+++ b/Test/B01cd.ztst
@@ -33,7 +33,7 @@
 #
 # Tests should use subdirectories ending in `.tmp'.  These will be
 # removed with all the contents even if the test is aborted.
- mkdir cdtst.tmp cdtst.tmp/real cdtst.tmp/sub
+ mkdir cdtst.tmp cdtst.tmp/foo cdtst.tmp/real cdtst.tmp/sub
 
  ln -s ../real cdtst.tmp/sub/fake
 
@@ -149,6 +149,21 @@ F:something is broken.  But you already knew that.
 -f:(workers/45367) cd -P squashes multiple leading slashes
 >/dev  
 
+ chpwd_hook() { hook_pwd=$PWD; }
+ chpwd_functions=(chpwd_hook)
+ cd $mydir/cdtst.tmp/foo &&
+ (cd $mydir; mv $mydir/cdtst.tmp/{foo,bar}) &&
+ print $PWD &&
+ print $hook_pwd &&
+ cd . &&
+ print $PWD
+ print $hook_pwd &&
+0q:cd . with moved PWD
+>$mydir/cdtst.tmp/foo
+>$mydir/cdtst.tmp/foo
+>$mydir/cdtst.tmp/bar
+>$mydir/cdtst.tmp/bar
+
 %clean
 # This optional section cleans up after the test, if necessary,
 # e.g. killing processes etc.  This is in addition to the removal of *.tmp

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: cd does not update $PWD in a timely fashion.
  2020-06-27  7:25   ` Matthew Martin
@ 2020-06-27 20:42     ` Daniel Shahaf
  2020-07-01  4:41       ` Matthew Martin
  0 siblings, 1 reply; 5+ messages in thread
From: Daniel Shahaf @ 2020-06-27 20:42 UTC (permalink / raw)
  To: Matthew Martin; +Cc: zsh-workers

Matthew Martin wrote on Sat, 27 Jun 2020 02:25 -0500:
> +++ b/Test/B01cd.ztst
> @@ -149,6 +149,21 @@ F:something is broken.  But you already knew that.
> + chpwd_hook() { hook_pwd=$PWD; }
> + chpwd_functions=(chpwd_hook)
> + cd $mydir/cdtst.tmp/foo &&
> + (cd $mydir; mv $mydir/cdtst.tmp/{foo,bar}) &&
> + print $PWD &&
> + print $hook_pwd &&
> + cd . &&
> + print $PWD

Is the lack of «&&» on this line intentional?

> + print $hook_pwd &&

Run «unset chpwd_functions» here so as not to affect subsequent tests
(once they're added)?  Compare 46067.

> +0q:cd . with moved PWD

Cheers,

Daniel

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: cd does not update $PWD in a timely fashion.
  2020-06-27 20:42     ` Daniel Shahaf
@ 2020-07-01  4:41       ` Matthew Martin
  0 siblings, 0 replies; 5+ messages in thread
From: Matthew Martin @ 2020-07-01  4:41 UTC (permalink / raw)
  To: zsh-workers

On Sat, Jun 27, 2020 at 08:42:08PM +0000, Daniel Shahaf wrote:
> Matthew Martin wrote on Sat, 27 Jun 2020 02:25 -0500:
> > + print $PWD
> 
> Is the lack of «&&» on this line intentional?
> 
> > + print $hook_pwd &&
> 
> Run «unset chpwd_functions» here so as not to affect subsequent tests
> (once they're added)?  Compare 46067.

It wasn't and thanks for the pointer.



diff --git a/Src/builtin.c b/Src/builtin.c
index 770930579..ff84ce936 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -841,7 +841,6 @@ int
 bin_cd(char *nam, char **argv, Options ops, int func)
 {
     LinkNode dir;
-    struct stat st1, st2;
 
     if (isset(RESTRICTED)) {
 	zwarnnam(nam, "restricted");
@@ -860,23 +859,6 @@ bin_cd(char *nam, char **argv, Options ops, int func)
     }
     cd_new_pwd(func, dir, OPT_ISSET(ops, 'q'));
 
-    if (stat(unmeta(pwd), &st1) < 0) {
-	setjobpwd();
-	zsfree(pwd);
-	pwd = NULL;
-	pwd = metafy(zgetcwd(), -1, META_DUP);
-    } else if (stat(".", &st2) < 0) {
-	if (chdir(unmeta(pwd)) < 0)
-	    zwarn("unable to chdir(%s): %e", pwd, errno);
-    } else if (st1.st_ino != st2.st_ino || st1.st_dev != st2.st_dev) {
-	if (chasinglinks) {
-	    setjobpwd();
-	    zsfree(pwd);
-	    pwd = NULL;
-	    pwd = metafy(zgetcwd(), -1, META_DUP);
-	} else if (chdir(unmeta(pwd)) < 0)
-	    zwarn("unable to chdir(%s): %e", pwd, errno);
-    }
     unqueue_signals();
     return 0;
 }
@@ -1210,6 +1192,7 @@ static void
 cd_new_pwd(int func, LinkNode dir, int quiet)
 {
     char *new_pwd, *s;
+    struct stat st1, st2;
     int dirstacksize;
 
     if (func == BIN_PUSHD)
@@ -1239,6 +1222,22 @@ cd_new_pwd(int func, LinkNode dir, int quiet)
 	}
     }
 
+    if (stat(unmeta(new_pwd), &st1) < 0) {
+	zsfree(new_pwd);
+	new_pwd = NULL;
+	new_pwd = metafy(zgetcwd(), -1, META_DUP);
+    } else if (stat(".", &st2) < 0) {
+	if (chdir(unmeta(new_pwd)) < 0)
+	    zwarn("unable to chdir(%s): %e", new_pwd, errno);
+    } else if (st1.st_ino != st2.st_ino || st1.st_dev != st2.st_dev) {
+	if (chasinglinks) {
+	    zsfree(new_pwd);
+	    new_pwd = NULL;
+	    new_pwd = metafy(zgetcwd(), -1, META_DUP);
+	} else if (chdir(unmeta(new_pwd)) < 0)
+	    zwarn("unable to chdir(%s): %e", new_pwd, errno);
+    }
+
     /* shift around the pwd variables, to make oldpwd and pwd relate to the
        current (i.e. new) pwd */
     zsfree(oldpwd);
diff --git a/Test/B01cd.ztst b/Test/B01cd.ztst
index 21e751dcb..bc6757549 100644
--- a/Test/B01cd.ztst
+++ b/Test/B01cd.ztst
@@ -33,7 +33,7 @@
 #
 # Tests should use subdirectories ending in `.tmp'.  These will be
 # removed with all the contents even if the test is aborted.
- mkdir cdtst.tmp cdtst.tmp/real cdtst.tmp/sub
+ mkdir cdtst.tmp cdtst.tmp/foo cdtst.tmp/real cdtst.tmp/sub
 
  ln -s ../real cdtst.tmp/sub/fake
 
@@ -149,6 +149,23 @@ F:something is broken.  But you already knew that.
 -f:(workers/45367) cd -P squashes multiple leading slashes
 >/dev  
 
+ chpwd_hook() { hook_pwd=$PWD; }
+ chpwd_functions=(chpwd_hook)
+ cd $mydir/cdtst.tmp/foo &&
+ (cd $mydir && mv $mydir/cdtst.tmp/{foo,bar}) &&
+ print $PWD &&
+ print $hook_pwd &&
+ cd . &&
+ print $PWD &&
+ print $hook_pwd
+ chpwd_functions=()
+ unfunction chpwd_hook
+0q:cd . with moved PWD
+>$mydir/cdtst.tmp/foo
+>$mydir/cdtst.tmp/foo
+>$mydir/cdtst.tmp/bar
+>$mydir/cdtst.tmp/bar
+
 %clean
 # This optional section cleans up after the test, if necessary,
 # e.g. killing processes etc.  This is in addition to the removal of *.tmp

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-07-01  4:42 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-26 17:24 cd does not update $PWD in a timely fashion Han Boetes
2020-06-26 17:32 ` Matthew Martin
2020-06-27  7:25   ` Matthew Martin
2020-06-27 20:42     ` Daniel Shahaf
2020-07-01  4:41       ` Matthew Martin

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).