GNU bug report logs - #23324
shell-resync-dirs does not handle dirs with whitespace

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 23324 in the body.
You can then email your comments to 23324 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Noah Friedman <friedman <at> splode.com>
To: bug-gnu-emacs <at> gnu.org
Subject: shell-resync-dirs does not handle dirs with whitespace
Date: Wed, 20 Apr 2016 19:38:43 -0700 (PDT)

[Message part 1 (text/plain, inline)]

I have a fix for this, but I'd like for someone else to look over it and
perhaps sanity check it before I commit it.  Volunteers?

This is a very old limitation.

Let's say I have three directories in my shell buffer directory stack:

	/s/software/wwwroot/Java Development Kit
	/s/software/wwwroot/Perl Compatible Regular Expressions
	~/src/build/depot/main/tools/build

If I run M-x shell-resync-dirs, I simply get the error:

	Couldn’t cd: (error No such directory found via CDPATH environment variable)

because the directory "/s/software/wwwroot/Java" doesn't exist.  The parser
considers whitespace to be a separator between directory tokens.

This version handles that case.

Diff and full function attached below.

[ChangeLog (text/plain, inline)]

2016-04-21  Noah Friedman  <friedman <at> splode.com>

	* lisp/shell.el (shell-resync-dirs): Correctly handle
	whitespace in directory names.

[shell.el.diff (text/plain, inline)]

--- 2016-04-20--18-08-56--0c9f35a/lisp/shell.el.~1~	2016-01-01 01:34:24.000000000 -0800
+++ 2016-04-20--18-08-56--0c9f35a/lisp/shell.el	2016-04-20 19:24:25.534756498 -0700
@@ -985,45 +985,62 @@
       ;; If the process echoes commands, don't insert a fake command in
       ;; the buffer or it will appear twice.
       (unless comint-process-echoes
-	(insert shell-dirstack-query) (insert "\n"))
+	(insert shell-dirstack-query "\n"))
       (sit-for 0)			; force redisplay
       (comint-send-string proc shell-dirstack-query)
       (comint-send-string proc "\n")
       (set-marker pmark (point))
       (let ((pt (point))
-	    (regexp
-	     (concat
-	      (if comint-process-echoes
-		  ;; Skip command echo if the process echoes
-		  (concat "\\(" (regexp-quote shell-dirstack-query) "\n\\)")
-		"\\(\\)")
-	      "\\(.+\n\\)")))
+	    (regexp (concat
+                     (if comint-process-echoes
+                         ;; Skip command echo if the process echoes
+                         (concat "\\(" (regexp-quote shell-dirstack-query) "\n\\)")
+                       "\\(\\)")
+                     "\\(.+\n\\)")))
 	;; This extra newline prevents the user's pending input from spoofing us.
-	(insert "\n") (backward-char 1)
+	(insert "\n")
+        (backward-char 1)
 	;; Wait for one line.
 	(while (not (looking-at regexp))
-	  (accept-process-output proc)
+	  (accept-process-output proc 1)
 	  (goto-char pt)))
-      (goto-char pmark) (delete-char 1) ; remove the extra newline
+      (goto-char pmark)
+      (delete-char 1) ; remove the extra newline
+
       ;; That's the dirlist. grab it & parse it.
