Re: [PATCH] emacs: notmuch-show: remove extraneous shell quoting

Subject: Re: [PATCH] emacs: notmuch-show: remove extraneous shell quoting

Date: Wed, 14 Sep 2016 12:51:51 +0300

To: Matt Armstrong, notmuch@notmuchmail.org

Cc:

From: Tomi Ollila


On Wed, Sep 14 2016, Matt Armstrong <marmstrong@google.com> wrote:

> Remove shell quoting from notmuch-show--build-buffer.  The args list
> is ultimately passed to call-process, which passes them verbatim to
> the subprocess (typically, notmuch).  The quoting, intended for a
> shell, is unnecessary and confusing.

To motivate someone(tm) to test and review this patch, I'd like to know
which problem does this _fix_ (if any). If it does not then such a good
change as this looks like it is takes a bit longer to be taken to review
(and the reviewers need to be more careful for spotting potential
regressions...) 

Tomi

>
> This code originally appeared in
> 9193455fa1476ea3957475e77379b75efa6dd90b, aiming to support remote
> notmuch execution over ssh.  The commit message included this example
> shell script (simplified):
>
>   #!/bin/sh
>   ssh remotehost notmuch $@
>
> That script doesn't properly quote "$@".  Even if it did, it wouldn't
> work because the shell ssh starts on the remote host does another
> round of shell word splitting.  Fortunately, notmuch already provides
> an example of a correct shell script on
> https://notmuchmail.org/remoteusage/ (again simplified):
>
>   #!/bin/bash
>   printf -v ARGS "%q " "$@"
>   ssh remotehost notmuch $ARGS
>
> With the above script I'm able to use notmuch in "remote" mode.
> Quoted search terms work as expected, etc.
>
> ---
>  emacs/notmuch-show.el | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 5a585f3..da2d685 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -1267,9 +1267,9 @@ first relevant message.
>  If no messages match the query return NIL."
>    (let* ((basic-args (list notmuch-show-thread-id))
>  	 (args (if notmuch-show-query-context
> -		   (append (list "\'") basic-args
> -			   (list "and (" notmuch-show-query-context ")\'"))
> -		 (append (list "\'") basic-args (list "\'"))))
> +		   (append basic-args
> +			   (list "and (" notmuch-show-query-context ")"))
> +		 basic-args))
>  	 (cli-args (cons "--exclude=false"
>  			 (when notmuch-show-elide-non-matching-messages
>  			   (list "--entire-thread=false"))))
> -- 
> tg: (89c8d27..) t/remote-quoting (depends on: master)
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch

Thread: