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