bug#59804: shell-resync-dirs hangs in (t)csh

[Nicolas Graner]

Eli Zaretskii <eliz@gnu.org> wrote on 2022-12-04 10:17:
>> From: Nicolas Graner <nicolas@graner.name>
>> Date: Sat, 03 Dec 2022 12:37:11 +0100
>> 
>> In a shell buffer where the running shell is csh or tcsh, the command
>> shell-resync-dirs never returns.
>> 
>> You can test this even without changing your normal shell by typing
>> `csh' in any shell buffer, then M-<RET>
>> Emacs hangs until you quit with C-g.
>> 
>> The reason is that the `dirs' command in (t)csh (unlike its equivalent
>> in bash) adds a trailing space to its output. This triggers an infinite
>> loop.
>> 
>> As evidence that the trailing space is the culprit, note that this
>> kludge, whiche removes it, fixes the problem:
>> 
>> (setq shell-dirstack-query "dirs | sed 's/ $//'")
>
> Thanks for the analysis.  Does the patch below fix this problem?  If not,
> could you point out more precisely where the command infloops and why?
>
> diff --git a/lisp/shell.el b/lisp/shell.el
> index b396bc2..dadbdcb 100644
> --- a/lisp/shell.el
> +++ b/lisp/shell.el
> @@ -1162,6 +1162,7 @@ shell-resync-dirs
>           (dlsl nil)
>           (pos 0)
>           (ds nil))
> +    (setq dls (string-trim-right dls "[ ]+"))
>      ;; Split the dirlist into whitespace and non-whitespace chunks.
>      ;; dlsl will be a reversed list of tokens.
>      (while (string-match "\\(\\S-+\\|\\s-+\\)" dls pos)

It works in most situations, but not if a directory name actually ends
with a space.
Upon further investigation, I found there are two distinct issues here.

1) When the dirs command prints "foo ", there is no way to tell if the
directory is named "foo" and the shell (e.g. csh) added a trailing
space, or the directory is named "foo " and the shell (e.g. bash)
printed it unchanged. There may even be more than one trailing space.

The only clean solution I can think of is to introduce a new
shell-dependent variable, similar to shell-dirstack-query, say
shell-dirstack-query-suffix, which would be " " for csh and tcsh, and ""
for most other shells. Your patch above would become:

  (setq dls (string-trim-right dls shell-dirstack-query-suffix))

2) the loop that goes wild, later in the same function, starts with:
   (while newelt

It tries to construct an existing directory name by concatenating tokens
from the output of dirs, starting from the last. It runs indefinitely if
this doesn't yield a valid directory name. This can happen for several
reasons: an extra space at the end as discussed above, a non-printable
character in a directory name, a directory that was deleted after being
pushed onto the dirstack, and maybe more. This can happen in any shell,
not just csh. This loop should therefore be fixed, regardless of the
trailing space issue.

The following patch fixes the bug by ensuring the loop terminates in all
cases, but I am not sure it always does the right thing when no
directory name is found. It should be reviewed by someone who
understands the code better than me.

diff --git a/lisp/shell.el b/lisp/shell.el
index b396bc2b180..abeaba04ab4 100644
--- a/lisp/shell.el
+++ b/lisp/shell.el
@@ -1173,7 +1173,7 @@ shell-resync-dirs
     (while dlsl
       (let ((newelt "")
             tem1 tem2)
-        (while newelt
+        (while (and newelt dlsl)
           ;; We need tem1 because we don't want to prepend
           ;; `comint-file-name-prefix' repeatedly into newelt via tem2.
           (setq tem1 (pop dlsl)