From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16679 invoked by alias); 21 Aug 2015 19:55:14 -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: 36269 Received: (qmail 19613 invoked from network); 21 Aug 2015 19:55:13 -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=Fx7bn7o3lbD3H1dvE4xOExtrnuomd4VSuyFFq2dFkyE=; b=SxBRi6cB0zHGnawXcnRVm5RQxSRiZ1Ir3esZlJa1sVQRdtnDxO60zW0IsXnp96vRgZ Go1A+bEWAT9NFWDCg1Y2uABwWEY+uZ08RMmBitIaKxKZzLz3eHkAd9GOU0ERWASqHVAU T51gZCUx24IXGieAxTu7OlTcwbepAxZRq68aEOapfJP0M2+gGkC/ASgJIi/T7WQvJz3s AkDElxbPJay2yrbN/Qty0W0Z+Co+22ZmyO+/MP8TpJ3qer0eSQQYiKz0tyvQpd4uEn5Q XTldJRUSAtAWme8ab0+uBREB9wdTevhN6ont97ENd141gs1bCq0kVJjyAZBx7ECluYTD pbAw== MIME-Version: 1.0 X-Received: by 10.55.201.85 with SMTP id q82mr21745002qki.27.1440186907941; Fri, 21 Aug 2015 12:55:07 -0700 (PDT) In-Reply-To: References: <20150722115307.GC2171@tarsus.local2> <20150811220632.GE1859@tarsus.local2> Date: Fri, 21 Aug 2015 20:55:07 +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=001a11479eee0f2119051dd7a54e --001a11479eee0f2119051dd7a54e Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Hey Daniel I think this covers it. I know you mentioned I shouldn't need to use the ${(P)} in __git_commit_ranges, but there were these two calls to __git_commits with either '$*' or '$* $suf', and they operated on compadd params before the change, so I thought it's safer this way. What do you think? Regards, Obszczymucha On 19 August 2015 at 18:10, Mateusz Karbowy wro= te: > Hello Daniel > > Thanks again for your comprehensive reply and sorry I stalled it for so l= ong. > 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 th= e >>> >> current branch. >>> >> This patch changes this to show recent commits from all branches exc= ept 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 opti= on >>> >> to merge strategy]' \ >>> >> '(-e --edit -x -n --no-commit -s --signoff)--ff[fast forward, i= f >>> >> 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 anoth= er >>> > 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 a= rgv >>> > 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 k= eep the >>> > compadd flags passed in argv and pass the rev-list flags via some oth= er >>> > channel, such as a well-known parameter name (the caller defines >>> > a variable with a specific declared-in-the-(internal)-API name and th= e >>> > 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 (wit= h 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 zparseop= ts) 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-emai= l. >> >> 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. Tha= nks! >> >> You're welcome. >> >> Cheers, >> >> Daniel --001a11479eee0f2119051dd7a54e 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_idm21s281 ZGlmZiAtLWdpdCBhL0NvbXBsZXRpb24vVW5peC9Db21tYW5kL19naXQgYi9Db21wbGV0aW9uL1Vu aXgvQ29tbWFuZC9fZ2l0CmluZGV4IGI4ZWRjMTAuLjVmNGZlYWQgMTAwNjQ0Ci0tLSBhL0NvbXBs ZXRpb24vVW5peC9Db21tYW5kL19naXQKKysrIGIvQ29tcGxldGlvbi9Vbml4L0NvbW1hbmQvX2dp dApAQCAtNDk1LDYgKzQ5NSw4IEBAIF9naXQtY2hlY2tvdXQgKCkgewogCiAoKCAkK2Z1bmN0aW9u c1tfZ2l0LWNoZXJyeS1waWNrXSApKSB8fAogX2dpdC1jaGVycnktcGljayAoKSB7CisgIGxvY2Fs IC1hIGdpdF9jb21taXRfb3B0cworICBnaXRfY29tbWl0X29wdHM9KC0tYWxsIC0tbm90IEhFQUQg LS1ub3QpCiAgIF9hcmd1bWVudHMgXAogICAgICcoLSA6KS0tcXVpdFtlbmQgcmV2ZXJ0IG9yIGNo ZXJyeS1waWNrIHNlcXVlbmNlXScgXAogICAgICcoLSA6KS0tY29udGludWVbcmVzdW1lIHJldmVy dCBvciBjaGVycnktcGljayBzZXF1ZW5jZV0nIFwKQEAgLTUxMSw3ICs1MTMsNyBAQCBfZ2l0LWNo ZXJyeS1waWNrICgpIHsKICAgICAnKid7LXMsLS1zdHJhdGVneT19J1t1c2UgZ2l2ZW4gbWVyZ2Ug c3RyYXRlZ3ldOm1lcmdlIHN0cmF0ZWd5Ol9fZ2l0X21lcmdlX3N0cmF0ZWdpZXMnIFwKICAgICAn Kid7LVgsLS1zdHJhdGVneS1vcHRpb249fSdbcGFzcyBtZXJnZS1zdHJhdGVneS1zcGVjaWZpYyBv cHRpb24gdG8gbWVyZ2Ugc3RyYXRlZ3ldJyBcCiAgICAgJygtZSAtLWVkaXQgLXggLW4gLS1uby1j b21taXQgLXMgLS1zaWdub2ZmKS0tZmZbZmFzdCBmb3J3YXJkLCBpZiBwb3NzaWJsZV0nIFwKLSAg ICAnOiA6X19naXRfY29tbWl0X3JhbmdlcycKKyAgICAnOiA6IF9fZ2l0X2NvbW1pdF9yYW5nZXMg LU8gZXhwbCAtQyBnaXRfY29tbWl0X29wdHMnCiB9CiAKICgoICQrZnVuY3Rpb25zW19naXQtY2l0 b29sXSApKSB8fApAQCAtNTYwMiwyMCArNTYwNCwyOSBAQCBfX2dpdF9yZW1vdGVfYnJhbmNoX25h bWVzX25vcHJlZml4ICgpIHsKIAogKCggJCtmdW5jdGlvbnNbX19naXRfY29tbWl0X29iamVjdHNf cHJlZmVyX3JlY2VudF0gKSkgfHwKIF9fZ2l0X2NvbW1pdF9vYmplY3RzX3ByZWZlcl9yZWNlbnQg KCkgewotICBfX2dpdF9yZWNlbnRfY29tbWl0cyB8fCBfX2dpdF9jb21taXRfb2JqZWN0cworICBs b2NhbCAtQSBjb21taXRfb3B0c19hcnJheV9uYW1lCisgIHpwYXJzZW9wdHMgLUQgLUUgQzo9Y29t bWl0X29wdHNfYXJyYXlfbmFtZQorCisgIF9fZ2l0X3JlY2VudF9jb21taXRzIC1DICRjb21taXRf b3B0c19hcnJheV9uYW1lIHx8IF9fZ2l0X2NvbW1pdF9vYmplY3RzCiB9CiAKICgoICQrZnVuY3Rp b25zW19fZ2l0X2NvbW1pdHNdICkpIHx8CiBfX2dpdF9jb21taXRzICgpIHsKKyAgIyBTZXBhcmF0 ZSBjb21wYWRkIG9wdGlvbnMgd2l0aCBnaXQgY29tbWl0IG9wdGlvbnMuCisgIGxvY2FsIC1BIGNv bXBhZGRfb3B0c19hcnJheV9uYW1lIGNvbW1pdF9vcHRzX2FycmF5X25hbWUKKyAgenBhcnNlb3B0 cyAtRCAtRSBPOj1jb21wYWRkX29wdHNfYXJyYXlfbmFtZSBDOj1jb21taXRfb3B0c19hcnJheV9u YW1lCisgICgoICQjY29tcGFkZF9vcHRzX2FycmF5X25hbWUgKSkgJiYgc2V0IC0tICIkeyhAUClj b21wYWRkX29wdHNfYXJyYXlfbmFtZX0iCisKICAgIyBUT0RPOiBkZWFsIHdpdGggdGhpbmdzIHRo YXQgX19naXRfaGVhZHMgYW5kIF9fZ2l0X3RhZ3MgaGFzIGluIGNvbW1vbiAoaS5lLiwKICAgIyBp ZiBib3RoIGV4aXN0cywgdGhleSBuZWVkIHRvIGJlIGNvbXBsZXRlZCB0byBoZWFkcy94IGFuZCB0 YWdzL3guCi0gIGxvY2FsIC1hIHNvcHRzIHJvcHQKKyAgbG9jYWwgLWEgc29wdHMgcm9wdCBleHBs CiAgIHpwYXJzZW9wdHMgLUUgLWEgc29wdHMgUzogcjo9cm9wdCBSOiBxCiAgIHNvcHRzKz0oICRy b3B0OnEgKQorICBleHBsPSggJEAgKQogICBfYWx0ZXJuYXRpdmUgXAogICAgICJoZWFkczo6X19n aXRfaGVhZHMgJHNvcHRzIiBcCiAgICAgImNvbW1pdC10YWdzOjpfX2dpdF9jb21taXRfdGFncyAk c29wdHMiIFwKLSAgICAnY29tbWl0LW9iamVjdHM6Ol9fZ2l0X2NvbW1pdF9vYmplY3RzX3ByZWZl cl9yZWNlbnQnCisgICAgJ2NvbW1pdC1vYmplY3RzOjogX19naXRfY29tbWl0X29iamVjdHNfcHJl ZmVyX3JlY2VudCAtTyBleHBsIC1DICRjb21taXRfb3B0c19hcnJheV9uYW1lJwogfQogCiAoKCAk K2Z1bmN0aW9uc1tfX2dpdF9oZWFkc10gKSkgfHwKQEAgLTU2NzAsMTMgKzU2ODEsMTcgQEAgX19n aXRfY29tbWl0X29iamVjdHMgKCkgewogKCggJCtmdW5jdGlvbnNbX19naXRfcmVjZW50X2NvbW1p dHNdICkpIHx8CiBfX2dpdF9yZWNlbnRfY29tbWl0cyAoKSB7CiAgIGxvY2FsIGdpdGRpciBleHBs IHN0YXJ0Ci0gIGRlY2xhcmUgLWEgZGVzY3IgdGFncyBoZWFkcyBjb21taXRzCisgIGxvY2FsIC1B IGNvbW1pdF9vcHRzX2FycmF5X25hbWUKKyAgZGVjbGFyZSAtYSBkZXNjciB0YWdzIGhlYWRzIGNv bW1pdHMgY29tbWl0X29wdHMKICAgbG9jYWwgaSBqIGsgcmV0CiAgIGludGVnZXIgZGlzdGFuY2Vf ZnJvbV9oZWFkCiAKKyAgenBhcnNlb3B0cyAtRCAtRSBDOj1jb21taXRfb3B0c19hcnJheV9uYW1l CisgICgoICQjY29tbWl0X29wdHNfYXJyYXlfbmFtZSApKSAmJiBjb21taXRfb3B0cz0oICIkeyhA UCljb21taXRfb3B0c19hcnJheV9uYW1lfSIgKQorCiAgICMgQ2FyZWZ1bDogbW9zdCAlZCB3aWxs IGV4cGFuZCB0byB0aGUgZW1wdHkgc3RyaW5nLiAgUXVvdGUgcHJvcGVybHkhCiAgICMgTk9URTog d2UgY291bGQgdXNlICVEIGRpcmVjdGx5LCBidXQgaXQncyBub3QgYXZhaWxhYmxlIGluIGdpdCAx LjkuMSBhdCBsZWFzdC4KLSAgY29tbWl0cz0oIiR7KGYpIiQoX2NhbGxfcHJvZ3JhbSBjb21taXRz IGdpdCAtLW5vLXBhZ2VyIGxvZyAtMjAgLS1mb3JtYXQ9JyVoJW4lZCVuJXNcIFwoJWNyXCknKSJ9 IikKKyAgY29tbWl0cz0oIiR7KGYpIiQoX2NhbGxfcHJvZ3JhbSBjb21taXRzIGdpdCAtLW5vLXBh Z2VyIGxvZyAkY29tbWl0X29wdHMgLTIwIC0tZm9ybWF0PSclaCVuJWQlbiVzXCBcKCVjclwpJyki fSIpCiAgIF9fZ2l0X2NvbW1hbmRfc3VjY2Vzc2Z1bCAkcGlwZXN0YXR1cyB8fCByZXR1cm4gMQog CiAgIGZvciBpIGogayBpbiAiJGNvbW1pdHNbQF0iIDsgZG8KQEAgLTU3NTksMTMgKzU3NzQsMTkg QEAgX19naXRfY29tbWl0czIgKCkgewogCiAoKCAkK2Z1bmN0aW9uc1tfX2dpdF9jb21taXRfcmFu Z2VzXSApKSB8fAogX19naXRfY29tbWl0X3JhbmdlcyAoKSB7Ci0gIGxvY2FsIC1hIHN1ZgorICBs b2NhbCAtQSBjb21wYWRkX29wdHNfYXJyYXlfbmFtZSBjb21taXRfb3B0c19hcnJheV9uYW1lCisg IHpwYXJzZW9wdHMgLUQgLUUgTzo9Y29tcGFkZF9vcHRzX2FycmF5X25hbWUgQzo9Y29tbWl0X29w dHNfYXJyYXlfbmFtZQorICAoKCAkI2NvbXBhZGRfb3B0c19hcnJheV9uYW1lICkpICYmIHNldCAt LSAiJHsoQFApY29tcGFkZF9vcHRzX2FycmF5X25hbWV9IgorCisgIGxvY2FsIC1hIGV4cGwgc3Vm CiAgIGlmIGNvbXBzZXQgLVAgJyouLigufCknOyB0aGVuCi0gICAgX19naXRfY29tbWl0cyAkKgor ICAgIGV4cGw9KCAkKiApCiAgIGVsc2UKICAgICBjb21wc2V0IC1TICcuLionIHx8IHN1Zj0oIC1T IC4uIC1yICcuQH4gXjpcdFxuXC0nICkKLSAgICBfX2dpdF9jb21taXRzICQqICRzdWYKKyAgICBl eHBsPSggJCogJHN1ZiApCiAgIGZpCisKKyAgX19naXRfY29tbWl0cyAtTyBleHBsIC1DICRjb21t aXRfb3B0c19hcnJheV9uYW1lCiB9CiAKICgoICQrZnVuY3Rpb25zW19fZ2l0X2NvbW1pdF9yYW5n ZXMyXSApKSB8fAo= --001a11479eee0f2119051dd7a54e--