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

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

[Daniel Shahaf]

Reference: window_pane_spawn() in tmux 2.5.

Also, document _cmdstring and _precommand.
---
Re-sending with the new file included.

Cheers,

Daniel

 Completion/Unix/Command/_tmux       |  2 +-
 Completion/Unix/Type/_cmdambivalent |  9 +++++++++
 Doc/Zsh/compsys.yo                  | 16 ++++++++++++++++
 3 files changed, 26 insertions(+), 1 deletion(-)
 create mode 100644 Completion/Unix/Type/_cmdambivalent

diff --git a/Completion/Unix/Command/_tmux b/Completion/Unix/Command/_tmux
index 0917da604..5f5bea922 100644
--- a/Completion/Unix/Command/_tmux
+++ b/Completion/Unix/Command/_tmux
@@ -526,7 +526,7 @@ _tmux-new-session() {
     '-t+[specify target session]:session:__tmux-sessions' \
     '-x[specify width]:width' \
     '-y[specify height]:height' \
-    '*:: :_cmdstring'
+    '*:: :_cmdambivalent'
 }
 
 _tmux-new-window() {
diff --git a/Completion/Unix/Type/_cmdambivalent b/Completion/Unix/Type/_cmdambivalent
new file mode 100644
index 000000000..04824e3a5
--- /dev/null
+++ b/Completion/Unix/Type/_cmdambivalent
@@ -0,0 +1,9 @@
+#autoload
+
+if (( CURRENT == 1 && ${#words} == 1 )); then
+  _cmdstring
+elif (( CURRENT == 1 )); then
+  _command_names -e
+else
+  _normal
+fi
diff --git a/Doc/Zsh/compsys.yo b/Doc/Zsh/compsys.yo
index 47b07e85e..47c30843c 100644
--- a/Doc/Zsh/compsys.yo
+++ b/Doc/Zsh/compsys.yo
@@ -3014,6 +3014,17 @@ tt(-n), tt(-F), tt(-X) are passed to tt(compadd).
 
 See tt(_description) for a description of var(tag) and var(descr).
 )
+findex(_cmdambivalent)
+item(tt(_cmdambivalent))(
+Completes an external command.
+If there is a single argument, complete the command in a single word, like tt(_cmdstring);
+otherwise, complete the command in word-separated arguments, like tt(_precommand).
+)
+findex(_cmdstring)
+item(tt(_cmdstring))(
+Completes an external command as a single argument, as for
+tt(system+LPAR()...+RPAR()).
+)
 findex(_complete)
 item(tt(_complete))(
 This completer generates all possible completions in a context-sensitive
@@ -3214,6 +3225,11 @@ tt(old-menu), see
 ifzman(the section `Completion System Configuration' above)\
 ifnzman(noderef(Completion System Configuration)).
 )
+findex(_precommand)
+item(tt(_precommand))(
+Complete an external command in word-separated arguments, as for
+tt(exec) and tt(/usr/bin/env).
+)
 findex(_prefix)
 item(tt(_prefix))(
 This completer can be used to try completion with the suffix (everything

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

[Bart Schaefer]

On Thu, Jul 20, 2017 at 2:06 PM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> +item(tt(_cmdambivalent))(
> +Completes an external command.
> +If there is a single argument, complete the command in a single word, like tt(_cmdstring);
> +otherwise, complete the command in word-separated arguments, like tt(_precommand).
> +)

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".

Which in turn means this can only be used for argument-rest contexts, correct?

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

[Daniel Shahaf]

Bart Schaefer wrote on Thu, 20 Jul 2017 20:12 -0700:
> 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!

> Which in turn means this can only be used for argument-rest contexts, correct?

It is designed for this use-case, yes.

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

[Oliver Kiddle]

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!

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.

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.

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.

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.

Oliver

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

[Daniel Shahaf]

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

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

[Oliver Kiddle]

Daniel Shahaf wrote:
> 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))

I'd just say that
   the external command and its arguments are completed as separate [positional] arguments
"word-separated" is a confusing term as "words" are not the separators.

> 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.
Fine aside from the otherwise typo.

> See also tt(_cmdstring), tt(_cmdrest), and tt(_precommand).
Assuming those three exist both in form and documentation.`

> 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.)

Yes, that makes sense.

> Why not document _precommand?  Firstly, documenting it would help

> 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

We currently document functions in three sections:
  CONTROL FUNCTIONS
  BINDABLE COMMANDS
  UTILITY FUNCTIONS
_precommand's primary purpose is not really as a utility function but
any function can be treated as one. Maybe we could add another section
or subdivide as you suggest. Certainly grouping all the tags/labels
functions together might be an improvement.

Oliver

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

[Daniel Shahaf]

Oliver Kiddle wrote on Tue, 25 Jul 2017 07:57 +0200:
> Daniel Shahaf wrote:
> > Why not document _precommand?  Firstly, documenting it would help
> 
> > 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
> 
> We currently document functions in three sections:
>   CONTROL FUNCTIONS
>   BINDABLE COMMANDS
>   UTILITY FUNCTIONS
> _precommand's primary purpose is not really as a utility function but
> any function can be treated as one.

I see.  However, I think it's better to have it documented than
undocumented, and the "Utility functions" is the place where it fits
best among the three existing options, so I think to add it there.  It
can move to another (sub)section when such is added.

Agreed with your other (snipped) points.

Cheers,

Daniel