-      (let* ((dl (buffer-substring (match-beginning 2) (1- (match-end 2))))
-	     (dl-len (length dl))
-	     (ds '())			; new dir stack
-	     (i 0))
-	(while (< i dl-len)
-	  ;; regexp = optional whitespace, (non-whitespace), optional whitespace
-	  (string-match "\\s *\\(\\S +\\)\\s *" dl i) ; pick off next dir
-	  (setq ds (cons (concat comint-file-name-prefix
-				 (substring dl (match-beginning 1)
-					    (match-end 1)))
-			 ds))
-	  (setq i (match-end 0)))
-	(let ((ds (nreverse ds)))
-	  (with-demoted-errors "Couldn't cd: %s"
-	    (shell-cd (car ds))
-	    (setq shell-dirstack (cdr ds)
-		  shell-last-dir (car shell-dirstack))
-	    (shell-dirstack-message)))))
+      (let* ((dls (buffer-substring-no-properties (match-beginning 0) (1- (match-end 0))))
+             (dlsl '())
+             (pos 0)
+             (ds '()))
+        ;; Split the dirlist into whitespace and non-whitespace chunks.
+        ;; dlsl will be a reversed list of tokens.
+        (while (string-match "\\(\\S-+\\|\\s-+\\)" dls pos)
+          (push (match-string 1 dls) dlsl)
+          (setq pos (match-end 1)))
+
+        ;; prepend trailing entries until they form an existing directory,
+        ;; whitespace and all.  discard the next whitespace and repeat.
+        (while dlsl
+          (let ((newelt "")
+                tem1 tem2)
+            (while newelt
+              ;; We need tem1 because we don't want to prepend
+              ;; comint-file-name-prefix repeatedly into newelt via tem2.
+              (setq tem1 (pop dlsl)
+                    tem2 (concat comint-file-name-prefix tem newelt))
+              (cond ((file-directory-p tem2)
+                     (push tem2 ds)
+                     (when (string= " " (car dlsl))
+                       (pop dlsl))
+                     (setq newelt nil))
+                    (t
+                     (setq newelt (concat tem1 newelt)))))))
+
+        (with-demoted-errors "Couldn't cd: %s"
+          (shell-cd (car ds))
+          (setq shell-dirstack (cdr ds)
+                shell-last-dir (car shell-dirstack))
+          (shell-dirstack-message))))
     (if started-at-pmark (goto-char (marker-position pmark)))))
 
 ;; For your typing convenience:

[shell-resync-dirs.el (text/plain, inline)]

