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

[Mateusz Karbowy <mateusz.karbowy@xxxxxxxxx>]

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

On 22 July 2015 at 12:53, Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx> 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
>

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