* bug#49229: 27.2; `M-x shell' fails over TRAMP from local MS Windows
@ 2021-06-25 23:05 Jim Porter
2021-06-26 14:33 ` Michael Albinus
0 siblings, 1 reply; 10+ messages in thread
From: Jim Porter @ 2021-06-25 23:05 UTC (permalink / raw)
To: 49229
[-- Attachment #1: Type: text/plain, Size: 2059 bytes --]
(Note: I primarily tested on 27.2, but this doesn't look to be any
different on 28.0.50.) When invoking `M-x shell' over TRAMP from a
local MS Windows system, the default remote shell path is corrupted:
emacs -Q
C-x C-f /sshx:server:~/some/file.txt
M-x shell
;; See the default prompt value:
;; /sshx:server:/path/to/some//bin/sh
;; ("/path/to/some/" is greyed out)
RET
The result is: "env: ‘c:/bin/sh’: No such file or directory". You can
also see this with the code used in `M-x shell' to get the remote
shell path:
(read-file-name
"Remote shell path: " default-directory shell-file-name
t shell-file-name)
Eval'ing that from a TRAMP buffer and hitting RET returns "/bin/sh"
(i.e. `shell-file-name'); that is, we lost the TRAMP prefix, even
though the prompt made it look like we'd keep it. If you edit the path
to, say, "/sshx:server:/path/to/some//usr/bin/zsh" and hit RET, the
result is "/sshx:server:/usr/bin/zsh", which is good. The result of
this call is then passed to `expand-file-name', which on MS Windows,
turns "/bin/sh" into "c:/bin/sh". Finally, that gets called on the
remote (running GNU/Linux), and things break.
I've attached a WIP patch that resolves this, but I don't think it's
quite right (hence, I didn't use `git format-patch'). This seems to be
more of an issue with `read-file-name' not being smart enough; even if
we set the `default-filename' argument to nil, the default return
value is still a local (non-TRAMP) path, which isn't right. Since
`read-file-name' is better able to tell whether the user wanted the
default value or they specifically wanted a local shell, it might be
better to fix the issue there. However, that's a pretty widely-used
function, and I'm hesitant to change the behavior in
potentially-breaking ways.
If the current WIP patch does look good though, I can clean it up (add
a comment and a commit message) for it to be merged. Or I can try to
fix `read-file-name' if there's agreement about how it should work in
this case.
[-- Attachment #2: shell-tramp.patch --]
[-- Type: application/octet-stream, Size: 1131 bytes --]
diff --git a/lisp/shell.el b/lisp/shell.el
index 62de5be817..e7369464fd 100644
--- a/lisp/shell.el
+++ b/lisp/shell.el
@@ -751,15 +751,16 @@ shell
(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")))
+ (when-let ((remote-host (file-remote-p default-directory))
+ ((called-interactively-p 'any))
+ ((null explicit-shell-file-name))
+ ((null (getenv "ESHELL"))))
(setq-local explicit-shell-file-name
(file-local-name
(expand-file-name
(read-file-name "Remote shell path: " default-directory
- shell-file-name t shell-file-name)))))
+ (concat remote-host shell-file-name) t
+ shell-file-name)))))
;; Rain or shine, BUFFER must be current by now.
(unless (comint-check-proc buffer)
^ permalink raw reply related [flat|nested] 10+ messages in thread
* bug#49229: 27.2; `M-x shell' fails over TRAMP from local MS Windows
2021-06-25 23:05 bug#49229: 27.2; `M-x shell' fails over TRAMP from local MS Windows Jim Porter
@ 2021-06-26 14:33 ` Michael Albinus
2021-06-26 18:01 ` Jim Porter
0 siblings, 1 reply; 10+ messages in thread
From: Michael Albinus @ 2021-06-26 14:33 UTC (permalink / raw)
To: Jim Porter; +Cc: 49229
Jim Porter <jporterbugs@gmail.com> writes:
Hi Jim,
> Eval'ing that from a TRAMP buffer and hitting RET returns "/bin/sh"
> (i.e. `shell-file-name'); that is, we lost the TRAMP prefix, even
> though the prompt made it look like we'd keep it. If you edit the path
> to, say, "/sshx:server:/path/to/some//usr/bin/zsh" and hit RET, the
> result is "/sshx:server:/usr/bin/zsh", which is good. The result of
> this call is then passed to `expand-file-name', which on MS Windows,
> turns "/bin/sh" into "c:/bin/sh". Finally, that gets called on the
> remote (running GNU/Linux), and things break.
Thanks for this report.
Occasionally, I've seen this problem on MS Windows already. Since I
don't run anything on MS Windows unless for bug hunting, I couldn't
locate it yet. With your recipe, it's reproducible now. It's not related
to "M-x shell" only, but more general.
> I've attached a WIP patch that resolves this, but I don't think it's
> quite right (hence, I didn't use `git format-patch'). This seems to be
> more of an issue with `read-file-name' not being smart enough; even if
> we set the `default-filename' argument to nil, the default return
> value is still a local (non-TRAMP) path, which isn't right. Since
> `read-file-name' is better able to tell whether the user wanted the
> default value or they specifically wanted a local shell, it might be
> better to fix the issue there. However, that's a pretty widely-used
> function, and I'm hesitant to change the behavior in
> potentially-breaking ways.
I've pushed a fix to the master branches of Emacs and Tramp. Could you,
pls, check?
Best regards, Michael.
^ permalink raw reply [flat|nested] 10+ messages in thread
* bug#49229: 27.2; `M-x shell' fails over TRAMP from local MS Windows
2021-06-26 14:33 ` Michael Albinus
@ 2021-06-26 18:01 ` Jim Porter
2021-06-27 8:41 ` Michael Albinus
0 siblings, 1 reply; 10+ messages in thread
From: Jim Porter @ 2021-06-26 18:01 UTC (permalink / raw)
To: Michael Albinus; +Cc: 49229
On Sat, Jun 26, 2021 at 7:33 AM Michael Albinus <michael.albinus@gmx.de> wrote:
>
> Occasionally, I've seen this problem on MS Windows already. Since I
> don't run anything on MS Windows unless for bug hunting, I couldn't
> locate it yet. With your recipe, it's reproducible now. It's not related
> to "M-x shell" only, but more general.
Looking at your patch, it makes sense to me. It's not where I
originally expected the fix to go, but I remember some previous corner
cases that were fixed with `tramp-drop-volume-letter', so this should
be good.
> I've pushed a fix to the master branches of Emacs and Tramp. Could you,
> pls, check?
I tested on MS Windows and it works correctly for me. (Note that I
just copied and eval'ed the new version of `tramp-file-name-handler',
since I don't have a proper build environment on MS Windows.)
While testing it, I discovered one other oddity though. This doesn't
necessarily need a fix, but it's a bit surprising, and I'll mention it
here in case someone thinks it's a problem worth fixing.
If I erase the default text and instead enter
"C:/Windows/System32/cmd.exe" into the `M-x shell' prompt, it treats
*that* as a remote path too. Now, the prompt does say to enter a
*remote* shell path, so if I enter a local path, I made a mistake.
However, the default shell path for `M-x shell' from a remote
directory is a TRAMP path ("/sshx:server:/path/to/some//bin/sh"), so
it's surprising that when I delete the TRAMP host prefix, I still end
up running a shell on the remote server.
Perhaps it would be nicer if, when `M-x shell' prompted for the remote
shell path, it didn't include the TRAMP prefix by default (e.g. the
default value would just be "/bin/sh"). That might not interact well
with `read-file-name' completion though; is it possible to use
file-name completion on a remote path without the TRAMP prefix?
^ permalink raw reply [flat|nested] 10+ messages in thread
* bug#49229: 27.2; `M-x shell' fails over TRAMP from local MS Windows
2021-06-26 18:01 ` Jim Porter
@ 2021-06-27 8:41 ` Michael Albinus
2021-06-28 0:47 ` Jim Porter
0 siblings, 1 reply; 10+ messages in thread
From: Michael Albinus @ 2021-06-27 8:41 UTC (permalink / raw)
To: Jim Porter; +Cc: 49229
[-- Attachment #1: Type: text/plain, Size: 1504 bytes --]
Jim Porter <jporterbugs@gmail.com> writes:
Hi Jim,
>> I've pushed a fix to the master branches of Emacs and Tramp. Could you,
>> pls, check?
>
> I tested on MS Windows and it works correctly for me. (Note that I
> just copied and eval'ed the new version of `tramp-file-name-handler',
> since I don't have a proper build environment on MS Windows.)
Thanks for confirmation.
> If I erase the default text and instead enter
> "C:/Windows/System32/cmd.exe" into the `M-x shell' prompt, it treats
> *that* as a remote path too. Now, the prompt does say to enter a
> *remote* shell path, so if I enter a local path, I made a mistake.
> However, the default shell path for `M-x shell' from a remote
> directory is a TRAMP path ("/sshx:server:/path/to/some//bin/sh"), so
> it's surprising that when I delete the TRAMP host prefix, I still end
> up running a shell on the remote server.
`read-file-name' as used in `shell' just reads a file name, no matter
whether a local or remote one.
> Perhaps it would be nicer if, when `M-x shell' prompted for the remote
> shell path, it didn't include the TRAMP prefix by default (e.g. the
> default value would just be "/bin/sh"). That might not interact well
> with `read-file-name' completion though; is it possible to use
> file-name completion on a remote path without the TRAMP prefix?
No, that doesn't work. File name completion and alike wouldn't work any
more.
But we could teach `read-file-name' to accept only remote file
names. What about this patch:
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 602 bytes --]
diff --git a/lisp/shell.el b/lisp/shell.el
index 62de5be817..4339e8c0a3 100644
--- a/lisp/shell.el
+++ b/lisp/shell.el
@@ -759,7 +759,8 @@ shell
(file-local-name
(expand-file-name
(read-file-name "Remote shell path: " default-directory
- shell-file-name t shell-file-name)))))
+ shell-file-name t shell-file-name
+ #'file-remote-p)))))
;; Rain or shine, BUFFER must be current by now.
(unless (comint-check-proc buffer)
[-- Attachment #3: Type: text/plain, Size: 208 bytes --]
It is not perfect, one can still enter "/sudo::/bin/sh" when
`default-directory' is "/ssh::". But I wouldn't count this as mistake,
it would be rather a "user error by intention" :-)
Best regards, Michael.
^ permalink raw reply related [flat|nested] 10+ messages in thread
* bug#49229: 27.2; `M-x shell' fails over TRAMP from local MS Windows
2021-06-27 8:41 ` Michael Albinus
@ 2021-06-28 0:47 ` Jim Porter
2021-06-28 6:18 ` Michael Albinus
0 siblings, 1 reply; 10+ messages in thread
From: Jim Porter @ 2021-06-28 0:47 UTC (permalink / raw)
To: Michael Albinus; +Cc: 49229
On Sun, Jun 27, 2021 at 1:41 AM Michael Albinus <michael.albinus@gmx.de> wrote:
>
> But we could teach `read-file-name' to accept only remote file
> names. What about this patch:
>
>
> It is not perfect, one can still enter "/sudo::/bin/sh" when
> `default-directory' is "/ssh::". But I wouldn't count this as mistake,
> it would be rather a "user error by intention" :-)
This seems like a reasonable compromise to me. Like you say, it's not
perfect, but it should at least prevent users from mistakenly entering
local paths here.
^ permalink raw reply [flat|nested] 10+ messages in thread
* bug#49229: 27.2; `M-x shell' fails over TRAMP from local MS Windows
2021-06-28 0:47 ` Jim Porter
@ 2021-06-28 6:18 ` Michael Albinus
2021-07-01 17:07 ` Jim Porter
0 siblings, 1 reply; 10+ messages in thread
From: Michael Albinus @ 2021-06-28 6:18 UTC (permalink / raw)
To: Jim Porter; +Cc: 49229-done
Version: 28.1
Jim Porter <jporterbugs@gmail.com> writes:
Hi Jim,
> This seems like a reasonable compromise to me. Like you say, it's not
> perfect, but it should at least prevent users from mistakenly entering
> local paths here.
I've pushed the patch to master, closing the bug. The Tramp part of the
patch will appear with Tramp 2.5.1 on GNU ELPA.
Best regards, Michael.
^ permalink raw reply [flat|nested] 10+ messages in thread
* bug#49229: 27.2; `M-x shell' fails over TRAMP from local MS Windows
2021-06-28 6:18 ` Michael Albinus
@ 2021-07-01 17:07 ` Jim Porter
2021-07-02 12:58 ` Michael Albinus
0 siblings, 1 reply; 10+ messages in thread
From: Jim Porter @ 2021-07-01 17:07 UTC (permalink / raw)
To: Michael Albinus; +Cc: 49229
(Hopefully I don't need to do anything special to comment on a closed bug...)
On Sun, Jun 27, 2021 at 11:18 PM Michael Albinus <michael.albinus@gmx.de> wrote:
>
> I've pushed the patch to master, closing the bug. The Tramp part of the
> patch will appear with Tramp 2.5.1 on GNU ELPA.
I discovered a minor issue with the Tramp part of the patch. On MS
Windows, if I'm editing a remote file and then open a local file, the
newly-opened local file's default directory doesn't have a volume
letter. This still works overall, but it does break the "~" file
alias.
Here are steps to reproduce:
C-x C-f /sshx:server:/path/to/file.txt
;; "/sshx:server:/path/to/" is pre-filled here:
C-x C-f /sshx:server:/path/to//~/local.txt
;; local.txt opens/saves correctly, but...
C-x C-f
;; Initial value in minibuffer is "/Users/Jim/Documents/".
;; Before Tramp 2.5.1, it was "~/".
This might cause some problems in other places, but so far the only
issue I've found is the file alias for "~" no longer working. However,
the fact that `default-directory' for local.txt isn't an absolute path
could cause issues if we use that `default-directory' elsewhere. Since
"/Users/Jim/Documents/" is relative to the current drive, it can mean
different things depending on what Emacs thinks the current drive is
at the time.
Maybe it would be best to revert the Tramp part of this patch and do
something similar to the patch in my original message. That would
ensure that nothing unexpected happens as a result of
`default-directory' being relative to the current drive. If that makes
sense, I can put together a proper patch for the `M-x shell' part.
^ permalink raw reply [flat|nested] 10+ messages in thread
* bug#49229: 27.2; `M-x shell' fails over TRAMP from local MS Windows
2021-07-01 17:07 ` Jim Porter
@ 2021-07-02 12:58 ` Michael Albinus
2021-07-02 21:30 ` Jim Porter
0 siblings, 1 reply; 10+ messages in thread
From: Michael Albinus @ 2021-07-02 12:58 UTC (permalink / raw)
To: Jim Porter; +Cc: 49229
[-- Attachment #1: Type: text/plain, Size: 750 bytes --]
Jim Porter <jporterbugs@gmail.com> writes:
Hi Jim,
> (Hopefully I don't need to do anything special to comment on a closed bug...)
No problem,
> Maybe it would be best to revert the Tramp part of this patch and do
> something similar to the patch in my original message. That would
> ensure that nothing unexpected happens as a result of
> `default-directory' being relative to the current drive. If that makes
> sense, I can put together a proper patch for the `M-x shell' part.
As I've said in my reply, I believe the problem is more general, and not
restricted to just the `shell' function.
So I have pushed a patch to master, which reverts my Tramp change, and
which adds the following change to `read-file-name-default' of minibuffer.el:
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 503 bytes --]
*** /tmp/ediffZeoKgh 2021-07-02 14:52:22.462303385 +0200
--- /home/albinus/src/emacs/lisp/minibuffer.el 2021-07-02 14:50:37.716338781 +0200
***************
*** 3161,3166 ****
--- 3161,3167 ----
(unless val (error "No file name specified"))
(if (and default-filename
+ (not (file-remote-p dir))
(string-equal val (if (consp insdef) (car insdef) insdef)))
(setq val default-filename))
(setq val (substitute-in-file-name val))
[-- Attachment #3: Type: text/plain, Size: 47 bytes --]
Could you, pls, test?
Best regards, Michael.
^ permalink raw reply [flat|nested] 10+ messages in thread
* bug#49229: 27.2; `M-x shell' fails over TRAMP from local MS Windows
2021-07-02 12:58 ` Michael Albinus
@ 2021-07-02 21:30 ` Jim Porter
2021-07-03 6:42 ` Michael Albinus
0 siblings, 1 reply; 10+ messages in thread
From: Jim Porter @ 2021-07-02 21:30 UTC (permalink / raw)
To: Michael Albinus; +Cc: 49229
On Fri, Jul 2, 2021 at 5:58 AM Michael Albinus <michael.albinus@gmx.de> wrote:
>
> As I've said in my reply, I believe the problem is more general, and not
> restricted to just the `shell' function.
>
> So I have pushed a patch to master, which reverts my Tramp change, and
> which adds the following change to `read-file-name-default' of minibuffer.el:
>
>
> Could you, pls, test?
Thanks for the fix. I tested this both by running `M-x shell' from a
remote buffer and by opening a local file while in a remote buffer and
both of these work as expected. Note that I just eval'ed the updated
functions in my Emacs 27.2 instance, so it's not a *perfect* test;
still, hopefully it's enough to verify that this works correctly.
^ permalink raw reply [flat|nested] 10+ messages in thread
* bug#49229: 27.2; `M-x shell' fails over TRAMP from local MS Windows
2021-07-02 21:30 ` Jim Porter
@ 2021-07-03 6:42 ` Michael Albinus
0 siblings, 0 replies; 10+ messages in thread
From: Michael Albinus @ 2021-07-03 6:42 UTC (permalink / raw)
To: Jim Porter; +Cc: 49229
Jim Porter <jporterbugs@gmail.com> writes:
Hi Jim,
> Thanks for the fix. I tested this both by running `M-x shell' from a
> remote buffer and by opening a local file while in a remote buffer and
> both of these work as expected. Note that I just eval'ed the updated
> functions in my Emacs 27.2 instance, so it's not a *perfect* test;
> still, hopefully it's enough to verify that this works correctly.
Thanks for confirmation. The Tramp part of the patch will appear with
the next Tramp ELPA release, likely 2.5.1.1.
Best regards, Michael.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-07-03 6:42 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-25 23:05 bug#49229: 27.2; `M-x shell' fails over TRAMP from local MS Windows Jim Porter
2021-06-26 14:33 ` Michael Albinus
2021-06-26 18:01 ` Jim Porter
2021-06-27 8:41 ` Michael Albinus
2021-06-28 0:47 ` Jim Porter
2021-06-28 6:18 ` Michael Albinus
2021-07-01 17:07 ` Jim Porter
2021-07-02 12:58 ` Michael Albinus
2021-07-02 21:30 ` Jim Porter
2021-07-03 6:42 ` Michael Albinus
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).