Re: [PATCH v2] Correct completion of 'tmux new <TAB>'.

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

Oliver Kiddle wrote on Sun, 23 Jul 2017 09:48 +0200:
> Daniel Shahaf wrote:
> > > I believe I understand what this is doing, but "if there is a single
> > > argument" isn't entirely clear.  It does NOT mean "if there is a
> > > single positional parameter in the call to this function" (which is
> > > how I first read it, before I actually looked at the function
> > > definition); rather it means "if there is more than one word in
> > > argument position on the command line".
> > > 
> >
> > I'll change to your wording before committing.  Thanks for the
> > blind review!
> > 
> > +item(tt(_cmdambivalent))(
> > +Completes an external command.
> > +If there is more than one word in argument position on the command line, complete the command in a single word, like tt(_cmdstring);
> > +otherwise, complete the command in word-separated arguments, like tt(_precommand).
> > +)
> 
> For the documentation wouldn't it perhaps be better to describe the
> basic use first. i.e. it is for commands that are ambivalent about
> taking either a quoted command-string or a command and series of
> arguments.

How about:

item(tt(_cmdambivalent))(
Completes the remaining positional arguments as an external command.
The external command is completed as word-separated arguments (in the manner of tt(/usr/bin/env))
if there are two or more remaining positional arguments on the command line,
and as a quoted command string (in the manner of tt(system+LPAR()...+RPAR())) othrewise.
See also tt(_cmdstring), tt(_cmdrest), and tt(_precommand).

This function takes no arguments.
)

> Your implementation will favour the _cmdstring variant in the sense that
> completing a command-name will give you a quoted space as the suffix.
> I think it would be preferable to check if the first and only argument
> contains any spaces before switching to the _cmdstring variant and so
> avoid unnecessary quoting.

Makes sense.  In addition to checking for spaces, we could also check
for an opening quote.  (An opening quote is more likely to signify a
quoted shell command than an argv[0] with spaces.)

> We might also want to consider the many cases where we use the following
> _arguments idiom:
> 
>   '(-):command:_command_names -e' \
>   '*::args:_normal'
> 
> This also appears in if .. then .. else form in _strace and _socket.
> It might be good to cover this with a single helper - _cmdrest perhaps
> - but note that _precommand is not for this purpose: it completes
> functions and aliases. I'm not convinced that _precommand should be
> documented in this section at all. It wasn't really meant as a helper
> but as a as a catch-all handler for the zsh precommand modifiers. Note
> that it is in Zsh/Command.

Why not document _precommand?  Firstly, documenting it would help
clarify that completes not only external commands but also functions and
aliases.  Secondly, it could easily be useful to users.  (I generally
write completion functions for my custom zshrc functions; I can easily
imagine myself writing some precommandish thing and wanting to have
completion after it.)

> I see a precommands array was added around 2009 in _precommand to
> facilitate _calendar's check to see if it was run with command. What
> do we want to include in this. It might be useful to collect all the
> wrapper commands and not just zsh precommand modifiers. It is somewhat
> similar to the _comp_priv_prefix that we added more recently.

Not quite what you meant to say, I think, but having that section of the
documentation grouped by logical function would make a lot of sense.
Right now, anyone who tries to understand _tags by RTFM'ing needs to
scroll back and forth between _tags, _requested, _wanted, and I forget
what else; they're pages away from each other.

> Oliver

Cheers,

Daniel