(defun shell-resync-dirs ()
  "Resync the buffer's idea of the current directory stack.
This command queries the shell with the command bound to
`shell-dirstack-query' (default \"dirs\"), reads the next
line output and parses it to form the new directory stack.
DON'T issue this command unless the buffer is at a shell prompt.
Also, note that if some other subprocess decides to do output
immediately after the query, its output will be taken as the
new directory stack -- you lose.  If this happens, just do the
command again."
  (interactive)
  (let* ((proc (get-buffer-process (current-buffer)))
	 (pmark (process-mark proc))
	 (started-at-pmark (= (point) (marker-position pmark))))
    (save-excursion
      (goto-char pmark)
      ;; If the process echoes commands, don't insert a fake command in
      ;; the buffer or it will appear twice.
      (unless comint-process-echoes
	(insert shell-dirstack-query "\n"))
      (sit-for 0)			; force redisplay
      (comint-send-string proc shell-dirstack-query)
      (comint-send-string proc "\n")
      (set-marker pmark (point))
      (let ((pt (point))
	    (regexp (concat
                     (if comint-process-echoes
                         ;; Skip command echo if the process echoes
                         (concat "\\(" (regexp-quote shell-dirstack-query) "\n\\)")
                       "\\(\\)")
                     "\\(.+\n\\)")))
	;; This extra newline prevents the user's pending input from spoofing us.
	(insert "\n")
        (backward-char 1)
	;; Wait for one line.
	(while (not (looking-at regexp))
	  (accept-process-output proc 1)
	  (goto-char pt)))
      (goto-char pmark)
      (delete-char 1) ; remove the extra newline

      ;; That's the dirlist. grab it & parse it.
      (let* ((dls (buffer-substring-no-properties (match-beginning 0) (1- (match-end 0))))
             (dlsl '())
             (pos 0)
             (ds '()))
        ;; Split the dirlist into whitespace and non-whitespace chunks.
        ;; dlsl will be a reversed list of tokens.
        (while (string-match "\\(\\S-+\\|\\s-+\\)" dls pos)
          (push (match-string 1 dls) dlsl)
          (setq pos (match-end 1)))

        ;; prepend trailing entries until they form an existing directory,
        ;; whitespace and all.  discard the next whitespace and repeat.
        (while dlsl
          (let ((newelt "")
                tem1 tem2)
            (while newelt
              ;; We need tem1 because we don't want to prepend
              ;; comint-file-name-prefix repeatedly into newelt via tem2.
              (setq tem1 (pop dlsl)
                    tem2 (concat comint-file-name-prefix tem newelt))
              (cond ((file-directory-p tem2)
                     (push tem2 ds)
                     (when (string= " " (car dlsl))
                       (pop dlsl))
                     (setq newelt nil))
                    (t
                     (setq newelt (concat tem1 newelt)))))))

        (with-demoted-errors "Couldn't cd: %s"
          (shell-cd (car ds))
          (setq shell-dirstack (cdr ds)
                shell-last-dir (car shell-dirstack))
          (shell-dirstack-message))))
    (if started-at-pmark (goto-char (marker-position pmark)))))

Message #16 received at 23324 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> gmail.com>
To: Noah Friedman <friedman <at> splode.com>
Cc: 23324 <at> debbugs.gnu.org
Subject: Re: bug#23324: shell-resync-dirs does not handle dirs with whitespace
Date: Mon, 03 Sep 2018 13:08:42 -0400

forcemerge 23324 11608 9379
quit

Noah Friedman <friedman <at> splode.com> writes:

> I have a fix for this, but I'd like for someone else to look over it and
> perhaps sanity check it before I commit it.  Volunteers?

> This is a very old limitation.

By now this is a fairly old patch, but still in need of review it seems.

> +      (let* ((dls (buffer-substring-no-properties (match-beginning 0) (1- (match-end 0))))
> +             (dlsl '())
> +             (pos 0)
> +             (ds '()))
> +        ;; Split the dirlist into whitespace and non-whitespace chunks.
> +        ;; dlsl will be a reversed list of tokens.
> +        (while (string-match "\\(\\S-+\\|\\s-+\\)" dls pos)
> +          (push (match-string 1 dls) dlsl)
> +          (setq pos (match-end 1)))
> +
> +        ;; prepend trailing entries until they form an existing directory,
> +        ;; whitespace and all.  discard the next whitespace and repeat.

I think this loop is going in the wrong direction (i.e., it should
rather be appending leading entries).  Because of this, it can be fooled
by subdirectories with a name matching a substring of a dirs entry:

    ~$ mkdir -p 'foo bar/bar/'
    ~$ cd 'foo bar/'
    ~/foo bar$ command dirs # M-x dirs
    # infloops...

> +          (let ((newelt "")
> +                tem1 tem2)

I'm also not a fan of the somewhat inscrutable tem1 & tem2 names.

Message #19 received at 23324 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Noah Friedman <friedman <at> splode.com>
Cc: 23324 <at> debbugs.gnu.org, 9379 <at> debbugs.gnu.org
Subject: Re: bug#23324: shell-resync-dirs does not handle dirs with whitespace
Date: Thu, 27 Jun 2019 18:25:07 +0200

Noah Friedman <friedman <at> splode.com> writes:

> 2016-04-21  Noah Friedman  <friedman <at> splode.com>
>
> 	* lisp/shell.el (shell-resync-dirs): Correctly handle
> 	whitespace in directory names.

The patch doesn't seem to be correct:

In shell-resync-dirs:
shell.el:1044:58:Warning: reference to free variable ‘tem’

Should probably be either tem1 or tem2?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #24 received at 23324 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Noam Postavsky <npostavs <at> gmail.com>
Cc: 23324 <at> debbugs.gnu.org, Noah Friedman <friedman <at> splode.com>
Subject: Re: bug#23324: shell-resync-dirs does not handle dirs with whitespace
Date: Wed, 12 Aug 2020 13:44:51 +0200

Noam Postavsky <npostavs <at> gmail.com> writes:

> I think this loop is going in the wrong direction (i.e., it should
> rather be appending leading entries).  Because of this, it can be fooled
> by subdirectories with a name matching a substring of a dirs entry:
>
>     ~$ mkdir -p 'foo bar/bar/'
>     ~$ cd 'foo bar/'
>     ~/foo bar$ command dirs # M-x dirs
>     # infloops...
>
>> +          (let ((newelt "")
>> +                tem1 tem2)
>
> I'm also not a fan of the somewhat inscrutable tem1 & tem2 names.

I've respun the patch so that it actually works (tem1 was mis-spelled in
one place, and there was a bunch of unrelated white space fix-ups that
I've remove), but haven't done anything else to it.

Noam, I tried your test case, but I couldn't get it to infloop.  Is
there some additional steps needed to make it infloop?

diff --git a/lisp/shell.el b/lisp/shell.el
index f5e18bbc72..ed4b1efde0 100644
--- a/lisp/shell.el
+++ b/lisp/shell.el
@@ -1039,24 +1039,40 @@ shell-resync-dirs
 	  (goto-char pt)))
       (goto-char pmark) (delete-char 1) ; remove the extra newline
       ;; That's the dirlist. grab it & parse it.
-      (let* ((dl (buffer-substring (match-beginning 2) (1- (match-end 2))))
-	     (dl-len (length dl))
-	     (ds '())			; new dir stack
-	     (i 0))
-	(while (< i dl-len)
-	  ;; regexp = optional whitespace, (non-whitespace), optional whitespace
-	  (string-match "\\s *\\(\\S +\\)\\s *" dl i) ; pick off next dir
-	  (setq ds (cons (concat comint-file-name-prefix
-				 (substring dl (match-beginning 1)
-					    (match-end 1)))
-			 ds))
-	  (setq i (match-end 0)))
-	(let ((ds (nreverse ds)))
-	  (with-demoted-errors "Couldn't cd: %s"
-	    (shell-cd (car ds))
-	    (setq shell-dirstack (cdr ds)
-		  shell-last-dir (car shell-dirstack))
-	    (shell-dirstack-message)))))
+      (let* ((dls (buffer-substring-no-properties
+                   (match-beginning 0) (1- (match-end 0))))
+             (dlsl '())
+             (pos 0)
+             (ds '()))
+        ;; Split the dirlist into whitespace and non-whitespace chunks.
+        ;; dlsl will be a reversed list of tokens.
+        (while (string-match "\\(\\S-+\\|\\s-+\\)" dls pos)
+          (push (match-string 1 dls) dlsl)
+          (setq pos (match-end 1)))
+
+        ;; prepend trailing entries until they form an existing directory,
+        ;; whitespace and all.  discard the next whitespace and repeat.
+        (while dlsl
+          (let ((newelt "")
+                tem1 tem2)
+            (while newelt
+              ;; We need tem1 because we don't want to prepend
+              ;; comint-file-name-prefix repeatedly into newelt via tem2.
+              (setq tem1 (pop dlsl)
+                    tem2 (concat comint-file-name-prefix tem1 newelt))
+              (cond ((file-directory-p tem2)
+                     (push tem2 ds)
+                     (when (string= " " (car dlsl))
+                       (pop dlsl))
+                     (setq newelt nil))
+                    (t
+                     (setq newelt (concat tem1 newelt)))))))
+
+        (with-demoted-errors "Couldn't cd: %s"
+          (shell-cd (car ds))
+          (setq shell-dirstack (cdr ds)
+                shell-last-dir (car shell-dirstack))
+          (shell-dirstack-message))))
     (if started-at-pmark (goto-char (marker-position pmark)))))
 
 ;; For your typing convenience:

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #27 received at 23324 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Noam Postavsky <npostavs <at> gmail.com>
Cc: 23324 <at> debbugs.gnu.org, Noah Friedman <friedman <at> splode.com>,
 11608 <at> debbugs.gnu.org
Subject: Re: bug#23324: shell-resync-dirs does not handle dirs with whitespace
Date: Wed, 19 Aug 2020 16:00:54 +0200

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> I've respun the patch so that it actually works (tem1 was mis-spelled in
> one place, and there was a bunch of unrelated white space fix-ups that
> I've remove), but haven't done anything else to it.
>
> Noam, I tried your test case, but I couldn't get it to infloop.  Is
> there some additional steps needed to make it infloop?

There wasn't any response in a week, so I went ahead and applied the
patch to Emacs 28.  If this leads to problems, please revert (or fix :-/)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.