[PATCH 2/5] _git: Offer @~$n as completion of recent commits.

[PATCH 2/5] _git: Offer @~$n as completion of recent commits.

[Daniel Shahaf]

Suggested-by: Oliver Kiddle (users/20705)
---
Output after the first two patches:

% git commit --fixup=<TAB>
ed49c5f  @~0   - [HEAD]    _git: Offer @~$n as completion of recent commits. (2 minutes ago)
⋮
2685bbc  @~6   - [HEAD~6]  Merge branch 'master' of git://git.code.sf.net/p/zsh/code (15 hours ago)
506d592  @~7   - [HEAD~7]  36943: restore scan for reclaimable blocks in freeheap() (15 hours ago)
e3c6845        - [e3c6845] unposted: _beep completion: Actually hook it for the 'beep' command. (15 hours ago)
779b311        - [779b311] 36913 + 36945: vcs_info quilt: Pass patch subject lines to gen-applied-string (15 hours ago)
c62db9e        - [c62db9e] 36912: vcs_info quilt: Tolerate being in child of .pc's parent (15 hours ago)
272119b  @~8   - [HEAD~8]  unposted: small typo again (16 hours ago)
c8c42d6  @~9   - [HEAD~9]  unposted: small typo (16 hours ago)


 Completion/Unix/Command/_git | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Completion/Unix/Command/_git b/Completion/Unix/Command/_git
index 7f9881f..11e2395 100644
--- a/Completion/Unix/Command/_git
+++ b/Completion/Unix/Command/_git
@@ -5695,13 +5695,14 @@ __git_recent_commits () {
       else
         label="[HEAD~$distance_from_head]"
       fi
+      descr+=("@~${distance_from_head}":"${label} $k") # CROSSREF: use the same label as below
 
       # Prepare for the next first-parent-ancestry commit.
       (( ++distance_from_head ))
       next_first_parent_ancestral_line_commit=${parents%% *}
     fi
     # label is now 9 bytes, so the descriptions ($k) will be aligned.
-    descr+=($i:"${label} $k")
+    descr+=($i:"${label} $k") # CROSSREF: use the same label as above
 
     j=${${j# \(}%\)} # strip leading ' (' and trailing ')'
     j=${j/ ->/,}  # Convert " -> master, origin/master".
-- 
2.1.4

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

[Daniel Shahaf]

Daniel Shahaf wrote on Sun, Oct 25, 2015 at 18:34:58 +0000:
> Suggested-by: Oliver Kiddle (users/20705)
> ---
> Output after the first two patches:
> 
> % git commit --fixup=<TAB>
> ed49c5f  @~0   - [HEAD]    _git: Offer @~$n as completion of recent commits. (2 minutes ago)
> ⋮
> 2685bbc  @~6   - [HEAD~6]  Merge branch 'master' of git://git.code.sf.net/p/zsh/code (15 hours ago)
> 506d592  @~7   - [HEAD~7]  36943: restore scan for reclaimable blocks in freeheap() (15 hours ago)
> e3c6845        - [e3c6845] unposted: _beep completion: Actually hook it for the 'beep' command. (15 hours ago)
> 779b311        - [779b311] 36913 + 36945: vcs_info quilt: Pass patch subject lines to gen-applied-string (15 hours ago)
> c62db9e        - [c62db9e] 36912: vcs_info quilt: Tolerate being in child of .pc's parent (15 hours ago)
> 272119b  @~8   - [HEAD~8]  unposted: small typo again (16 hours ago)
> c8c42d6  @~9   - [HEAD~9]  unposted: small typo (16 hours ago)

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:

     1	% autoload -Uz compinit
     2	% compinit
     3	% git <TAB><Esc>
     4	% cd $(mktemp -d)
     5	% git init && git commit -mm --allow-empty
     6	Initialized empty Git repository in /tmp/tmp.Y5qmiIcfSX/.git/
     7	[master (root-commit) 9b99116] m
     8	% _f() { repeat 1 __git_recent_commits }
     9	% compdef _f f
  ✓ 10	% f <TAB>
    11	HEAD                                                                       master
    12	9b99116  @~0  -- [HEAD]    m (4 seconds ago)
    13	% _f() { repeat 2 __git_recent_commits }
    14	% compdef _f f
  ✗ 15	% f <TAB>
    16	HEAD                                                                 master
    17	9b99116
    18	@~0
    19	-- [HEAD]    m (25 seconds ago)
    20	9b99116
    21	@~0
    22	-- [HEAD]    m (25 seconds ago)

A workaround is to disable the list-grouped style:

    23	% zstyle :completion::complete:f::commits list-grouped false
  ✓ 24	% f <TAB>
    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.

I can't fix this issue properly right now, so I'll revert 36959 until
a fix can be made:

diff --git a/Completion/Unix/Command/_git b/Completion/Unix/Command/_git
index ad1037e..dc9c296 100644
--- a/Completion/Unix/Command/_git
+++ b/Completion/Unix/Command/_git
@@ -5696,7 +5696,10 @@ __git_recent_commits () {
       else
         label="[HEAD~$distance_from_head]"
       fi
-      descr+=("@~${distance_from_head}":"${label} $k") # CROSSREF: use the same label as below
+      ## Disabled because _describe renders the output unhelpfuly when this function
+      ## is called twice during a single completion operation, and list-grouped is
+      ## in its default setting (enabled).
+      #descr+=("@~${distance_from_head}":"${label} $k") # CROSSREF: use the same label as below
 
       # Prepare for the next first-parent-ancestry commit.
       (( ++distance_from_head ))

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
     4	% f
     5	bar
     6	foo
     7	-- aaa
     8	bar
     9	foo
    10	-- aaa

Basically, __git_recent_commits should disable the "Print descriptions one per line,
above the candidate completion" behaviour of _describe (which, IIRC, is
ultimately implemented by the "-E" flag passed to compadd from the 'case
CRT_EXPL' block in bin_compdescribe().)

Help with any of this will be welcome.

Daniel

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

[Oliver Kiddle]

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
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
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
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.

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.

_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.

I've been thinking that it could be useful to add a helper like
_describe but lacking the feature of grouping matches with a common
description. That's the key feature of _describe. _describe is often
used when it perhaps shouldn't be because it is also a simple and
convenient function when you want descriptions. Any ideas on a name for
such a new helper?

Oliver

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

[Daniel Shahaf]

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.
> 

'git show ARG' can be either a tree or a revision.  The latter calls
into __git_recent_commits directly; the former calls it because one form
to specify a tree is ${committish}:${subpath} (as in 'git show
HEAD^:Etc/').  That's why two codepaths reach the same function... it's
not a mistake, it's simply reflecting git's syntax.

> I've been thinking that it could be useful to add a helper like
> _describe but lacking the feature of grouping matches with a common
> description. That's the key feature of _describe. _describe is often
> used when it perhaps shouldn't be because it is also a simple and
> convenient function when you want descriptions. Any ideas on a name for
> such a new helper?

"_annotate".

Thanks for the rest of the email — a bit busy now but will reply when
I can.

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

[Daniel Shahaf]

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.)