From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-1.0 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_DNSWL_NONE autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 12317 invoked from network); 26 Jun 2020 17:00:53 -0000 Received: from ns1.primenet.com.au (HELO primenet.com.au) (203.24.36.2) by inbox.vuxu.org with ESMTPUTF8; 26 Jun 2020 17:00:53 -0000 Received: (qmail 1322 invoked by alias); 26 Jun 2020 17:00:43 -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: List-Unsubscribe: Sender: zsh-workers@zsh.org X-Seq: 46124 Received: (qmail 134 invoked by uid 1010); 26 Jun 2020 17:00:43 -0000 X-Qmail-Scanner-Diagnostics: from wout1-smtp.messagingengine.com by f.primenet.com.au (envelope-from , uid 7791) with qmail-scanner-2.11 (clamdscan: 0.102.3/25850. spamassassin: 3.4.4. Clear:RC:0(64.147.123.24):SA:0(-2.6/5.0):. Processed in 1.904539 secs); 26 Jun 2020 17:00:43 -0000 X-Envelope-From: d.s@daniel.shahaf.name X-Qmail-Scanner-Mime-Attachments: | X-Qmail-Scanner-Zip-Files: | Received-SPF: none (ns1.primenet.com.au: domain at daniel.shahaf.name does not designate permitted sender hosts) X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduhedrudeluddgudduudcutefuodetggdotefrod ftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfgh necuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd enucfjughrpeffhffvuffkjghfofggtgfgsehtqhdttdertdejnecuhfhrohhmpeffrghn ihgvlhcuufhhrghhrghfuceougdrshesuggrnhhivghlrdhshhgrhhgrfhdrnhgrmhgvqe enucggtffrrghtthgvrhhnpeelvdevvddvledthfelhefgtdduteejueejleeulefgvdeh heehheegveelgeelvdenucffohhmrghinhepghhithhhuhgsrdgtohhmnecukfhppeejle drudejiedrfeelrdeileenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgr ihhlfhhrohhmpegurdhssegurghnihgvlhdrshhhrghhrghfrdhnrghmvg X-ME-Proxy: Date: Fri, 26 Jun 2020 17:00:03 +0000 From: Daniel Shahaf To: Miroslav =?UTF-8?B?S2/FoWvDoXI=?= Cc: zsh-workers@zsh.org Subject: Re: _git: Improve handling of aliases Message-ID: <20200626170003.49d91fff@tarpaulin.shahaf.local2> In-Reply-To: <20200626101704.lhzsaqrdov5rhbr7@mkoskar.com> References: <7f7706bd28646cf47d77a0e1b5f89cff40ed9ffe.1592995874.git.mk@mkoskar.com> <20200624115630.GA25495@tarpaulin.shahaf.local2> <20200626101704.lhzsaqrdov5rhbr7@mkoskar.com> X-Mailer: Claws Mail 3.17.3 (GTK+ 2.24.32; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable [To the list: There's an unrelated question about the ${foo[(I)${(~j)}]} syntax at the end.] Miroslav Ko=C5=A1k=C3=A1r wrote on Fri, 26 Jun 2020 12:17 +0200: > Hi Daniel, >=20 > thanks for your feedback! >=20 You're welcome. > > You moved the surrounding block code further down the function > > unchanged. I'd recommend to split the patch into two parts: one that > > just moves the code without changing it, and one that adds new > > functionality. That'll make it much easier to review. =20 >=20 > Hmm, ok how about this: >=20 > 1) fixing up for nested aliases and preceding options > 2) basic parsing of shell aliases for completing of last simple command > (this would be the "new functionality" bit then) >=20 Sorry, I don't follow. I'm not sure how your (1) and (2) map to my points. To clarify, I was asking that you arrange the series to have one commit that simply moves lines around in the file with as few changes as possible, and then one (or more) other commits that make functional changes. The idea is to make it easier to review the diff=C2=A0=E2=80=94 n= ot just not before it's merged, but also later on when reviewing the history. Perhaps the series should have three commits: your (1), your (2), and a "move lines around in the file" commit (not necessarily in this order)? > For 1) I'm thinking about this: >=20 > a) keep it as is more/less (fixing the small _call_program issue abov= e): >=20 > ... > local -A git_aliases > local a > for a in ${(0)"$(_call_program aliases git config -z --get-regexp= \^alias\\.)"}; do The caret is insufficiently escaped: it needs to be protected from EXTENDED_GLOB both on this physical line of code, and later when it's passed to =C2=ABeval=C2=BB inside _call_program. The dot is insufficiently escaped: the physical line of code will eat one backslash, =C2=ABeval=C2=BB will eat the other, and the dot will not be backslashed by the time the regexp compiler sees it. > git_aliases[${${a/$'\n'*}/alias.}]=3D${a#*$'\n'} =20 > done > local git_alias=3Dgit_aliases[\$words[1]] > if (( ${(P)+git_alias} && !$+commands[git-$words[1]] && !$+functi= ons[_git-$words[1]] )); then > git_alias=3D${(P)git_alias} =20 > ... >=20 > * I've got rid of "aliases", "k" and "v" to shorten it a bit Normally I'd say that clarity is more important than golf and churn, but on this instance, it does look better not to have to allocate mental registers to those used-on-two-lines-and-never-again variables. Unshadowing the predefined $aliases variable is certainly a bonus. On the other hand, having git_alias contain values of different types at different times isn't exactly best practice, but *shrug*. By the way, I've been meaning to ask you if you could please leave the whitespace changes out, or confined to a separate commit. > b) much shorter and IMO cleaner: >=20 > ... > local git_alias > if git_alias=3D${"$(git config -z --get alias.$words[1] 2>/dev/nu= ll)"/$'\0'*} \ > && (( !$+commands[git-$words[1]] && !$+functions[_git-$words[1]= ] )); then > ... >=20 > * basically the difference is that we don't ask for all aliases > but only for the one we're interested in Sure. Alternatively, we could continue to retrieve all aliases but cache that list between calls (see _store_cache; there are examples in _subversion). > * it loses configurability of _call_program, which IMO is > rather pointless anyway I'm not too happy about this. That configurability is already there. Presumably it was added for a reason, and there's no reason to break anybody's proverbial spacebar heating. Also, it's the house style. > > > + local git_alias=3Dgit_aliases[\$words[1]] > > > + if (( ${(P)+git_alias} && !$+commands[git-$words[1]] && !$+f= unctions[_git-$words[1]] )); then > > > + git_alias=3D${(P)git_alias} > > > + local len=3D$#words > > > + if [[ $git_alias =3D \!* ]]; then > > > + local i > > > + local -a git_alias_tail > > > + for i in "${(z)git_alias##\!}"; do > > > + git_alias_tail+=3D("$i") > > > + [[ $i =3D \| ]] && git_alias_tail=3D() =20 > >=20 > > Would it be possible to add support to everything that could be used > > there, in addition to pipes? I think you just need to call whatever = =C2=ABsh > > -c =C2=BB does, i.e., _cmdstring. That would add support for comm= ents > > and for a bunch of things other than =C2=AB|=C2=BB that can be followed= by the > > start of a simple command (for example, the tokens and reserved words > > in https://github.com/zsh-users/zsh-syntax-highlighting/blob/91d2eeaf23= c47341e8dc7ad66dbf85e38c2674de/highlighters/main/main-highlighter.zsh#L391-= L416). > >=20 > > Adding support for comments specifically could also be done by using > > the ${(Z)} flag, but I don't know how common that is. If =C2=ABfoo =3D= bar # baz=C2=BB > > lines in .gitconfig pass the =C2=AB#=C2=BB sign through to sh, then sup= port for > > comments should probably be added, one way or another. =20 >=20 > I agree, I've hastily put only split on '|' there. I've looked into > _cmdstring and I don't believe this will be of any help here as it's > completing single command argument only. Well, yes and no. The arguments to shell aliases seem to be handled similarly to arguments to =C2=ABeval=C2=BB: joined by spaces and then passe= d to system(3). That means it's valid to pass the entire string in a single shell word, or to translate any of the spaces in the desired result string into word breaks at the shell input level, just like =C2=ABeval hello world=C2=BB and =C2=ABeval 'hello world'=C2=BB are equivalent. So, how about completing this the same way =C2=ABeval -- =C2=BB is com= pleted? Currently ${_comps[eval]} is _precommand, which just calls _normal, but that's incomplete=C2=B9. If we write the code to do something along the lines of =C2=ABwords=3D(eval -- =E2=80=A6); CURRENT=3D=E2=80=A6; _normal=C2= =BB, it'll automatically grow support for completing both variants once such support is added to =C2=ABeval=C2=BB's completion.=C2=B2 =C2=B9 For example, =C2=ABeval 'git -c' =C2=BB completes files rather = than configuration options, because it takes 'git -c' rather than just 'git' to be the command word. =C2=B2 That'd be something along the lines of =C2=ABwords=3D( ${=3D:-"${wor= ds}"} )=C2=BB, I guess? Plus adjusting $CURRENT, etc.. > It seems to me most sane here is to complete using _normal on last > simple command contained in that alias. So something like this should > work fine: >=20 > local i > local -a git_alias_tail > for i in "${(z)git_alias##\!}"; do > git_alias_tail+=3D("$i") > case $i in ';' | $'\n' | '||' | '&&' | '|' | '|&' | '&' | '&!' | '&= |') git_alias_tail=3D() ;; esac > done > words=3D("$git_alias_tail[@]" "${words[@]:1}") As above, I'd prefer to delegate to =C2=ABeval=C2=BB's completion. Alterna= tively, my previous point about supporting comments stands. I was going to mention the =C2=ABfoo[1,(I)bar]=3D()=C2=BB syntax, but it do= esn't work as I expect: % a=3D( foo '&&' bar '||' baz ) % b=3D( '||' '&&' )=20 % print ${a[(I)${(~j.|.)b}]}=20 2 %=20 Why doesn't that print 4? It does print 4 if $b[1] is set to =C2=AB'\|\|'= =C2=BB. Cheers, Daniel