From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12496 invoked by alias); 10 Aug 2015 21:31:49 -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: 36087 Received: (qmail 8815 invoked from network); 10 Aug 2015 21:31:45 -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,FREEMAIL_FROM,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-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; bh=87FXDyXguC1VXAVTQubuAKYjlsyRy+tjxuYmVcNZvQo=; b=y0FLGG9sKkIRJmW9NHr8WiljmCWpVbeiLuzLtoUAdimc5J+e/YYx4568NklJVmFE+U Gxg9768f2aWoMQrokeEOZqLMLepoH0O/P2dtT46MuivBF21G070e6imqMbQjliw1VI90 5oYLvk50vH7zlIGAf9PowCXZc/GfXF1qAPRyFqnC0vWEOvhxBVD7S28IIZY0zHPHA61/ rLvdoYbARuhYG1XXkyYNQw03RkDBnsJ9n/57QaOgJXxlCEd6vGVFSP1QIvW85JjDdGgf xa2xrqMFroArz+EQNhE4jA5OCHz9xgoP3e9FGnFX9Gz3M0476+54m9D/UDz8g7Mm5vb1 kcUw== MIME-Version: 1.0 X-Received: by 10.55.19.98 with SMTP id d95mr41359953qkh.71.1439242302998; Mon, 10 Aug 2015 14:31:42 -0700 (PDT) In-Reply-To: <20150722115307.GC2171@tarsus.local2> References: <20150722115307.GC2171@tarsus.local2> Date: Mon, 10 Aug 2015 22:31:42 +0100 Message-ID: Subject: Re: PATCH: 3.0.8: git completion update for cherry-pick From: Mateusz Karbowy To: Daniel Shahaf Cc: zsh-workers@zsh.org Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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=3D}'[use given merge strategy]:merge >> strategy:__git_merge_strategies' \ >> '*'{-X,--strategy-option=3D}'[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 =E2=80=94 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 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?). >> - commits=3D("${(f)"$(_call_program commits git --no-pager log -20 >> --format=3D'%h%n%d%n%s\ \(%cr\)')"}") >> + commits=3D("${(f)"$(_call_program commits git --no-pager log $opts >> -20 --format=3D'%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 > Good piece of advice. I thought something was wrong with the lines. Thanks! Regards, Mateusz