From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6637 invoked by alias); 11 Aug 2015 22:18:02 -0000 Mailing-List: contact zsh-workers-help@zsh.org; run by ezmlm Precedence: bulk X-No-Archive: yes List-Id: Zsh Workers List List-Post: List-Help: X-Seq: 36111 Received: (qmail 16283 invoked from network); 11 Aug 2015 22:18:01 -0000 X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on f.primenet.com.au X-Spam-Level: X-Spam-Status: No, score=-2.7 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,RCVD_IN_DNSWL_LOW,RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.0 DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d= daniel.shahaf.name; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-sasl-enc:x-sasl-enc; s=mesmtp; bh=qYKIAcsq/5l6Z1kG M6YIhbiRO/k=; b=NN6oFbruZEbzlUzBaSxoGKk2/fOyssaM6SuXh+hRgAPpy9cm jqJKOqtf4zYdaEfXKuzWMN9oO4I7+hULImY08eaJxOGFZ0sb2FWPTtdurk1ZwUXy 3uIpP8s1SVFIbRboYaDQL/spBRUMj0arPaTnJFCFSDNHdf7jeSUiMYUS1/c= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-sasl-enc:x-sasl-enc; s=smtpout; bh=qYKIAcsq/5l6Z1k GM6YIhbiRO/k=; b=eo9UnD2oI9oxMw7Zq5S87AbkxvrtiA1+zYP2//upqOowvtM sSGL/mq1c6aB7aTD0k3brqvAsZ3CETD7NKYi9R3SQl1vEXxIOFyMX20btKGtvx53 vxAlaPPb+B5dLp1y7LaaKuZ3rP279RUgM9ZgdmUEZ8+ZMCugvpog4pSuwpHA= X-Sasl-enc: hhFGgvypZB26BgaqwdSCzhRR6Yk5DHl9WQw69+duggrx 1439330795 Date: Tue, 11 Aug 2015 22:06:32 +0000 From: Daniel Shahaf To: Mateusz Karbowy Cc: zsh-workers@zsh.org Subject: Re: PATCH: 3.0.8: git completion update for cherry-pick Message-ID: <20150811220632.GE1859@tarsus.local2> References: <20150722115307.GC2171@tarsus.local2> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Mateusz Karbowy wrote on Mon, Aug 10, 2015 at 22:31:42 +0100: > On 22 July 2015 at 12:53, Daniel Shahaf 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