From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18214 invoked by alias); 19 Aug 2015 17:10:39 -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: 36245 Received: (qmail 25466 invoked from network); 19 Aug 2015 17:10:38 -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.0 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM 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; bh=SCT7pICIeceF8JNmv4qSZHowIh7UCM6PeaNCo+n8QfU=; b=jM7kHrzA8xhrmCzGMY5vBjyaPPUe9I5/8majFPdNzSKhE3HxC5PYz59ZWr+TUm4kZS VWH41dyXuRG3pQCosBaSSigXzo/wFKxTtYOWtcHZg0eNXIVI3ZqPThfSDhYcfDReCphk 2Fbtrx060//T1ub2qnEZ6pm+7lShLSQM/ruFq+TtI/uk80N1BDJedtwOnm5rprqzQueb T/FlXk/SW1lkKvdJCwiAs1LPPPSxIMR2K1sBH0ylZY6wlmKXXb7lU79WeCqk/VdlDBSD VyPv3S7e9XFBjEjJOy6ubezSY28IC8HSS86qTZZEjZTh+Fj2LRi4638dLJXmN684cKjj kO2w== MIME-Version: 1.0 X-Received: by 10.140.144.207 with SMTP id 198mr26169781qhq.38.1440004233849; Wed, 19 Aug 2015 10:10:33 -0700 (PDT) In-Reply-To: <20150811220632.GE1859@tarsus.local2> References: <20150722115307.GC2171@tarsus.local2> <20150811220632.GE1859@tarsus.local2> Date: Wed, 19 Aug 2015 18:10:33 +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: multipart/mixed; boundary=001a1136f9f4d59c9e051dad1cb0 --001a1136f9f4d59c9e051dad1cb0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Hello Daniel Thanks again for your comprehensive reply and sorry I stalled it for so lon= g. I went for the variable approach, as I'm a noob if it comes to zsh internals and your first suggestion is still a bit arcane to me :) CR please. Regards, Mateusz On 11 August 2015 at 23:06, Daniel Shahaf wrote: > 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 exce= pt 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 optio= n >> >> 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 anothe= r >> > 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 ar= gv >> > 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 ke= ep the >> > compadd flags passed in argv and pass the rev-list flags via some othe= r >> > 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 =C2=ABfoo=C2= =BB (no > leading space) is equivalent to the action =C2=AB foo ${expl}=C2=BB (with= leading > space). > > What we could do here is have foo called as =C2=AB foo -O expl -a "--all > --not HEAD"=C2=BB and let foo do =C2=ABgetopts "a:O:"=C2=BB (or zparseopt= s) and convert > the word 'expl' to the contents of the array by that name; compare the > call =C2=AB_alternative -O expl $other_arguments=C2=BB 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=3D1; 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. Than= ks! > > You're welcome. > > Cheers, > > Daniel --001a1136f9f4d59c9e051dad1cb0 Content-Type: text/plain; charset=US-ASCII; name="_git.patch.txt" Content-Disposition: attachment; filename="_git.patch.txt" Content-Transfer-Encoding: base64 X-Attachment-Id: f_idj1c55r0 ZGlmZiAtLWdpdCBhL0NvbXBsZXRpb24vVW5peC9Db21tYW5kL19naXQgYi9Db21wbGV0aW9uL1Vu aXgvQ29tbWFuZC9fZ2l0CmluZGV4IGI4ZWRjMTAuLmRmOTYzYzQgMTAwNjQ0Ci0tLSBhL0NvbXBs ZXRpb24vVW5peC9Db21tYW5kL19naXQKKysrIGIvQ29tcGxldGlvbi9Vbml4L0NvbW1hbmQvX2dp dApAQCAtNDk1LDYgKzQ5NSw3IEBAIF9naXQtY2hlY2tvdXQgKCkgewogCiAoKCAkK2Z1bmN0aW9u c1tfZ2l0LWNoZXJyeS1waWNrXSApKSB8fAogX2dpdC1jaGVycnktcGljayAoKSB7CisgIGxvY2Fs IF9fR0lUX0VYVFJBX09QVElPTlNfRk9SX1JFQ0VOVF9DT01NSVRTPSctLWFsbCAtLW5vdCBIRUFE JwogICBfYXJndW1lbnRzIFwKICAgICAnKC0gOiktLXF1aXRbZW5kIHJldmVydCBvciBjaGVycnkt cGljayBzZXF1ZW5jZV0nIFwKICAgICAnKC0gOiktLWNvbnRpbnVlW3Jlc3VtZSByZXZlcnQgb3Ig Y2hlcnJ5LXBpY2sgc2VxdWVuY2VdJyBcCkBAIC01Njc2LDcgKzU2NzcsNyBAQCBfX2dpdF9yZWNl bnRfY29tbWl0cyAoKSB7CiAKICAgIyBDYXJlZnVsOiBtb3N0ICVkIHdpbGwgZXhwYW5kIHRvIHRo ZSBlbXB0eSBzdHJpbmcuICBRdW90ZSBwcm9wZXJseSEKICAgIyBOT1RFOiB3ZSBjb3VsZCB1c2Ug JUQgZGlyZWN0bHksIGJ1dCBpdCdzIG5vdCBhdmFpbGFibGUgaW4gZ2l0IDEuOS4xIGF0IGxlYXN0 LgotICBjb21taXRzPSgiJHsoZikiJChfY2FsbF9wcm9ncmFtIGNvbW1pdHMgZ2l0IC0tbm8tcGFn ZXIgbG9nIC0yMCAtLWZvcm1hdD0nJWglbiVkJW4lc1wgXCglY3JcKScpIn0iKQorICBjb21taXRz PSgiJHsoZikiJChfY2FsbF9wcm9ncmFtIGNvbW1pdHMgZ2l0IC0tbm8tcGFnZXIgbG9nICRfX0dJ VF9FWFRSQV9PUFRJT05TX0ZPUl9SRUNFTlRfQ09NTUlUUyAtMjAgLS1mb3JtYXQ9JyVoJW4lZCVu JXNcIFwoJWNyXCknKSJ9IikKICAgX19naXRfY29tbWFuZF9zdWNjZXNzZnVsICRwaXBlc3RhdHVz IHx8IHJldHVybiAxCiAKICAgZm9yIGkgaiBrIGluICIkY29tbWl0c1tAXSIgOyBkbwo= --001a1136f9f4d59c9e051dad1cb0--