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

Previous Next

Package: emacs;

Reported by: Noah Friedman <friedman <at> splode.com>

Date: Thu, 21 Apr 2016 03:38:02 UTC

Severity: minor

Tags: confirmed, fixed, patch

Merged with 9379, 11608, 23326

Found in versions 24.0.50, 24.0.94

Fixed in version 28.1

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

Bug is archived. No further changes may be made.

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.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to bug-gnu-emacs <at> gnu.org:
bug#23324; Package emacs. (Thu, 21 Apr 2016 03:38:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Noah Friedman <friedman <at> splode.com>:
New bug report received and forwarded. Copy sent to bug-gnu-emacs <at> gnu.org. (Thu, 21 Apr 2016 03:38:02 GMT) Full text and rfc822 format available.

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

Merged 23324 23326. Request was from Glenn Morris <rgm <at> gnu.org> to control <at> debbugs.gnu.org. (Thu, 21 Apr 2016 15:35:01 GMT) Full text and rfc822 format available.

Severity set to 'minor' from 'normal' Request was from Glenn Morris <rgm <at> gnu.org> to control <at> debbugs.gnu.org. (Thu, 21 Apr 2016 15:35:01 GMT) Full text and rfc822 format available.

Added tag(s) patch. Request was from Lars Magne Ingebrigtsen <larsi <at> gnus.org> to control <at> debbugs.gnu.org. (Mon, 25 Apr 2016 23:20:02 GMT) Full text and rfc822 format available.

Forcibly Merged 9379 11608 23324 23326. Request was from Noam Postavsky <npostavs <at> gmail.com> to control <at> debbugs.gnu.org. (Mon, 03 Sep 2018 17:09:02 GMT) Full text and rfc822 format available.

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#23324; Package emacs. (Mon, 03 Sep 2018 17:09:02 GMT) Full text and rfc822 format available.

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.





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#23324; Package emacs. (Thu, 27 Jun 2019 16:26:01 GMT) Full text and rfc822 format available.

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




Added tag(s) confirmed. Request was from Lars Ingebrigtsen <larsi <at> gnus.org> to control <at> debbugs.gnu.org. (Thu, 27 Jun 2019 16:26:02 GMT) Full text and rfc822 format available.

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#23324; Package emacs. (Wed, 12 Aug 2020 11:46:01 GMT) Full text and rfc822 format available.

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




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#23324; Package emacs. (Wed, 19 Aug 2020 14:02:02 GMT) Full text and rfc822 format available.

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




Added tag(s) fixed. Request was from Lars Ingebrigtsen <larsi <at> gnus.org> to control <at> debbugs.gnu.org. (Wed, 19 Aug 2020 14:02:03 GMT) Full text and rfc822 format available.

bug marked as fixed in version 28.1, send any further explanations to 11608 <at> debbugs.gnu.org and Oleksandr Manzyuk <manzyuk <at> gmail.com> Request was from Lars Ingebrigtsen <larsi <at> gnus.org> to control <at> debbugs.gnu.org. (Wed, 19 Aug 2020 14:02:03 GMT) Full text and rfc822 format available.

bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Thu, 17 Sep 2020 11:24:04 GMT) Full text and rfc822 format available.

This bug report was last modified 3 years and 193 days ago.

Previous Next


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