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

[Mateusz Karbowy <mateusz.karbowy@gmail.com>]

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