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

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

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