bug#54487: 29.0.50; connection-local value for `shell-file-name' not set in Dired buffers over Tramp

bug#54487: 29.0.50; connection-local value for `shell-file-name' not set in Dired buffers over Tramp

[Jim Porter]

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

bug#54487: 29.0.50; connection-local value for `shell-file-name' not set in Dired buffers over Tramp

[Michael Albinus]

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

bug#54487: 29.0.50; connection-local value for `shell-file-name' not set in Dired buffers over Tramp

[Eli Zaretskii]

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

bug#54487: 29.0.50; connection-local value for `shell-file-name' not set in Dired buffers over Tramp

[Michael Albinus]

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.

bug#54487: 29.0.50; connection-local value for `shell-file-name' not set in Dired buffers over Tramp

[Eli Zaretskii]

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

bug#54487: 29.0.50; connection-local value for `shell-file-name' not set in Dired buffers over Tramp

[Michael Albinus]

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.

bug#54487: 29.0.50; connection-local value for `shell-file-name' not set in Dired buffers over Tramp

[Eli Zaretskii]

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

bug#54487: 29.0.50; connection-local value for `shell-file-name' not set in Dired buffers over Tramp

[Michael Albinus]

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.

bug#54487: 29.0.50; connection-local value for `shell-file-name' not set in Dired buffers over Tramp

[Jim Porter]

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

bug#54487: 29.0.50; connection-local value for `shell-file-name' not set in Dired buffers over Tramp

[Michael Albinus]

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.

bug#54487: 29.0.50; connection-local value for `shell-file-name' not set in Dired buffers over Tramp

[Michael Albinus]

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.

bug#54487: 29.0.50; connection-local value for `shell-file-name' not set in Dired buffers over Tramp

[Jim Porter]

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.

bug#54487: 29.0.50; connection-local value for `shell-file-name' not set in Dired buffers over Tramp

[Michael Albinus]

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.