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