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

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

[Nicolas Graner]

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/ $//'")

-- Nicolas

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

[Eli Zaretskii]

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

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)

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

[Eli Zaretskii]

> From: Nicolas Graner <nicolas@graner.name>
> Cc: 59804@debbugs.gnu.org
> Date: Sun, 04 Dec 2022 19:41:24 +0100
> 
> > --- 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))

Is it certain that csh/tcsh always add just one space?  If so, we could
simply remove that one space in the case of these two shells.

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

OK, thanks.  I think I'd prefer for now to fix just the trailing space part,
which AFAIU happens always, and leave the more exotic situations as a FIXME
comment, or maybe I will make the change you propose on the master branch.

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

[Eli Zaretskii]

> Cc: 59804@debbugs.gnu.org
> Date: Sun, 04 Dec 2022 21:15:27 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > 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.
> 
> OK, thanks.  I think I'd prefer for now to fix just the trailing space part,
> which AFAIU happens always, and leave the more exotic situations as a FIXME
> comment, or maybe I will make the change you propose on the master branch.

I've now installed the simple patch I suggested.  I'm leaving the bug
open because the rest of the issue you describe has yet to be solved,
probably by someone who knows more about "dirs" in various shells than
I do.

Thanks.