Re: _git: Improve handling of aliases

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

[To the list: There's an unrelated question about the
${foo[(I)${(~j)}]} syntax at the end.]

Miroslav Koškár wrote on Fri, 26 Jun 2020 12:17 +0200:
> Hi Daniel,
> 
> thanks for your feedback!
> 

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.  
> 
> Hmm, ok how about this:
> 
> 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)
> 

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 — not 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:
> 
>     a) keep it as is more/less (fixing the small _call_program issue above):
> 
>         ...
>         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 «eval» inside _call_program.

The dot is insufficiently escaped: the physical line of code will eat
one backslash, «eval» will eat the other, and the dot will not be
backslashed by the time the regexp compiler sees it.

>           git_aliases[${${a/$'\n'*}/alias.}]=${a#*$'\n'}  
>         done
>         local git_alias=git_aliases[\$words[1]]
>         if (( ${(P)+git_alias} && !$+commands[git-$words[1]] && !$+functions[_git-$words[1]] )); then
>           git_alias=${(P)git_alias}  
>         ...
> 
>         * 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:
> 
>         ...
>         local git_alias
>         if git_alias=${"$(git config -z --get alias.$words[1] 2>/dev/null)"/$'\0'*} \
>           && (( !$+commands[git-$words[1]] && !$+functions[_git-$words[1]] )); then
>         ...
> 
>         * 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=git_aliases[\$words[1]]
> > > +        if (( ${(P)+git_alias} && !$+commands[git-$words[1]] && !$+functions[_git-$words[1]] )); then
> > > +          git_alias=${(P)git_alias}
> > > +          local len=$#words
> > > +          if [[ $git_alias = \!* ]]; then
> > > +            local i
> > > +            local -a git_alias_tail
> > > +            for i in "${(z)git_alias##\!}"; do
> > > +              git_alias_tail+=("$i")
> > > +              [[ $i = \| ]] && git_alias_tail=()  
> > 
> > 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 «sh
> > -c <TAB>» does, i.e., _cmdstring.  That would add support for comments
> > and for a bunch of things other than «|» 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/91d2eeaf23c47341e8dc7ad66dbf85e38c2674de/highlighters/main/main-highlighter.zsh#L391-L416).
> > 
> > Adding support for comments specifically could also be done by using
> > the ${(Z)} flag, but I don't know how common that is.  If «foo = bar # baz»
> > lines in .gitconfig pass the «#» sign through to sh, then support for
> > comments should probably be added, one way or another.  
> 
> 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 «eval»: joined by spaces and then passed 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 «eval hello
world» and «eval 'hello world'» are equivalent.

So, how about completing this the same way «eval -- <TAB>» is completed?
Currently ${_comps[eval]} is _precommand, which just calls _normal, but
that's incomplete¹.  If we write the code to do something along the
lines of «words=(eval -- …); CURRENT=…; _normal», it'll automatically
grow support for completing both variants once such support is added to
«eval»'s completion.²

¹ For example, «eval 'git -c' <TAB>» completes files rather than
  configuration options, because it takes 'git -c' rather than just 'git'
  to be the command word.

² That'd be something along the lines of «words=( ${=:-"${words}"} )»,
  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:
> 
>     local i
>     local -a git_alias_tail
>     for i in "${(z)git_alias##\!}"; do
>       git_alias_tail+=("$i")
>       case $i in ';' | $'\n' | '||' | '&&' | '|' | '|&' | '&' | '&!' | '&|') git_alias_tail=() ;; esac
>     done
>     words=("$git_alias_tail[@]" "${words[@]:1}")

As above, I'd prefer to delegate to «eval»'s completion.  Alternatively,
my previous point about supporting comments stands.

I was going to mention the «foo[1,(I)bar]=()» syntax, but it doesn't
work as I expect:

    % a=( foo '&&' bar '||' baz )
    % b=( '||' '&&' ) 
    % print ${a[(I)${(~j.|.)b}]} 
    2
    % 

Why doesn't that print 4?  It does print 4 if $b[1] is set to «'\|\|'».

Cheers,

Daniel