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

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

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