Re: master f8b2a01a9e: * lisp/shell.el (shell): Query shell file name from `interactive`

Re: master f8b2a01a9e: * lisp/shell.el (shell): Query shell file name from `interactive`

[Michael Albinus]

Stefan Monnier via Mailing list for Emacs changes <emacs-diffs@gnu.org>
writes:

Hi Stefan,

> @@ -771,7 +773,20 @@ Make the shell buffer the current buffer, and return it.
>  		     (expand-file-name
>  		      (read-directory-name
>  		       "Default directory: " default-directory default-directory
> -		       t nil))))))))
> +		       t nil))))))
> +    ;; On remote hosts, the local `shell-file-name' might be useless.
> +    (when (and (file-remote-p default-directory)
> +               (null explicit-shell-file-name)
> +               (null (getenv "ESHELL")))
> +      ;; `expand-file-name' shall not add the MS Windows volume letter
> +      ;; (Bug#49229).
> +      (replace-regexp-in-string
> +       "^[[:alpha:]]:" ""
> +       (file-local-name
> +        (expand-file-name
> +         (read-file-name "Remote shell path: " default-directory
> +                         shell-file-name t shell-file-name
> +                         #'file-remote-p)))))))
>    (setq buffer (if (or buffer (not (derived-mode-p 'shell-mode))
>                         (comint-check-proc (current-buffer)))
>                     (get-buffer-create (or buffer "*shell*"))
> @@ -782,21 +797,8 @@ Make the shell buffer the current buffer, and return it.
>    (pop-to-buffer buffer display-comint-buffer-action)
>
>    (with-connection-local-variables
> -   ;; On remote hosts, the local `shell-file-name' might be useless.
> -   (when (and (file-remote-p default-directory)
> -              (called-interactively-p 'any)
> -              (null explicit-shell-file-name)
> -              (null (getenv "ESHELL")))
> -     ;; `expand-file-name' shall not add the MS Windows volume letter
> -     ;; (Bug#49229).
> -     (setq-local explicit-shell-file-name
> -                 (replace-regexp-in-string
> -                  "^[[:alpha:]]:" ""
> -                  (file-local-name
> -                   (expand-file-name
> -                    (read-file-name "Remote shell path: " default-directory
> -                                    shell-file-name t shell-file-name
> -                                    #'file-remote-p))))))

This misses the `with-connection-local-variables' wrapper in the
`interactive' form. Is this intended? It would be a serious regression.

Best regards, Michael.

Re: master f8b2a01a9e: * lisp/shell.el (shell): Query shell file name from `interactive`

[Lars Ingebrigtsen]

If I'm interpreting Hydra correctly, it's saying that this change
somehow broke a test?

https://hydra.nixos.org/build/178360594

I may well be wrong.

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

Re: master f8b2a01a9e: * lisp/shell.el (shell): Query shell file name from `interactive`

[Stefan Monnier]

> This misses the `with-connection-local-variables' wrapper in the
> `interactive' form. Is this intended? It would be a serious regression.

Oh, I missed that, sorry,


        Stefan

Re: master f8b2a01a9e: * lisp/shell.el (shell): Query shell file name from `interactive`

[Stefan Monnier]

Stefan Monnier [2022-05-28 10:12:45] wrote:
>> This misses the `with-connection-local-variables' wrapper in the
>> `interactive' form. Is this intended? It would be a serious regression.
> Oh, I missed that, sorry,

Does the patch below look correct to you?
It also tries to address the FIXME while we're at it.


        Stefan


2022-05-28  Stefan Monnier  <monnier@iro.umontreal.ca>

    * lisp/shell.el (shell): Make sure we set the `default-directory` of
    the actual shell buffer after querying the user.
    Apply connection-local variables when prompting the user for a remote
    shell file-name.


diff --git a/lisp/shell.el b/lisp/shell.el
index 1fcd1a1d1cc..8bcc578406a 100644
--- a/lisp/shell.el
+++ b/lisp/shell.el
@@ -755,38 +755,47 @@ shell
 
 \(Type \\[describe-mode] in the shell buffer for a list of commands.)"
   (interactive
-   (list
-    (and current-prefix-arg
-	 (prog1
-	     (read-buffer "Shell buffer: "
-			  ;; If the current buffer is an inactive
-			  ;; shell buffer, use it as the default.
-			  (if (and (eq major-mode 'shell-mode)
-				   (null (get-buffer-process (current-buffer))))
-			      (buffer-name)
-			    (generate-new-buffer-name "*shell*")))
-	   (if (file-remote-p default-directory)
-	       ;; It must be possible to declare a local default-directory.
-               ;; FIXME: This can't be right: it changes the default-directory
-               ;; of the current-buffer rather than of the *shell* buffer.
-	       (setq default-directory
-		     (expand-file-name
-		      (read-directory-name
-		       "Default directory: " default-directory default-directory
-		       t nil))))))
-    ;; On remote hosts, the local `shell-file-name' might be useless.
-    (when (and (file-remote-p default-directory)
-               (null explicit-shell-file-name)
-               (null (getenv "ESHELL")))
-      ;; `expand-file-name' shall not add the MS Windows volume letter
-      ;; (Bug#49229).
-      (replace-regexp-in-string
-       "^[[:alpha:]]:" ""
-       (file-local-name
-        (expand-file-name
-         (read-file-name "Remote shell path: " default-directory
-                         shell-file-name t shell-file-name
-                         #'file-remote-p)))))))
+   (let* ((buffer
+           (and current-prefix-arg
+		(read-buffer "Shell buffer: "
+			     ;; If the current buffer is an inactive
+			     ;; shell buffer, use it as the default.
+			     (if (and (eq major-mode 'shell-mode)
+				      (null (get-buffer-process
+				             (current-buffer))))
+				 (buffer-name)
+			       (generate-new-buffer-name "*shell*")))))
+	  (buf (if (or buffer (not (derived-mode-p 'shell-mode))
+                       (comint-check-proc (current-buffer)))
+                   (get-buffer-create (or buffer "*shell*"))
+                 ;; If the current buffer is a dead shell buffer, use it.
+                 (current-buffer))))
+
+     (with-current-buffer buf
+       (when (and buffer (file-remote-p default-directory))
+	 ;; It must be possible to declare a local default-directory.
+	 (setq default-directory
+	       (expand-file-name
+		(read-directory-name
+		 "Default directory: " default-directory default-directory
+		 t nil))))
+       (list
+        buffer
+        ;; On remote hosts, the local `shell-file-name' might be useless.
+        (with-connection-local-variables
+         (when (and (file-remote-p default-directory)
+                    (null explicit-shell-file-name)
+                    (null (getenv "ESHELL")))
+           ;; `expand-file-name' shall not add the MS Windows volume letter
+           ;; (Bug#49229).
+           (replace-regexp-in-string
+            "^[[:alpha:]]:" ""
+            (file-local-name
+             (expand-file-name
+              (read-file-name "Remote shell path: " default-directory
+                              shell-file-name t shell-file-name
+                              #'file-remote-p))))))))))
+
   (setq buffer (if (or buffer (not (derived-mode-p 'shell-mode))
                        (comint-check-proc (current-buffer)))
                    (get-buffer-create (or buffer "*shell*"))

Re: master f8b2a01a9e: * lisp/shell.el (shell): Query shell file name from `interactive`

[Michael Albinus]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

Hi Stefan,

> Does the patch below look correct to you?
> It also tries to address the FIXME while we're at it.

I'm not sure. You use `buffer', and not `buf' for the interactive
spec. OTOH, you set `default-directory' in `buf', which might cure it.

Anyway, it would be an incompatible (but appreciated) change. If you
have an existing shell buffer, it would be reused as-it-is, and you
cannot change its remoteness. Something, which has been plagued me since
ever.

Hmm, do we have test cases for this, for both the local and the remote
case, for both an existing and a new shell buffer? (If not, I could
volunteer to write them, but it might take some days then.)

>         Stefan

Best regards, Michael.

Re: master f8b2a01a9e: * lisp/shell.el (shell): Query shell file name from `interactive`

[Stefan Monnier]

>> Does the patch below look correct to you?
>> It also tries to address the FIXME while we're at it.
> I'm not sure. You use `buffer', and not `buf' for the interactive spec.

Could you clarify what you mean by that?

I check nullness of `buffer` so as to preserve the existing behavior
where we prompt for the shell's directory only when current-prefix-arg
is specified.

And the interactive spec returns (list buffer ...) rather than (list
buf ...) again so as to preserve the existing behavior (tho I don't
think it would make any difference in practice other than the
information it leaves in `command-history`).

> OTOH, you set `default-directory' in `buf', which might cure it.

Cure what?

> Anyway, it would be an incompatible (but appreciated) change.

What change are you referring to?

> If you have an existing shell buffer, it would be reused as-it-is, and
> you cannot change its remoteness.  Something, which has been plagued me
> since ever.

Hmm... I don't understand.  AFAICT both with my new code and with the
existing code `C-u M-x shell` will prompt for the shell's directory if
it's done from within (and "to") a shell buffer with  remote
default-directory, no?

> Hmm, do we have test cases for this, for both the local and the remote
> case, for both an existing and a new shell buffer?

AFAICT we don't (we have rather few tests in `shell-tests.el`).

> (If not, I could volunteer to write them, but it might take some
> days then.)

That would be very appreciated.


        Stefan

Re: master f8b2a01a9e: * lisp/shell.el (shell): Query shell file name from `interactive`

[Michael Albinus]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

Hi Stefan,

>>> Does the patch below look correct to you?
>>> It also tries to address the FIXME while we're at it.
>> I'm not sure. You use `buffer', and not `buf' for the interactive spec.
>
> Could you clarify what you mean by that?
>
> I check nullness of `buffer` so as to preserve the existing behavior
> where we prompt for the shell's directory only when current-prefix-arg
> is specified.
>
> And the interactive spec returns (list buffer ...) rather than (list
> buf ...) again so as to preserve the existing behavior (tho I don't
> think it would make any difference in practice other than the
> information it leaves in `command-history`).
>
>> OTOH, you set `default-directory' in `buf', which might cure it.
>
> Cure what?

I've tested the patch now, it looks correct.

>> Anyway, it would be an incompatible (but appreciated) change.
>
> What change are you referring to?

Forget it. I hoped, that if you re-use the *shell* buffer, which has an
exited shell, that this buffer would change it's default-directory to
the default-directory of the current buffer when the shell is
re-invoked. Something like this:

--8<---------------cut here---------------start------------->8---
M-x cd RET /tmp
M-x shell
; You're in the *shell* buffer, running the shell on your local host.
exit
; The *shell* buffer has exited the shell.

C-x b <the previous buffer>
M-x cd RET /ssh:remotehost:/tmp
M-x shell
; The *shell* buffer shall be remote now.
--8<---------------cut here---------------end--------------->8---

But this didn't happen :-(

>> If you have an existing shell buffer, it would be reused as-it-is, and
>> you cannot change its remoteness.  Something, which has been plagued me
>> since ever.
>
> Hmm... I don't understand.  AFAICT both with my new code and with the
> existing code `C-u M-x shell` will prompt for the shell's directory if
> it's done from within (and "to") a shell buffer with  remote
> default-directory, no?

I'm speaking about reusing an existing *shell* buffer, and invoking M-x
shell w/o a prefix. It would be helpful, if there could be an
interaction when the remoteness of default-directory of the current and
the *shell* buffer don't match.

>> Hmm, do we have test cases for this, for both the local and the remote
>> case, for both an existing and a new shell buffer?
>
> AFAICT we don't (we have rather few tests in `shell-tests.el`).
>
>> (If not, I could volunteer to write them, but it might take some
>> days then.)
>
> That would be very appreciated.

I'll see what can be done.

>         Stefan

Best regards, Michael.

Re: master f8b2a01a9e: * lisp/shell.el (shell): Query shell file name from `interactive`

[Stefan Monnier]

>> Cure what?
> I've tested the patch now, it looks correct.

Great, thanks, pushed.

>>> Anyway, it would be an incompatible (but appreciated) change.
>> What change are you referring to?
> Forget it. I hoped, that if you re-use the *shell* buffer, which has an
> exited shell, that this buffer would change it's default-directory to
> the default-directory of the current buffer when the shell is
> re-invoked.

Ah... yes, I see what you mean, indeed, I've wished for that (and not
only for remote shells).


        Stefan

Re: master f8b2a01a9e: * lisp/shell.el (shell): Query shell file name from `interactive`

[Michael Albinus]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

Hi Stefan,

>> I hoped, that if you re-use the *shell* buffer, which has an
>> exited shell, that this buffer would change it's default-directory to
>> the default-directory of the current buffer when the shell is
>> re-invoked.
>
> Ah... yes, I see what you mean, indeed, I've wished for that (and not
> only for remote shells).

Shall I try to change this? Or would it be too much of an incompatible
change?

>         Stefan

Best regards, Michael.

Re: master f8b2a01a9e: * lisp/shell.el (shell): Query shell file name from `interactive`

[Stefan Monnier]

>> Ah... yes, I see what you mean, indeed, I've wished for that (and not
>> only for remote shells).
> Shall I try to change this? Or would it be too much of an incompatible
> change?

I rarely use `shell` and I don't have a good intuition for how other
people use it, so I don't know.


        Stefan