bug#65902: 29.0.92; emacsclient-mail.desktop fails due to complicated escaping

[sbaugh@catern.com]

[-- Attachment #1: Type: text/plain, Size: 4293 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: sbaugh@catern.com
>> Date: Sat, 23 Sep 2023 20:24:07 +0000 (UTC)
>> Cc: 65902@debbugs.gnu.org, sbaugh@janestreet.com, jporterbugs@gmail.com
>> 
>> > IIUC, this kind of solution is fine by me, but the protocol of
>> > accessing and using server-eval-args-left in the Lisp expressions
>> > specified on the emacsclient command line should be well-documented to
>> > avoid any confusion and UB.
>> 
>> Added a docstring and included it in NEWS.  I would have put it in the
>> manual, but it seems rather niche to put in the Emacs manual and I
>> didn't see a natural place to put it in the Emacs Lisp manual.
>
> The natural place is in the Emacs user manual, in "emacsclient
> Options", where --eval is described.  Where else?

Ah true, obvious, I added it there.

>> Passing arbitrary arguments to functions through emacsclient --eval
>> requires complicated escaping to avoid them being parsed as Lisp (as
>> seen in emacsclient-mail.desktop before this change).
>> 
>> This new variable server-eval-args-left allows access to the arguments
>> before they are parsed as Lisp.  By removing arguments from the
>> variable before they're parsed, a snippet of Lisp can consume
>> arguments, as in emacsclient-mail.desktop.
>> 
>> org-protocol might be able to use this as well, which might allow it
>> to drop its current advice on server-visit-files.
>
> The right place to keep this information is in the manual and the doc
> strings, not just in the Git commit log message.

Done.

>> +(defvar server-eval-args-left nil
>> +  "List of eval args not yet processed.
>> +
>> +When the server receives a request to eval one or more strings,
>> +it stores them in this variable.  Then, until this variable is
>> +nil, it `pop's a string from this variable and evaluates it with
>> +`server-eval-and-print'.  Adding or removing strings from this
>> +variable during this process will affect what is evaluated.
>
> This describes the implementation rather than the intended usage.

Fixed.

>> +This allows an expression passed to \"emacsclient --eval\" to
>> +consume one or more subsequent arguments before they're parsed or
>> +evaluated, with (pop server-eval-args-left).  This is useful if
>> +those arguments are arbitrary strings which should not be
>> +evaluated.
>
> This describes a way of using this, but without the important part:
> that any processed argument _must_ be popped, or it will be evaluated
> again.  See the doc string of command-line-functions for what I have
> in mind.
>
> All in all, I think this should be rewritten in terms of how to use
> this, and the result moved to the Emacs manual, leaving just the
> minimum here.

Done.

>> +See also `command-line-args-left' for a similar variable which
>> +works for command line invocations of \"emacs\".")
>
> This "see also" is not useful, because the doc string of
> command-line-args-left is minimal and doesn't add any information
> (which is okay, since that variable is basically meant for internal
> Emacs handling of command-line arguments, unlike this one).

Okay, fair, probably that should point at `argv' instead.

>> --- a/lisp/startup.el
>> +++ b/lisp/startup.el
>> @@ -120,7 +120,10 @@ command-switch-alist
>>      "List of command-line args not yet processed.
>>  This is a convenience alias, so that one can write (pop argv)
>>  inside of --eval command line arguments in order to access
>> -following arguments."))
>> +following arguments.
>> +
>> +See also `server-eval-args-left' for a similar variable which
>> +works for invocations of \"emacsclient --eval\"."))
>
> And neither is this, because the use cases of the two variables are
> almost completely unrelated.

The docstring of argv says:

This is a convenience alias, so that one can write (pop argv)
inside of --eval command line arguments in order to access
following arguments.

That is exactly the same way this new variable is used.

And in fact it's used for the exact same purpose in message-mailto.  The
reason "emacs" doesn't require complicated escaping is because
message-mailto does (pop command-line-args-left) internally.

I agree that argv/command-line-args-left has other use cases besides
this one.  But one of argv's use cases is the exact same use case of
server-eval-args-left.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-server-eval-args-left-to-server.el.patch --]
[-- Type: text/x-patch, Size: 8438 bytes --]

From bd49b918e57363593660f605c3e0efc3d0155c2b Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@catern.com>
Date: Thu, 21 Sep 2023 21:35:50 -0400
Subject: [PATCH] Add server-eval-args-left to server.el

Passing arbitrary arguments to functions through emacsclient --eval
requires complicated escaping to avoid them being parsed as Lisp (as
seen in emacsclient-mail.desktop before this change).

This new variable server-eval-args-left allows access to the arguments
before they are parsed as Lisp.  By removing arguments from the
variable before they're parsed, a snippet of Lisp can consume
arguments, as in emacsclient-mail.desktop.

org-protocol might be able to use this as well, which might allow it
to drop its current advice on server-visit-files.

* etc/emacsclient-mail.desktop: Use server-eval-args-left. (bug#65902)
* lisp/server.el (server-eval-args-left): Add.
(server-process-filter, server-execute): Make -eval arguments
available through server-eval-args-left.
* lisp/startup.el (argv): Mention server-eval-args-left in docstring.
* etc/NEWS: Announce server-eval-args-left.
* doc/emacs/misc.texi (emacsclient Options): Document
server-eval-args-left.
---
 doc/emacs/misc.texi          |  9 +++++++++
 etc/NEWS                     | 10 ++++++++++
 etc/emacsclient-mail.desktop |  7 ++-----
 lisp/server.el               | 27 ++++++++++++++++++++-------
 lisp/startup.el              |  5 ++++-
 5 files changed, 45 insertions(+), 13 deletions(-)

diff --git a/doc/emacs/misc.texi b/doc/emacs/misc.texi
index 7a88b7ef5e0..1ce1713b01c 100644
--- a/doc/emacs/misc.texi
+++ b/doc/emacs/misc.texi
@@ -2078,6 +2078,15 @@ emacsclient Options
 @command{emacsclient} are interpreted as a list of expressions to
 evaluate, @emph{not} as a list of files to visit.
 
+@vindex server-eval-args-left
+If you have arbitrary data which you want to provide as input to one
+of your expressions, you can pass the data as another argument to
+@command{emacsclient} and use @var{server-eval-args-left} in the
+expression to access the data.  Be careful to have your expression
+remove the data from @var{server-eval-args-left} regardless of whether
+your code succeeds, such as by using @code{pop}, otherwise Emacs will
+attempt to evaluate the data as a Lisp expression.
+
 @item -f @var{server-file}
 @itemx --server-file=@var{server-file}
 Specify a server file (@pxref{TCP Emacs server}) for connecting to an
diff --git a/etc/NEWS b/etc/NEWS
index cd4c414a64c..de1d79fcb98 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -121,6 +121,16 @@ Anything following the symbol 'mode-line-format-right-align' in
 right-aligned to is controlled by the new user option
 'mode-line-right-align-edge'.
 
+** Emacs Server and Client
+
+---
+*** 'server-eval-args-left' can be used to pop subsequent eval args
+When '--eval' is passed to emacsclient and Emacs is evaluating each
+argument, this variable is set to those which have not yet been
+evaluated.  It can be used to 'pop' arguments to prevent them from
+being evaluated, which is useful when those arguments contain
+arbitrary data.
+
 \f
 * Editing Changes in Emacs 30.1
 
diff --git a/etc/emacsclient-mail.desktop b/etc/emacsclient-mail.desktop
index 0a2420ddead..5962fa1764c 100644
--- a/etc/emacsclient-mail.desktop
+++ b/etc/emacsclient-mail.desktop
@@ -1,10 +1,7 @@
 [Desktop Entry]
 Categories=Network;Email;
 Comment=GNU Emacs is an extensible, customizable text editor - and more
-# We want to pass the following commands to the shell wrapper:
-# u=$(echo "$1" | sed 's/[\"]/\\&/g'); exec emacsclient --alternate-editor= --display="$DISPLAY" --eval "(message-mailto \"$u\")"
-# Special chars '"', '$', and '\' must be escaped as '\\"', '\\$', and '\\\\'.
-Exec=sh -c "u=\\$(echo \\"\\$1\\" | sed 's/[\\\\\\"]/\\\\\\\\&/g'); exec emacsclient --alternate-editor= --display=\\"\\$DISPLAY\\" --eval \\"(message-mailto \\\\\\"\\$u\\\\\\")\\"" sh %u
+Exec=emacsclient --alternate-editor= --eval '(message-mailto (pop server-eval-args-left))' %u
 Icon=emacs
 Name=Emacs (Mail, Client)
 MimeType=x-scheme-handler/mailto;
@@ -16,7 +13,7 @@ Actions=new-window;new-instance;
 
 [Desktop Action new-window]
 Name=New Window
-Exec=sh -c "u=\\$(echo \\"\\$1\\" | sed 's/[\\\\\\"]/\\\\\\\\&/g'); exec emacsclient --alternate-editor= --create-frame --eval \\"(message-mailto \\\\\\"\\$u\\\\\\")\\"" sh %u
+Exec=emacsclient --alternate-editor= --create-frame --eval '(message-mailto (pop server-eval-args-left))' %u
 
 [Desktop Action new-instance]
 Name=New Instance
diff --git a/lisp/server.el b/lisp/server.el
index c3325e5a24c..3970d7a7e81 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -1189,6 +1189,7 @@ server-process-filter
 		parent-id  ; Window ID for XEmbed
 		dontkill   ; t if client should not be killed.
 		commands
+		evalexprs
 		dir
 		use-current-frame
 		frame-parameters  ;parameters for newly created frame
@@ -1319,8 +1320,7 @@ server-process-filter
                  (let ((expr (pop args-left)))
                    (if coding-system
                        (setq expr (decode-coding-string expr coding-system)))
-                   (push (lambda () (server-eval-and-print expr proc))
-                         commands)
+                   (push expr evalexprs)
                    (setq filepos nil)))
 
                 ;; -env NAME=VALUE:  An environment variable.
@@ -1345,7 +1345,7 @@ server-process-filter
 	    ;; arguments, use an existing frame.
 	    (and nowait
 		 (not (eq tty-name 'window-system))
-		 (or files commands)
+		 (or files commands evalexprs)
 		 (setq use-current-frame t))
 
 	    (setq frame
@@ -1394,7 +1394,7 @@ server-process-filter
                  (let ((default-directory
                          (if (and dir (file-directory-p dir))
                              dir default-directory)))
-                   (server-execute proc files nowait commands
+                   (server-execute proc files nowait commands evalexprs
                                    dontkill frame tty-name)))))
 
             (when (or frame files)
@@ -1404,22 +1404,35 @@ server-process-filter
     ;; condition-case
     (t (server-return-error proc err))))
 
-(defun server-execute (proc files nowait commands dontkill frame tty-name)
+(defvar server-eval-args-left nil
+  "List of eval args not yet processed.
+
+Adding or removing strings from this variable while the Emacs
+server is processing a series of eval requests will affect what
+Emacs evaluates.
+
+See also `argv' for a similar variable which works for
+invocations of \"emacs\".")
+
+(defun server-execute (proc files nowait commands evalexprs dontkill frame tty-name)
   ;; This is run from timers and process-filters, i.e. "asynchronously".
   ;; But w.r.t the user, this is not really asynchronous since the timer
   ;; is run after 0s and the process-filter is run in response to the
   ;; user running `emacsclient'.  So it is OK to override the
-  ;; inhibit-quit flag, which is good since `commands' (as well as
+  ;; inhibit-quit flag, which is good since `evalexprs' (as well as
   ;; find-file-noselect via the major-mode) can run arbitrary code,
   ;; including code that needs to wait.
   (with-local-quit
     (condition-case err
         (let ((buffers (server-visit-files files proc nowait)))
           (mapc 'funcall (nreverse commands))
+          (let ((server-eval-args-left (nreverse evalexprs)))
+            (while server-eval-args-left
+              (server-eval-and-print (pop server-eval-args-left) proc)))
 	  ;; If we were told only to open a new client, obey
 	  ;; `initial-buffer-choice' if it specifies a file
           ;; or a function.
-          (unless (or files commands)
+          (unless (or files commands evalexprs)
             (let ((buf
                    (cond ((stringp initial-buffer-choice)
 			  (find-file-noselect initial-buffer-choice))
diff --git a/lisp/startup.el b/lisp/startup.el
index 7f601668369..f2c84751211 100644
--- a/lisp/startup.el
+++ b/lisp/startup.el
@@ -120,7 +120,10 @@ command-switch-alist
     "List of command-line args not yet processed.
 This is a convenience alias, so that one can write (pop argv)
 inside of --eval command line arguments in order to access
-following arguments."))
+following arguments.
+
+See also `server-eval-args-left' for a similar variable which
+works for invocations of \"emacsclient --eval\"."))
 (internal-make-var-non-special 'argv)
 
 (defvar command-line-args-left nil
-- 
2.41.0