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

[Mateusz Karbowy <mateusz.karbowy@xxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxx> 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@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?
>> >
>> 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

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