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

[Reuben Thomas]

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