PATCH: 3.0.8: git completion update for cherry-pick

PATCH: 3.0.8: git completion update for cherry-pick

[Mateusz Karbowy]

Currently completion for cherry-pick displays recent commits from the
current branch.
This patch changes this to show recent commits from all branches except HEAD.

Obszczymucha

diff --git a/_git b/_git
index b8edc10..8b9eb2b 100644
--- a/_git
+++ b/_git
@@ -511,7 +511,7 @@ _git-cherry-pick () {
     '*'{-s,--strategy=}'[use given merge strategy]:merge
strategy:__git_merge_strategies' \
     '*'{-X,--strategy-option=}'[pass merge-strategy-specific option
to merge strategy]' \
     '(-e --edit -x -n --no-commit -s --signoff)--ff[fast forward, if
possible]' \
-    ': :__git_commit_ranges'
+    ': :__git_commit_ranges --all --not HEAD'
 }

 (( $+functions[_git-citool] )) ||
@@ -5602,20 +5602,20 @@ __git_remote_branch_names_noprefix () {

 (( $+functions[__git_commit_objects_prefer_recent] )) ||
 __git_commit_objects_prefer_recent () {
-  __git_recent_commits || __git_commit_objects
+  __git_recent_commits $* || __git_commit_objects
 }

 (( $+functions[__git_commits] )) ||
 __git_commits () {
   # TODO: deal with things that __git_heads and __git_tags has in common (i.e.,
   # if both exists, they need to be completed to heads/x and tags/x.
-  local -a sopts ropt
-  zparseopts -E -a sopts S: r:=ropt R: q
+  local -a sopts ropt recentopts
+  zparseopts -E -a sopts S: r:=ropt R: q -all=recentopts -not+:=recentopts
   sopts+=( $ropt:q )
   _alternative \
     "heads::__git_heads $sopts" \
     "commit-tags::__git_commit_tags $sopts" \
-    'commit-objects::__git_commit_objects_prefer_recent'
+    "commit-objects::__git_commit_objects_prefer_recent $recentopts"
 }

 (( $+functions[__git_heads] )) ||
@@ -5669,14 +5669,16 @@ __git_commit_objects () {

 (( $+functions[__git_recent_commits] )) ||
 __git_recent_commits () {
-  local gitdir expl start
+  local gitdir expl start opts
   declare -a descr tags heads commits
   local i j k ret
   integer distance_from_head

+  zparseopts -D -E -a opts -- -all -not:
+
   # Careful: most %d will expand to the empty string.  Quote properly!
   # NOTE: we could use %D directly, but it's not available in git
1.9.1 at least.
-  commits=("${(f)"$(_call_program commits git --no-pager log -20
--format='%h%n%d%n%s\ \(%cr\)')"}")
+  commits=("${(f)"$(_call_program commits git --no-pager log $opts
-20 --format='%h%n%d%n%s\ \(%cr\)')"}")
   __git_command_successful $pipestatus || return 1

   for i j k in "$commits[@]" ; do

Re: PATCH: 3.0.8: git completion update for cherry-pick

[Daniel Shahaf]

Mateusz Karbowy wrote on Sat, Jul 18, 2015 at 23:19:05 +0100:
> Currently completion for cherry-pick displays recent commits from the
> current branch.
> This patch changes this to show recent commits from all branches except HEAD.
> 

Good morning again Mateusz, thanks for the patch.  Review below.

> Obszczymucha
> 
> diff --git a/_git b/_git
> index b8edc10..8b9eb2b 100644
> --- a/_git
> +++ b/_git
> @@ -511,7 +511,7 @@ _git-cherry-pick () {
>      '*'{-s,--strategy=}'[use given merge strategy]:merge
> strategy:__git_merge_strategies' \
>      '*'{-X,--strategy-option=}'[pass merge-strategy-specific option
> to merge strategy]' \
>      '(-e --edit -x -n --no-commit -s --signoff)--ff[fast forward, if
> possible]' \
> -    ': :__git_commit_ranges'
> +    ': :__git_commit_ranges --all --not HEAD'

Two things about this.

One, how about passing the rev-list flags in a single word, as in:
       ': :__git_commit_ranges "--all --not HEAD"'
This both simplifies the callees (they don't need to reimplement git's
option parsing) and is more extensible (it'd be trivial to make another
caller use some other set of rev-list options if we need to).

Two, since there is no space after the second colon, _arguments will
call __git_commit_ranges with compadd flags in argv.  (If you print argv
in the callee, you'll see it has both a -J option for compadd and
a --all --not option for git.)  I don't think we should pass both
rev-list flags and compadd flags in argv.  So, we have two options:
either add a space after the second colon (which inhibits passing
compadd flags — does this have undesired side effects?), or keep the
compadd flags passed in argv and pass the rev-list flags via some other
channel, such as a well-known parameter name (the caller defines
a variable with a specific declared-in-the-(internal)-API name and the
callee checks whether the variable by that name is defined).

What do you think?

> -  commits=("${(f)"$(_call_program commits git --no-pager log -20
> --format='%h%n%d%n%s\ \(%cr\)')"}")
> +  commits=("${(f)"$(_call_program commits git --no-pager log $opts
> -20 --format='%h%n%d%n%s\ \(%cr\)')"}")

Your mailer inserted a hard line break (here) and munged trailing
whitespace (elsewhere).  Next time you might want to attach your patch
(to avoid it being munged) with a *.txt extension (to make it easier to
open).

Cheers,

Daniel

Re: PATCH: 3.0.8: git completion update for cherry-pick

[Mateusz Karbowy]

On 22 July 2015 at 12:53, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> Mateusz Karbowy wrote on Sat, Jul 18, 2015 at 23:19:05 +0100:
>> Currently completion for cherry-pick displays recent commits from the
>> current branch.
>> This patch changes this to show recent commits from all branches except HEAD.
>>
>
> Good morning again Mateusz, thanks for the patch.  Review below.
>
>> Obszczymucha
>>
>> diff --git a/_git b/_git
>> index b8edc10..8b9eb2b 100644
>> --- a/_git
>> +++ b/_git
>> @@ -511,7 +511,7 @@ _git-cherry-pick () {
>>      '*'{-s,--strategy=}'[use given merge strategy]:merge
>> strategy:__git_merge_strategies' \
>>      '*'{-X,--strategy-option=}'[pass merge-strategy-specific option
>> to merge strategy]' \
>>      '(-e --edit -x -n --no-commit -s --signoff)--ff[fast forward, if
>> possible]' \
>> -    ': :__git_commit_ranges'
>> +    ': :__git_commit_ranges --all --not HEAD'
>
> Two things about this.
>
> One, how about passing the rev-list flags in a single word, as in:
>        ': :__git_commit_ranges "--all --not HEAD"'
> This both simplifies the callees (they don't need to reimplement git's
> option parsing) and is more extensible (it'd be trivial to make another
> caller use some other set of rev-list options if we need to).
>
> Two, since there is no space after the second colon, _arguments will
> call __git_commit_ranges with compadd flags in argv.  (If you print argv
> in the callee, you'll see it has both a -J option for compadd and
> a --all --not option for git.)  I don't think we should pass both
> rev-list flags and compadd flags in argv.  So, we have two options:
> either add a space after the second colon (which inhibits passing
> compadd flags — does this have undesired side effects?), or keep the
> compadd flags passed in argv and pass the rev-list flags via some other
> channel, such as a well-known parameter name (the caller defines
> a variable with a specific declared-in-the-(internal)-API name and the
> callee checks whether the variable by that name is defined).
>
> What do you think?
>
Thanks for spotting this. I'm not entirely sure what would be the
impact of adding space after the second colon is, but it feels risky.
So I'd rather not do it unless someone more experienced tells me it's
fully safe :)
The second option with the variable - could you elaborate on this, I
didn't quite get what kind of variable you mean (is there any example
you know of so I can take a look?).

>> -  commits=("${(f)"$(_call_program commits git --no-pager log -20
>> --format='%h%n%d%n%s\ \(%cr\)')"}")
>> +  commits=("${(f)"$(_call_program commits git --no-pager log $opts
>> -20 --format='%h%n%d%n%s\ \(%cr\)')"}")
>
> Your mailer inserted a hard line break (here) and munged trailing
> whitespace (elsewhere).  Next time you might want to attach your patch
> (to avoid it being munged) with a *.txt extension (to make it easier to
> open).
>
> Cheers,
>
> Daniel
>
Good piece of advice. I thought something was wrong with the lines. Thanks!

Regards,
Mateusz

Re: PATCH: 3.0.8: git completion update for cherry-pick

[Daniel Shahaf]

Mateusz Karbowy wrote on Mon, Aug 10, 2015 at 22:31:42 +0100:
> On 22 July 2015 at 12:53, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > Mateusz Karbowy wrote on Sat, Jul 18, 2015 at 23:19:05 +0100:
> >> Currently completion for cherry-pick displays recent commits from the
> >> current branch.
> >> This patch changes this to show recent commits from all branches except HEAD.
> >>
> >
> > Good morning again Mateusz, thanks for the patch.  Review below.
> >
> >> Obszczymucha
> >>
> >> diff --git a/_git b/_git
> >> index b8edc10..8b9eb2b 100644
> >> --- a/_git
> >> +++ b/_git
> >> @@ -511,7 +511,7 @@ _git-cherry-pick () {
> >>      '*'{-s,--strategy=}'[use given merge strategy]:merge
> >> strategy:__git_merge_strategies' \
> >>      '*'{-X,--strategy-option=}'[pass merge-strategy-specific option
> >> to merge strategy]' \
> >>      '(-e --edit -x -n --no-commit -s --signoff)--ff[fast forward, if
> >> possible]' \
> >> -    ': :__git_commit_ranges'
> >> +    ': :__git_commit_ranges --all --not HEAD'
> >
> > Two things about this.
> >
> > One, how about passing the rev-list flags in a single word, as in:
> >        ': :__git_commit_ranges "--all --not HEAD"'
> > This both simplifies the callees (they don't need to reimplement git's
> > option parsing) and is more extensible (it'd be trivial to make another
> > caller use some other set of rev-list options if we need to).
> >
> > Two, since there is no space after the second colon, _arguments will
> > call __git_commit_ranges with compadd flags in argv.  (If you print argv
> > in the callee, you'll see it has both a -J option for compadd and
> > a --all --not option for git.)  I don't think we should pass both
> > rev-list flags and compadd flags in argv.  So, we have two options:
> > either add a space after the second colon (which inhibits passing
> > compadd flags — does this have undesired side effects?), or keep the
> > compadd flags passed in argv and pass the rev-list flags via some other
> > channel, such as a well-known parameter name (the caller defines
> > a variable with a specific declared-in-the-(internal)-API name and the
> > callee checks whether the variable by that name is defined).
> >
> > What do you think?
> >
> Thanks for spotting this. I'm not entirely sure what would be the
> impact of adding space after the second colon is, but it feels risky.
> So I'd rather not do it unless someone more experienced tells me it's
> fully safe :)

The space is documented: grep the zshcompsys(1) man page for "The forms
for action are as follows".  I believe the difference is that _arguments
won't implicitly pass ${expl} as additional arguments, so the callee
will have to add them itself; that is, I believe the action «foo» (no
leading space) is equivalent to the action « foo ${expl}» (with leading
space).

What we could do here is have foo called as « foo -O expl -a "--all
--not HEAD"» and let foo do «getopts "a:O:"» (or zparseopts) and convert
the word 'expl' to the contents of the array by that name; compare the
call «_alternative -O expl $other_arguments» in _git-send-email.

Speaking of which, the handling of expl down in __git_commit_objects
might need to be revisited.  (This function is one of the few that call
compadd directly.)

> The second option with the variable - could you elaborate on this, I
> didn't quite get what kind of variable you mean (is there any example
> you know of so I can take a look?).

I mean communicating state via a global variable; basically:

    f() { local X=1; g }
    g() { h }
    h() { if (( $+X )); then foo; else bar; fi }

where 'f' is _git-cherry-pick, 'g' is __git_commit_ranges and
__git_commits, and 'h' is __git_commit_objects_prefer_recent.  And
instead of X, a name such as __GIT_EXTRA_OPTIONS_FOR_RECENT_COMMITS.

Obviously this has all the usual drawbacks of global state and
action-at-a-distance.

> > Your mailer inserted a hard line break and munged trailing
> > whitespace.  Next time you might want to attach your patch (to avoid
> > it being munged) with a *.txt extension (to make it easier to open).
> 
> Good piece of advice. I thought something was wrong with the lines. Thanks!

You're welcome.

Cheers,

Daniel

Re: PATCH: 3.0.8: git completion update for cherry-pick

[Mateusz Karbowy]

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

Hello Daniel

Thanks again for your comprehensive reply and sorry I stalled it for so long.
I went for the variable approach, as I'm a noob if it comes to zsh
internals and your first suggestion is still a bit arcane to me :)

CR please.

Regards,
Mateusz

On 11 August 2015 at 23:06, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> Mateusz Karbowy wrote on Mon, Aug 10, 2015 at 22:31:42 +0100:
>> On 22 July 2015 at 12:53, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>> > Mateusz Karbowy wrote on Sat, Jul 18, 2015 at 23:19:05 +0100:
>> >> Currently completion for cherry-pick displays recent commits from the
>> >> current branch.
>> >> This patch changes this to show recent commits from all branches except HEAD.
>> >>
>> >
>> > Good morning again Mateusz, thanks for the patch.  Review below.
>> >
>> >> Obszczymucha
>> >>
>> >> diff --git a/_git b/_git
>> >> index b8edc10..8b9eb2b 100644
>> >> --- a/_git
>> >> +++ b/_git
>> >> @@ -511,7 +511,7 @@ _git-cherry-pick () {
>> >>      '*'{-s,--strategy=}'[use given merge strategy]:merge
>> >> strategy:__git_merge_strategies' \
>> >>      '*'{-X,--strategy-option=}'[pass merge-strategy-specific option
>> >> to merge strategy]' \
>> >>      '(-e --edit -x -n --no-commit -s --signoff)--ff[fast forward, if
>> >> possible]' \
>> >> -    ': :__git_commit_ranges'
>> >> +    ': :__git_commit_ranges --all --not HEAD'
>> >
>> > Two things about this.
>> >
>> > One, how about passing the rev-list flags in a single word, as in:
>> >        ': :__git_commit_ranges "--all --not HEAD"'
>> > This both simplifies the callees (they don't need to reimplement git's
>> > option parsing) and is more extensible (it'd be trivial to make another
>> > caller use some other set of rev-list options if we need to).
>> >
>> > Two, since there is no space after the second colon, _arguments will
>> > call __git_commit_ranges with compadd flags in argv.  (If you print argv
>> > in the callee, you'll see it has both a -J option for compadd and
>> > a --all --not option for git.)  I don't think we should pass both
>> > rev-list flags and compadd flags in argv.  So, we have two options:
>> > either add a space after the second colon (which inhibits passing
>> > compadd flags — does this have undesired side effects?), or keep the
>> > compadd flags passed in argv and pass the rev-list flags via some other
>> > channel, such as a well-known parameter name (the caller defines
>> > a variable with a specific declared-in-the-(internal)-API name and the
>> > callee checks whether the variable by that name is defined).
>> >
>> > What do you think?
>> >
>> Thanks for spotting this. I'm not entirely sure what would be the
>> impact of adding space after the second colon is, but it feels risky.
>> So I'd rather not do it unless someone more experienced tells me it's
>> fully safe :)
>
> The space is documented: grep the zshcompsys(1) man page for "The forms
> for action are as follows".  I believe the difference is that _arguments
> won't implicitly pass ${expl} as additional arguments, so the callee
> will have to add them itself; that is, I believe the action «foo» (no
> leading space) is equivalent to the action « foo ${expl}» (with leading
> space).
>
> What we could do here is have foo called as « foo -O expl -a "--all
> --not HEAD"» and let foo do «getopts "a:O:"» (or zparseopts) and convert
> the word 'expl' to the contents of the array by that name; compare the
> call «_alternative -O expl $other_arguments» in _git-send-email.
>
> Speaking of which, the handling of expl down in __git_commit_objects
> might need to be revisited.  (This function is one of the few that call
> compadd directly.)
>
>> The second option with the variable - could you elaborate on this, I
>> didn't quite get what kind of variable you mean (is there any example
>> you know of so I can take a look?).
>
> I mean communicating state via a global variable; basically:
>
>     f() { local X=1; g }
>     g() { h }
>     h() { if (( $+X )); then foo; else bar; fi }
>
> where 'f' is _git-cherry-pick, 'g' is __git_commit_ranges and
> __git_commits, and 'h' is __git_commit_objects_prefer_recent.  And
> instead of X, a name such as __GIT_EXTRA_OPTIONS_FOR_RECENT_COMMITS.
>
> Obviously this has all the usual drawbacks of global state and
> action-at-a-distance.
>
>> > Your mailer inserted a hard line break and munged trailing
>> > whitespace.  Next time you might want to attach your patch (to avoid
>> > it being munged) with a *.txt extension (to make it easier to open).
>>
>> Good piece of advice. I thought something was wrong with the lines. Thanks!
>
> You're welcome.
>
> Cheers,
>
> Daniel

