bug#71896: shell-resync-dirs hang

[Troy Hinckley <troyhinckley@dabrev.com>]

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

Thanks Eli,
The proposed patch throws an error when `last’ is “” because (substring “” -1) produces (args-out-of-range “” -1 nil).

-Troy Hinckley
On Jul 6, 2024 at 4:58 AM -0500, Eli Zaretskii <eliz@gnu.org>, wrote:
> > Date: Mon, 1 Jul 2024 21:22:35 -0500
> > From: Troy Hinckley <troyhinckley@dabrev.com>
> >
> > This is related to #59804 and #54384.
> >
> > I will occasionally (about 30% of the time) see hangs when running shell-resync-dirs in Emacs 29. I tracked this
> > down to two issues:
> >
> > First is in shell-eval-command. For Emacs 29 this function was added to fix #54384. It has this section in the
> > code:
> >
> > ```
> > ;; Wait until we get a prompt (which will be a line without
> > ;; a newline). This is far from fool-proof -- if something
> > ;; outputs incomplete data and then sleeps, we'll think
> > ;; we've received the prompt.
> > (while (not (let* ((lines (string-lines result))
> > (last (car (last lines))))
> > (and (length> lines 0)
> > (not (equal last ""))
> > (or (not prev)
> > (not (equal last prev)))
> > (setq prev last))))
> > (accept-process-output proc 0 100))
> > ```
> >
> > Note that is says that is looking for “a line without a newline” to determine if we have reached the prompt.
> > However this code does not actually do that. If the result ends in a newline it will still terminate the loop and not
> > wait for more input. We can see that by the fact that the following code evaluates to nil.
> >
> > ```
> > (let ((result "dirs\n") prev)
> > (not (let* ((lines (string-lines result))
> > (last (car (last lines))))
> > (and (length> lines 0)
> > (not (equal last ""))
> > (or (not prev)
> > (not (equal last prev)))
> > (setq prev last)))))
> > ```
> >
> > I am not sure what this code is supposed to do, but the issue arrises if the process output sends anything to
> > this function it will terminate and not wait for more input. In my case the issue is that the shell is echoing the
> > command followed by the result (comint-process-echoes). About 30% of the time these two lines get sent as
> > part of two different outputs. Meaning the second line (the directory for shell-resync-dirs) does not get captured
> > and instead gets printed to the terminal.
>
> Does the patch below solve the problem in shell-eval-command?
>
> > This leads us to the hang. The issue is this code in shell-resync-dirs:
> >
> > ```
> > (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)))))))
> > ```
> >
> > This loop can only terminate if tem2 is a valid directory. Otherwise it will take the default case in the cond and
> > loop forever. And since the bug in shell-eval-command does not provide the directory when the process output
> > is split, we get a hang.
> >
> > I believe both of these need to be fixed to properly fix the bug.
> >
> > For the shell-eval-command I don’t understand what that loop is trying to do now, so I am not sure how to fix it
> > without breaking its functionality. I would just use (string-suffix-p “\n” result) to check if the output ends in a
> > newline, but the code is obviously trying to do something more complex there.
> >
> > If we fix that issue then it will resolve the hang in shell-resync-dirs, but I think that is just glossing over the
> > problem. That functions should never hang, no matter what output it get’s from the shell. My recommendation
> > would be to add `(and dlsl newelt)` as the condition for the inner while loop with newelt. That way if dlsl is
> > empty, it will terminate the loop since there is nothing more to process. This fixed the issue for me.
>
> Thanks, I think I agree with your suggestion for shell-resync-dirs.
> But please undo the fix you evidently made there to avoid the infloop,
> and see if the patch below for shell-eval-command makes
> shell-resync-dirs do its job by correctly resynchronizing
> shell-dirtrack.
>
> diff --git a/lisp/shell.el b/lisp/shell.el
> index e1936ff..f86156e 100644
> --- a/lisp/shell.el
> +++ b/lisp/shell.el
> @@ -1629,10 +1629,12 @@ shell-eval-command
> ;; a newline). This is far from fool-proof -- if something
> ;; outputs incomplete data and then sleeps, we'll think
> ;; we've received the prompt.
> - (while (not (let* ((lines (string-lines result))
> - (last (car (last lines))))
> + (while (not (let* ((lines (string-lines result nil t))
> + (last (car (last lines)))
> + (last-end (substring last -1)))
> (and (length> lines 0)
> - (not (equal last ""))
> + (not (member last '("" "\n")))
> + (not (equal last-end "\n"))
> (or (not prev)
> (not (equal last prev)))
> (setq prev last))))

[-- Attachment #2: Type: text/html, Size: 5940 bytes --]