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

[Mateusz Karbowy <mateusz.karbowy@xxxxxxxxx>]

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