[-- Attachment #2: _git.patch.txt --]
[-- Type: text/plain, Size: 1019 bytes --]

diff --git a/Completion/Unix/Command/_git b/Completion/Unix/Command/_git
index b8edc10..df963c4 100644
--- a/Completion/Unix/Command/_git
+++ b/Completion/Unix/Command/_git
@@ -495,6 +495,7 @@ _git-checkout () {
 
 (( $+functions[_git-cherry-pick] )) ||
 _git-cherry-pick () {
+  local __GIT_EXTRA_OPTIONS_FOR_RECENT_COMMITS='--all --not HEAD'
   _arguments \
     '(- :)--quit[end revert or cherry-pick sequence]' \
     '(- :)--continue[resume revert or cherry-pick sequence]' \
@@ -5676,7 +5677,7 @@ __git_recent_commits () {
 
   # Careful: most %d will expand to the empty string.  Quote properly!
   # NOTE: we could use %D directly, but it's not available in git 1.9.1 at least.
-  commits=("${(f)"$(_call_program commits git --no-pager log -20 --format='%h%n%d%n%s\ \(%cr\)')"}")
+  commits=("${(f)"$(_call_program commits git --no-pager log $__GIT_EXTRA_OPTIONS_FOR_RECENT_COMMITS -20 --format='%h%n%d%n%s\ \(%cr\)')"}")
   __git_command_successful $pipestatus || return 1
 
   for i j k in "$commits[@]" ; do

Re: PATCH: 3.0.8: git completion update for cherry-pick

[Mateusz Karbowy]

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

Hey Daniel

I think this covers it. I know you mentioned I shouldn't need to use
the ${(P)} in __git_commit_ranges, but there were these two calls to
__git_commits with either '$*' or '$* $suf', and they operated on
compadd params before the change, so I thought it's safer this way.
What do you think?

Regards,
Obszczymucha

On 19 August 2015 at 18:10, Mateusz Karbowy <mateusz.karbowy@gmail.com> wrote:
> Hello Daniel
>
> Thanks again for your comprehensive reply and sorry I stalled it for so long.
> I went for the variable approach, as I'm a noob if it comes to zsh
> internals and your first suggestion is still a bit arcane to me :)
>
> CR please.
>
> Regards,
> Mateusz
>
> On 11 August 2015 at 23:06, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>> Mateusz Karbowy wrote on Mon, Aug 10, 2015 at 22:31:42 +0100:
>>> On 22 July 2015 at 12:53, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>>> > Mateusz Karbowy wrote on Sat, Jul 18, 2015 at 23:19:05 +0100:
>>> >> Currently completion for cherry-pick displays recent commits from the
>>> >> current branch.
>>> >> This patch changes this to show recent commits from all branches except HEAD.
>>> >>
>>> >
>>> > Good morning again Mateusz, thanks for the patch.  Review below.
>>> >
>>> >> Obszczymucha
>>> >>
>>> >> diff --git a/_git b/_git
>>> >> index b8edc10..8b9eb2b 100644
>>> >> --- a/_git
>>> >> +++ b/_git
>>> >> @@ -511,7 +511,7 @@ _git-cherry-pick () {
>>> >>      '*'{-s,--strategy=}'[use given merge strategy]:merge
>>> >> strategy:__git_merge_strategies' \
>>> >>      '*'{-X,--strategy-option=}'[pass merge-strategy-specific option
>>> >> to merge strategy]' \
>>> >>      '(-e --edit -x -n --no-commit -s --signoff)--ff[fast forward, if
>>> >> possible]' \
>>> >> -    ': :__git_commit_ranges'
>>> >> +    ': :__git_commit_ranges --all --not HEAD'
>>> >
>>> > Two things about this.
>>> >
>>> > One, how about passing the rev-list flags in a single word, as in:
>>> >        ': :__git_commit_ranges "--all --not HEAD"'
>>> > This both simplifies the callees (they don't need to reimplement git's
>>> > option parsing) and is more extensible (it'd be trivial to make another
>>> > caller use some other set of rev-list options if we need to).
>>> >
>>> > Two, since there is no space after the second colon, _arguments will
>>> > call __git_commit_ranges with compadd flags in argv.  (If you print argv
>>> > in the callee, you'll see it has both a -J option for compadd and
>>> > a --all --not option for git.)  I don't think we should pass both
>>> > rev-list flags and compadd flags in argv.  So, we have two options:
>>> > either add a space after the second colon (which inhibits passing
>>> > compadd flags — does this have undesired side effects?), or keep the
>>> > compadd flags passed in argv and pass the rev-list flags via some other
>>> > channel, such as a well-known parameter name (the caller defines
>>> > a variable with a specific declared-in-the-(internal)-API name and the
>>> > callee checks whether the variable by that name is defined).
>>> >
>>> > What do you think?
>>> >
>>> Thanks for spotting this. I'm not entirely sure what would be the
>>> impact of adding space after the second colon is, but it feels risky.
>>> So I'd rather not do it unless someone more experienced tells me it's
>>> fully safe :)
>>
>> The space is documented: grep the zshcompsys(1) man page for "The forms
>> for action are as follows".  I believe the difference is that _arguments
>> won't implicitly pass ${expl} as additional arguments, so the callee
>> will have to add them itself; that is, I believe the action «foo» (no
>> leading space) is equivalent to the action « foo ${expl}» (with leading
>> space).
>>
>> What we could do here is have foo called as « foo -O expl -a "--all
>> --not HEAD"» and let foo do «getopts "a:O:"» (or zparseopts) and convert
>> the word 'expl' to the contents of the array by that name; compare the
>> call «_alternative -O expl $other_arguments» in _git-send-email.
>>
>> Speaking of which, the handling of expl down in __git_commit_objects
>> might need to be revisited.  (This function is one of the few that call
>> compadd directly.)
>>
>>> The second option with the variable - could you elaborate on this, I
>>> didn't quite get what kind of variable you mean (is there any example
>>> you know of so I can take a look?).
>>
>> I mean communicating state via a global variable; basically:
>>
>>     f() { local X=1; g }
>>     g() { h }
>>     h() { if (( $+X )); then foo; else bar; fi }
>>
>> where 'f' is _git-cherry-pick, 'g' is __git_commit_ranges and
>> __git_commits, and 'h' is __git_commit_objects_prefer_recent.  And
>> instead of X, a name such as __GIT_EXTRA_OPTIONS_FOR_RECENT_COMMITS.
>>
>> Obviously this has all the usual drawbacks of global state and
>> action-at-a-distance.
>>
>>> > Your mailer inserted a hard line break and munged trailing
>>> > whitespace.  Next time you might want to attach your patch (to avoid
>>> > it being munged) with a *.txt extension (to make it easier to open).
>>>
>>> Good piece of advice. I thought something was wrong with the lines. Thanks!
>>
>> You're welcome.
>>
>> Cheers,
>>
>> Daniel

[-- Attachment #2: _git.patch.txt --]
[-- Type: text/plain, Size: 3773 bytes --]

diff --git a/Completion/Unix/Command/_git b/Completion/Unix/Command/_git
index b8edc10..5f4fead 100644
--- a/Completion/Unix/Command/_git
+++ b/Completion/Unix/Command/_git
@@ -495,6 +495,8 @@ _git-checkout () {
 
 (( $+functions[_git-cherry-pick] )) ||
 _git-cherry-pick () {
+  local -a git_commit_opts
+  git_commit_opts=(--all --not HEAD --not)
   _arguments \
     '(- :)--quit[end revert or cherry-pick sequence]' \
     '(- :)--continue[resume revert or cherry-pick sequence]' \
@@ -511,7 +513,7 @@ _git-cherry-pick () {
     '*'{-s,--strategy=}'[use given merge strategy]:merge strategy:__git_merge_strategies' \
     '*'{-X,--strategy-option=}'[pass merge-strategy-specific option to merge strategy]' \
     '(-e --edit -x -n --no-commit -s --signoff)--ff[fast forward, if possible]' \
-    ': :__git_commit_ranges'
+    ': : __git_commit_ranges -O expl -C git_commit_opts'
 }
 
 (( $+functions[_git-citool] )) ||
@@ -5602,20 +5604,29 @@ __git_remote_branch_names_noprefix () {
 
 (( $+functions[__git_commit_objects_prefer_recent] )) ||
 __git_commit_objects_prefer_recent () {
-  __git_recent_commits || __git_commit_objects
+  local -A commit_opts_array_name
+  zparseopts -D -E C:=commit_opts_array_name
+
+  __git_recent_commits -C $commit_opts_array_name || __git_commit_objects
 }
 
 (( $+functions[__git_commits] )) ||
 __git_commits () {
+  # Separate compadd options with git commit options.
+  local -A compadd_opts_array_name commit_opts_array_name
+  zparseopts -D -E O:=compadd_opts_array_name C:=commit_opts_array_name
+  (( $#compadd_opts_array_name )) && set -- "${(@P)compadd_opts_array_name}"
+
   # TODO: deal with things that __git_heads and __git_tags has in common (i.e.,
   # if both exists, they need to be completed to heads/x and tags/x.
-  local -a sopts ropt
+  local -a sopts ropt expl
   zparseopts -E -a sopts S: r:=ropt R: q
   sopts+=( $ropt:q )
+  expl=( $@ )
   _alternative \
     "heads::__git_heads $sopts" \
     "commit-tags::__git_commit_tags $sopts" \
-    'commit-objects::__git_commit_objects_prefer_recent'
+    'commit-objects:: __git_commit_objects_prefer_recent -O expl -C $commit_opts_array_name'
 }
 
 (( $+functions[__git_heads] )) ||
@@ -5670,13 +5681,17 @@ __git_commit_objects () {
 (( $+functions[__git_recent_commits] )) ||
 __git_recent_commits () {
   local gitdir expl start
-  declare -a descr tags heads commits
+  local -A commit_opts_array_name
+  declare -a descr tags heads commits commit_opts
   local i j k ret
   integer distance_from_head
 
+  zparseopts -D -E C:=commit_opts_array_name
+  (( $#commit_opts_array_name )) && commit_opts=( "${(@P)commit_opts_array_name}" )
+
   # Careful: most %d will expand to the empty string.  Quote properly!
   # NOTE: we could use %D directly, but it's not available in git 1.9.1 at least.
-  commits=("${(f)"$(_call_program commits git --no-pager log -20 --format='%h%n%d%n%s\ \(%cr\)')"}")
+  commits=("${(f)"$(_call_program commits git --no-pager log $commit_opts -20 --format='%h%n%d%n%s\ \(%cr\)')"}")
   __git_command_successful $pipestatus || return 1
 
   for i j k in "$commits[@]" ; do
@@ -5759,13 +5774,19 @@ __git_commits2 () {
 
 (( $+functions[__git_commit_ranges] )) ||
 __git_commit_ranges () {
-  local -a suf
+  local -A compadd_opts_array_name commit_opts_array_name
+  zparseopts -D -E O:=compadd_opts_array_name C:=commit_opts_array_name
+  (( $#compadd_opts_array_name )) && set -- "${(@P)compadd_opts_array_name}"
+
+  local -a expl suf
   if compset -P '*..(.|)'; then
-    __git_commits $*
+    expl=( $* )
   else
     compset -S '..*' || suf=( -S .. -r '.@~ ^:\t\n\-' )
-    __git_commits $* $suf
+    expl=( $* $suf )
   fi
+
+  __git_commits -O expl -C $commit_opts_array_name
 }
 
 (( $+functions[__git_commit_ranges2] )) ||

Re: PATCH: 3.0.8: git completion update for cherry-pick

[Daniel Shahaf]

As mentioned on IRC — I'd like to review this but I don't have time to
do so right now.  @zsh-workers, feel free to beat me to it.

Mateusz Karbowy wrote on Fri, Aug 21, 2015 at 20:55:07 +0100:
> Hey Daniel
> 
> I think this covers it. I know you mentioned I shouldn't need to use
> the ${(P)} in __git_commit_ranges, but there were these two calls to
> __git_commits with either '$*' or '$* $suf', and they operated on
> compadd params before the change, so I thought it's safer this way.
> What do you think?

> diff --git a/Completion/Unix/Command/_git b/Completion/Unix/Command/_git
> index b8edc10..5f4fead 100644
> --- a/Completion/Unix/Command/_git
> +++ b/Completion/Unix/Command/_git
> @@ -495,6 +495,8 @@ _git-checkout () {
>  
>  (( $+functions[_git-cherry-pick] )) ||
>  _git-cherry-pick () {
> +  local -a git_commit_opts
> +  git_commit_opts=(--all --not HEAD --not)
>    _arguments \
>      '(- :)--quit[end revert or cherry-pick sequence]' \
>      '(- :)--continue[resume revert or cherry-pick sequence]' \
> @@ -511,7 +513,7 @@ _git-cherry-pick () {
>      '*'{-s,--strategy=}'[use given merge strategy]:merge strategy:__git_merge_strategies' \
>      '*'{-X,--strategy-option=}'[pass merge-strategy-specific option to merge strategy]' \
>      '(-e --edit -x -n --no-commit -s --signoff)--ff[fast forward, if possible]' \
> -    ': :__git_commit_ranges'
> +    ': : __git_commit_ranges -O expl -C git_commit_opts'
>  }
>  
>  (( $+functions[_git-citool] )) ||
> @@ -5602,20 +5604,29 @@ __git_remote_branch_names_noprefix () {
>  
>  (( $+functions[__git_commit_objects_prefer_recent] )) ||
>  __git_commit_objects_prefer_recent () {
> -  __git_recent_commits || __git_commit_objects
> +  local -A commit_opts_array_name
> +  zparseopts -D -E C:=commit_opts_array_name
> +
> +  __git_recent_commits -C $commit_opts_array_name || __git_commit_objects
>  }
>  
>  (( $+functions[__git_commits] )) ||
>  __git_commits () {
> +  # Separate compadd options with git commit options.
> +  local -A compadd_opts_array_name commit_opts_array_name
> +  zparseopts -D -E O:=compadd_opts_array_name C:=commit_opts_array_name
> +  (( $#compadd_opts_array_name )) && set -- "${(@P)compadd_opts_array_name}"
> +
>    # TODO: deal with things that __git_heads and __git_tags has in common (i.e.,
>    # if both exists, they need to be completed to heads/x and tags/x.
> -  local -a sopts ropt
> +  local -a sopts ropt expl
>    zparseopts -E -a sopts S: r:=ropt R: q
>    sopts+=( $ropt:q )
> +  expl=( $@ )
>    _alternative \
>      "heads::__git_heads $sopts" \
>      "commit-tags::__git_commit_tags $sopts" \
> -    'commit-objects::__git_commit_objects_prefer_recent'
> +    'commit-objects:: __git_commit_objects_prefer_recent -O expl -C $commit_opts_array_name'
>  }
>  
>  (( $+functions[__git_heads] )) ||
> @@ -5670,13 +5681,17 @@ __git_commit_objects () {
>  (( $+functions[__git_recent_commits] )) ||
>  __git_recent_commits () {
>    local gitdir expl start
> -  declare -a descr tags heads commits
> +  local -A commit_opts_array_name
> +  declare -a descr tags heads commits commit_opts
>    local i j k ret
>    integer distance_from_head
>  
> +  zparseopts -D -E C:=commit_opts_array_name
> +  (( $#commit_opts_array_name )) && commit_opts=( "${(@P)commit_opts_array_name}" )
> +
>    # Careful: most %d will expand to the empty string.  Quote properly!
>    # NOTE: we could use %D directly, but it's not available in git 1.9.1 at least.
> -  commits=("${(f)"$(_call_program commits git --no-pager log -20 --format='%h%n%d%n%s\ \(%cr\)')"}")
> +  commits=("${(f)"$(_call_program commits git --no-pager log $commit_opts -20 --format='%h%n%d%n%s\ \(%cr\)')"}")
>    __git_command_successful $pipestatus || return 1
>  
>    for i j k in "$commits[@]" ; do
> @@ -5759,13 +5774,19 @@ __git_commits2 () {
>  
>  (( $+functions[__git_commit_ranges] )) ||
>  __git_commit_ranges () {
> -  local -a suf
> +  local -A compadd_opts_array_name commit_opts_array_name
> +  zparseopts -D -E O:=compadd_opts_array_name C:=commit_opts_array_name
> +  (( $#compadd_opts_array_name )) && set -- "${(@P)compadd_opts_array_name}"
> +
> +  local -a expl suf
>    if compset -P '*..(.|)'; then
> -    __git_commits $*
> +    expl=( $* )
>    else
>      compset -S '..*' || suf=( -S .. -r '.@~ ^:\t\n\-' )
> -    __git_commits $* $suf
> +    expl=( $* $suf )
>    fi
> +
> +  __git_commits -O expl -C $commit_opts_array_name
>  }
>  
>  (( $+functions[__git_commit_ranges2] )) ||

Re: PATCH: 3.0.8: git completion update for cherry-pick

[Mateusz Karbowy]

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

My last patch interfered with git-checkout. I've fixed it this time.

On 22 July 2015 at 12:53, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> Mateusz Karbowy wrote on Sat, Jul 18, 2015 at 23:19:05 +0100:
>> Currently completion for cherry-pick displays recent commits from the
>> current branch.
>> This patch changes this to show recent commits from all branches except HEAD.
>>
>
> Good morning again Mateusz, thanks for the patch.  Review below.
>
>> Obszczymucha
>>
>> diff --git a/_git b/_git
>> index b8edc10..8b9eb2b 100644
>> --- a/_git
>> +++ b/_git
>> @@ -511,7 +511,7 @@ _git-cherry-pick () {
>>      '*'{-s,--strategy=}'[use given merge strategy]:merge
>> strategy:__git_merge_strategies' \
>>      '*'{-X,--strategy-option=}'[pass merge-strategy-specific option
>> to merge strategy]' \
>>      '(-e --edit -x -n --no-commit -s --signoff)--ff[fast forward, if
>> possible]' \
>> -    ': :__git_commit_ranges'
>> +    ': :__git_commit_ranges --all --not HEAD'
>
> Two things about this.
>
> One, how about passing the rev-list flags in a single word, as in:
>        ': :__git_commit_ranges "--all --not HEAD"'
> This both simplifies the callees (they don't need to reimplement git's
> option parsing) and is more extensible (it'd be trivial to make another
> caller use some other set of rev-list options if we need to).
>
> Two, since there is no space after the second colon, _arguments will
> call __git_commit_ranges with compadd flags in argv.  (If you print argv
> in the callee, you'll see it has both a -J option for compadd and
> a --all --not option for git.)  I don't think we should pass both
> rev-list flags and compadd flags in argv.  So, we have two options:
> either add a space after the second colon (which inhibits passing
> compadd flags — does this have undesired side effects?), or keep the
> compadd flags passed in argv and pass the rev-list flags via some other
> channel, such as a well-known parameter name (the caller defines
> a variable with a specific declared-in-the-(internal)-API name and the
> callee checks whether the variable by that name is defined).
>
> What do you think?
>
>> -  commits=("${(f)"$(_call_program commits git --no-pager log -20
>> --format='%h%n%d%n%s\ \(%cr\)')"}")
>> +  commits=("${(f)"$(_call_program commits git --no-pager log $opts
>> -20 --format='%h%n%d%n%s\ \(%cr\)')"}")
>
> Your mailer inserted a hard line break (here) and munged trailing
> whitespace (elsewhere).  Next time you might want to attach your patch
> (to avoid it being munged) with a *.txt extension (to make it easier to
> open).
>
> Cheers,
>
> Daniel
>

[-- Attachment #2: _git.patch.txt --]
[-- Type: text/plain, Size: 3719 bytes --]

diff --git a/Completion/Unix/Command/_git b/Completion/Unix/Command/_git
index b8edc10..c7c639c 100644
--- a/Completion/Unix/Command/_git
+++ b/Completion/Unix/Command/_git
@@ -495,6 +495,8 @@ _git-checkout () {
 
 (( $+functions[_git-cherry-pick] )) ||
 _git-cherry-pick () {
+  local -a git_commit_opts
+  git_commit_opts=(--all --not HEAD --not)
   _arguments \
     '(- :)--quit[end revert or cherry-pick sequence]' \
     '(- :)--continue[resume revert or cherry-pick sequence]' \
@@ -511,7 +513,7 @@ _git-cherry-pick () {
     '*'{-s,--strategy=}'[use given merge strategy]:merge strategy:__git_merge_strategies' \
     '*'{-X,--strategy-option=}'[pass merge-strategy-specific option to merge strategy]' \
     '(-e --edit -x -n --no-commit -s --signoff)--ff[fast forward, if possible]' \
-    ': :__git_commit_ranges'
+    ': : __git_commit_ranges -O expl -C git_commit_opts'
 }
 
 (( $+functions[_git-citool] )) ||
@@ -5602,20 +5604,29 @@ __git_remote_branch_names_noprefix () {
 
 (( $+functions[__git_commit_objects_prefer_recent] )) ||
 __git_commit_objects_prefer_recent () {
-  __git_recent_commits || __git_commit_objects
+  local -a commit_opts_argument
+  zparseopts -D -E C:=commit_opts_argument
+
+  __git_recent_commits $commit_opts_argument || __git_commit_objects
 }
 
 (( $+functions[__git_commits] )) ||
 __git_commits () {
+  # Separate compadd options with git commit options.
+  local -a compadd_opts_argument commit_opts_argument
+  zparseopts -D -E O:=compadd_opts_argument C:=commit_opts_argument
+  (( $#compadd_opts_argument )) && set -- "${(@P)compadd_opts_argument[2]}"
+
   # TODO: deal with things that __git_heads and __git_tags has in common (i.e.,
   # if both exists, they need to be completed to heads/x and tags/x.
-  local -a sopts ropt
+  local -a sopts ropt expl
   zparseopts -E -a sopts S: r:=ropt R: q
   sopts+=( $ropt:q )
+  expl=( $@ )
   _alternative \
     "heads::__git_heads $sopts" \
     "commit-tags::__git_commit_tags $sopts" \
-    'commit-objects::__git_commit_objects_prefer_recent'
+    'commit-objects:: __git_commit_objects_prefer_recent -O expl $commit_opts_argument'
 }
 
 (( $+functions[__git_heads] )) ||
@@ -5670,13 +5681,16 @@ __git_commit_objects () {
 (( $+functions[__git_recent_commits] )) ||
 __git_recent_commits () {
   local gitdir expl start
-  declare -a descr tags heads commits
+  declare -a descr tags heads commits commit_opts_argument commit_opts
   local i j k ret
   integer distance_from_head
 
+  zparseopts -D -E C:=commit_opts_argument
+  (( $#commit_opts_argument )) && commit_opts=( "${(@P)commit_opts_argument[2]}" )
+
   # Careful: most %d will expand to the empty string.  Quote properly!
   # NOTE: we could use %D directly, but it's not available in git 1.9.1 at least.
-  commits=("${(f)"$(_call_program commits git --no-pager log -20 --format='%h%n%d%n%s\ \(%cr\)')"}")
+  commits=("${(f)"$(_call_program commits git --no-pager log $commit_opts -20 --format='%h%n%d%n%s\ \(%cr\)')"}")
   __git_command_successful $pipestatus || return 1
 
   for i j k in "$commits[@]" ; do
@@ -5759,13 +5773,19 @@ __git_commits2 () {
 
 (( $+functions[__git_commit_ranges] )) ||
 __git_commit_ranges () {
-  local -a suf
+  local -a compadd_opts_argument commit_opts_argument
+  zparseopts -D -E O:=compadd_opts_argument C:=commit_opts_argument
+  (( $#compadd_opts_argument )) && set -- "${(@P)compadd_opts_argument[2]}"
+
+  local -a expl suf
   if compset -P '*..(.|)'; then
-    __git_commits $*
+    expl=( $* )
   else
     compset -S '..*' || suf=( -S .. -r '.@~ ^:\t\n\-' )
-    __git_commits $* $suf
+    expl=( $* $suf )
   fi
+
+  __git_commits -O expl $commit_opts_argument
 }
 
 (( $+functions[__git_commit_ranges2] )) ||

Re: PATCH: 3.0.8: git completion update for cherry-pick

[Daniel Shahaf]

Mateusz Karbowy wrote on Tue, Aug 25, 2015 at 23:26:21 +0100:
> My last patch interfered with git-checkout. I've fixed it this time.
> 

Sorry for the late reply.

I have only two comments about this patch: one bugfix and one question.

> @@ -511,7 +513,7 @@ _git-cherry-pick () {
>      '*'{-s,--strategy=}'[use given merge strategy]:merge strategy:__git_merge_strategies' \
>      '*'{-X,--strategy-option=}'[pass merge-strategy-specific option to merge strategy]' \
>      '(-e --edit -x -n --no-commit -s --signoff)--ff[fast forward, if possible]' \
> -    ': :__git_commit_ranges'
> +    ': : __git_commit_ranges -O expl -C git_commit_opts'

There are other callers of __git_commit_ranges that pass compadd options in
argv.  There is already a compadd option "-C", therefore, choosing this option
letter prevents any caller of __git_commit_ranges from passing the compadd -C
option.

Is this a problem?  Should we find some other way to pass the name
"git_commit_opts"?  If so, what option letters are available?  (Since "-O" is
available, maybe we could pass both array names in it as '-O
"expl:git_commit_opts"'?  I.e., if a colon is present in the argument
value, split on it, else assume the entire argument is a single
parameter name, as in the conventional "-O expl"?)

>  __git_commits () {
> ...
> +  expl=( $@ )

The $@ should be double-quoted to avoid eliding empty arguments.

> 

Apart from these two issues, the patch seems ready for commit to me.

Thanks again.

Cheers,

Daniel

Re: PATCH: 3.0.8: git completion update for cherry-pick

[Bart Schaefer]

On Aug 27, 11:11pm, Daniel Shahaf wrote:
}
} Apart from these two issues, the patch seems ready for commit to me.

Let's get this (and 36304) in soon, so PWS can close -test-3 and
release 5.1.

Re: PATCH: 3.0.8: git completion update for cherry-pick

[Daniel Shahaf]

Bart Schaefer wrote on Thu, Aug 27, 2015 at 16:19:52 -0700:
> On Aug 27, 11:11pm, Daniel Shahaf wrote:
> }
> } Apart from these two issues, the patch seems ready for commit to me.
> 
> Let's get this (and 36304) in soon, so PWS can close -test-3 and
> release 5.1.

I've already pushed 36304.

I don't think this patch needs to delay 5.1; it's not a regression.

Re: PATCH: 3.0.8: git completion update for cherry-pick

[Mateusz Karbowy]

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

Daniel,

I've fixed the no-quotes issue with $@ and now I'm passing both
compadd and git commit options using one -O parameter (the array names
are separated with colon, as you suggested).

Regards,
Obszczymucha

On 28 August 2015 at 00:28, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> Bart Schaefer wrote on Thu, Aug 27, 2015 at 16:19:52 -0700:
>> On Aug 27, 11:11pm, Daniel Shahaf wrote:
>> }
>> } Apart from these two issues, the patch seems ready for commit to me.
>>
>> Let's get this (and 36304) in soon, so PWS can close -test-3 and
>> release 5.1.
>
> I've already pushed 36304.
>
> I don't think this patch needs to delay 5.1; it's not a regression.

[-- Attachment #2: _git.patch.txt --]
[-- Type: text/plain, Size: 3799 bytes --]

diff --git a/Completion/Unix/Command/_git b/Completion/Unix/Command/_git
index b8edc10..ef125e4 100644
--- a/Completion/Unix/Command/_git
+++ b/Completion/Unix/Command/_git
@@ -495,6 +495,8 @@ _git-checkout () {
 
 (( $+functions[_git-cherry-pick] )) ||
 _git-cherry-pick () {
+  local -a git_commit_opts
+  git_commit_opts=(--all --not HEAD --not)
   _arguments \
     '(- :)--quit[end revert or cherry-pick sequence]' \
     '(- :)--continue[resume revert or cherry-pick sequence]' \
@@ -511,7 +513,7 @@ _git-cherry-pick () {
     '*'{-s,--strategy=}'[use given merge strategy]:merge strategy:__git_merge_strategies' \
     '*'{-X,--strategy-option=}'[pass merge-strategy-specific option to merge strategy]' \
     '(-e --edit -x -n --no-commit -s --signoff)--ff[fast forward, if possible]' \
-    ': :__git_commit_ranges'
+    ': : __git_commit_ranges -O expl:git_commit_opts'
 }
 
 (( $+functions[_git-citool] )) ||
@@ -5602,20 +5604,28 @@ __git_remote_branch_names_noprefix () {
 
 (( $+functions[__git_commit_objects_prefer_recent] )) ||
 __git_commit_objects_prefer_recent () {
-  __git_recent_commits || __git_commit_objects
+  local -a argument_array_names
+  zparseopts -D -E O:=argument_array_names
+
+  __git_recent_commits $argument_array_names || __git_commit_objects
 }
 
 (( $+functions[__git_commits] )) ||
 __git_commits () {
+  local -a argument_array_names
+  zparseopts -D -E O:=argument_array_names
+  (( $#argument_array_names )) && argument_array_names=( "${(@s/:/)argument_array_names[2]}" ) && set -- "${(@P)argument_array_names[1]}"
+
   # TODO: deal with things that __git_heads and __git_tags has in common (i.e.,
   # if both exists, they need to be completed to heads/x and tags/x.
-  local -a sopts ropt
+  local -a sopts ropt expl
   zparseopts -E -a sopts S: r:=ropt R: q
   sopts+=( $ropt:q )
+  expl=( "$@" )
   _alternative \
     "heads::__git_heads $sopts" \
     "commit-tags::__git_commit_tags $sopts" \
-    'commit-objects::__git_commit_objects_prefer_recent'
+    'commit-objects:: __git_commit_objects_prefer_recent -O expl:$argument_array_names[2]'
 }
 
 (( $+functions[__git_heads] )) ||
@@ -5670,13 +5680,17 @@ __git_commit_objects () {
 (( $+functions[__git_recent_commits] )) ||
 __git_recent_commits () {
   local gitdir expl start
-  declare -a descr tags heads commits
+  declare -a descr tags heads commits argument_array_names commit_opts
   local i j k ret
   integer distance_from_head
 
+  zparseopts -D -E O:=argument_array_names
+  (( $#argument_array_names )) && argument_array_names=( "${(@s/:/)argument_array_names[2]}" )
+  (( $#argument_array_names > 1 )) && commit_opts=( "${(@P)argument_array_names[2]}" )
+
   # Careful: most %d will expand to the empty string.  Quote properly!
   # NOTE: we could use %D directly, but it's not available in git 1.9.1 at least.
-  commits=("${(f)"$(_call_program commits git --no-pager log -20 --format='%h%n%d%n%s\ \(%cr\)')"}")
+  commits=("${(f)"$(_call_program commits git --no-pager log $commit_opts -20 --format='%h%n%d%n%s\ \(%cr\)')"}")
   __git_command_successful $pipestatus || return 1
 
   for i j k in "$commits[@]" ; do
@@ -5759,13 +5773,19 @@ __git_commits2 () {
 
 (( $+functions[__git_commit_ranges] )) ||
 __git_commit_ranges () {
-  local -a suf
+  local -a argument_array_names
+  zparseopts -D -E O:=argument_array_names
+  (( $#argument_array_names )) && argument_array_names=( "${(@s/:/)argument_array_names[2]}" ) && set -- "${(@P)argument_array_names[1]}"
+
+  local -a expl suf
   if compset -P '*..(.|)'; then
-    __git_commits $*
+    expl=( $* )
   else
     compset -S '..*' || suf=( -S .. -r '.@~ ^:\t\n\-' )
-    __git_commits $* $suf
+    expl=( $* $suf )
   fi
+
+  __git_commits -O expl:$argument_array_names[2]
 }
 
 (( $+functions[__git_commit_ranges2] )) ||

Re: PATCH: 3.0.8: git completion update for cherry-pick

[Daniel Shahaf]

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

Mateusz Karbowy wrote on Sat, Aug 29, 2015 at 17:53:57 +0100:
> I've fixed the no-quotes issue with $@ and now I'm passing both
> compadd and git commit options using one -O parameter (the array names
> are separated with colon, as you suggested).

Looks good.

I've made a few changes (see attached series):

1. Some style tweaks

2. Avoid printing wrong [HEAD~$n] descriptions in __git_recent_commits

3. Extra safety check in case somebody passed just the traditional '-O
expl' with no colon

I'll commit the patch with those changes (as soon as I get an X-Seq
number for this email).  Many thanks!

Daniel


[-- Attachment #2: 0001-minor-Comment-and-style-fixes.-No-functional-change.patch --]
[-- Type: text/x-patch, Size: 2791 bytes --]

>From 496068c764074a5816b09d456b34e0e6096d988a Mon Sep 17 00:00:00 2001
From: Daniel Shahaf <d.s@daniel.shahaf.name>
Date: Sun, 30 Aug 2015 11:26:39 +0000
Subject: [PATCH 1/3] minor: Comment and style fixes.  No functional change.

---
 Completion/Unix/Command/_git | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/Completion/Unix/Command/_git b/Completion/Unix/Command/_git
index 18f9e7c..69d7719 100644
--- a/Completion/Unix/Command/_git
+++ b/Completion/Unix/Command/_git
@@ -5568,7 +5568,10 @@ __git_commit_objects_prefer_recent () {
 __git_commits () {
   local -a argument_array_names
   zparseopts -D -E O:=argument_array_names
-  (( $#argument_array_names )) && argument_array_names=( "${(@s/:/)argument_array_names[2]}" ) && set -- "${(@P)argument_array_names[1]}"
+  # Turn (-O foo:bar) to (foo bar)
+  (( $#argument_array_names )) && argument_array_names=( "${(@s/:/)argument_array_names[2]}" )
+  set -- "${(@P)argument_array_names[1]}"
+  local commit_opts__argument_name=$argument_array_names[2]
 
   # TODO: deal with things that __git_heads and __git_tags has in common (i.e.,
   # if both exists, they need to be completed to heads/x and tags/x.
@@ -5579,7 +5582,7 @@ __git_commits () {
   _alternative \
     "heads::__git_heads $sopts" \
     "commit-tags::__git_commit_tags $sopts" \
-    'commit-objects:: __git_commit_objects_prefer_recent -O expl:$argument_array_names[2]'
+    'commit-objects:: __git_commit_objects_prefer_recent -O expl:$commit_opts__argument_name'
 }
 
 (( $+functions[__git_heads] )) ||
@@ -5639,6 +5642,7 @@ __git_recent_commits () {
   integer distance_from_head
 
   zparseopts -D -E O:=argument_array_names
+  # Turn (-O foo:bar) to (foo bar)
   (( $#argument_array_names )) && argument_array_names=( "${(@s/:/)argument_array_names[2]}" )
   (( $#argument_array_names > 1 )) && commit_opts=( "${(@P)argument_array_names[2]}" )
 
@@ -5729,9 +5733,13 @@ __git_commits2 () {
 __git_commit_ranges () {
   local -a argument_array_names
   zparseopts -D -E O:=argument_array_names
-  (( $#argument_array_names )) && argument_array_names=( "${(@s/:/)argument_array_names[2]}" ) && set -- "${(@P)argument_array_names[1]}"
+  # Turn (-O foo:bar) to (foo bar)
+  (( $#argument_array_names )) && argument_array_names=( "${(@s/:/)argument_array_names[2]}" )
+  set -- "${(@P)argument_array_names[1]}"
+  local commit_opts__argument_name=$argument_array_names[2]
 
-  local -a expl suf
+  local -a suf
+  local -a expl
   if compset -P '*..(.|)'; then
     expl=( $* )
   else
@@ -5739,7 +5747,7 @@ __git_commit_ranges () {
     expl=( $* $suf )
   fi
 
-  __git_commits -O expl:$argument_array_names[2]
+  __git_commits -O expl:$commit_opts__argument_name
 }
 
 (( $+functions[__git_commit_ranges2] )) ||
-- 
2.1.4


[-- Attachment #3: 0002-Don-t-show-wrong-labels.patch --]
[-- Type: text/x-patch, Size: 2124 bytes --]

>From a7936c3d11dae2cecfb185cba2db5310c9e9c2b4 Mon Sep 17 00:00:00 2001
From: Daniel Shahaf <d.s@daniel.shahaf.name>
Date: Sun, 30 Aug 2015 11:41:32 +0000
Subject: [PATCH 2/3] Don't show wrong labels

---
 Completion/Unix/Command/_git | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/Completion/Unix/Command/_git b/Completion/Unix/Command/_git
index 69d7719..dd2d771 100644
--- a/Completion/Unix/Command/_git
+++ b/Completion/Unix/Command/_git
@@ -5640,6 +5640,7 @@ __git_recent_commits () {
   declare -a descr tags heads commits argument_array_names commit_opts
   local i j k ret
   integer distance_from_head
+  local label
 
   zparseopts -D -E O:=argument_array_names
   # Turn (-O foo:bar) to (foo bar)
@@ -5654,17 +5655,25 @@ __git_recent_commits () {
   for i j k in "$commits[@]" ; do
     # Note: the after-the-colon part must be unique across the entire array;
     # see workers/34768
-    if (( distance_from_head == 0 )); then
-      descr+=($i:"[HEAD]    $k")
+    if (( $#commit_opts )); then
+      # $commit_opts is set, so the commits we receive might not be in order,
+      # or might not be ancestors of HEAD.  However, we must make the
+      # description unique (due to workers/34768), which we do by including the
+      # hash.  Git always prints enough hash digits to make the output unique.)
+      label="[$i]"
+    elif (( distance_from_head == 0 )); then
+      label="[HEAD]   "
     elif (( distance_from_head == 1 )); then
-      descr+=($i:"[HEAD^]   $k")
+      label="[HEAD^]  "
     elif (( distance_from_head == 2 )); then
-      descr+=($i:"[HEAD^^]  $k")
+      label="[HEAD^^] "
     elif (( distance_from_head < 10 )); then
-      descr+=($i:"[HEAD~$distance_from_head]  $k")
+      label="[HEAD~$distance_from_head] "
     else
-      descr+=($i:"[HEAD~$distance_from_head] $k")
+      label="[HEAD~$distance_from_head]"
     fi
+    # label is now 9 bytes, so the descriptions ($k) will be aligned.
+    descr+=($i:"${label} $k")
     (( ++distance_from_head ))
 
     j=${${j# \(}%\)} # strip leading ' (' and trailing ')'
-- 
2.1.4


[-- Attachment #4: 0003-belt-and-braces-for-the-O-expl-empty-string-case.patch --]
[-- Type: text/x-patch, Size: 1105 bytes --]

>From 7fe49eca0d39d3cc566fb6480b87fb16fc1caf13 Mon Sep 17 00:00:00 2001
From: Daniel Shahaf <d.s@daniel.shahaf.name>
Date: Sun, 30 Aug 2015 13:01:29 +0000
Subject: [PATCH 3/3] belt and braces for the -O 'expl:<empty string>' case

---
 Completion/Unix/Command/_git | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Completion/Unix/Command/_git b/Completion/Unix/Command/_git
index dd2d771..2c79ed0 100644
--- a/Completion/Unix/Command/_git
+++ b/Completion/Unix/Command/_git
@@ -5645,7 +5645,8 @@ __git_recent_commits () {
   zparseopts -D -E O:=argument_array_names
   # Turn (-O foo:bar) to (foo bar)
   (( $#argument_array_names )) && argument_array_names=( "${(@s/:/)argument_array_names[2]}" )
-  (( $#argument_array_names > 1 )) && commit_opts=( "${(@P)argument_array_names[2]}" )
+  (( $#argument_array_names > 1 )) && && ${(P)+argument_array_names[2]} &&
+    commit_opts=( "${(@P)argument_array_names[2]}" )
 
   # Careful: most %d will expand to the empty string.  Quote properly!
   # NOTE: we could use %D directly, but it's not available in git 1.9.1 at least.
-- 
2.1.4

Re: PATCH: 3.0.8: git completion update for cherry-pick

[Mateusz Karbowy]

Thanks Daniel! :)

On 30 August 2015 at 23:39, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> Mateusz Karbowy wrote on Sat, Aug 29, 2015 at 17:53:57 +0100:
>> I've fixed the no-quotes issue with $@ and now I'm passing both
>> compadd and git commit options using one -O parameter (the array names
>> are separated with colon, as you suggested).
>
> Looks good.
>
> I've made a few changes (see attached series):
>
> 1. Some style tweaks
>
> 2. Avoid printing wrong [HEAD~$n] descriptions in __git_recent_commits
>
> 3. Extra safety check in case somebody passed just the traditional '-O
> expl' with no colon
>
> I'll commit the patch with those changes (as soon as I get an X-Seq
> number for this email).  Many thanks!
>
> Daniel
>

Re: PATCH: 3.0.8: git completion update for cherry-pick

[Daniel Shahaf]

Daniel Shahaf wrote on Sun, Aug 30, 2015 at 22:39:45 +0000:
> I've made a few changes (see attached series):
...
> 3. Extra safety check in case somebody passed just the traditional '-O
> expl' with no colon
...
> -  (( $#argument_array_names > 1 )) && commit_opts=( "${(@P)argument_array_names[2]}" )
> +  (( $#argument_array_names > 1 )) && && ${(P)+argument_array_names[2]} &&
> +    commit_opts=( "${(@P)argument_array_names[2]}" )

That was an invalid syntax.

diff --git a/Completion/Unix/Command/_git b/Completion/Unix/Command/_git
index 2c79ed0..7f7c3eb 100644
--- a/Completion/Unix/Command/_git
+++ b/Completion/Unix/Command/_git
@@ -5645,7 +5645,7 @@ __git_recent_commits () {
   zparseopts -D -E O:=argument_array_names
   # Turn (-O foo:bar) to (foo bar)
   (( $#argument_array_names )) && argument_array_names=( "${(@s/:/)argument_array_names[2]}" )
-  (( $#argument_array_names > 1 )) && && ${(P)+argument_array_names[2]} &&
+  (( $#argument_array_names > 1 )) && (( ${(P)+argument_array_names[2]} )) &&
     commit_opts=( "${(@P)argument_array_names[2]}" )
 
   # Careful: most %d will expand to the empty string.  Quote properly!