Re: __git_recent_commits cannot be called twice Re: [PATCH 2/5] _git: Offer @~$n as completion of recent commits.

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

Oliver Kiddle wrote on Sat, Oct 31, 2015 at 21:24:41 +0100:
> Daniel Shahaf wrote:
> 
> > The new output works fine in 'git commit --fixup=<TAB>', but not in 'git
> > show <TAB>'.  This is because the latter calls __git_recent_commits via
> > two distinct codepaths.  Here's a minimal example:
> 
> Calling one function via two codepaths is the basic problem here. We
> should avoid doing that rather than trying to hack it to work.
> 
> >     25	HEAD    master
> >     26	@~0      -- [HEAD]    m (71 seconds ago)
> >     27	5d97266  -- [HEAD]    m (71 seconds ago)
> > 
> > This is only a workaround because grouping @~0 and 5d97266 is desirable,
> > since they both refer to the same commit.
> 
> Is it really useful to add matches for the abbreviated hashes such as
> 5d97266? I never type them. Matches for the HEAD form might be more

I would use the @<TAB> syntax when trying to complete "the penultimate
commit" (when I don't even know its hash), but I would type a hash
directly when trying to complete a commit from 'git log' output by
typing its first few hex digits.

> useful. I'd also consider using the prefix-needed style with the @
> form and perhaps even with HEAD. It might be better to cope with the

I think it depends on context: prefix-needed would make sense for recent
commits in context of 'log' or 'diff' but not for 'commit
--fixup=<TAB>'.  (Incidentally, the latter is also an example of
a second way that hashes are useful: 'git commit --fixup=<TAB>7f<TAB>'
is an effective way of picking an arbitrary commit from the list shown
by the first <TAB>.)

> problem of more than one commit having the same comment by not using
> _describe but adding matches manually.
> 
> > The underlying problematic behaviour is reproducible without _git:
> > 
> >      1	$ zsh -f
> >      2	% _f() { local -a x=(foo:aaa bar:aaa);  repeat 2 _describe -tt 'desc' x }
> >      3	% compdef _f f
> 
> _describe removes the 'rows' token from compstate[list]. So given the
(compstate[list] and LIST_ROWS_FIRST being related)
> following set of matches/descriptions:
>   w x -- 0
>   y z -- 1
> It adds matches in column first order:
>   w y x z 0 1
> 
> This means the descriptions are all added last so in this case:
>   compadd -E2 '-- 0' '-- 1'
> 
> With two separate _describe calls, it is like adding the matches in this
> order (with setopt nolistrowsfirst):
>   w x 0 y z 1
> The description matches are very wide due to space padding so it is no
> longer able to arrange matches in columns - hence the output you
> described.
> 

That makes sense.

I've been able to produce the buggy output even when there's just one
pair of matches, though: for example, 'f' in the following example
produces the buggy output when LIST_PACKED is unset.  Presumably that's
just a bug in the width calculation code (uniquification not happening
early enough?).

    # to be run in an 80-column terminal
    # (else change the "10 hours ago" string" to have width $((COLUMNS - 16)))
    autoload -Uz compinit
    compinit
    _f() {
      typeset -a _tmpd
      _tmpd=( 5d97266 )
      typeset -a _tmpm
      _tmpm=( 5d97266 )
      compadd -2V commits -X '%B> %Urecent commit object name%u%b' -d _tmpd -a _tmpm
      typeset -a _tmpd
      _tmpd=( '@~0' )
      typeset -a _tmpm
      _tmpm=( '@~0' )
      compadd -2V commits -X '%B> %Urecent commit object name%u%b' -d _tmpd -a _tmpm
      typeset -a _tmpd
      _tmpd=( '- [HEAD]    m (10 hours ago)                                   ' )
      typeset -a _tmpm
      _tmpm=(  )
      compadd -E1 -V commits -X '%B> %Urecent commit object name%u%b' -d _tmpd -a _tmpm
    }
    _g() { _f; _f } 
    compdef _f f; compdef _g g; 
    # optionally setopt listrowsfirst and/or listpacked

(That 'f' is just the guts of _describe, that's why the strange _tmpx
variables.)

> Perhaps you could change compdescribe to do things in row first order
> but it may have other associated problems, particularly when it comes to
> the algorithm used to pack matches in suitable column widths.
> 

I probably could, if only I had access to the secret repository
containing the source code comments for the completion code ;-)

I don't think I understand the completion code (_describe, compdescribe,
compadd) well enough to decide what change to make and how to make it.

> _describe was written with the assumption that it is fully responsible
> for the match group it is adding matches for. It is best to only use it
> when that is the case.

Isn't that what __git_recent_commits is doing?  _describe is solely
responsible for the "hashes and @~N" portion of the output.

> 

All these implementation considerations aside, the output I'm currently
aiming for is:
    5d97266  @~0  - [HEAD]    m (10 hours ago)
    3edcf9b  @~1  - [HEAD~1]  m (11 hours ago)
that is: one line per commit, two matches per line.  But I'm open to be
convinced that some other output would be better.

Thanks,

Daniel

P.S.  I'm sorry for having taken so long to reply.  If I'd known I wouldn't
be able to reply for so long, I wouldn't have sent the email when I did.
(I'd have waited to send the email until such a time when I was able to
reply timely.)