* bug#54487: 29.0.50; connection-local value for `shell-file-name' not set in Dired buffers over Tramp
@ 2022-03-21 4:58 Jim Porter
2022-03-21 10:25 ` Michael Albinus
0 siblings, 1 reply; 13+ messages in thread
From: Jim Porter @ 2022-03-21 4:58 UTC (permalink / raw)
To: 54487
Hopefully I've summarized the issue correctly in the bug title. To see
this in action, run the following from `emacs -Q' on an MS-Windows
system ("host" in this example is a remote GNU/Linux system):
C-x C-f /ssh:host:~
M-x rgrep RET
some text RET RET RET
The rgrep output will look something like:
find [...] --null -e "some text" "{}" +
find: paths must precede expression: `^^!^'
You can click the "[...]" to see the full invocation. However, even
without doing that, if you look carefully, you'll notice that the
shell-quoting uses the MS-Windows rules, not that of /bin/sh. For the
MS-Windows shell, spaces are quoted by wrapping the entire argument in
double-quotes ("like this"); for /bin/sh, spaces are escaped via a
backslash (like\ this).
Presumably, that's because if you eval `shell-file-name' in the Dired
buffer, it reports ".../path/to/cmdproxy.exe". When in a remote *file*,
`shell-file-name' is correctly set to "/bin/sh".
This also comes up in other (non-Dired) situations. For example:
C-x C-f /ssh:host:~/some-file.txt
M-x rgrep RET
some text RET RET RET
;; everything looks ok
;; now, from the rgrep buffer...
M-x rgrep
some text RET RET RET
;; same error as in the original case above
^ permalink raw reply [flat|nested] 13+ messages in thread* bug#54487: 29.0.50; connection-local value for `shell-file-name' not set in Dired buffers over Tramp 2022-03-21 4:58 bug#54487: 29.0.50; connection-local value for `shell-file-name' not set in Dired buffers over Tramp Jim Porter @ 2022-03-21 10:25 ` Michael Albinus 2022-03-21 12:40 ` Eli Zaretskii 0 siblings, 1 reply; 13+ messages in thread From: Michael Albinus @ 2022-03-21 10:25 UTC (permalink / raw) To: Jim Porter; +Cc: 54487 [-- Attachment #1: Type: text/plain, Size: 1528 bytes --] Jim Porter <jporterbugs@gmail.com> writes: Hi Jim, > Hopefully I've summarized the issue correctly in the bug title. To see > this in action, run the following from `emacs -Q' on an MS-Windows > system ("host" in this example is a remote GNU/Linux system): > > C-x C-f /ssh:host:~ > M-x rgrep RET > some text RET RET RET > > The rgrep output will look something like: > > find [...] --null -e "some text" "{}" + > find: paths must precede expression: `^^!^' I confirm the bug, it happens also for me. > You can click the "[...]" to see the full invocation. However, even > without doing that, if you look carefully, you'll notice that the > shell-quoting uses the MS-Windows rules, not that of /bin/sh. For the > MS-Windows shell, spaces are quoted by wrapping the entire argument in > double-quotes ("like this"); for /bin/sh, spaces are escaped via a > backslash (like\ this). > > Presumably, that's because if you eval `shell-file-name' in the Dired > buffer, it reports ".../path/to/cmdproxy.exe". When in a remote > *file*, `shell-file-name' is correctly set to "/bin/sh". It is not a problem of shell-file-name, if you check the Tramp debug buffer you'll see, that a proper shell ("/bin/sh" in my case) is applied. The problem is rather quoting the arguments with shell-quote-argument. It applies the quoting according to the value of system-type. If this is 'ms-dos or 'windows-nt, MS Windows quoting rules are applied. The appended patch fixes this for me, could you pls check? Best regards, Michael. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: Type: text/x-patch, Size: 5239 bytes --] diff --git a/lisp/progmodes/grep.el b/lisp/progmodes/grep.el index ccc58e6773..85e872bfc2 100644 --- a/lisp/progmodes/grep.el +++ b/lisp/progmodes/grep.el @@ -611,6 +611,11 @@ grep-hello-file (write-region "Copyright\n" nil result)) result)) +(defun grep-shell-quote-argument (argument) + (let ((system-type + (if (file-remote-p default-directory) 'not-windows system-type))) + (shell-quote-argument argument))) + ;;;###autoload (defun grep-compute-defaults () "Compute the defaults for the `grep' command. @@ -636,8 +641,8 @@ grep-compute-defaults (intern (or (file-remote-p default-directory) "localhost"))) (host-defaults (assq host-id grep-host-defaults-alist)) (defaults (assq nil grep-host-defaults-alist)) - (quot-braces (shell-quote-argument "{}")) - (quot-scolon (shell-quote-argument ";"))) + (quot-braces (grep-shell-quote-argument "{}")) + (quot-scolon (grep-shell-quote-argument ";"))) ;; There are different defaults on different hosts. They must be ;; computed for every host once. (dolist (setting '(grep-command grep-template @@ -820,7 +825,7 @@ grep-tag-default (defun grep-default-command () "Compute the default grep command for \\[universal-argument] \\[grep] to offer." - (let ((tag-default (shell-quote-argument (grep-tag-default))) + (let ((tag-default (grep-shell-quote-argument (grep-tag-default))) ;; This a regexp to match single shell arguments. ;; Could someone please add comments explaining it? (sh-arg-re @@ -963,7 +968,7 @@ grep-expand-keywords ("<F>" . files) ("<N>" . (null-device)) ("<X>" . excl) - ("<R>" . (shell-quote-argument (or regexp "")))) + ("<R>" . (grep-shell-quote-argument (or regexp "")))) "List of substitutions performed by `grep-expand-template'. If car of an element matches, the cdr is evalled in order to get the substitution string. @@ -1134,10 +1139,10 @@ lgrep (mapconcat (lambda (ignore) (cond ((stringp ignore) - (shell-quote-argument ignore)) + (grep-shell-quote-argument ignore)) ((consp ignore) (and (funcall (car ignore) dir) - (shell-quote-argument + (grep-shell-quote-argument (cdr ignore)))))) grep-find-ignored-files " --exclude="))) @@ -1245,44 +1250,44 @@ rgrep-default-command (grep-expand-template grep-find-template regexp - (concat (shell-quote-argument "(") + (concat (grep-shell-quote-argument "(") " " find-name-arg " " (mapconcat - #'shell-quote-argument + #'grep-shell-quote-argument (split-string files) (concat " -o " find-name-arg " ")) " " - (shell-quote-argument ")")) + (grep-shell-quote-argument ")")) dir (concat (and grep-find-ignored-directories (concat "-type d " - (shell-quote-argument "(") - ;; we should use shell-quote-argument here + (grep-shell-quote-argument "(") + ;; we should use grep-shell-quote-argument here " -path " - (mapconcat (lambda (d) (shell-quote-argument (concat "*/" d))) + (mapconcat (lambda (d) (grep-shell-quote-argument (concat "*/" d))) (rgrep-find-ignored-directories dir) " -o -path ") " " - (shell-quote-argument ")") + (grep-shell-quote-argument ")") " -prune -o ")) (and grep-find-ignored-files - (concat (shell-quote-argument "!") " -type d " - (shell-quote-argument "(") - ;; we should use shell-quote-argument here + (concat (grep-shell-quote-argument "!") " -type d " + (grep-shell-quote-argument "(") + ;; we should use grep-shell-quote-argument here " -name " (mapconcat (lambda (ignore) (cond ((stringp ignore) - (shell-quote-argument ignore)) + (grep-shell-quote-argument ignore)) ((consp ignore) (and (funcall (car ignore) dir) - (shell-quote-argument + (grep-shell-quote-argument (cdr ignore)))))) grep-find-ignored-files " -o -name ") " " - (shell-quote-argument ")") + (grep-shell-quote-argument ")") " -prune -o "))))) (defun grep-find-toggle-abbreviation () ^ permalink raw reply related [flat|nested] 13+ messages in thread
* bug#54487: 29.0.50; connection-local value for `shell-file-name' not set in Dired buffers over Tramp 2022-03-21 10:25 ` Michael Albinus @ 2022-03-21 12:40 ` Eli Zaretskii 2022-03-21 14:06 ` Michael Albinus 0 siblings, 1 reply; 13+ messages in thread From: Eli Zaretskii @ 2022-03-21 12:40 UTC (permalink / raw) To: Michael Albinus; +Cc: jporterbugs, 54487 > From: Michael Albinus <michael.albinus@gmx.de> > Date: Mon, 21 Mar 2022 11:25:28 +0100 > Cc: 54487@debbugs.gnu.org > > The problem is rather quoting the arguments with shell-quote-argument. It > applies the quoting according to the value of system-type. If this is > 'ms-dos or 'windows-nt, MS Windows quoting rules are applied. > > The appended patch fixes this for me, could you pls check? Is it really a good idea to solve this only for Grep? Shouldn't shell quoting always use this logic (with some variable that callers could bind in exceptional cases, which I presume will be rare)? Or am I missing something? ^ permalink raw reply [flat|nested] 13+ messages in thread
* bug#54487: 29.0.50; connection-local value for `shell-file-name' not set in Dired buffers over Tramp 2022-03-21 12:40 ` Eli Zaretskii @ 2022-03-21 14:06 ` Michael Albinus 2022-03-21 14:52 ` Eli Zaretskii 2022-03-21 18:04 ` Jim Porter 0 siblings, 2 replies; 13+ messages in thread From: Michael Albinus @ 2022-03-21 14:06 UTC (permalink / raw) To: Eli Zaretskii; +Cc: jporterbugs, 54487 Eli Zaretskii <eliz@gnu.org> writes: Hi Eli, > Is it really a good idea to solve this only for Grep? Shouldn't shell > quoting always use this logic (with some variable that callers could > bind in exceptional cases, which I presume will be rare)? Or am I > missing something? I had the same feeling after sending the patch, so I've started to rework this. I came out with the following solution: --8<---------------cut here---------------start------------->8--- shell-quote-argument is a compiled Lisp function in ‘../../../src/emacs/lisp/subr.el’. (shell-quote-argument ARGUMENT &optional POSIX) Quote ARGUMENT for passing as argument to an inferior shell. This function is designed to work with the syntax of your system’s standard shell, and might produce incorrect results with unusual shells. See Info node ‘(elisp)Security Considerations’. If the optional POSIX argument is non-nil, ARGUMENT is quoted according to POSIX rules. --8<---------------cut here---------------end--------------->8--- I'll wait until Jim confirms that this works in general, then I would apply a patch along this spec. Best regards, Michael. ^ permalink raw reply [flat|nested] 13+ messages in thread
* bug#54487: 29.0.50; connection-local value for `shell-file-name' not set in Dired buffers over Tramp 2022-03-21 14:06 ` Michael Albinus @ 2022-03-21 14:52 ` Eli Zaretskii 2022-03-21 15:02 ` Michael Albinus 2022-03-21 18:04 ` Jim Porter 1 sibling, 1 reply; 13+ messages in thread From: Eli Zaretskii @ 2022-03-21 14:52 UTC (permalink / raw) To: Michael Albinus; +Cc: jporterbugs, 54487 > From: Michael Albinus <michael.albinus@gmx.de> > Cc: jporterbugs@gmail.com, 54487@debbugs.gnu.org > Date: Mon, 21 Mar 2022 15:06:28 +0100 > > (shell-quote-argument ARGUMENT &optional POSIX) > > Quote ARGUMENT for passing as argument to an inferior shell. > > This function is designed to work with the syntax of your system’s > standard shell, and might produce incorrect results with unusual shells. > See Info node ‘(elisp)Security Considerations’. > > If the optional POSIX argument is non-nil, ARGUMENT is quoted > according to POSIX rules. Thanks. Please augment the last sentence by using "according to POSIX shell quoting rules, regardless of the system's shell." Or something similar. ^ permalink raw reply [flat|nested] 13+ messages in thread
* bug#54487: 29.0.50; connection-local value for `shell-file-name' not set in Dired buffers over Tramp 2022-03-21 14:52 ` Eli Zaretskii @ 2022-03-21 15:02 ` Michael Albinus 2022-03-21 15:05 ` Eli Zaretskii 0 siblings, 1 reply; 13+ messages in thread From: Michael Albinus @ 2022-03-21 15:02 UTC (permalink / raw) To: Eli Zaretskii; +Cc: jporterbugs, 54487 Eli Zaretskii <eliz@gnu.org> writes: Hi Eli, >> If the optional POSIX argument is non-nil, ARGUMENT is quoted >> according to POSIX rules. > > Thanks. > > Please augment the last sentence by using "according to POSIX shell > quoting rules, regardless of the system's shell." Or something > similar. Sure. I'm just testing the full patch, and it looks promising. While I'm at this, I'm thinking whether I shall change computing of grep defaults to connection-local variables. This would be more in line with handling such local variables in recent Emacsen. WDYT? Best regards, Michael. ^ permalink raw reply [flat|nested] 13+ messages in thread
* bug#54487: 29.0.50; connection-local value for `shell-file-name' not set in Dired buffers over Tramp 2022-03-21 15:02 ` Michael Albinus @ 2022-03-21 15:05 ` Eli Zaretskii 2022-03-21 15:09 ` Michael Albinus 0 siblings, 1 reply; 13+ messages in thread From: Eli Zaretskii @ 2022-03-21 15:05 UTC (permalink / raw) To: Michael Albinus; +Cc: jporterbugs, 54487 > From: Michael Albinus <michael.albinus@gmx.de> > Cc: jporterbugs@gmail.com, 54487@debbugs.gnu.org > Date: Mon, 21 Mar 2022 16:02:09 +0100 > > While I'm at this, I'm thinking whether I shall change computing of grep > defaults to connection-local variables. This would be more in line with > handling such local variables in recent Emacsen. WDYT? Probably. But I don't have enough experience with remote processes, so it is probably best to wait for others to chime in. ^ permalink raw reply [flat|nested] 13+ messages in thread
* bug#54487: 29.0.50; connection-local value for `shell-file-name' not set in Dired buffers over Tramp 2022-03-21 15:05 ` Eli Zaretskii @ 2022-03-21 15:09 ` Michael Albinus 0 siblings, 0 replies; 13+ messages in thread From: Michael Albinus @ 2022-03-21 15:09 UTC (permalink / raw) To: Eli Zaretskii; +Cc: jporterbugs, 54487 Eli Zaretskii <eliz@gnu.org> writes: Hi Eli, >> While I'm at this, I'm thinking whether I shall change computing of grep >> defaults to connection-local variables. This would be more in line with >> handling such local variables in recent Emacsen. WDYT? > > Probably. But I don't have enough experience with remote processes, > so it is probably best to wait for others to chime in. OK. Anyway, I will push first a patch for the given bug. Other changes, if any, will be different patches. Best regards, Michael. ^ permalink raw reply [flat|nested] 13+ messages in thread
* bug#54487: 29.0.50; connection-local value for `shell-file-name' not set in Dired buffers over Tramp 2022-03-21 14:06 ` Michael Albinus 2022-03-21 14:52 ` Eli Zaretskii @ 2022-03-21 18:04 ` Jim Porter 2022-03-22 9:44 ` Michael Albinus 2022-03-23 18:58 ` Michael Albinus 1 sibling, 2 replies; 13+ messages in thread From: Jim Porter @ 2022-03-21 18:04 UTC (permalink / raw) To: Michael Albinus, Eli Zaretskii; +Cc: 54487 On 3/21/2022 7:06 AM, Michael Albinus wrote: > I'll wait until Jim confirms that this works in general, then I would > apply a patch along this spec. The patch you posted works for me. Setting `shell-file-name' to "/bin/sh" worked in my tests because it makes the function `w32-shell-dos-semantics' return nil, so this condition in `shell-quote-argument' isn't matched: ((and (eq system-type 'windows-nt) (w32-shell-dos-semantics)) That makes the shell-quoting use POSIX-style rules instead, which is what we want if the default-directory is remote. Reading that code, I think the `w32-shell-dos-semantics' part of that condition is there to handle things like Cygwin builds, so maybe it's not quite right to rely on that for the case I described in the original report. (That said, I think it would only be an issue for some truly esoteric configurations.) On the other hand, I think I like the idea of having grep be aware of connection-local variables even better. That's more flexible, and also should work for the reverse case: if you call rgrep from a Tramp file buffer, but change the search directory to a local path, rgrep uses POSIX shell-quoting. It should use MS-Windows shell-quoting in that case (since it's running the command on the local Windows system). ^ permalink raw reply [flat|nested] 13+ messages in thread
* bug#54487: 29.0.50; connection-local value for `shell-file-name' not set in Dired buffers over Tramp 2022-03-21 18:04 ` Jim Porter @ 2022-03-22 9:44 ` Michael Albinus 2022-03-23 11:53 ` Michael Albinus 2022-03-23 18:58 ` Michael Albinus 1 sibling, 1 reply; 13+ messages in thread From: Michael Albinus @ 2022-03-22 9:44 UTC (permalink / raw) To: Jim Porter; +Cc: 54487 Jim Porter <jporterbugs@gmail.com> writes: Hi Jim, > On 3/21/2022 7:06 AM, Michael Albinus wrote: >> I'll wait until Jim confirms that this works in general, then I would >> apply a patch along this spec. I've pushed a fix to master. It is different from what I have shown before, but shall serve as well. > The patch you posted works for me. Setting `shell-file-name' to > "/bin/sh" worked in my tests because it makes the function > `w32-shell-dos-semantics' return nil, so this condition in > `shell-quote-argument' isn't matched: > > ((and (eq system-type 'windows-nt) (w32-shell-dos-semantics)) > > That makes the shell-quoting use POSIX-style rules instead, which is > what we want if the default-directory is remote. Reading that code, I > think the `w32-shell-dos-semantics' part of that condition is there to > handle things like Cygwin builds, so maybe it's not quite right to > rely on that for the case I described in the original report. (That > said, I think it would only be an issue for some truly esoteric > configurations.) Fiddling with shell-file-name doesn't help in this case, because connection-local variables are not applied in every remote buffer, like in dired buffers. The more general collection of Tramp-aware connection-local variables could damage something else, that's why it is appled on programmatic request only. > On the other hand, I think I like the idea of having grep be aware of > connection-local variables even better. That's more flexible, and also > should work for the reverse case: if you call rgrep from a Tramp file > buffer, but change the search directory to a local path, rgrep uses > POSIX shell-quoting. It should use MS-Windows shell-quoting in that > case (since it's running the command on the local Windows system). Yep, I'll start now to work on this. The plan is to collect these specific connection-local variables in an own :application, so that they are set only with the given "grep" scope. For the scope of this bug report, it could be closed. But I'll like to keep it open for now in order to discuss possible problems with the connection-local variables approach. Best regards, Michael. ^ permalink raw reply [flat|nested] 13+ messages in thread
* bug#54487: 29.0.50; connection-local value for `shell-file-name' not set in Dired buffers over Tramp 2022-03-22 9:44 ` Michael Albinus @ 2022-03-23 11:53 ` Michael Albinus 2022-03-23 16:55 ` Jim Porter 0 siblings, 1 reply; 13+ messages in thread From: Michael Albinus @ 2022-03-23 11:53 UTC (permalink / raw) To: Jim Porter; +Cc: 54487-done Version: 29.1 Michael Albinus <michael.albinus@gmx.de> writes: Hi Jim, > For the scope of this bug report, it could be closed. But I'll like to > keep it open for now in order to discuss possible problems with the > connection-local variables approach. I've checked the needed changes, and it would be too invasive. In grep.el, host specific settings are computed by global variables. Furthermore, the compilation buffer is created only after computing these settings. This would require a larger rewrite of grep.el. I believe it isn't worth then. Closing the bug. Best regards, Michael. ^ permalink raw reply [flat|nested] 13+ messages in thread
* bug#54487: 29.0.50; connection-local value for `shell-file-name' not set in Dired buffers over Tramp 2022-03-23 11:53 ` Michael Albinus @ 2022-03-23 16:55 ` Jim Porter 0 siblings, 0 replies; 13+ messages in thread From: Jim Porter @ 2022-03-23 16:55 UTC (permalink / raw) To: 54487, michael.albinus On 3/23/2022 4:53 AM, Michael Albinus wrote: > I've checked the needed changes, and it would be too invasive. In > grep.el, host specific settings are computed by global variables. > Furthermore, the compilation buffer is created only after computing > these settings. Thanks for looking into it. I'm looking at doing this for a similar package, so if I can come up with a solution there, maybe I can see about porting it to grep.el too. If I come up with a simple way to do this, I'll just file another bug with a patch though. ^ permalink raw reply [flat|nested] 13+ messages in thread
* bug#54487: 29.0.50; connection-local value for `shell-file-name' not set in Dired buffers over Tramp 2022-03-21 18:04 ` Jim Porter 2022-03-22 9:44 ` Michael Albinus @ 2022-03-23 18:58 ` Michael Albinus 1 sibling, 0 replies; 13+ messages in thread From: Michael Albinus @ 2022-03-23 18:58 UTC (permalink / raw) To: Jim Porter; +Cc: 54487 Jim Porter <jporterbugs@gmail.com> writes: Hi Jim, > On the other hand, I think I like the idea of having grep be aware of > connection-local variables even better. That's more flexible, and also > should work for the reverse case: if you call rgrep from a Tramp file > buffer, but change the search directory to a local path, rgrep uses > POSIX shell-quoting. It should use MS-Windows shell-quoting in that > case (since it's running the command on the local Windows system). With commit ef0a0d30c5 this shall work now, even w/o connection-local variables. Best regards, Michael. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-03-23 18:58 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-03-21 4:58 bug#54487: 29.0.50; connection-local value for `shell-file-name' not set in Dired buffers over Tramp Jim Porter 2022-03-21 10:25 ` Michael Albinus 2022-03-21 12:40 ` Eli Zaretskii 2022-03-21 14:06 ` Michael Albinus 2022-03-21 14:52 ` Eli Zaretskii 2022-03-21 15:02 ` Michael Albinus 2022-03-21 15:05 ` Eli Zaretskii 2022-03-21 15:09 ` Michael Albinus 2022-03-21 18:04 ` Jim Porter 2022-03-22 9:44 ` Michael Albinus 2022-03-23 11:53 ` Michael Albinus 2022-03-23 16:55 ` Jim Porter 2022-03-23 18:58 ` 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).