[PATCH] _git: Add completion for help subcommand

[PATCH] _git: Add completion for help subcommand

[Aaron Schrab]

---
 Completion/Unix/Command/_git |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/Completion/Unix/Command/_git b/Completion/Unix/Command/_git
index c394e08..50b503a 100644
--- a/Completion/Unix/Command/_git
+++ b/Completion/Unix/Command/_git
@@ -190,6 +190,7 @@ _git_commands () {
     'fetch:download objects and a head from another repository'
     'gc:cleanup unnecessary files and optimize the local repository'
     'grep:print lines matching a pattern'
+    'help:display help information about git subcommands'
     'init:create empty git object database'
     'log:show commit logs'
     'merge:grand unified merge driver'
@@ -490,6 +491,19 @@ _git-hash-object () {
     '(--stdin):file:_files' && ret=0
 }
 
+(( $+functions[_git-help] )) ||
+_git-help () {
+  local formats='(--info -i --man -m --web -w)'
+  _arguments -S \
+    - '(all)' \
+    {--all,-a}'[List all available commands]' \
+    - 'format' \
+    "$formats"{--man,-m}'[Display help in man page format]' \
+    "$formats"{--info,-i}'[Display help in info format]' \
+    "$formats"{--web,-w}'[Display help in web browser]' \
+    ':command:_git_commands' && ret=0
+}
+
 (( $+functions[_git-index-pack] )) ||
 _git-index-pack () {
   local -a stdin_arguments
-- 
1.7.1

Re: [PATCH] _git: Add completion for help subcommand

[Nikolai Weibull]

On Sat, Aug 21, 2010 at 14:21, Aaron Schrab <aaron@schrab.com> wrote:

Seems fine except for this:

> +  local formats='(--info -i --man -m --web -w)'

Although this is sort of nice it doesn’t follow the style of this or
other completion files, so please write out the whole list in each
line.

Re: [PATCH] _git: Add completion for help subcommand

[Benjamin R. Haskell]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1108 bytes --]

On Thu, 26 Aug 2010, Nikolai Weibull wrote:

> On Sat, Aug 21, 2010 at 14:21, Aaron Schrab wrote:
> 
> Seems fine except for this:
> 
> > +  local formats='(--info -i --man -m --web -w)'
> 
> Although this is sort of nice it doesn’t follow the style of this or 
> other completion files, so please write out the whole list in each 
> line.

Doesn't seem very common, but a quick grep[1] of Completion/Unix/Command 
shows at least the following use that style to an extent:

_bogoutil
_cvs
_global
_iconv
_whois
_wiggle

It always kind of bothered me that more didn't.

e.g. the lines in _git starting:

'(--name-only --name-status -u -p --stat --patch-with-stat [...]
'(--name-only --name-status -u -p --stat --patch-with-stat [...]
'(--name-only --name-status -u -p --stat --patch-with-stat [...]
'(--name-only --name-status -u -p --stat --patch-with-stat [...]
'(--name-only --name-status -u -p --stat --patch-with-stat [...]

It's just visual clutter.  Is there a particular reason not to use a 
variable?

-- 
Best,
Ben

[1] many false positives, but:
grep $'^\\s*["\'].*\\$' Completion/Unix/Command/*

Re: [PATCH] _git: Add completion for help subcommand

[Nikolai Weibull]

On Thu, Aug 26, 2010 at 05:29, Benjamin R. Haskell <zsh@benizi.com> wrote:
> On Thu, 26 Aug 2010, Nikolai Weibull wrote:
>
>> On Sat, Aug 21, 2010 at 14:21, Aaron Schrab wrote:
>>
>> Seems fine except for this:
>>
>> > +  local formats='(--info -i --man -m --web -w)'
>>
>> Although this is sort of nice it doesn’t follow the style of this or
>> other completion files, so please write out the whole list in each
>> line.

> It's just visual clutter.  Is there a particular reason not to use a
> variable?

Perhaps.  The common pattern I saw when I began writing completion functions was

(    --b --c)--a
(--a     --c)--b
(--a --b    )--c

I’d really appreciate some kind of vote or input on this.  If we
decide on using variables they should be used throughout.

Re: _git: Add completion for help subcommand

[Aaron Schrab]

At 00:16 +0200 26 Aug 2010, Nikolai Weibull <now@bitwi.se> wrote:
>On Sat, Aug 21, 2010 at 14:21, Aaron Schrab <aaron@schrab.com> wrote:
>
>Seems fine except for this:
>
>> +  local formats='(--info -i --man -m --web -w)'
>
>Although this is sort of nice it doesn’t follow the style of this or
>other completion files, so please write out the whole list in each
>line.

I think that results in unnecessary duplication, but I guess the list 
isn't big enough or likely to change often enough for that to be a major 
issue.

So here's a version with the requested modification.

---
 Completion/Unix/Command/_git |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/Completion/Unix/Command/_git b/Completion/Unix/Command/_git
index c394e08..bccd43a 100644
--- a/Completion/Unix/Command/_git
+++ b/Completion/Unix/Command/_git
@@ -190,6 +190,7 @@ _git_commands () {
     'fetch:download objects and a head from another repository'
     'gc:cleanup unnecessary files and optimize the local repository'
     'grep:print lines matching a pattern'
+    'help:display help information about git subcommands'
     'init:create empty git object database'
     'log:show commit logs'
     'merge:grand unified merge driver'
@@ -490,6 +491,18 @@ _git-hash-object () {
     '(--stdin):file:_files' && ret=0
 }
 
+(( $+functions[_git-help] )) ||
+_git-help () {
+  _arguments -S \
+    - '(all)' \
+    {--all,-a}'[List all available commands]' \
+    - 'format' \
+    "(--info -i --man -m --web -w)"{--man,-m}'[Display help in man page format]' \
+    "(--info -i --man -m --web -w)"{--info,-i}'[Display help in info format]' \
+    "(--info -i --man -m --web -w)"{--web,-w}'[Display help in web browser]' \
+    ':command:_git_commands' && ret=0
+}
+
 (( $+functions[_git-index-pack] )) ||
 _git-index-pack () {
   local -a stdin_arguments
-- 
1.7.1

Re: _git: Add completion for help subcommand

[Nikolai Weibull]

On Fri, Aug 27, 2010 at 02:20, Aaron Schrab <aaron@schrab.com> wrote:
> At 00:16 +0200 26 Aug 2010, Nikolai Weibull <now@bitwi.se> wrote:
>>
>> On Sat, Aug 21, 2010 at 14:21, Aaron Schrab <aaron@schrab.com> wrote:
>>
>> Seems fine except for this:
>>
>>> +  local formats='(--info -i --man -m --web -w)'
>>
>> Although this is sort of nice it doesn’t follow the style of this or
>> other completion files, so please write out the whole list in each
>> line.
>
> I think that results in unnecessary duplication, but I guess the list isn't
> big enough or likely to change often enough for that to be a major issue.

Actually, why not use

- '(formats)' \,

explicitly making the formats mutually exclusive?

Re: _git: Add completion for help subcommand

[Aaron Schrab]

At 10:29 +0200 27 Aug 2010, Nikolai Weibull <now@bitwi.se> wrote:
>Actually, why not use
>
>- '(formats)' \,
>
>explicitly making the formats mutually exclusive?

I hadn't done that previously because as I was conceiving the issue, I 
saw the argument to specifyg the command for which help was desired as 
part of that set as well, and using that syntax would make that part of 
the mutually exclusive set.  I've now rethought that, and have moved the 
command argument into the common options, allowing use of that syntax.

Here's a revised version of the patch.

---
  Completion/Unix/Command/_git |   14 ++++++++++++++
  1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/Completion/Unix/Command/_git b/Completion/Unix/Command/_git
index c394e08..5fc0765 100644
--- a/Completion/Unix/Command/_git
+++ b/Completion/Unix/Command/_git
@@ -190,6 +190,7 @@ _git_commands () {
      'fetch:download objects and a head from another repository'
      'gc:cleanup unnecessary files and optimize the local repository'
      'grep:print lines matching a pattern'
+    'help:display help information about git subcommands'
      'init:create empty git object database'
      'log:show commit logs'
      'merge:grand unified merge driver'
@@ -490,6 +491,19 @@ _git-hash-object () {
      '(--stdin):file:_files' && ret=0
  }
  
+(( $+functions[_git-help] )) ||
+_git-help () {
+  _arguments -S \
+    ':command:_git_commands' \
+    - '(all)' \
+    '(*)'{--all,-a}'[List all available commands]' \
+    - '(format)' \
+    {--man,-m}'[Display help in man page format]' \
+    {--info,-i}'[Display help in info format]' \
+    {--web,-w}'[Display help in web browser]' \
+    && ret=0
+}
+
  (( $+functions[_git-index-pack] )) ||
  _git-index-pack () {
    local -a stdin_arguments
-- 
1.7.0.4

Re: _git: Add completion for help subcommand

[Nikolai Weibull]

On Sat, Aug 28, 2010 at 01:18, Aaron Schrab <aaron@schrab.com> wrote:
> At 10:29 +0200 27 Aug 2010, Nikolai Weibull <now@bitwi.se> wrote:
>>
>> Actually, why not use
>>
>> - '(formats)' \,
>>
>> explicitly making the formats mutually exclusive?

> Here's a revised version of the patch.

A final nit-pick: Why put --all in a separate group?

Re: _git: Add completion for help subcommand

[Aaron Schrab]

At 01:43 +0200 28 Aug 2010, Nikolai Weibull <now@bitwi.se> wrote:
>A final nit-pick: Why put --all in a separate group?

Because it supersedes any other option.  I suppose it could be put into 
the common options, with other options excluded and the other options 
excluding it.  But if that's going to be the preferred way of handling 
this type of situation, what's the point of having support for mutually 
exclusive sets of options?

But, I did notice that there was an error in the previous version of the 
patch.  It didn't actually prevent completion of command names with 
--all.  I'd been testing a copy of the file other than the one I was 
modifying.  Revised, and hopefully final, version below.

 From e87683251e5c5c5737a118031582ee4500bac4ee Mon Sep 17 00:00:00 2001
From: Aaron Schrab <aaron@schrab.com>
Date: Thu, 19 Aug 2010 19:11:06 -0400
Subject: [PATCH] _git: Add completion for help subcommand

---
  Completion/Unix/Command/_git |   14 ++++++++++++++
  1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/Completion/Unix/Command/_git b/Completion/Unix/Command/_git
index c394e08..570d08f 100644
--- a/Completion/Unix/Command/_git
+++ b/Completion/Unix/Command/_git
@@ -190,6 +190,7 @@ _git_commands () {
      'fetch:download objects and a head from another repository'
      'gc:cleanup unnecessary files and optimize the local repository'
      'grep:print lines matching a pattern'
+    'help:display help information about git subcommands'
      'init:create empty git object database'
      'log:show commit logs'
      'merge:grand unified merge driver'
@@ -490,6 +491,19 @@ _git-hash-object () {
      '(--stdin):file:_files' && ret=0
  }
  
+(( $+functions[_git-help] )) ||
+_git-help () {
+  _arguments -S \
+    ':command:_git_commands' \
+    - '(all)' \
+    '(:)'{--all,-a}'[List all available commands]' \
+    - '(format)' \
+    {--man,-m}'[Display help in man page format]' \
+    {--info,-i}'[Display help in info format]' \
+    {--web,-w}'[Display help in web browser]' \
+    && ret=0
+}
+
  (( $+functions[_git-index-pack] )) ||
  _git-index-pack () {
    local -a stdin_arguments
-- 
1.7.0.4

Re: _git: Add completion for help subcommand

[Nikolai Weibull]

On Sat, Aug 28, 2010 at 02:51, Aaron Schrab <aaron@schrab.com> wrote:
> At 01:43 +0200 28 Aug 2010, Nikolai Weibull <now@bitwi.se> wrote:
>>
>> A final nit-pick: Why put --all in a separate group?
>
> Because it supersedes any other option.

All you need to do is put (- :) before --all instead and all options
and arguments are excluded.

Re: _git: Add completion for help subcommand

[Aaron Schrab]

At 09:54 +0200 28 Aug 2010, Nikolai Weibull <now@bitwi.se> wrote:
>On Sat, Aug 28, 2010 at 02:51, Aaron Schrab <aaron@schrab.com> wrote:
>> At 01:43 +0200 28 Aug 2010, Nikolai Weibull <now@bitwi.se> wrote:
>>>
>>> A final nit-pick: Why put --all in a separate group?
>>
>> Because it supersedes any other option.
>
>All you need to do is put (- :) before --all instead and all options
>and arguments are excluded.

No, I'd also need to put (--all -a) before all of the other options to 
replicate having it in a separate group.  Which I certainly could do.  
But I'll again ask why have that support for groups in _arguments, but 
then object to using it?

Re: _git: Add completion for help subcommand

[Nikolai Weibull]

On Sat, Aug 28, 2010 at 16:16, Aaron Schrab <aaron@schrab.com> wrote:
> At 09:54 +0200 28 Aug 2010, Nikolai Weibull <now@bitwi.se> wrote:
>>
>> On Sat, Aug 28, 2010 at 02:51, Aaron Schrab <aaron@schrab.com> wrote:
>>>
>>> At 01:43 +0200 28 Aug 2010, Nikolai Weibull <now@bitwi.se> wrote:
>>>>
>>>> A final nit-pick: Why put --all in a separate group?
>>>
>>> Because it supersedes any other option.
>>
>> All you need to do is put (- :) before --all instead and all options
>> and arguments are excluded.
>
> No, I'd also need to put (--all -a) before all of the other options to
> replicate having it in a separate group.  Which I certainly could do.

Hm, true, my bad.

> But I'll again ask why have that support for groups in _arguments, but then
> object to using it?

Hm, I don’t know what you mean.

Your last patch now seems optimal.  Thank you for having the patience
to deal with my input.

Re: [PATCH] _git: Add completion for help subcommand

[Bart Schaefer]

On Aug 26,  9:55am, Nikolai Weibull wrote:
} Subject: Re: [PATCH] _git: Add completion for help subcommand
}
} On Thu, Aug 26, 2010 at 05:29, Benjamin R. Haskell <zsh@benizi.com> wrote:
} > On Thu, 26 Aug 2010, Nikolai Weibull wrote:
} >
} >> On Sat, Aug 21, 2010 at 14:21, Aaron Schrab wrote:
} >>
} >> Seems fine except for this:
} >>
} >> > +  local formats='(--info -i --man -m --web -w)'
} >>
} >> Although this is sort of nice it doesn't follow the style of this or
} >> other completion files, so please write out the whole list in each
} >> line.
} 
} > It's just visual clutter.  Is there a particular reason not to use a
} > variable?
} 
} Perhaps. The common pattern I saw when I began writing completion
} functions was
}
} (    --b --c)--a
} (--a     --c)--b
} (--a --b    )--c
} 
} I'd really appreciate some kind of vote or input on this.  If we
} decide on using variables they should be used throughout.
}-- End of excerpt from Nikolai Weibull


Nikolai, I think you're missing the point of the abc example you quoted.

In that example, every set inside the parens is different, and the writer
of the function chose to lay them out so that it was obvious where the
differences were.

In the cases that use variables, the subset represented by the variable
is identical in every instance, so it makes sense to write out that
subset only once in the variable assignment.

E.g., both styles are consistent and they could even be mixed:

  def=(--d --e --f)
  (    --b --c $def --g)--a
  (--a     --c $def --g)--b
  (--a --b     $def --g)--c
  (--a --b --c $def    )--g

There's no reason to pick one or the other and require it everywhere.

Re: [PATCH] _git: Add completion for help subcommand

[Nikolai Weibull]

On Mon, Aug 30, 2010 at 18:43, Bart Schaefer <schaefer@brasslantern.com> wrote:

> On Aug 26,  9:55am, Nikolai Weibull wrote:

> } Perhaps. The common pattern I saw when I began writing completion
> } functions was
> }
> } (    --b --c)--a
> } (--a     --c)--b
> } (--a --b    )--c

> Nikolai, I think you're missing the point of the abc example you quoted.
>
> In that example, every set inside the parens is different, and the writer
> of the function chose to lay them out so that it was obvious where the
> differences were.

That had not alluded me.  What I never “understood” was why the
standard form wasn’t to use a variable in this case.  I mean, the
difference is that you exclude the current option, which is of course
excluded by itself (unless its defined as being able to appear many
times).

Re: [PATCH] _git: Add completion for help subcommand

[Bart Schaefer]

On Aug 30,  8:59pm, Nikolai Weibull wrote:
} Subject: Re: [PATCH] _git: Add completion for help subcommand
}
} > On Aug 26,  9:55am, Nikolai Weibull wrote:
} 
} > } (    --b --c)--a
} > } (--a     --c)--b
} > } (--a --b    )--c
} 
} What I never "understood" was why the standard form wasn't to use a
} variable in this case. I mean, the difference is that you exclude the
} current option, which is of course excluded by itself (unless its
} defined as being able to appear many times).

Hmm.  I'm not certain, now that you mention it.  The functions that
use the above form are generally some of the earliest completions
that were ever written (or were cribbed from those early ones), so it
may be that at one point in the past it was a problem to exclude the
same option one was defining, e.g., (--a)--a would break.  If that's
not currently the case [I don't know either way] then you're right,
one might as well use the style with the variable.

-- 

Re: _git: Add completion for help subcommand

[Aaron Schrab]

At 20:51 -0400 27 Aug 2010, I wrote:
>Revised, and hopefully final, version below.

It doesn't look like this has been committed to the repository yet.  Is 
there some further objection to it, or has it simply been overlooked?

 From e87683251e5c5c5737a118031582ee4500bac4ee Mon Sep 17 00:00:00 2001
From: Aaron Schrab <aaron@schrab.com>
Date: Thu, 19 Aug 2010 19:11:06 -0400
Subject: [PATCH] _git: Add completion for help subcommand

---
  Completion/Unix/Command/_git |   14 ++++++++++++++
  1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/Completion/Unix/Command/_git b/Completion/Unix/Command/_git
index c394e08..570d08f 100644
--- a/Completion/Unix/Command/_git
+++ b/Completion/Unix/Command/_git
@@ -190,6 +190,7 @@ _git_commands () {
      'fetch:download objects and a head from another repository'
      'gc:cleanup unnecessary files and optimize the local repository'
      'grep:print lines matching a pattern'
+    'help:display help information about git subcommands'
      'init:create empty git object database'
      'log:show commit logs'
      'merge:grand unified merge driver'
@@ -490,6 +491,19 @@ _git-hash-object () {
      '(--stdin):file:_files' && ret=0
  }
+(( $+functions[_git-help] )) ||
+_git-help () {
+  _arguments -S \
+    ':command:_git_commands' \
+    - '(all)' \
+    '(:)'{--all,-a}'[List all available commands]' \
+    - '(format)' \
+    {--man,-m}'[Display help in man page format]' \
+    {--info,-i}'[Display help in info format]' \
+    {--web,-w}'[Display help in web browser]' \
+    && ret=0
+}
+
  (( $+functions[_git-index-pack] )) ||
  _git-index-pack () {
    local -a stdin_arguments
-- 
1.7.0.4

Re: _git: Add completion for help subcommand

[Wayne Davison]

[-- Attachment #1: Type: text/plain, Size: 377 bytes --]

On Wed, Dec 1, 2010 at 4:44 PM, Aaron Schrab <aaron@schrab.com> wrote:

> It doesn't look like this has been committed to the repository yet.  Is
> there some further objection to it, or has it simply been overlooked?
>

I re-read the discussion, and it looked to me like everything got resolved,
so I committed your patch.  If not, we can always patch it.  Thanks!

..wayne..