From: Daniel Shahaf <d.s@daniel.shahaf.name>
To: Mateusz Karbowy <mateusz.karbowy@gmail.com>
Cc: zsh-workers@zsh.org
Subject: Re: PATCH: 3.0.8: git completion update for cherry-pick
Date: Tue, 11 Aug 2015 22:06:32 +0000 [thread overview]
Message-ID: <20150811220632.GE1859@tarsus.local2> (raw)
In-Reply-To: <CAFiR=JuAH5r4C9YHCvtLG1Gh0pSQFXpE37=hiXWaFf5oMQxPzA@mail.gmail.com>
Mateusz Karbowy wrote on Mon, Aug 10, 2015 at 22:31:42 +0100:
> On 22 July 2015 at 12:53, Daniel Shahaf <d.s@daniel.shahaf.name> 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
next prev parent reply other threads:[~2015-08-11 22:18 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-18 22:19 Mateusz Karbowy
2015-07-22 11:53 ` Daniel Shahaf
2015-08-10 21:31 ` Mateusz Karbowy
2015-08-11 22:06 ` Daniel Shahaf [this message]
2015-08-19 17:10 ` Mateusz Karbowy
2015-08-21 19:55 ` Mateusz Karbowy
2015-08-23 6:42 ` Daniel Shahaf
2015-08-25 22:26 ` Mateusz Karbowy
2015-08-27 23:11 ` Daniel Shahaf
2015-08-27 23:19 ` Bart Schaefer
2015-08-27 23:28 ` Daniel Shahaf
2015-08-29 16:53 ` Mateusz Karbowy
2015-08-30 22:39 ` Daniel Shahaf
2015-08-31 7:50 ` Mateusz Karbowy
2015-09-01 4:19 ` Daniel Shahaf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150811220632.GE1859@tarsus.local2 \
--to=d.s@daniel.shahaf.name \
--cc=mateusz.karbowy@gmail.com \
--cc=zsh-workers@zsh.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.vuxu.org/mirror/zsh/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).