* Re: master 0ad1c0d: * lisp/net/tramp.el (tramp-handle-make-process): Handle shell commands.
[not found] ` <20201218123339.A90E820B72@vcs0.savannah.gnu.org>
@ 2020-12-18 15:15 ` Stefan Monnier
2020-12-18 15:24 ` Michael Albinus
0 siblings, 1 reply; 12+ messages in thread
From: Stefan Monnier @ 2020-12-18 15:15 UTC (permalink / raw)
To: Michael Albinus; +Cc: emacs-devel
Hi Michael,
> + ;; Quote shell command.
> + (when (and (= (length command) 3)
> + (stringp (nth 0 command))
> + (string-match-p "sh$" (nth 0 command))
> + (stringp (nth 1 command))
> + (string-equal "-c" (nth 1 command))
> + (stringp (nth 2 command)))
> + (setcar (cddr command) (tramp-shell-quote-argument (nth 2 command))))
This looks odd (what is special about the quoting needs of "sh -c" which
wouldn't apply to other commands?). Do you have some bug# or test case
associated to it so we can better understand why it's needed?
Stefan
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: master 0ad1c0d: * lisp/net/tramp.el (tramp-handle-make-process): Handle shell commands.
2020-12-18 15:15 ` master 0ad1c0d: * lisp/net/tramp.el (tramp-handle-make-process): Handle shell commands Stefan Monnier
@ 2020-12-18 15:24 ` Michael Albinus
2020-12-18 15:38 ` Stefan Monnier
0 siblings, 1 reply; 12+ messages in thread
From: Michael Albinus @ 2020-12-18 15:24 UTC (permalink / raw)
To: Stefan Monnier; +Cc: emacs-devel
Stefan Monnier <monnier@iro.umontreal.ca> writes:
> Hi Michael,
Hi Stefan
>> + ;; Quote shell command.
>> + (when (and (= (length command) 3)
>> + (stringp (nth 0 command))
>> + (string-match-p "sh$" (nth 0 command))
>> + (stringp (nth 1 command))
>> + (string-equal "-c" (nth 1 command))
>> + (stringp (nth 2 command)))
>> + (setcar (cddr command) (tramp-shell-quote-argument (nth 2 command))))
>
> This looks odd (what is special about the quoting needs of "sh -c" which
> wouldn't apply to other commands?). Do you have some bug# or test case
> associated to it so we can better understand why it's needed?
See the thread "Tramp and conversion of \r\n into \n" in
emacs-devel. Martin gave me tons of Tramp traces.
I'm also not so happy with this special handling, and I expect we'll
find a better solution. I'm still working on this.
However, it is at least a proof of concept. People using lsp-mode call
also many other asynchronous processes, and so I'll get lot of traces
from real life examples to compare.
And yes, based on these examples I will also extend tramp-tests.el.
> Stefan
Best regards, Michael.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: master 0ad1c0d: * lisp/net/tramp.el (tramp-handle-make-process): Handle shell commands.
2020-12-18 15:24 ` Michael Albinus
@ 2020-12-18 15:38 ` Stefan Monnier
2020-12-18 15:39 ` Stefan Monnier
2020-12-18 16:02 ` Michael Albinus
0 siblings, 2 replies; 12+ messages in thread
From: Stefan Monnier @ 2020-12-18 15:38 UTC (permalink / raw)
To: Michael Albinus; +Cc: emacs-devel
>>> + ;; Quote shell command.
>>> + (when (and (= (length command) 3)
>>> + (stringp (nth 0 command))
>>> + (string-match-p "sh$" (nth 0 command))
>>> + (stringp (nth 1 command))
>>> + (string-equal "-c" (nth 1 command))
>>> + (stringp (nth 2 command)))
>>> + (setcar (cddr command) (tramp-shell-quote-argument (nth 2 command))))
>>
>> This looks odd (what is special about the quoting needs of "sh -c" which
>> wouldn't apply to other commands?). Do you have some bug# or test case
>> associated to it so we can better understand why it's needed?
>
> See the thread "Tramp and conversion of \r\n into \n" in
> emacs-devel. Martin gave me tons of Tramp traces.
>
> I'm also not so happy with this special handling, and I expect we'll
> find a better solution. I'm still working on this.
I commend you make that clear in a comment (including a reference to
the relevant thread).
BTW, what would go wrong if you did
(setq command (mapcar #'tramp-shell-quote-argument command))?
Stefan
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: master 0ad1c0d: * lisp/net/tramp.el (tramp-handle-make-process): Handle shell commands.
2020-12-18 15:38 ` Stefan Monnier
@ 2020-12-18 15:39 ` Stefan Monnier
2020-12-18 16:02 ` Michael Albinus
1 sibling, 0 replies; 12+ messages in thread
From: Stefan Monnier @ 2020-12-18 15:39 UTC (permalink / raw)
To: Michael Albinus; +Cc: emacs-devel
> I commend you make that clear in a comment (including a reference to
Hmm... not sure where the "re" went,
Stefan
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: master 0ad1c0d: * lisp/net/tramp.el (tramp-handle-make-process): Handle shell commands.
2020-12-18 15:38 ` Stefan Monnier
2020-12-18 15:39 ` Stefan Monnier
@ 2020-12-18 16:02 ` Michael Albinus
2020-12-18 16:28 ` Stefan Monnier
2020-12-18 16:30 ` Andreas Schwab
1 sibling, 2 replies; 12+ messages in thread
From: Michael Albinus @ 2020-12-18 16:02 UTC (permalink / raw)
To: Stefan Monnier; +Cc: emacs-devel
Stefan Monnier <monnier@iro.umontreal.ca> writes:
> BTW, what would go wrong if you did
> (setq command (mapcar #'tramp-shell-quote-argument command))?
Tried that, didn't work. Imagine, you call "M-x rgrep" on a remote
machine. This ends up in a make-process call with :command
--8<---------------cut here---------------start------------->8---
("/bin/sh" "-c" "find . -type d \\( -path \\*/SCCS -o -path \\*/RCS -o -path \\*/CVS -o -path \\*/MCVS -o -path \\*/.src -o -path \\*/.svn -o -path \\*/.git -o -path \\*/.hg -o -path \\*/.bzr -o -path \\*/_MTN -o -path \\*/_darcs -o -path \\*/\\{arch\\} \\) -prune -o \\! -type d \\( -name .\\#\\* -o -name \\*.o -o -name \\*\\~ -o -name \\*.bin -o -name \\*.lbin -o -name \\*.so -o -name \\*.a -o -name \\*.ln -o -name \\*.blg -o -name \\*.bbl -o -name \\*.elc -o -name \\*.lof -o -name \\*.glo -o -name \\*.idx -o -name \\*.lot -o -name \\*.fmt -o -name \\*.tfm -o -name \\*.class -o -name \\*.fas -o -name \\*.lib -o -name \\*.mem -o -name \\*.x86f -o -name \\*.sparcf -o -name \\*.dfsl -o -name \\*.pfsl -o -name \\*.d64fsl -o -name \\*.p64fsl -o -name \\*.lx64fsl -o -name \\*.lx32fsl -o -name \\*.dx64fsl -o -name \\*.dx32fsl -o -name \\*.fx64fsl -o -name \\*.fx32fsl -o -name \\*.sx64fsl -o -name \\*.sx32fsl -o -name \\*.wx64fsl -o -name \\*.wx32fsl -o -name \\*.fasl -o -name \\*.ufsl -o -name \\*.fsl -o -name \\*.dxl -o -name \\*.lo -o -name \\*.la -o -name \\*.gmo -o -name \\*.mo -o -name \\*.toc -o -name \\*.aux -o -name \\*.cp -o -name \\*.fn -o -name \\*.ky -o -name \\*.pg -o -name \\*.tp -o -name \\*.vr -o -name \\*.cps -o -name \\*.fns -o -name \\*.kys -o -name \\*.pgs -o -name \\*.tps -o -name \\*.vrs -o -name \\*.pyc -o -name \\*.pyo \\) -prune -o -type f \\( -name \\*.cc -o -name \\*.cxx -o -name \\*.cpp -o -name \\*.C -o -name \\*.CC -o -name \\*.c\\+\\+ \\) -exec grep --color -i -nH --null -e a \\{\\} +")
--8<---------------cut here---------------end--------------->8---
As you see, three arguments "/bin/sh" "-c" and "find ...". If I do your
proposal, Tramp would send finally
--8<---------------cut here---------------start------------->8---
ssh -q -o ControlMaster=auto -o ControlPath='tramp.%C' -o ControlPersist=no -e none detlef /bin/sh -c find\\\ .\\\ -type\\\ d\\\ \\\\\\\(\\\ -path\\\ \\\\\\\*/SCCS\\\ -o\\\ -path\\\ \\\\\\\*/RCS\\\ -o\\\ -path\\\ \\\\\\\*/CVS\\\ -o\\\ -path\\\ \\\\\\\*/MCVS\\\ -o\\\ -path\\\ \\\\\\\*/.src\\\ -o\\\ -path\\\ \\\\\\\*/.svn\\\ -o\\\ -path\\\ \\\\\\\*/.git\\\ -o\\\ -path\\\ \\\\\\\*/.hg\\\ -o\\\ -path\\\ \\\\\\\*/.bzr\\\ -o\\\ -path\\\ \\\\\\\*/_MTN\\\ -o\\\ -path\\\ \\\\\\\*/_darcs\\\ -o\\\ -path\\\ \\\\\\\*/\\\\\\\{arch\\\\\\\}\\\ \\\\\\\)\\\ -prune\\\ -o\\\ \\\\\\\!\\\ -type\\\ d\\\ \\\\\\\(\\\ -name\\\ .\\\\\\\#\\\\\\\*\\\ -o\\\ -name\\\ \\\\\\\*.o\\\ -o\\\ -name\\\ \\\\\\\*\\\\\\\~\\\ -o\\\ -name\\\ \\\\\\\*.bin\\\ -o\\\ -name\\\ \\\\\\\*.lbin\\\ -o\\\ -name\\\ \\\\\\\*.so\\\ -o\\\ -name\\\ \\\\\\\*.a\\\ -o\\\ -name\\\ \\\\\\\*.ln\\\ -o\\\ -name\\\ \\\\\\\*.blg\\\ -o\\\ -name\\\ \\\\\\\*.bbl\\\ -o\\\ -name\\\ \\\\\\\*.elc\\\ -o\\\ -name\\\ \\\\\\\*.lof\\\ -o\\\ -name\\\ \\\\\\\*.glo\\\ -o\\\ -name\\\ \\\\\\\*.idx\\\ -o\\\ -name\\\ \\\\\\\*.lot\\\ -o\\\ -name\\\ \\\\\\\*.fmt\\\ -o\\\ -name\\\ \\\\\\\*.tfm\\\ -o\\\ -name\\\ \\\\\\\*.class\\\ -o\\\ -name\\\ \\\\\\\*.fas\\\ -o\\\ -name\\\ \\\\\\\*.lib\\\ -o\\\ -name\\\ \\\\\\\*.mem\\\ -o\\\ -name\\\ \\\\\\\*.x86f\\\ -o\\\ -name\\\ \\\\\\\*.sparcf\\\ -o\\\ -name\\\ \\\\\\\*.dfsl\\\ -o\\\ -name\\\ \\\\\\\*.pfsl\\\ -o\\\ -name\\\ \\\\\\\*.d64fsl\\\ -o\\\ -name\\\ \\\\\\\*.p64fsl\\\ -o\\\ -name\\\ \\\\\\\*.lx64fsl\\\ -o\\\ -name\\\ \\\\\\\*.lx32fsl\\\ -o\\\ -name\\\ \\\\\\\*.dx64fsl\\\ -o\\\ -name\\\ \\\\\\\*.dx32fsl\\\ -o\\\ -name\\\ \\\\\\\*.fx64fsl\\\ -o\\\ -name\\\ \\\\\\\*.fx32fsl\\\ -o\\\ -name\\\ \\\\\\\*.sx64fsl\\\ -o\\\ -name\\\ \\\\\\\*.sx32fsl\\\ -o\\\ -name\\\ \\\\\\\*.wx64fsl\\\ -o\\\ -name\\\ \\\\\\\*.wx32fsl\\\ -o\\\ -name\\\ \\\\\\\*.fasl\\\ -o\\\ -name\\\ \\\\\\\*.ufsl\\\ -o\\\ -name\\\ \\\\\\\*.fsl\\\ -o\\\ -name\\\ \\\\\\\*.dxl\\\ -o\\\ -name\\\ \\\\\\\*.lo\\\ -o\\\ -name\\\ \\\\\\\*.la\\\ -o\\\ -name\\\ \\\\\\\*.gmo\\\ -o\\\ -name\\\ \\\\\\\*.mo\\\ -o\\\ -name\\\ \\\\\\\*.toc\\\ -o\\\ -name\\\ \\\\\\\*.aux\\\ -o\\\ -name\\\ \\\\\\\*.cp\\\ -o\\\ -name\\\ \\\\\\\*.fn\\\ -o\\\ -name\\\ \\\\\\\*.ky\\\ -o\\\ -name\\\ \\\\\\\*.pg\\\ -o\\\ -name\\\ \\\\\\\*.tp\\\ -o\\\ -name\\\ \\\\\\\*.vr\\\ -o\\\ -name\\\ \\\\\\\*.cps\\\ -o\\\ -name\\\ \\\\\\\*.fns\\\ -o\\\ -name\\\ \\\\\\\*.kys\\\ -o\\\ -name\\\ \\\\\\\*.pgs\\\ -o\\\ -name\\\ \\\\\\\*.tps\\\ -o\\\ -name\\\ \\\\\\\*.vrs\\\ -o\\\ -name\\\ \\\\\\\*.pyc\\\ -o\\\ -name\\\ \\\\\\\*.pyo\\\ \\\\\\\)\\\ -prune\\\ -o\\\ \\\ -type\\\ f\\\ \\\\\\\(\\\ -name\\\ \\\\\\\*.cc\\\ -o\\\ -name\\\ \\\\\\\*.cxx\\\ -o\\\ -name\\\ \\\\\\\*.cpp\\\ -o\\\ -name\\\ \\\\\\\*.C\\\ -o\\\ -name\\\ \\\\\\\*.CC\\\ -o\\\ -name\\\ \\\\\\\*.c\\\\\\\+\\\\\\\+\\\ \\\\\\\)\\\ -exec\\\ grep\\\ --color\\\ -i\\\ -nH\\\ --null\\\ -e\\\ a\\\ \\\\\\\{\\\\\\\}\\\ \\\+
--8<---------------cut here---------------end--------------->8---
This gives the error
--8<---------------cut here---------------start------------->8---
/bin/sh: 1: find . -type d \( -path \*/SCCS -o -path \*/RCS -o -path \*/CVS -o -path \*/MCVS -o -path \*/.src -o -path \*/.svn -o -path \*/.git -o -path \*/.hg -o -path \*/.bzr -o -path \*/_MTN -o -path \*/_darcs -o -path \*/\{arch\} \) -prune -o \! -type d \( -name .\#\* -o -name \*.o -o -name \*\~ -o -name \*.bin -o -name \*.lbin -o -name \*.so -o -name \*.a -o -name \*.ln -o -name \*.blg -o -name \*.bbl -o -name \*.elc -o -name \*.lof -o -name \*.glo -o -name \*.idx -o -name \*.lot -o -name \*.fmt -o -name \*.tfm -o -name \*.class -o -name \*.fas -o -name \*.lib -o -name \*.mem -o -name \*.x86f -o -name \*.sparcf -o -name \*.dfsl -o -name \*.pfsl -o -name \*.d64fsl -o -name \*.p64fsl -o -name \*.lx64fsl -o -name \*.lx32fsl -o -name \*.dx64fsl -o -name \*.dx32fsl -o -name \*.fx64fsl -o -name \*.fx32fsl -o -name \*.sx64fsl -o -name \*.sx32fsl -o -name \*.wx64fsl -o -name \*.wx32fsl -o -name \*.fasl -o -name \*.ufsl -o -name \*.fsl -o -name \*.dxl -o -name \*.lo -o -name \*.la -o -name \*.gmo -o -name \*.mo -o -name \*.toc -o -name \*.aux -o -name \*.cp -o -name \*.fn -o -name \*.ky -o -name \*.pg -o -name \*.tp -o -name \*.vr -o -name \*.cps -o -name \*.fns -o -name \*.kys -o -name \*.pgs -o -name \*.tps -o -name \*.vrs -o -name \*.pyc -o -name \*.pyo \) -prune -o -type f \( -name \*.cc -o -name \*.cxx -o -name \*.cpp -o -name \*.C -o -name \*.CC -o -name \*.c\+\+ \) -exec grep --color -i -nH --null -e a \{\} +: not found
--8<---------------cut here---------------end--------------->8---
> Stefan
Best regards, Michael.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: master 0ad1c0d: * lisp/net/tramp.el (tramp-handle-make-process): Handle shell commands.
2020-12-18 16:02 ` Michael Albinus
@ 2020-12-18 16:28 ` Stefan Monnier
2020-12-18 17:23 ` Michael Albinus
2020-12-18 16:30 ` Andreas Schwab
1 sibling, 1 reply; 12+ messages in thread
From: Stefan Monnier @ 2020-12-18 16:28 UTC (permalink / raw)
To: Michael Albinus; +Cc: emacs-devel
>> BTW, what would go wrong if you did
>> (setq command (mapcar #'tramp-shell-quote-argument command))?
>
> Tried that, didn't work. Imagine, you call "M-x rgrep" on a remote
> machine. This ends up in a make-process call with :command
[...]
> As you see, three arguments "/bin/sh" "-c" and "find ...". If I do your
> proposal, Tramp would send finally
I don't understand: your code leaves "sh" and "-c" and passes the third
through `tramp-shell-quote-argument`, but that should give the exact
same result as passing all three through `tramp-shell-quote-argument`
since "sh" and "-c" are both left unchanged by `tramp-shell-quote-argument`.
What am I missing?
Stefan
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: master 0ad1c0d: * lisp/net/tramp.el (tramp-handle-make-process): Handle shell commands.
2020-12-18 16:02 ` Michael Albinus
2020-12-18 16:28 ` Stefan Monnier
@ 2020-12-18 16:30 ` Andreas Schwab
1 sibling, 0 replies; 12+ messages in thread
From: Andreas Schwab @ 2020-12-18 16:30 UTC (permalink / raw)
To: Michael Albinus; +Cc: Stefan Monnier, emacs-devel
On Dez 18 2020, Michael Albinus wrote:
> Tried that, didn't work. Imagine, you call "M-x rgrep" on a remote
> machine. This ends up in a make-process call with :command
>
> ("/bin/sh" "-c" "find . -type d \\( -path \\*/SCCS -o -path \\*/RCS -o -path \\*/CVS -o -path \\*/MCVS -o -path \\*/.src -o -path \\*/.svn -o -path \\*/.git -o -path \\*/.hg -o -path \\*/.bzr -o -path \\*/_MTN -o -path \\*/_darcs -o -path \\*/\\{arch\\} \\) -prune -o \\! -type d \\( -name .\\#\\* -o -name \\*.o -o -name \\*\\~ -o -name \\*.bin -o -name \\*.lbin -o -name \\*.so -o -name \\*.a -o -name \\*.ln -o -name \\*.blg -o -name \\*.bbl -o -name \\*.elc -o -name \\*.lof -o -name \\*.glo -o -name \\*.idx -o -name \\*.lot -o -name \\*.fmt -o -name \\*.tfm -o -name \\*.class -o -name \\*.fas -o -name \\*.lib -o -name \\*.mem -o -name \\*.x86f -o -name \\*.sparcf -o -name \\*.dfsl -o -name \\*.pfsl -o -name \\*.d64fsl -o -name \\*.p64fsl -o -name \\*.lx64fsl -o -name \\*.lx32fsl -o -name \\*.dx64fsl -o -name \\*.dx32fsl -o -name \\*.fx64fsl -o -name \\*.fx32fsl -o -name \\*.sx64fsl -o -name \\*.sx32fsl -o -name \\*.wx64fsl -o -name \\*.wx32fsl -o -name \\*.fasl -o -name \\*.ufsl -o -name \\*.fsl -o -name \\*.dxl -o -name \\*.lo -o -name \\*.la -o -name \\*.gmo -o -name \\*.mo -o -name \\*.toc -o -name \\*.aux -o -name \\*.cp -o -name \\*.fn -o -name \\*.ky -o -name \\*.pg -o -name \\*.tp -o -name \\*.vr -o -name \\*.cps -o -name \\*.fns -o -name \\*.kys -o -name \\*.pgs -o -name \\*.tps -o -name \\*.vrs -o -name \\*.pyc -o -name \\*.pyo \\) -prune -o -type f \\( -name \\*.cc -o -name \\*.cxx -o -name \\*.cpp -o -name \\*.C -o -name \\*.CC -o -name \\*.c\\+\\+ \\) -exec grep --color -i -nH --null -e a \\{\\} +")
But for this particular list,
(setq command (mapcar #'tramp-shell-quote-argument command))
and
(setcar (cddr command) (tramp-shell-quote-argument (nth 2 command)))
produce exactly the same.
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: master 0ad1c0d: * lisp/net/tramp.el (tramp-handle-make-process): Handle shell commands.
2020-12-18 16:28 ` Stefan Monnier
@ 2020-12-18 17:23 ` Michael Albinus
2020-12-18 18:57 ` Stefan Monnier
0 siblings, 1 reply; 12+ messages in thread
From: Michael Albinus @ 2020-12-18 17:23 UTC (permalink / raw)
To: Stefan Monnier; +Cc: emacs-devel
Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>> BTW, what would go wrong if you did
>>> (setq command (mapcar #'tramp-shell-quote-argument command))?
>>
>> Tried that, didn't work. Imagine, you call "M-x rgrep" on a remote
>> machine. This ends up in a make-process call with :command
> [...]
>> As you see, three arguments "/bin/sh" "-c" and "find ...". If I do your
>> proposal, Tramp would send finally
>
> I don't understand: your code leaves "sh" and "-c" and passes the third
> through `tramp-shell-quote-argument`, but that should give the exact
> same result as passing all three through `tramp-shell-quote-argument`
> since "sh" and "-c" are both left unchanged by `tramp-shell-quote-argument`.
>
> What am I missing?
The example I gave you was simplified. If you look into
tramp-handle-make-process, you'll see a more complex machinery. And yes,
I've tried several variants of quoting before I found this working
solution.
But I don't believe it shall be solved in this function. Because it is
about quoting of a shell command, it is likely that it would be better
fixed in start-file-process-shell-command. I would prefer
(defun start-file-process-shell-command (name buffer &rest args)
"..."
(with-connection-local-variables
(start-file-process
name buffer
shell-file-name shell-command-switch
(shell-quote-argument (mapconcat 'identity args " ")))))
However, shell-quote-argument quotes wrt the local shell. A shell on the
remote host might quote differently. Maybe we make
start-file-process-shell-command a magic function? or shell-quote-argument?
Don't know. I'll experiment with this next days. People will be in the
Xmas break, so there's time.
> Stefan
Best regards, Michael.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: master 0ad1c0d: * lisp/net/tramp.el (tramp-handle-make-process): Handle shell commands.
2020-12-18 17:23 ` Michael Albinus
@ 2020-12-18 18:57 ` Stefan Monnier
2020-12-19 16:42 ` Michael Albinus
2020-12-20 18:54 ` Michael Albinus
0 siblings, 2 replies; 12+ messages in thread
From: Stefan Monnier @ 2020-12-18 18:57 UTC (permalink / raw)
To: Michael Albinus; +Cc: emacs-devel
> (defun start-file-process-shell-command (name buffer &rest args)
> "..."
> (with-connection-local-variables
> (start-file-process
> name buffer
> shell-file-name shell-command-switch
> (shell-quote-argument (mapconcat 'identity args " ")))))
Concatenating arguments with a " " separator is plain wrong.
That's why we have
(declare (advertised-calling-convention (name buffer command) "23.1"))
so we can hopefully soon drop support for that concatenation.
Any caller which passes several `args` should be fixed.
Any chance this is the problem you're seeing?
If we disregard those wrong callers, the current definition is:
(with-connection-local-variables
(start-file-process
name buffer
shell-file-name shell-command-switch command))
So the `command` doesn't need any quoting here: it's
start-file-process's responsability to make sure it starts a process
with those 3 strings (shell-file-name as the name of the executable,
`shell-command-switch` as the first arg and `command` as the second).
In the case of Tramp's implementation of `start-file-process`, you're
going to run this process by constructing a command to send to the
remote shell, so you'll indeed need to turn this list of strings
into a single string and you need to do it by quoting those strings
using the quoting that corresponds to that of the remote shell (which
is indeed what `tramp-shell-quote-argument` does, IIUC).
IOW, I'd expect `start-file-process` to do something like
(mapconcat #'tramp-shell-quote-argument args " ")
to construct the command to send to the remote shell.
Stefan
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: master 0ad1c0d: * lisp/net/tramp.el (tramp-handle-make-process): Handle shell commands.
2020-12-18 18:57 ` Stefan Monnier
@ 2020-12-19 16:42 ` Michael Albinus
2020-12-20 18:54 ` Michael Albinus
1 sibling, 0 replies; 12+ messages in thread
From: Michael Albinus @ 2020-12-19 16:42 UTC (permalink / raw)
To: Stefan Monnier; +Cc: emacs-devel
Stefan Monnier <monnier@iro.umontreal.ca> writes:
Hi Stefan,
>> (defun start-file-process-shell-command (name buffer &rest args)
>> "..."
>> (with-connection-local-variables
>> (start-file-process
>> name buffer
>> shell-file-name shell-command-switch
>> (shell-quote-argument (mapconcat 'identity args " ")))))
>
> Concatenating arguments with a " " separator is plain wrong.
> That's why we have
>
> (declare (advertised-calling-convention (name buffer command) "23.1"))
>
> so we can hopefully soon drop support for that concatenation.
>
> Any caller which passes several `args` should be fixed.
> Any chance this is the problem you're seeing?
>
> If we disregard those wrong callers, the current definition is:
>
> (with-connection-local-variables
> (start-file-process
> name buffer
> shell-file-name shell-command-switch command))
Perhaps. Let's go this way. I have changed the function to this
definition, and I have recompiled all *.el files in Emacs master
branch. No compilation error at least, which means everybody follows the
calling conventions.
> So the `command` doesn't need any quoting here: it's
> start-file-process's responsability to make sure it starts a process
> with those 3 strings (shell-file-name as the name of the executable,
> `shell-command-switch` as the first arg and `command` as the second).
>
> In the case of Tramp's implementation of `start-file-process`, you're
> going to run this process by constructing a command to send to the
> remote shell, so you'll indeed need to turn this list of strings
> into a single string and you need to do it by quoting those strings
> using the quoting that corresponds to that of the remote shell (which
> is indeed what `tramp-shell-quote-argument` does, IIUC).
> IOW, I'd expect `start-file-process` to do something like
>
> (mapconcat #'tramp-shell-quote-argument args " ")
>
> to construct the command to send to the remote shell.
Might work. However, some of my tests in tramp-tests.el fail now, and
other tests still work. So I need to check what's up, before I commit.
> Stefan
Best regards, Michael.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: master 0ad1c0d: * lisp/net/tramp.el (tramp-handle-make-process): Handle shell commands.
2020-12-18 18:57 ` Stefan Monnier
2020-12-19 16:42 ` Michael Albinus
@ 2020-12-20 18:54 ` Michael Albinus
2020-12-20 21:29 ` Stefan Monnier
1 sibling, 1 reply; 12+ messages in thread
From: Michael Albinus @ 2020-12-20 18:54 UTC (permalink / raw)
To: Stefan Monnier; +Cc: emacs-devel
Stefan Monnier <monnier@iro.umontreal.ca> writes:
Hi Stefan,
>> (defun start-file-process-shell-command (name buffer &rest args)
>> "..."
>> (with-connection-local-variables
>> (start-file-process
>> name buffer
>> shell-file-name shell-command-switch
>> (shell-quote-argument (mapconcat 'identity args " ")))))
>
> Concatenating arguments with a " " separator is plain wrong.
> That's why we have
>
> (declare (advertised-calling-convention (name buffer command) "23.1"))
>
> so we can hopefully soon drop support for that concatenation.
I've dropped this.
> So the `command` doesn't need any quoting here: it's
> start-file-process's responsability to make sure it starts a process
> with those 3 strings (shell-file-name as the name of the executable,
> `shell-command-switch` as the first arg and `command` as the second).
>
> In the case of Tramp's implementation of `start-file-process`, you're
> going to run this process by constructing a command to send to the
> remote shell, so you'll indeed need to turn this list of strings
> into a single string and you need to do it by quoting those strings
> using the quoting that corresponds to that of the remote shell (which
> is indeed what `tramp-shell-quote-argument` does, IIUC).
> IOW, I'd expect `start-file-process` to do something like
>
> (mapconcat #'tramp-shell-quote-argument args " ")
>
> to construct the command to send to the remote shell.
My recent patch (pushed) is along these lines. In
tramp-handle-make-process, but this is just an implementation detail.
It is a little bit more complicate, because it needs also some quoting
for the purpose of ssh, but it works for this.
There are still some problems when the connection is not performed via
ssh; this I will continue to investigate.
> Stefan
Best regards, Michael.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: master 0ad1c0d: * lisp/net/tramp.el (tramp-handle-make-process): Handle shell commands.
2020-12-20 18:54 ` Michael Albinus
@ 2020-12-20 21:29 ` Stefan Monnier
0 siblings, 0 replies; 12+ messages in thread
From: Stefan Monnier @ 2020-12-20 21:29 UTC (permalink / raw)
To: Michael Albinus; +Cc: emacs-devel
>> Concatenating arguments with a " " separator is plain wrong.
>> That's why we have
>>
>> (declare (advertised-calling-convention (name buffer command) "23.1"))
>>
>> so we can hopefully soon drop support for that concatenation.
>
> I've dropped this.
Thanks.
>> So the `command` doesn't need any quoting here: it's
>> start-file-process's responsability to make sure it starts a process
>> with those 3 strings (shell-file-name as the name of the executable,
>> `shell-command-switch` as the first arg and `command` as the second).
>>
>> In the case of Tramp's implementation of `start-file-process`, you're
>> going to run this process by constructing a command to send to the
>> remote shell, so you'll indeed need to turn this list of strings
>> into a single string and you need to do it by quoting those strings
>> using the quoting that corresponds to that of the remote shell (which
>> is indeed what `tramp-shell-quote-argument` does, IIUC).
>> IOW, I'd expect `start-file-process` to do something like
>>
>> (mapconcat #'tramp-shell-quote-argument args " ")
>>
>> to construct the command to send to the remote shell.
>
> My recent patch (pushed) is along these lines. In
> tramp-handle-make-process, but this is just an implementation detail.
I'm glad we could find a less hackish solution (tho those levels of
quoting are involved, "hackish" is largely unavoidable).
Stefan
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-12-20 21:29 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20201218123338.4927.85373@vcs0.savannah.gnu.org>
[not found] ` <20201218123339.A90E820B72@vcs0.savannah.gnu.org>
2020-12-18 15:15 ` master 0ad1c0d: * lisp/net/tramp.el (tramp-handle-make-process): Handle shell commands Stefan Monnier
2020-12-18 15:24 ` Michael Albinus
2020-12-18 15:38 ` Stefan Monnier
2020-12-18 15:39 ` Stefan Monnier
2020-12-18 16:02 ` Michael Albinus
2020-12-18 16:28 ` Stefan Monnier
2020-12-18 17:23 ` Michael Albinus
2020-12-18 18:57 ` Stefan Monnier
2020-12-19 16:42 ` Michael Albinus
2020-12-20 18:54 ` Michael Albinus
2020-12-20 21:29 ` Stefan Monnier
2020-12-18 16:30 ` Andreas Schwab
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).