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

[Reuben Thomas <rrt@sc3d.org>]

[-- 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 --]