* bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor @ 2016-12-01 15:31 Reuben Thomas 2016-12-02 10:20 ` Eli Zaretskii ` (2 more replies) 0 siblings, 3 replies; 19+ messages in thread From: Reuben Thomas @ 2016-12-01 15:31 UTC (permalink / raw) To: 25082 [-- 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor 2016-12-01 15:31 bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor Reuben Thomas @ 2016-12-02 10:20 ` Eli Zaretskii 2016-12-02 15:31 ` Reuben Thomas 2016-12-08 23:10 ` Glenn Morris 2017-08-30 21:02 ` bug#25082: Patch installed Reuben Thomas 2 siblings, 1 reply; 19+ messages in thread From: Eli Zaretskii @ 2016-12-02 10:20 UTC (permalink / raw) To: Reuben Thomas; +Cc: 25082 > 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? ^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor 2016-12-02 10:20 ` Eli Zaretskii @ 2016-12-02 15:31 ` Reuben Thomas 0 siblings, 0 replies; 19+ messages in thread From: Reuben Thomas @ 2016-12-02 15:31 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 25082 [-- 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor 2016-12-01 15:31 bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor Reuben Thomas 2016-12-02 10:20 ` Eli Zaretskii @ 2016-12-08 23:10 ` Glenn Morris 2016-12-09 13:44 ` Reuben Thomas 2017-08-23 22:59 ` Reuben Thomas 2017-08-30 21:02 ` bug#25082: Patch installed Reuben Thomas 2 siblings, 2 replies; 19+ messages in thread From: Glenn Morris @ 2016-12-08 23:10 UTC (permalink / raw) To: Reuben Thomas; +Cc: 25082 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? ^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor 2016-12-08 23:10 ` Glenn Morris @ 2016-12-09 13:44 ` Reuben Thomas 2017-08-20 21:08 ` Reuben Thomas 2017-08-23 22:59 ` Reuben Thomas 1 sibling, 1 reply; 19+ messages in thread From: Reuben Thomas @ 2016-12-09 13:44 UTC (permalink / raw) To: Glenn Morris; +Cc: 25082 [-- 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 --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor 2016-12-09 13:44 ` Reuben Thomas @ 2017-08-20 21:08 ` Reuben Thomas 2017-08-22 0:16 ` Glenn Morris 0 siblings, 1 reply; 19+ messages in thread From: Reuben Thomas @ 2017-08-20 21:08 UTC (permalink / raw) To: Glenn Morris; +Cc: 25082 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor 2017-08-20 21:08 ` Reuben Thomas @ 2017-08-22 0:16 ` Glenn Morris 2017-08-22 0:23 ` Reuben Thomas 0 siblings, 1 reply; 19+ messages in thread From: Glenn Morris @ 2017-08-22 0:16 UTC (permalink / raw) To: Reuben Thomas; +Cc: 25082 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/. ^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor 2017-08-22 0:16 ` Glenn Morris @ 2017-08-22 0:23 ` Reuben Thomas 2017-08-22 0:52 ` Reuben Thomas 0 siblings, 1 reply; 19+ messages in thread From: Reuben Thomas @ 2017-08-22 0:23 UTC (permalink / raw) To: Glenn Morris; +Cc: 25082 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor 2017-08-22 0:23 ` Reuben Thomas @ 2017-08-22 0:52 ` Reuben Thomas 0 siblings, 0 replies; 19+ messages in thread From: Reuben Thomas @ 2017-08-22 0:52 UTC (permalink / raw) To: Glenn Morris; +Cc: 25082 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor 2016-12-08 23:10 ` Glenn Morris 2016-12-09 13:44 ` Reuben Thomas @ 2017-08-23 22:59 ` Reuben Thomas 2017-08-27 14:55 ` Eli Zaretskii 1 sibling, 1 reply; 19+ messages in thread From: Reuben Thomas @ 2017-08-23 22:59 UTC (permalink / raw) To: Glenn Morris; +Cc: 25082 [-- 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor 2017-08-23 22:59 ` Reuben Thomas @ 2017-08-27 14:55 ` Eli Zaretskii 2017-08-28 10:15 ` Reuben Thomas 0 siblings, 1 reply; 19+ messages in thread From: Eli Zaretskii @ 2017-08-27 14:55 UTC (permalink / raw) To: Reuben Thomas; +Cc: 25082 > 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor 2017-08-27 14:55 ` Eli Zaretskii @ 2017-08-28 10:15 ` Reuben Thomas 2017-08-29 15:43 ` Eli Zaretskii 0 siblings, 1 reply; 19+ messages in thread From: Reuben Thomas @ 2017-08-28 10:15 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 25082 [-- 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor 2017-08-28 10:15 ` Reuben Thomas @ 2017-08-29 15:43 ` Eli Zaretskii 2017-08-29 15:49 ` Reuben Thomas 0 siblings, 1 reply; 19+ messages in thread From: Eli Zaretskii @ 2017-08-29 15:43 UTC (permalink / raw) To: Reuben Thomas; +Cc: 25082 > 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor 2017-08-29 15:43 ` Eli Zaretskii @ 2017-08-29 15:49 ` Reuben Thomas 2017-08-29 16:49 ` Eli Zaretskii 0 siblings, 1 reply; 19+ messages in thread From: Reuben Thomas @ 2017-08-29 15:49 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 25082 [-- 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 --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor 2017-08-29 15:49 ` Reuben Thomas @ 2017-08-29 16:49 ` Eli Zaretskii 2017-08-29 22:29 ` Reuben Thomas 0 siblings, 1 reply; 19+ messages in thread From: Eli Zaretskii @ 2017-08-29 16:49 UTC (permalink / raw) To: Reuben Thomas; +Cc: 25082 > 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.) ^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor 2017-08-29 16:49 ` Eli Zaretskii @ 2017-08-29 22:29 ` Reuben Thomas 2017-08-30 16:48 ` Eli Zaretskii 0 siblings, 1 reply; 19+ messages in thread From: Reuben Thomas @ 2017-08-29 22:29 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 25082 [-- 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor 2017-08-29 22:29 ` Reuben Thomas @ 2017-08-30 16:48 ` Eli Zaretskii 2017-08-30 21:01 ` Reuben Thomas 0 siblings, 1 reply; 19+ messages in thread From: Eli Zaretskii @ 2017-08-30 16:48 UTC (permalink / raw) To: Reuben Thomas; +Cc: 25082 > 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor 2017-08-30 16:48 ` Eli Zaretskii @ 2017-08-30 21:01 ` Reuben Thomas 0 siblings, 0 replies; 19+ messages in thread From: Reuben Thomas @ 2017-08-30 21:01 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 25082 [-- 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 --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#25082: Patch installed 2016-12-01 15:31 bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor Reuben Thomas 2016-12-02 10:20 ` Eli Zaretskii 2016-12-08 23:10 ` Glenn Morris @ 2017-08-30 21:02 ` Reuben Thomas 2 siblings, 0 replies; 19+ messages in thread From: Reuben Thomas @ 2017-08-30 21:02 UTC (permalink / raw) To: 25082-done [-- 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 --] ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2017-08-30 21:02 UTC | newest] Thread overview: 19+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-12-01 15:31 bug#25082: [PATCH] Add support to emacsclient for command-lline options in ALTERNATE_EDITOR/--alternate-editor Reuben Thomas 2016-12-02 10:20 ` Eli Zaretskii 2016-12-02 15:31 ` Reuben Thomas 2016-12-08 23:10 ` Glenn Morris 2016-12-09 13:44 ` Reuben Thomas 2017-08-20 21:08 ` Reuben Thomas 2017-08-22 0:16 ` Glenn Morris 2017-08-22 0:23 ` Reuben Thomas 2017-08-22 0:52 ` Reuben Thomas 2017-08-23 22:59 ` Reuben Thomas 2017-08-27 14:55 ` Eli Zaretskii 2017-08-28 10:15 ` Reuben Thomas 2017-08-29 15:43 ` Eli Zaretskii 2017-08-29 15:49 ` Reuben Thomas 2017-08-29 16:49 ` Eli Zaretskii 2017-08-29 22:29 ` Reuben Thomas 2017-08-30 16:48 ` Eli Zaretskii 2017-08-30 21:01 ` Reuben Thomas 2017-08-30 21:02 ` bug#25082: Patch installed Reuben Thomas
Code repositories for project(s) associated with this external index https://git.savannah.gnu.org/cgit/emacs.git https://git.savannah.gnu.org/cgit/emacs/org-mode.git This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.