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

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

Mateusz Karbowy wrote on Tue, Aug 25, 2015 at 23:26:21 +0100:
> My last patch interfered with git-checkout. I've fixed it this time.
> 

Sorry for the late reply.

I have only two comments about this patch: one bugfix and one question.

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

There are other callers of __git_commit_ranges that pass compadd options in
argv.  There is already a compadd option "-C", therefore, choosing this option
letter prevents any caller of __git_commit_ranges from passing the compadd -C
option.

Is this a problem?  Should we find some other way to pass the name
"git_commit_opts"?  If so, what option letters are available?  (Since "-O" is
available, maybe we could pass both array names in it as '-O
"expl:git_commit_opts"'?  I.e., if a colon is present in the argument
value, split on it, else assume the entire argument is a single
parameter name, as in the conventional "-O expl"?)

>  __git_commits () {
> ...
> +  expl=( $@ )

The $@ should be double-quoted to avoid eliding empty arguments.

> 

Apart from these two issues, the patch seems ready for commit to me.

Thanks again.

Cheers,

Daniel