Re: completion: git: --fixup: problem with _describe -2V and duplicate commit subjects

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

[ compdescribe question inside ]

On Tue, Mar 24, 2015 at 01:07:48AM +0100, Daniel Hahler wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 

Inline PGP and inline patches don't go well together (inline PGP
corrupts lines with a '-' on the first column.).  It would be slightly
easier to process your patches if you either attached them, or used MIME
PGP rather than inline PGP.  Thanks.

> On 23.03.2015 23:05, I wrote:
> 
> > I've noticed that with duplicate descriptions for different commits,
> > the new completion for --fixup (__git_recent_commits) does not behave
> > properly, triggered by the "-2V" used with _describe.
> > 
> > When completing "git commit --fixup=" in a repo with two commits having
> > "init" as its subject:
> > 
> > Without "-2V":
> > 
> >  -- commit object name --
> > b1fc0ca  05a98c0  -- init
> > 

This one behaves correctly, though of course it no longer sorts
completions most recent first (since -V did that).

> > With "-2V":
> > 
> >  -- commit object name --
> > -- init
> >  -- commit object name --
> > b1fc0ca                          05a98c0
> > 

Yes, I see that.  Here's a more minimal reproducer:

[[[
$ zsh -f
% autoload compinit
% compinit
% a=(alice:foo bob:foo)
% compdef '_describe -2Vx person a' f
% zstyle ':completion:*:descriptions' format '%B> %U%d%u%b'
% zstyle ':completion:*' group-name ''
% f <TAB>
> person
-- foo                                                            
> person
alice                             bob
% zstyle ':completion:*' completer _all_matches  _complete  _ignored 
% f <TAB>
> person
-- foo                                                            
> person
alice                             bob
> all matches
 alice bob
]]]

I've tracked this down a little.  It has to do with the C implementation of
compdescribe passing "-E1" to compadd, here:

 [in Src/Zle/computil.c]
 737         case CRT_EXPL:
 738             {
 739                 /* add columns as safety margin */
 740                 VARARR(char, dbuf, cd_state.suf + cd_state.slen +
 741                        zterm_columns);
 742                 char buf[20], *p, *pp, *d;
 743                 int i = run->count, remw, w, l;
 744
 745                 sprintf(buf, "-E%d", i);

This also explains the space in front of "alice" in the "all matches"
line: "compadd -E1" adds an empty match.

So, in essence, compadd sees an empty match with description 'foo' and
two matches, 'alice' and 'bob', with no descriptions; which causes it to
print
[[[
-- foo
alice bob
]]]
when descriptions are disabled.  I'm not sure what should be done
instead.  One option is to disable the 'group by description' step for
the 'git commit --fixup=' use case, and just let the list of matches to
include two lines with the same description text.  Or perhaps the "one
empty match with a description and two real matches without
descriptions" trick should be changed.  Thoughts?

> > This does not only affect the duplicate commits, but will move all
> > hashes to a separate "commit object name" section, leaving the
> > descriptions without them.
> > 
> > This was added in 236da69, where the options are passed on to next_label.
> 
> As a workaround I could imagine also adding the date, which is useful
> by itself:
...
> - -  local i j k
> +  local i j k l
...
> It would be nice if this could be shortened, e.g. by removing the timezone offset and
> removing the current year, month etc.
> 

I suppose you could use %ct to get the seconds-since-epoch value and
strftime it yourself.  However, I believe this code has to work even if
the strftime builtin is not available, so it'd have to be something like
"use %ci & strftime if strftime(1) is available, else fallback to %cd".

> There's a "relative date" option ("%cr"), but that might produce duplicates again.
> If this can be fixed, it might be a good alternative to "%cd".
> 

Actually, I'm tempted to deduplicate it by throwing the hash into the
description:

diff --git a/Completion/Unix/Command/_git b/Completion/Unix/Command/_git
index 5524cb0..87ec986 100644
--- a/Completion/Unix/Command/_git
+++ b/Completion/Unix/Command/_git
@@ -5651,7 +5651,7 @@ __git_recent_commits () {
   __git_command_successful $pipestatus || return 1
 
   for i j k in "$commits[@]" ; do
-    descr+=($i:$k)
+    descr+=("$i:[$i] $k")
     j=${${j# \(}%\)} # strip leading ' (' and trailing ')'
     for j in ${(s:, :)j}; do
       if [[ $j == 'tag: '* ]] ; then

The hash will ensure descriptions are unique (git prints more hash
digits than requested if the amount requested would be ambiguous), which
works around the problem; and in the common case the [$i] prefix would
be the same width in every line, making it easier to visually skip it.
So I'm inclined to use the above, and fix _describe separately.

We can of course add the date still, as you suggested; I just think if
we add the date it should be because the date is useful, not because it
uniquifies, since adding the hash is a better uniquifier.

WDYT?

Thanks,

Daniel

P.S. Sorry for the delayed reply, I was offline last week.

> 
> I've noticed btw that with `zsh -f` this is not the same, but commits are not grouped
> together and appear to get sorted.  Therefore some zstyle might be involved here.
> 
> 
> Regards,
> Daniel.