bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor

bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor

[Reuben Thomas]

[-- Attachment #1.1: Type: text/plain, Size: 540 bytes --]

The attached patch adds support to emacsclient for command-line options
when specifying the alternate editor, so that for example one can now say:

ALTERNATE_EDITOR="emacs -Q -nw"

I have added documentation to NEWS. It did not seem necessary to change the
man page, as nowhere does it imply that you can't do this, and I
implemented the feature when I found that it didn't work as I expected;
other similar environment variables with which users may be familiar, such
as EDITOR and VISUAL, already work like this.

-- 
http://rrt.sc3d.org

[-- Attachment #1.2: Type: text/html, Size: 1005 bytes --]

[-- Attachment #2: 0005-Add-support-for-arguments-in-ALTERNATE_EDITOR-to-ema.patch --]
[-- Type: text/x-patch, Size: 4237 bytes --]

From 20f8b12cb667d99bd62dda338558c797f29f6861 Mon Sep 17 00:00:00 2001
From: Reuben Thomas <rrt@sc3d.org>
Date: Thu, 1 Dec 2016 15:21:57 +0000
Subject: [PATCH 5/5] Add support for arguments in ALTERNATE_EDITOR to
 emacsclient

* lib-src/emacsclient.c (fail): Parse ALTERNATE_EDITOR, or
corresponding command-line argument, into space-separated tokens.
* etc/NEWS: Document.
---
 etc/NEWS              |  4 +++
 lib-src/emacsclient.c | 73 ++++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 59 insertions(+), 18 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 33b8a42..c4f4569 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -268,6 +268,10 @@ variable of this kind to swap modifiers in Emacs.
 ---
 ** New input methods: 'cyrillic-tuvan', 'polish-prefix'.
 
+++
+** emacsclient now accepts command-line options in ALTERNATE_EDITOR
+and --alternate-editor. For example, ALTERNATE_EDITOR="emacs -Q -nw".
+
 \f
 * Editing Changes in Emacs 26.1
 
diff --git a/lib-src/emacsclient.c b/lib-src/emacsclient.c
index 2909d63..b561db6 100644
--- a/lib-src/emacsclient.c
+++ b/lib-src/emacsclient.c
@@ -109,6 +109,9 @@ char *w32_getenv (const char *);
 /* Name used to invoke this program.  */
 const char *progname;
 
+/* The first argument to main.  */
+int main_argc;
+
 /* The second argument to main.  */
 char **main_argv;
 
@@ -192,6 +195,35 @@ xmalloc (size_t size)
   return result;
 }
 
+/* Like realloc but get fatal error if memory is exhausted.  */
+
+static void *
+xrealloc (void *ptr, size_t size)
+{
+  void *result = realloc (ptr, size);
+  if (result == NULL)
+    {
+      perror ("realloc");
+      exit (EXIT_FAILURE);
+    }
+  return result;
+}
+
+/* Like strdup but get a fatal error if memory is exhausted. */
+char *xstrdup (const char *);
+
+char *
+xstrdup (const char *s)
+{
+  char *result = strdup (s);
+  if (result == NULL)
+    {
+      perror ("strdup");
+      exit (EXIT_FAILURE);
+    }
+  return result;
+}
+
 /* From sysdep.c */
 #if !defined (HAVE_GET_CURRENT_DIR_NAME) || defined (BROKEN_GET_CURRENT_DIR_NAME)
 
@@ -255,21 +287,6 @@ get_current_dir_name (void)
 
 #ifdef WINDOWSNT
 
-/* Like strdup but get a fatal error if memory is exhausted. */
-char *xstrdup (const char *);
-
-char *
-xstrdup (const char *s)
-{
-  char *result = strdup (s);
-  if (result == NULL)
-    {
-      perror ("strdup");
-      exit (EXIT_FAILURE);
-    }
-  return result;
-}
-
 #define REG_ROOT "SOFTWARE\\GNU\\Emacs"
 
 char *w32_get_resource (HKEY, const char *, LPDWORD);
@@ -651,7 +668,7 @@ Report bugs with M-x report-emacs-bug.\n");
 }
 
 /* Try to run a different command, or --if no alternate editor is
-   defined-- exit with an errorcode.
+   defined-- exit with an decoderde.
    Uses argv, but gets it from the global variable main_argv.  */
 
 static _Noreturn void
@@ -659,9 +676,27 @@ fail (void)
 {
   if (alternate_editor)
     {
-      int i = optind - 1;
+      size_t extra_args_size = (main_argc - optind + 1) * sizeof (char *);
+      size_t new_argv_size = extra_args_size;
+      char **new_argv = NULL;
+      /* Needed because strtok overwrites its input.  */
+      char *s = xstrdup (alternate_editor);
+      unsigned toks = 0;
+      char *tok = strtok(s, " ");
+
+      /* Unpack alternate_editor's space-separated tokens into new_argv.  */
+      do
+        {
+          toks++;
+          new_argv = xrealloc (new_argv, new_argv_size + toks * sizeof (char *));
+          new_argv[toks - 1] = tok;
+        }
+      while ((tok = strtok (NULL, " ")));
+
+      /* Append main_argv arguments to new_argv.  */
+      memcpy (&new_argv[toks], main_argv + optind, extra_args_size);
 
-      execvp (alternate_editor, main_argv + i);
+      execvp (s, new_argv);
       message (true, "%s: error executing alternate editor \"%s\"\n",
 	       progname, alternate_editor);
     }
@@ -674,6 +709,7 @@ fail (void)
 int
 main (int argc, char **argv)
 {
+  main_argc = argc;
   main_argv = argv;
   progname = argv[0];
   message (true, "%s: Sorry, the Emacs server is supported only\n"
@@ -1607,6 +1643,7 @@ main (int argc, char **argv)
   int start_daemon_if_needed;
   int exit_status = EXIT_SUCCESS;
 
+  main_argc = argc;
   main_argv = argv;
   progname = argv[0];
 
-- 
2.7.4

bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor

[Eli Zaretskii]

> From: Reuben Thomas <rrt@sc3d.org>
> Date: Thu, 1 Dec 2016 15:31:18 +0000
> 
> The attached patch adds support to emacsclient for command-line options when specifying the alternate
> editor, so that for example one can now say:
> 
> ALTERNATE_EDITOR="emacs -Q -nw"
> 
> I have added documentation to NEWS. It did not seem necessary to change the man page, as nowhere does
> it imply that you can't do this, and I implemented the feature when I found that it didn't work as I expected;
> other similar environment variables with which users may be familiar, such as EDITOR and VISUAL, already
> work like this.

Thanks.

Does this work if -batch is added to the switches?  If so, can you add
a test for this capability?

bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor

[Reuben Thomas]

[-- Attachment #1.1: Type: text/plain, Size: 1482 bytes --]

On 2 December 2016 at 10:20, Eli Zaretskii <eliz@gnu.org> wrote:

> Does this work if -batch is added to the switches?


​It depends what you mean by "work". Since, according to the original
implementation, only non-option are passed to the ALTERNATE_EDITOR,
​nothing useful happens (that I can see), because Emacs is run as

emacs --batch file1 file2 # etc.

and so it starts up and immediately shuts down. If, for example you try to
run

export ALTERNATE_EDITOR="emacs --batch"
emacsclient --eval "(print t)"

then the actual process run is:

emacs --batch "(print t)"

which again does nothing useful. But maybe that's OK, because I am guessing
you ask this because:

If so, can you add
> ​ ​
> a test for this capability?
>

​There seems to be nothing much to check, other than that it returns 0, but
that is already sufficient, since with the current implementation,

export ALTERNATE_EDITOR="emacs --batch"
emacsclient foo

Gives the error:

emacsclient: error executing alternate editor "emacs --batch".

​So, I tried to write a test. It works fine except that it doesn't run the
right emacsclient binary (instead, it runs the one on my PATH).

Since I can't see any other tests that run built executables from the
source tree, I'm not sure how I should go about this (modify PATH?
Explicitly patch in srcdir somehow?). Hints appreciated.

I attach an updated patch with the test so far.

-- 
http://rrt.sc3d.org

[-- Attachment #1.2: Type: text/html, Size: 3816 bytes --]

[-- Attachment #2: 0005-Add-support-for-arguments-in-ALTERNATE_EDITOR-to-ema.patch --]
[-- Type: text/x-patch, Size: 4237 bytes --]

From 20f8b12cb667d99bd62dda338558c797f29f6861 Mon Sep 17 00:00:00 2001
From: Reuben Thomas <rrt@sc3d.org>
Date: Thu, 1 Dec 2016 15:21:57 +0000
Subject: [PATCH 5/5] Add support for arguments in ALTERNATE_EDITOR to
 emacsclient

* lib-src/emacsclient.c (fail): Parse ALTERNATE_EDITOR, or
corresponding command-line argument, into space-separated tokens.
* etc/NEWS: Document.
---
 etc/NEWS              |  4 +++
 lib-src/emacsclient.c | 73 ++++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 59 insertions(+), 18 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 33b8a42..c4f4569 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -268,6 +268,10 @@ variable of this kind to swap modifiers in Emacs.
 ---
 ** New input methods: 'cyrillic-tuvan', 'polish-prefix'.
 
+++
+** emacsclient now accepts command-line options in ALTERNATE_EDITOR
+and --alternate-editor. For example, ALTERNATE_EDITOR="emacs -Q -nw".
+
 \f
 * Editing Changes in Emacs 26.1
 
diff --git a/lib-src/emacsclient.c b/lib-src/emacsclient.c
index 2909d63..b561db6 100644
--- a/lib-src/emacsclient.c
+++ b/lib-src/emacsclient.c
@@ -109,6 +109,9 @@ char *w32_getenv (const char *);
 /* Name used to invoke this program.  */
 const char *progname;
 
+/* The first argument to main.  */
+int main_argc;
+
 /* The second argument to main.  */
 char **main_argv;
 
@@ -192,6 +195,35 @@ xmalloc (size_t size)
   return result;
 }
 
+/* Like realloc but get fatal error if memory is exhausted.  */
+
+static void *
+xrealloc (void *ptr, size_t size)
+{
+  void *result = realloc (ptr, size);
+  if (result == NULL)
+    {
+      perror ("realloc");
+      exit (EXIT_FAILURE);
+    }
+  return result;
+}
+
+/* Like strdup but get a fatal error if memory is exhausted. */
+char *xstrdup (const char *);
+
+char *
+xstrdup (const char *s)
+{
+  char *result = strdup (s);
+  if (result == NULL)
+    {
+      perror ("strdup");
+      exit (EXIT_FAILURE);
+    }
+  return result;
+}
+
 /* From sysdep.c */
 #if !defined (HAVE_GET_CURRENT_DIR_NAME) || defined (BROKEN_GET_CURRENT_DIR_NAME)
 
@@ -255,21 +287,6 @@ get_current_dir_name (void)
 
 #ifdef WINDOWSNT
 
-/* Like strdup but get a fatal error if memory is exhausted. */
-char *xstrdup (const char *);
-
-char *
-xstrdup (const char *s)
-{
-  char *result = strdup (s);
-  if (result == NULL)
-    {
-      perror ("strdup");
-      exit (EXIT_FAILURE);
-    }
-  return result;
-}
-
 #define REG_ROOT "SOFTWARE\\GNU\\Emacs"
 
 char *w32_get_resource (HKEY, const char *, LPDWORD);
@@ -651,7 +668,7 @@ Report bugs with M-x report-emacs-bug.\n");
 }
 
 /* Try to run a different command, or --if no alternate editor is
-   defined-- exit with an errorcode.
+   defined-- exit with an decoderde.
    Uses argv, but gets it from the global variable main_argv.  */
 
 static _Noreturn void
@@ -659,9 +676,27 @@ fail (void)
 {
   if (alternate_editor)
     {
-      int i = optind - 1;
+      size_t extra_args_size = (main_argc - optind + 1) * sizeof (char *);
+      size_t new_argv_size = extra_args_size;
+      char **new_argv = NULL;
+      /* Needed because strtok overwrites its input.  */
+      char *s = xstrdup (alternate_editor);
+      unsigned toks = 0;
+      char *tok = strtok(s, " ");
+
+      /* Unpack alternate_editor's space-separated tokens into new_argv.  */
+      do
+        {
+          toks++;
+          new_argv = xrealloc (new_argv, new_argv_size + toks * sizeof (char *));
+          new_argv[toks - 1] = tok;
+        }
+      while ((tok = strtok (NULL, " ")));
+
+      /* Append main_argv arguments to new_argv.  */
+      memcpy (&new_argv[toks], main_argv + optind, extra_args_size);
 
-      execvp (alternate_editor, main_argv + i);
+      execvp (s, new_argv);
       message (true, "%s: error executing alternate editor \"%s\"\n",
 	       progname, alternate_editor);
     }
@@ -674,6 +709,7 @@ fail (void)
 int
 main (int argc, char **argv)
 {
+  main_argc = argc;
   main_argv = argv;
   progname = argv[0];
   message (true, "%s: Sorry, the Emacs server is supported only\n"
@@ -1607,6 +1643,7 @@ main (int argc, char **argv)
   int start_daemon_if_needed;
   int exit_status = EXIT_SUCCESS;
 
+  main_argc = argc;
   main_argv = argv;
   progname = argv[0];
 
-- 
2.7.4

bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor

[Glenn Morris]

Reuben Thomas wrote:

> The attached patch adds support to emacsclient for command-line options
> when specifying the alternate editor, so that for example one can now say:
>
> ALTERNATE_EDITOR="emacs -Q -nw"

Obvious question: what happens if eg one wants to specify an absolute
file name for the alternate editor, and it contains spaces?

> implemented the feature when I found that it didn't work as I expected;
> other similar environment variables with which users may be familiar, such
> as EDITOR and VISUAL, already work like this.

Do you have a citation for that being how eg EDITOR is intended to work?

bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor

[Reuben Thomas]

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

On 8 December 2016 at 23:10, Glenn Morris <rgm@gnu.org> wrote:

> Reuben Thomas wrote:
>
> > The attached patch adds support to emacsclient for command-line options
> > when specifying the alternate editor, so that for example one can now
> say:
> >
> > ALTERNATE_EDITOR="emacs -Q -nw"
>
> Obvious question: what happens if eg one wants to specify an absolute
> file name for the alternate editor, and it contains spaces?
>

With my current implementation, that wouldn't work. I guess this would be
most likely to be a problem on Windows?

> implemented the feature when I found that it didn't work as I expected;
> > other similar environment variables with which users may be familiar,
> such
> > as EDITOR and VISUAL, already work like this.
>
> Do you have a citation for that being how eg EDITOR is intended to work?
>

​No. The documentation in environ(7) just says:

       EDITOR/VISUAL
              The user's preferred utility to edit text files.

However, I can't find a program that interprets it as a literal filename.

For example, Debian's sensible-editor editor-wrapper script does exactly
what I just did. If I set

export EDITOR='"spacey editor"'

it complains it can't find a program called: "spacey

If I do

export EDITOR="emacsclient -c"

it works nicely. So it doesn't parse quotes specially. However, that's not
cross-platform.

I would not be averse to adding more sophisticated parsing (as I don't
think that would confuse users), but I'd rather not have to write a lot
more error-prone C to achieve that.

I had a look in gnulib but couldn't find anything. If there's some code I
could lift from bash or something, that'd be ideal.

-- 
http://rrt.sc3d.org

[-- Attachment #2: Type: text/html, Size: 3833 bytes --]

bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor

[Reuben Thomas]

I just had another look at my patch for this bug. As part of it, I
wrote a test in test/lib-src/emacsclient-tests.el. It seems that
changes to the test Makefiles since I first wrote it make it fail now
because test/lib-src/emacsclient-tests.log now depends on
lib-src/emacsclient.el. Creating an empty file of this name makes the
tests run again, but presumably the test apparatus should be fixed to
understand that tests can be for a C program?

-- 
https://rrt.sc3d.org

bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor

[Glenn Morris]

Reuben Thomas wrote:

> I just had another look at my patch for this bug. As part of it, I
> wrote a test in test/lib-src/emacsclient-tests.el. It seems that
> changes to the test Makefiles since I first wrote it make it fail now
> because test/lib-src/emacsclient-tests.log now depends on
> lib-src/emacsclient.el. Creating an empty file of this name makes the
> tests run again, but presumably the test apparatus should be fixed to
> understand that tests can be for a C program?

This stuff is nothing to do with me, but look into changing
test_template in test/Makefile.in so that it treats lib-src/ like src/.

bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor

[Reuben Thomas]

On 22/08/17 01:16, Glenn Morris wrote:
> Reuben Thomas wrote:
>
>> I just had another look at my patch for this bug. As part of it, I
>> wrote a test in test/lib-src/emacsclient-tests.el. It seems that
>> changes to the test Makefiles since I first wrote it make it fail now
>> because test/lib-src/emacsclient-tests.log now depends on
>> lib-src/emacsclient.el. Creating an empty file of this name makes the
>> tests run again, but presumably the test apparatus should be fixed to
>> understand that tests can be for a C program?
> This stuff is nothing to do with me, but look into changing
> test_template in test/Makefile.in so that it treats lib-src/ like src/.
Thanks, I hadn't found that. I think I can do that!

-- 
https://rrt.sc3d.org

bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor

[Reuben Thomas]

On 22/08/17 01:23, Reuben Thomas wrote:
> On 22/08/17 01:16, Glenn Morris wrote:
>> Reuben Thomas wrote:
>>
>>> I just had another look at my patch for this bug. As part of it, I
>>> wrote a test in test/lib-src/emacsclient-tests.el. It seems that
>>> changes to the test Makefiles since I first wrote it make it fail now
>>> because test/lib-src/emacsclient-tests.log now depends on
>>> lib-src/emacsclient.el. Creating an empty file of this name makes the
>>> tests run again, but presumably the test apparatus should be fixed to
>>> understand that tests can be for a C program?
>> This stuff is nothing to do with me, but look into changing
>> test_template in test/Makefile.in so that it treats lib-src/ like src/.
> Thanks, I hadn't found that. I think I can do that

I've pushed a fix.

-- 
https://rrt.sc3d.org

bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor

[Reuben Thomas]

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

On 08/12/16 23:10, Glenn Morris wrote:
> Reuben Thomas wrote:
>
>> The attached patch adds support to emacsclient for command-line options
>> when specifying the alternate editor, so that for example one can now say:
>>
>> ALTERNATE_EDITOR="emacs -Q -nw"
> Obvious question: what happens if eg one wants to specify an absolute
> file name for the alternate editor, and it contains spaces?

I've modified my patch to cope with this case. Rather than write a
general escaping parser, or even one that just copes with escaping
quotes (likely to be complex and buggy in C; and I couldn't find code I
could easily lift from elsewhere), I simply allow " as a delimiter as
well as space for quotes. Escaping quotes is not supported.

This suffices to cope with the common case on Windows of needing to
quote a path containing spaces. It also copes with other simple cases.

Is this sufficient?

Updated patch attached.

-- 

https://rrt.sc3d.org


[-- Attachment #2: 0001-Add-support-for-arguments-in-ALTERNATE_EDITOR-to-ema.patch --]
[-- Type: text/x-patch, Size: 7346 bytes --]

From a9f38bcac3ee08f5542ebb4e775a23fff85aa6d5 Mon Sep 17 00:00:00 2001
From: Reuben Thomas <rrt@sc3d.org>
Date: Thu, 1 Dec 2016 15:21:57 +0000
Subject: [PATCH 1/2] Add support for arguments in ALTERNATE_EDITOR to
 emacsclient

* lib-src/emacsclient.c (fail): Parse ALTERNATE_EDITOR, or
corresponding command-line argument, into quote- or space-separated
tokens.  If a token starts with a quote, then it naturally is expected
to end with a quote; escaping is not supported. This is enough to cope
with the typical case of requiring the initial path to be quoted,
common on Windows where it may contain spaces.
* etc/NEWS: Document.
* test/lib-src/emacsclient-tests.el: Add a test.
---
 etc/NEWS                          |  4 ++
 lib-src/emacsclient.c             | 84 ++++++++++++++++++++++++++++++---------
 test/lib-src/emacsclient-tests.el | 50 +++++++++++++++++++++++
 3 files changed, 120 insertions(+), 18 deletions(-)
 create mode 100644 test/lib-src/emacsclient-tests.el

diff --git a/etc/NEWS b/etc/NEWS
index a9e2f5a..a0e001f 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -511,6 +511,10 @@ Linum mode and all similar packages are henceforth becoming obsolete.
 Users and developers are encouraged to switch to this new feature
 instead.
 
++++
+** emacsclient now accepts command-line options in ALTERNATE_EDITOR
+and --alternate-editor. For example, ALTERNATE_EDITOR="emacs -Q -nw".
+
 \f
 * Editing Changes in Emacs 26.1
 
diff --git a/lib-src/emacsclient.c b/lib-src/emacsclient.c
index f1d4e89..5e181cc 100644
--- a/lib-src/emacsclient.c
+++ b/lib-src/emacsclient.c
@@ -110,6 +110,9 @@ char *w32_getenv (const char *);
 /* Name used to invoke this program.  */
 const char *progname;
 
+/* The first argument to main.  */
+int main_argc;
+
 /* The second argument to main.  */
 char **main_argv;
 
@@ -201,6 +204,35 @@ xmalloc (size_t size)
   return result;
 }
 
+/* Like realloc but get fatal error if memory is exhausted.  */
+
+static void *
+xrealloc (void *ptr, size_t size)
+{
+  void *result = realloc (ptr, size);
+  if (result == NULL)
+    {
+      perror ("realloc");
+      exit (EXIT_FAILURE);
+    }
+  return result;
+}
+
+/* Like strdup but get a fatal error if memory is exhausted. */
+char *xstrdup (const char *);
+
+char *
+xstrdup (const char *s)
+{
+  char *result = strdup (s);
+  if (result == NULL)
+    {
+      perror ("strdup");
+      exit (EXIT_FAILURE);
+    }
+  return result;
+}
+
 /* From sysdep.c */
 #if !defined (HAVE_GET_CURRENT_DIR_NAME) || defined (BROKEN_GET_CURRENT_DIR_NAME)
 
@@ -264,21 +296,6 @@ get_current_dir_name (void)
 
 #ifdef WINDOWSNT
 
-/* Like strdup but get a fatal error if memory is exhausted. */
-char *xstrdup (const char *);
-
-char *
-xstrdup (const char *s)
-{
-  char *result = strdup (s);
-  if (result == NULL)
-    {
-      perror ("strdup");
-      exit (EXIT_FAILURE);
-    }
-  return result;
-}
-
 #define REG_ROOT "SOFTWARE\\GNU\\Emacs"
 
 char *w32_get_resource (HKEY, const char *, LPDWORD);
@@ -673,7 +690,7 @@ Report bugs with M-x report-emacs-bug.\n");
 }
 
 /* Try to run a different command, or --if no alternate editor is
-   defined-- exit with an errorcode.
+   defined-- exit with an error code.
    Uses argv, but gets it from the global variable main_argv.  */
 
 static _Noreturn void
@@ -681,9 +698,38 @@ fail (void)
 {
   if (alternate_editor)
     {
-      int i = optind - 1;
+      size_t extra_args_size = (main_argc - optind + 1) * sizeof (char *);
+      size_t new_argv_size = extra_args_size;
+      char **new_argv = NULL;
+      char *s = xstrdup (alternate_editor);
+      unsigned toks = 0;
+
+      /* Unpack alternate_editor's space-separated tokens into new_argv.  */
+      for (char *tok = s; tok != NULL && *tok != '\0';)
+        {
+          /* Allocate new token.  */
+          ++toks;
+          new_argv = xrealloc (new_argv, new_argv_size + toks * sizeof (char *));
+
+          /* Skip leading delimiters, and set separator, skipping any
+             opening quote.  */
+          size_t skip = strspn (tok, " \"");
+          tok += skip;
+          char sep = (skip > 0 && tok[-1] == '"') ? '"' : ' ';
+
+          /* Record start of token.  */
+          new_argv[toks - 1] = tok;
+
+          /* Find end of token and overwrite it with NUL.  */
+          tok = strchr (tok, sep);
+          if (tok != NULL)
+            *tok++ = '\0';
+        }
+
+      /* Append main_argv arguments to new_argv.  */
+      memcpy (&new_argv[toks], main_argv + optind, extra_args_size);
 
-      execvp (alternate_editor, main_argv + i);
+      execvp (*new_argv, new_argv);
       message (true, "%s: error executing alternate editor \"%s\"\n",
 	       progname, alternate_editor);
     }
@@ -696,6 +742,7 @@ fail (void)
 int
 main (int argc, char **argv)
 {
+  main_argc = argc;
   main_argv = argv;
   progname = argv[0];
   message (true, "%s: Sorry, the Emacs server is supported only\n"
@@ -1629,6 +1676,7 @@ main (int argc, char **argv)
   int start_daemon_if_needed;
   int exit_status = EXIT_SUCCESS;
 
+  main_argc = argc;
   main_argv = argv;
   progname = argv[0];
 
diff --git a/test/lib-src/emacsclient-tests.el b/test/lib-src/emacsclient-tests.el
new file mode 100644
index 0000000..ea757f6
--- /dev/null
+++ b/test/lib-src/emacsclient-tests.el
@@ -0,0 +1,50 @@
+;;; emacsclient-tests.el --- Test emacsclient
+
+;; Copyright (C) 2016 Free Software Foundation, Inc.
+
+;; This program is free software; you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; This program is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;
+
+;;; Code:
+
+(require 'ert)
+
+(defconst emacsclient-test-emacs
+  (expand-file-name "emacsclient" (concat
+                                   (file-name-directory
+                                    (directory-file-name
+                                     (file-name-directory invocation-directory)))
+                                   "lib-src"))
+  "Path to emacsclient binary in build tree.")
+
+(ert-deftest emacsclient-test-alternate-editor-allows-arguments ()
+  (let (process-environment process-environment)
+    (setenv "ALTERNATE_EDITOR" (concat
+                                (expand-file-name invocation-name invocation-directory)
+                                " --batch"))
+    (should (= 0 (call-process emacsclient-test-emacs nil nil nil "foo")))))
+
+(ert-deftest emacsclient-test-alternate-editor-allows-quotes ()
+  (let (process-environment process-environment)
+    (setenv "ALTERNATE_EDITOR" (concat
+                                "\""
+                                (expand-file-name invocation-name invocation-directory)
+                                "\"" " --batch"))
+    (should (= 0 (call-process emacsclient-test-emacs nil nil nil "foo")))))
+
+(provide 'emacsclient-tests)
+;;; emacsclient-tests.el ends here
-- 
2.7.4

bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor

[Eli Zaretskii]

> From: Reuben Thomas <rrt@sc3d.org>
> Date: Wed, 23 Aug 2017 23:59:01 +0100
> Cc: 25082@debbugs.gnu.org
> 
> I've modified my patch to cope with this case. Rather than write a
> general escaping parser, or even one that just copes with escaping
> quotes (likely to be complex and buggy in C; and I couldn't find code I
> could easily lift from elsewhere), I simply allow " as a delimiter as
> well as space for quotes. Escaping quotes is not supported.
> 
> This suffices to cope with the common case on Windows of needing to
> quote a path containing spaces. It also copes with other simple cases.
> 
> Is this sufficient?

It could be, but I'm not sure I understand clearly what is supported
and what isn't.  Could you please add this information to the NEWS
entry and in more detail to the user manual?  I think having these
details in the manual is important regardless.

Thanks.

bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor

[Reuben Thomas]

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

On 27 August 2017 at 15:55, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Reuben Thomas <rrt@sc3d.org>
>> Date: Wed, 23 Aug 2017 23:59:01 +0100
>> Cc: 25082@debbugs.gnu.org
>>
>> I've modified my patch to cope with this case. Rather than write a
>> general escaping parser, or even one that just copes with escaping
>> quotes (likely to be complex and buggy in C; and I couldn't find code I
>> could easily lift from elsewhere), I simply allow " as a delimiter as
>> well as space for quotes. Escaping quotes is not supported.
>>
>> This suffices to cope with the common case on Windows of needing to
>> quote a path containing spaces. It also copes with other simple cases.
>>
>> Is this sufficient?
>
> It could be, but I'm not sure I understand clearly what is supported
> and what isn't.  Could you please add this information to the NEWS
> entry and in more detail to the user manual?  I think having these
> details in the manual is important regardless.

I've added detail to NEWS. I am wary of adding more detail to the
manual, because it could prevent future improvements (for example,
implementation of quote escaping): we don't want users to rely on the
lack of quote escaping. The manual currently says:

@table @samp
@item -a @var{command}
@itemx --alternate-editor=@var{command}
Specify a command to run if @code{emacsclient} fails to contact Emacs.
This is useful when running @code{emacsclient} in a script.

Note that this does not document the current situation precisely (a
user could be forgiven for thinking that "emacs -Q -nw" would already
work.

The man page emacsclient(1) is similarly imprecise, though it is a
little different:

.B \-a, \-\-alternate-editor=EDITOR
if the Emacs server is not running, run the specified editor instead.

Here, the use of the word "editor" rather than "command" is more
suggestive of the current behavior. I have changed it to be in line
with the manual. I have also fixed a small error in the man page: it
said

If the value of EDITOR is the empty string, run "emacs \-\-daemon" to
start Emacs in daemon mode, and try to connect to it.

where it should say

If the value of ALTERNATE_EDITOR is the empty string, run "emacs \-\-daemon" to
start Emacs in daemon mode, and try to connect to it.

Updated patch attached.

-- 
https://rrt.sc3d.org

[-- Attachment #2: 0001-Add-support-for-arguments-in-ALTERNATE_EDITOR-to-ema.patch --]
[-- Type: text/x-patch, Size: 8627 bytes --]

From 88bb24fdf57305943f0b9fb8172643ffe452c676 Mon Sep 17 00:00:00 2001
From: Reuben Thomas <rrt@sc3d.org>
Date: Thu, 1 Dec 2016 15:21:57 +0000
Subject: [PATCH] Add support for arguments in ALTERNATE_EDITOR to emacsclient
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* lib-src/emacsclient.c (fail): Parse ALTERNATE_EDITOR, or
corresponding command-line argument, into quote- or space-separated
tokens.  If a token starts with a quote, then it naturally is expected
to end with a quote; escaping is not supported. This is enough to cope
with the typical case of requiring the initial path to be quoted,
common on Windows where it may contain spaces.
* etc/NEWS: Document.
* doc/man/emacsclient.1: Tweak to remove the implication that only an
editor can be specified (the manual already mentions a “command”).
Fix a small error where “EDITOR” is referred to rather than
“ALTERNATE_EDITOR”.
* test/lib-src/emacsclient-tests.el: Add tests.
---
 doc/man/emacsclient.1             |  6 +--
 etc/NEWS                          |  6 +++
 lib-src/emacsclient.c             | 84 ++++++++++++++++++++++++++++++---------
 test/lib-src/emacsclient-tests.el | 50 +++++++++++++++++++++++
 4 files changed, 125 insertions(+), 21 deletions(-)
 create mode 100644 test/lib-src/emacsclient-tests.el

diff --git a/doc/man/emacsclient.1 b/doc/man/emacsclient.1
index 010eeba..0467995 100644
--- a/doc/man/emacsclient.1
+++ b/doc/man/emacsclient.1
@@ -62,10 +62,10 @@ A missing
 is treated as column 1.
 This option applies only to the next file specified.
 .TP
-.B \-a, \-\-alternate-editor=EDITOR
-if the Emacs server is not running, run the specified editor instead.
+.B \-a, \-\-alternate-editor=COMMAND
+if the Emacs server is not running, run the specified command instead.
 This can also be specified via the ALTERNATE_EDITOR environment variable.
-If the value of EDITOR is the empty string, run "emacs \-\-daemon" to
+If the value of ALTERNATE_EDITOR is the empty string, run "emacs \-\-daemon" to
 start Emacs in daemon mode, and try to connect to it.
 .TP
 .B -c, \-\-create-frame
diff --git a/etc/NEWS b/etc/NEWS
index e8d6ea9..f29c942 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -518,6 +518,12 @@ Linum mode and all similar packages are henceforth becoming obsolete.
 Users and developers are encouraged to switch to this new feature
 instead.
 
++++
+** emacsclient now accepts command-line options in ALTERNATE_EDITOR
+and --alternate-editor. For example, ALTERNATE_EDITOR="emacs -Q -nw".
+Arguments may be quoted, so that for example an absolute path
+containing a space may be specified; quote escaping is not supported.
+
 \f
 * Editing Changes in Emacs 26.1
 
diff --git a/lib-src/emacsclient.c b/lib-src/emacsclient.c
index f1d4e89..5e181cc 100644
--- a/lib-src/emacsclient.c
+++ b/lib-src/emacsclient.c
@@ -110,6 +110,9 @@ char *w32_getenv (const char *);
 /* Name used to invoke this program.  */
 const char *progname;
 
+/* The first argument to main.  */
+int main_argc;
+
 /* The second argument to main.  */
 char **main_argv;
 
@@ -201,6 +204,35 @@ xmalloc (size_t size)
   return result;
 }
 
+/* Like realloc but get fatal error if memory is exhausted.  */
+
+static void *
+xrealloc (void *ptr, size_t size)
+{
+  void *result = realloc (ptr, size);
+  if (result == NULL)
+    {
+      perror ("realloc");
+      exit (EXIT_FAILURE);
+    }
+  return result;
+}
+
+/* Like strdup but get a fatal error if memory is exhausted. */
+char *xstrdup (const char *);
+
+char *
+xstrdup (const char *s)
+{
+  char *result = strdup (s);
+  if (result == NULL)
+    {
+      perror ("strdup");
+      exit (EXIT_FAILURE);
+    }
+  return result;
+}
+
 /* From sysdep.c */
 #if !defined (HAVE_GET_CURRENT_DIR_NAME) || defined (BROKEN_GET_CURRENT_DIR_NAME)
 
@@ -264,21 +296,6 @@ get_current_dir_name (void)
 
 #ifdef WINDOWSNT
 
-/* Like strdup but get a fatal error if memory is exhausted. */
-char *xstrdup (const char *);
-
-char *
-xstrdup (const char *s)
-{
-  char *result = strdup (s);
-  if (result == NULL)
-    {
-      perror ("strdup");
-      exit (EXIT_FAILURE);
-    }
-  return result;
-}
-
 #define REG_ROOT "SOFTWARE\\GNU\\Emacs"
 
 char *w32_get_resource (HKEY, const char *, LPDWORD);
@@ -673,7 +690,7 @@ Report bugs with M-x report-emacs-bug.\n");
 }
 
 /* Try to run a different command, or --if no alternate editor is
-   defined-- exit with an errorcode.
+   defined-- exit with an error code.
    Uses argv, but gets it from the global variable main_argv.  */
 
 static _Noreturn void
@@ -681,9 +698,38 @@ fail (void)
 {
   if (alternate_editor)
     {
-      int i = optind - 1;
+      size_t extra_args_size = (main_argc - optind + 1) * sizeof (char *);
+      size_t new_argv_size = extra_args_size;
+      char **new_argv = NULL;
+      char *s = xstrdup (alternate_editor);
+      unsigned toks = 0;
+
+      /* Unpack alternate_editor's space-separated tokens into new_argv.  */
+      for (char *tok = s; tok != NULL && *tok != '\0';)
+        {
+          /* Allocate new token.  */
+          ++toks;
+          new_argv = xrealloc (new_argv, new_argv_size + toks * sizeof (char *));
+
+          /* Skip leading delimiters, and set separator, skipping any
+             opening quote.  */
+          size_t skip = strspn (tok, " \"");
+          tok += skip;
+          char sep = (skip > 0 && tok[-1] == '"') ? '"' : ' ';
+
+          /* Record start of token.  */
+          new_argv[toks - 1] = tok;
+
+          /* Find end of token and overwrite it with NUL.  */
+          tok = strchr (tok, sep);
+          if (tok != NULL)
+            *tok++ = '\0';
+        }
+
+      /* Append main_argv arguments to new_argv.  */
+      memcpy (&new_argv[toks], main_argv + optind, extra_args_size);
 
-      execvp (alternate_editor, main_argv + i);
+      execvp (*new_argv, new_argv);
       message (true, "%s: error executing alternate editor \"%s\"\n",
 	       progname, alternate_editor);
     }
@@ -696,6 +742,7 @@ fail (void)
 int
 main (int argc, char **argv)
 {
+  main_argc = argc;
   main_argv = argv;
   progname = argv[0];
   message (true, "%s: Sorry, the Emacs server is supported only\n"
@@ -1629,6 +1676,7 @@ main (int argc, char **argv)
   int start_daemon_if_needed;
   int exit_status = EXIT_SUCCESS;
 
+  main_argc = argc;
   main_argv = argv;
   progname = argv[0];
 
diff --git a/test/lib-src/emacsclient-tests.el b/test/lib-src/emacsclient-tests.el
new file mode 100644
index 0000000..ea757f6
--- /dev/null
+++ b/test/lib-src/emacsclient-tests.el
@@ -0,0 +1,50 @@
+;;; emacsclient-tests.el --- Test emacsclient
+
+;; Copyright (C) 2016 Free Software Foundation, Inc.
+
+;; This program is free software; you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; This program is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;
+
+;;; Code:
+
+(require 'ert)
+
+(defconst emacsclient-test-emacs
+  (expand-file-name "emacsclient" (concat
+                                   (file-name-directory
+                                    (directory-file-name
+                                     (file-name-directory invocation-directory)))
+                                   "lib-src"))
+  "Path to emacsclient binary in build tree.")
+
+(ert-deftest emacsclient-test-alternate-editor-allows-arguments ()
+  (let (process-environment process-environment)
+    (setenv "ALTERNATE_EDITOR" (concat
+                                (expand-file-name invocation-name invocation-directory)
+                                " --batch"))
+    (should (= 0 (call-process emacsclient-test-emacs nil nil nil "foo")))))
+
+(ert-deftest emacsclient-test-alternate-editor-allows-quotes ()
+  (let (process-environment process-environment)
+    (setenv "ALTERNATE_EDITOR" (concat
+                                "\""
+                                (expand-file-name invocation-name invocation-directory)
+                                "\"" " --batch"))
+    (should (= 0 (call-process emacsclient-test-emacs nil nil nil "foo")))))
+
+(provide 'emacsclient-tests)
+;;; emacsclient-tests.el ends here
-- 
2.7.4

bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor

[Eli Zaretskii]

> From: Reuben Thomas <rrt@sc3d.org>
> Date: Mon, 28 Aug 2017 11:15:55 +0100
> Cc: Glenn Morris <rgm@gnu.org>, 25082@debbugs.gnu.org
> 
> >> Is this sufficient?
> >
> > It could be, but I'm not sure I understand clearly what is supported
> > and what isn't.  Could you please add this information to the NEWS
> > entry and in more detail to the user manual?  I think having these
> > details in the manual is important regardless.
> 
> I've added detail to NEWS. I am wary of adding more detail to the
> manual, because it could prevent future improvements (for example,
> implementation of quote escaping): we don't want users to rely on the
> lack of quote escaping.

We don't want them to rely on the lack of the escaping, but we also
want to tell them what is supported and how.

> @table @samp
> @item -a @var{command}
> @itemx --alternate-editor=@var{command}
> Specify a command to run if @code{emacsclient} fails to contact Emacs.
> This is useful when running @code{emacsclient} in a script.
> 
> Note that this does not document the current situation precisely (a
> user could be forgiven for thinking that "emacs -Q -nw" would already
> work.

Only if we say "shell command", not just "command".

> +Arguments may be quoted, so that for example an absolute path
> +containing a space may be specified; quote escaping is not supported.

I would say `quoted "like this"', since otherwise it isn't clear what
kind of quoting is supported.  And I think something similar needs to
be said in the manual.

> +      /* Unpack alternate_editor's space-separated tokens into new_argv.  */
> +      for (char *tok = s; tok != NULL && *tok != '\0';)
> +        {
> +          /* Allocate new token.  */
> +          ++toks;
> +          new_argv = xrealloc (new_argv, new_argv_size + toks * sizeof (char *));
> +
> +          /* Skip leading delimiters, and set separator, skipping any
> +             opening quote.  */
> +          size_t skip = strspn (tok, " \"");
> +          tok += skip;
> +          char sep = (skip > 0 && tok[-1] == '"') ? '"' : ' ';
> +
> +          /* Record start of token.  */
> +          new_argv[toks - 1] = tok;
> +
> +          /* Find end of token and overwrite it with NUL.  */
> +          tok = strchr (tok, sep);
> +          if (tok != NULL)
> +            *tok++ = '\0';
> +        }
> +
> +      /* Append main_argv arguments to new_argv.  */
> +      memcpy (&new_argv[toks], main_argv + optind, extra_args_size);
>  
> -      execvp (alternate_editor, main_argv + i);
> +      execvp (*new_argv, new_argv);

This won't work on Windows, btw, if the arguments include whitespace.
But that can be fixed by followup changes.

Thanks.

bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor

[Reuben Thomas]

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

On 29 August 2017 at 16:43, Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Reuben Thomas <rrt@sc3d.org>
> > Date: Mon, 28 Aug 2017 11:15:55 +0100
> > Cc: Glenn Morris <rgm@gnu.org>, 25082@debbugs.gnu.org
> >
> > >> Is this sufficient?
> > >
> > > It could be, but I'm not sure I understand clearly what is supported
> > > and what isn't.  Could you please add this information to the NEWS
> > > entry and in more detail to the user manual?  I think having these
> > > details in the manual is important regardless.
> >
> > I've added detail to NEWS. I am wary of adding more detail to the
> > manual, because it could prevent future improvements (for example,
> > implementation of quote escaping): we don't want users to rely on the
> > lack of quote escaping.
>
> We don't want them to rely on the lack of the escaping, but we also
> want to tell them what is supported and how.
>

So just to check, you would view the current lack of documentation of how
the current variable works as something that should ideally be improved?​


> > @table @samp
> > @item -a @var{command}
> > @itemx --alternate-editor=@var{command}
> > Specify a command to run if @code{emacsclient} fails to contact Emacs.
> > This is useful when running @code{emacsclient} in a script.
> >
> > Note that this does not document the current situation precisely (a
> > user could be forgiven for thinking that "emacs -Q -nw" would already
> > work.
>
> Only if we say "shell command", not just "command".
>

I don't think many readers will be so precise. I wasn't.​


> > +Arguments may be quoted, so that for example an absolute path
> > +containing a space may be specified; quote escaping is not supported.
>
> I would say `quoted "like this"', since otherwise it isn't clear what
> kind of quoting is supported.  And I think something similar needs to
> be said in the manual.
>

​OK, I'll do that.​

> +      /* Unpack alternate_editor's space-separated tokens into
> new_argv.  */
> > +      for (char *tok = s; tok != NULL && *tok != '\0';)
> > +        {
> > +          /* Allocate new token.  */
> > +          ++toks;
> > +          new_argv = xrealloc (new_argv, new_argv_size + toks * sizeof
> (char *));
> > +
> > +          /* Skip leading delimiters, and set separator, skipping any
> > +             opening quote.  */
> > +          size_t skip = strspn (tok, " \"");
> > +          tok += skip;
> > +          char sep = (skip > 0 && tok[-1] == '"') ? '"' : ' ';
> > +
> > +          /* Record start of token.  */
> > +          new_argv[toks - 1] = tok;
> > +
> > +          /* Find end of token and overwrite it with NUL.  */
> > +          tok = strchr (tok, sep);
> > +          if (tok != NULL)
> > +            *tok++ = '\0';
> > +        }
> > +
> > +      /* Append main_argv arguments to new_argv.  */
> > +      memcpy (&new_argv[toks], main_argv + optind, extra_args_size);
> >
> > -      execvp (alternate_editor, main_argv + i);
> > +      execvp (*new_argv, new_argv);
>
> This won't work on Windows, btw, if the arguments include whitespace.
> But that can be fixed by followup changes.
>

How not? That is precisely the case it aims to support.​ That's the whole
point of dealing with quotes! (Previous versions of the patch didn't
support quoting, and so didn't support spaces in arguments; this version
has a passing test to show that spaces in quoted arguments work.)

-- 
https://rrt.sc3d.org <http://rrt.sc3d.org>

[-- Attachment #2: Type: text/html, Size: 5399 bytes --]

bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor

[Eli Zaretskii]

> From: Reuben Thomas <rrt@sc3d.org>
> Date: Tue, 29 Aug 2017 16:49:23 +0100
> Cc: Glenn Morris <rgm@gnu.org>, 25082@debbugs.gnu.org
> 
>  > I've added detail to NEWS. I am wary of adding more detail to the
>  > manual, because it could prevent future improvements (for example,
>  > implementation of quote escaping): we don't want users to rely on the
>  > lack of quote escaping.
> 
>  We don't want them to rely on the lack of the escaping, but we also
>  want to tell them what is supported and how.
> 
> So just to check, you would view the current lack of documentation of how the current variable works as
> something that should ideally be improved?​

Yes.

>  > +Arguments may be quoted, so that for example an absolute path
>  > +containing a space may be specified; quote escaping is not supported.
> 
>  I would say `quoted "like this"', since otherwise it isn't clear what
>  kind of quoting is supported. And I think something similar needs to
>  be said in the manual.
> 
> ​OK, I'll do that.​

Thanks.

>  This won't work on Windows, btw, if the arguments include whitespace.
>  But that can be fixed by followup changes.
> 
> How not? That is precisely the case it aims to support.​ That's the whole point of dealing with quotes!
> (Previous versions of the patch didn't support quoting, and so didn't support spaces in arguments; this version
> has a passing test to show that spaces in quoted arguments work.)

Did you try that on MS-Windows?  If you did and it worked for you,
then perhaps I've misread the code.  I didn't actually try running it.

The issue I alluded to is a subtle misfeature in the Windows
implementation of execvp (and similar Posix functions): the arguments
you pass via the argv array get concatenated into a single
command-line string, and that string is passed to the Windows system
API that actually invokes the program.  So argv[] elements that
include whitespace need to be quoted(!) to work correctly on
MS-Windows.  (Of course, this quoting must be ifdef'ed away for Posix
platforms.)

bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor

[Reuben Thomas]

[-- Attachment #1.1: Type: text/plain, Size: 793 bytes --]

On 29 August 2017 at 17:49, Eli Zaretskii <eliz@gnu.org> wrote:

​[snip]

I attach an updated patch with the documentation changes (to manual and
NEWS).​


> The issue I alluded to is a subtle misfeature in the Windows
> implementation of execvp (and similar Posix functions): the arguments
> you pass via the argv array get concatenated into a single
> command-line string, and that string is passed to the Windows system
> API that actually invokes the program.  So argv[] elements that
> include whitespace need to be quoted(!) to work correctly on
> MS-Windows.  (Of course, this quoting must be ifdef'ed away for Posix
> platforms.)
>

​I see, thanks for the explanation. Doesn't w32_execvp handle this already?

-- 
https://rrt.sc3d.org <http://rrt.sc3d.org>

[-- Attachment #1.2: Type: text/html, Size: 1505 bytes --]

[-- Attachment #2: 0001-Add-support-for-arguments-in-ALTERNATE_EDITOR-to-ema.patch --]
[-- Type: text/x-patch, Size: 9448 bytes --]

From bf41bccabc521418591ce012064cc8a265796a82 Mon Sep 17 00:00:00 2001
From: Reuben Thomas <rrt@sc3d.org>
Date: Thu, 1 Dec 2016 15:21:57 +0000
Subject: [PATCH] Add support for arguments in ALTERNATE_EDITOR to emacsclient
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* lib-src/emacsclient.c (fail): Parse ALTERNATE_EDITOR, or
corresponding command-line argument, into quote- or space-separated
tokens.  If a token starts with a quote, then it naturally is expected
to end with a quote; escaping is not supported. This is enough to cope
with the typical case of requiring the initial path to be quoted,
common on Windows where it may contain spaces.
* etc/NEWS: Document.
* doc/emacs/misc.texi: Likewise.
* doc/man/emacsclient.1: Tweak to remove the implication that only an
editor can be specified (the manual already mentions a “command”).
Fix a small error where “EDITOR” is referred to rather than
“ALTERNATE_EDITOR”.
* test/lib-src/emacsclient-tests.el: Add tests.
---
 doc/emacs/misc.texi               |  4 +-
 doc/man/emacsclient.1             |  6 +--
 etc/NEWS                          |  7 ++++
 lib-src/emacsclient.c             | 84 ++++++++++++++++++++++++++++++---------
 test/lib-src/emacsclient-tests.el | 50 +++++++++++++++++++++++
 5 files changed, 129 insertions(+), 22 deletions(-)
 create mode 100644 test/lib-src/emacsclient-tests.el

diff --git a/doc/emacs/misc.texi b/doc/emacs/misc.texi
index 73a6bae..7602fbb 100644
--- a/doc/emacs/misc.texi
+++ b/doc/emacs/misc.texi
@@ -1821,8 +1821,10 @@ emacsclient Options
 @table @samp
 @item -a @var{command}
 @itemx --alternate-editor=@var{command}
-Specify a command to run if @code{emacsclient} fails to contact Emacs.
+Specify a shell command to run if @code{emacsclient} fails to contact Emacs.
 This is useful when running @code{emacsclient} in a script.
+The command may include arguments, which may be quoted "like this".
+Currently, escaping of quotes is not supported.
 
 As a special exception, if @var{command} is the empty string, then
 @code{emacsclient} starts Emacs in daemon mode (as @command{emacs
diff --git a/doc/man/emacsclient.1 b/doc/man/emacsclient.1
index 010eeba..daaacab 100644
--- a/doc/man/emacsclient.1
+++ b/doc/man/emacsclient.1
@@ -62,10 +62,10 @@ A missing
 is treated as column 1.
 This option applies only to the next file specified.
 .TP
-.B \-a, \-\-alternate-editor=EDITOR
-if the Emacs server is not running, run the specified editor instead.
+.B \-a, \-\-alternate-editor=COMMAND
+if the Emacs server is not running, run the specified shell command instead.
 This can also be specified via the ALTERNATE_EDITOR environment variable.
-If the value of EDITOR is the empty string, run "emacs \-\-daemon" to
+If the value of ALTERNATE_EDITOR is the empty string, run "emacs \-\-daemon" to
 start Emacs in daemon mode, and try to connect to it.
 .TP
 .B -c, \-\-create-frame
diff --git a/etc/NEWS b/etc/NEWS
index e8d6ea9..80f6d6b 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -518,6 +518,13 @@ Linum mode and all similar packages are henceforth becoming obsolete.
 Users and developers are encouraged to switch to this new feature
 instead.
 
++++
+** emacsclient now accepts command-line options in ALTERNATE_EDITOR
+and --alternate-editor. For example, ALTERNATE_EDITOR="emacs -Q -nw".
+Arguments may be quoted "like this", so that for example an absolute
+path containing a space may be specified; quote escaping is not
+supported.
+
 \f
 * Editing Changes in Emacs 26.1
 
diff --git a/lib-src/emacsclient.c b/lib-src/emacsclient.c
index f1d4e89..5e181cc 100644
--- a/lib-src/emacsclient.c
+++ b/lib-src/emacsclient.c
@@ -110,6 +110,9 @@ char *w32_getenv (const char *);
 /* Name used to invoke this program.  */
 const char *progname;
 
+/* The first argument to main.  */
+int main_argc;
+
 /* The second argument to main.  */
 char **main_argv;
 
@@ -201,6 +204,35 @@ xmalloc (size_t size)
   return result;
 }
 
+/* Like realloc but get fatal error if memory is exhausted.  */
+
+static void *
+xrealloc (void *ptr, size_t size)
+{
+  void *result = realloc (ptr, size);
+  if (result == NULL)
+    {
+      perror ("realloc");
+      exit (EXIT_FAILURE);
+    }
+  return result;
+}
+
+/* Like strdup but get a fatal error if memory is exhausted. */
+char *xstrdup (const char *);
+
+char *
+xstrdup (const char *s)
+{
+  char *result = strdup (s);
+  if (result == NULL)
+    {
+      perror ("strdup");
+      exit (EXIT_FAILURE);
+    }
+  return result;
+}
+
 /* From sysdep.c */
 #if !defined (HAVE_GET_CURRENT_DIR_NAME) || defined (BROKEN_GET_CURRENT_DIR_NAME)
 
@@ -264,21 +296,6 @@ get_current_dir_name (void)
 
 #ifdef WINDOWSNT
 
-/* Like strdup but get a fatal error if memory is exhausted. */
-char *xstrdup (const char *);
-
-char *
-xstrdup (const char *s)
-{
-  char *result = strdup (s);
-  if (result == NULL)
-    {
-      perror ("strdup");
-      exit (EXIT_FAILURE);
-    }
-  return result;
-}
-
 #define REG_ROOT "SOFTWARE\\GNU\\Emacs"
 
 char *w32_get_resource (HKEY, const char *, LPDWORD);
@@ -673,7 +690,7 @@ Report bugs with M-x report-emacs-bug.\n");
 }
 
 /* Try to run a different command, or --if no alternate editor is
-   defined-- exit with an errorcode.
+   defined-- exit with an error code.
    Uses argv, but gets it from the global variable main_argv.  */
 
 static _Noreturn void
@@ -681,9 +698,38 @@ fail (void)
 {
   if (alternate_editor)
     {
-      int i = optind - 1;
+      size_t extra_args_size = (main_argc - optind + 1) * sizeof (char *);
+      size_t new_argv_size = extra_args_size;
+      char **new_argv = NULL;
+      char *s = xstrdup (alternate_editor);
+      unsigned toks = 0;
+
+      /* Unpack alternate_editor's space-separated tokens into new_argv.  */
+      for (char *tok = s; tok != NULL && *tok != '\0';)
+        {
+          /* Allocate new token.  */
+          ++toks;
+          new_argv = xrealloc (new_argv, new_argv_size + toks * sizeof (char *));
+
+          /* Skip leading delimiters, and set separator, skipping any
+             opening quote.  */
+          size_t skip = strspn (tok, " \"");
+          tok += skip;
+          char sep = (skip > 0 && tok[-1] == '"') ? '"' : ' ';
+
+          /* Record start of token.  */
+          new_argv[toks - 1] = tok;
+
+          /* Find end of token and overwrite it with NUL.  */
+          tok = strchr (tok, sep);
+          if (tok != NULL)
+            *tok++ = '\0';
+        }
+
+      /* Append main_argv arguments to new_argv.  */
+      memcpy (&new_argv[toks], main_argv + optind, extra_args_size);
 
-      execvp (alternate_editor, main_argv + i);
+      execvp (*new_argv, new_argv);
       message (true, "%s: error executing alternate editor \"%s\"\n",
 	       progname, alternate_editor);
     }
@@ -696,6 +742,7 @@ fail (void)
 int
 main (int argc, char **argv)
 {
+  main_argc = argc;
   main_argv = argv;
   progname = argv[0];
   message (true, "%s: Sorry, the Emacs server is supported only\n"
@@ -1629,6 +1676,7 @@ main (int argc, char **argv)
   int start_daemon_if_needed;
   int exit_status = EXIT_SUCCESS;
 
+  main_argc = argc;
   main_argv = argv;
   progname = argv[0];
 
diff --git a/test/lib-src/emacsclient-tests.el b/test/lib-src/emacsclient-tests.el
new file mode 100644
index 0000000..ea757f6
--- /dev/null
+++ b/test/lib-src/emacsclient-tests.el
@@ -0,0 +1,50 @@
+;;; emacsclient-tests.el --- Test emacsclient
+
+;; Copyright (C) 2016 Free Software Foundation, Inc.
+
+;; This program is free software; you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; This program is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;
+
+;;; Code:
+
+(require 'ert)
+
+(defconst emacsclient-test-emacs
+  (expand-file-name "emacsclient" (concat
+                                   (file-name-directory
+                                    (directory-file-name
+                                     (file-name-directory invocation-directory)))
+                                   "lib-src"))
+  "Path to emacsclient binary in build tree.")
+
+(ert-deftest emacsclient-test-alternate-editor-allows-arguments ()
+  (let (process-environment process-environment)
+    (setenv "ALTERNATE_EDITOR" (concat
+                                (expand-file-name invocation-name invocation-directory)
+                                " --batch"))
+    (should (= 0 (call-process emacsclient-test-emacs nil nil nil "foo")))))
+
+(ert-deftest emacsclient-test-alternate-editor-allows-quotes ()
+  (let (process-environment process-environment)
+    (setenv "ALTERNATE_EDITOR" (concat
+                                "\""
+                                (expand-file-name invocation-name invocation-directory)
+                                "\"" " --batch"))
+    (should (= 0 (call-process emacsclient-test-emacs nil nil nil "foo")))))
+
+(provide 'emacsclient-tests)
+;;; emacsclient-tests.el ends here
-- 
2.7.4

bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor

[Eli Zaretskii]

> From: Reuben Thomas <rrt@sc3d.org>
> Date: Tue, 29 Aug 2017 23:29:24 +0100
> Cc: Glenn Morris <rgm@gnu.org>, 25082@debbugs.gnu.org
> 
> I attach an updated patch with the documentation changes (to manual and NEWS).​
LGTM, thanks.

>  The issue I alluded to is a subtle misfeature in the Windows
>  implementation of execvp (and similar Posix functions): the arguments
>  you pass via the argv array get concatenated into a single
>  command-line string, and that string is passed to the Windows system
>  API that actually invokes the program. So argv[] elements that
>  include whitespace need to be quoted(!) to work correctly on
>  MS-Windows. (Of course, this quoting must be ifdef'ed away for Posix
>  platforms.)
> 
> ​I see, thanks for the explanation. Doesn't w32_execvp handle this already?

Indeed, it does.  Thanks for reminding we already had that covered.

bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor

[Reuben Thomas]

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

On 30 August 2017 at 17:48, Eli Zaretskii <eliz@gnu.org> wrote:

> LGTM, thanks.
>

​Great, thanks as ever for your patient supervision. Pushed.​

-- 
https://rrt.sc3d.org <http://rrt.sc3d.org>

[-- Attachment #2: Type: text/html, Size: 716 bytes --]

bug#25082: Patch installed

[Reuben Thomas]

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

Patch installed, closing.

-- 
https://rrt.sc3d.org <http://rrt.sc3d.org>

[-- Attachment #2: Type: text/html, Size: 304 bytes --]