Re: [PATCH] compdescribe fix for unsorted groups (workers/34814) (was: Re: completion: git: --fixup: problem with _describe -2V and duplicate commit subjects)

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

Bart Schaefer wrote on Sat, May 16, 2015 at 21:07:35 -0700:
> On May 16, 10:54pm, Daniel Shahaf wrote:
> } Subject: Re: [PATCH] compdescribe fix for unsorted groups (workers/34814) 
> }
> } Daniel Shahaf wrote on Thu, May 14, 2015 at 14:36:27 +0000:
> } > The first attached patch seems to address this.  However, I encountered
> } > an odd problem with it, which I have not been able to narrow down.  When
> } > I apply the first patch, Daniel Hahler's recent series, and the second
> } > attached patch, and then try to complete 'git checkout 1<TAB>',¹ I get
> } > first a screenful of hashes, and then a screenful of descriptions, and
> } > so on alternating.  I'm not sure whether this indicates a bug in the
> } > first patch or an independent problem.
> } 
> } To be explicit: the problem goes away if I revert the first patch, but
> } keep the others applied.
> 
> You mean revert *all* of "PATCH 1/3", that is, both the compcore.c hunk
> (which seems to be a functional no-op) and all the hunks of computil.c?
> 

Yes.

And yes, the compcore.c hunk is intended to be a no-op (improve
readability but not affect generated machine code).

> The only difference I see from eyballing the patch in 35127 is in the
> computil.c cd_get() hunk where before -2V would be set only on an
> existing -J group, but post-patch might be set on an existing -V group.
> 
> I don't know why that would matter unless there is both a -J and a -V
> group in the same set of opts, in which case -2V is going to be set for
> whichever group is encountered first?  Am I understanding the intent of
> this code correctly?
> 

It's not both a -J and a -V, but two -V's.  It looks like this (when
doing 'f <TAB>' using the 'f' defined at the top of 35127):

    compadd: '-2V-default-' '-2' '-V' 'values' '-x' '%b> %uperson%u%b' '-d' '_tmpd' '-a' '_tmpm' 

The '-2V-default-' is added by cd_get() line 715.  The '-2 -V values' is
generated by _describe (via $_type there).  As you say, the first
specification wins, in this case that's the "-2V-default-" group.
The intent of the patch was to have compadd use the -2Vvalues group,
like it uses in the 'g <TAB>' case, which works correctly, and uses:

    compadd: '-2' '-2V' 'values' '-x' '%b> %uperson%u%b' '-d' '_tmpd' '-a' '_tmpm' 

> In any case I can't reproduce your reported problem, at least not quite
> as you explain it above.
> 
> Starting from commit d52bf91659522435d68f719389095001f050b6c5, I applied
> DH's seven patches and then your 35127, and:
> 
> torch% git checkout 1<TAB>
> torch% git checkout 15
> 153a99d  -- 35154: NEWS on arithmetic evaluation changes (2 days ago)
> 15aa99b  -- 35139: complete the new (b) parameter flag (2 days ago)
> 153a99d  -- 35154: NEWS on arithmetic evaluation changes (2 days ago)
> 15aa99b  -- 35139: complete the new (b) parameter flag (2 days ago)
> 

Okay, I repeated these exact steps, and I get:

$ zsh -f
% autoload compinit; compinit
% git co 1<TAB>
1
153a99d  -- 35154: NEWS on arithmetic evaluation changes (2 days ago)
15aa99b  -- 35139: complete the new (b) parameter flag (2 days ago)
153a99d  -- 35154: NEWS on arithmetic evaluation changes (2 days ago)
15aa99b  -- 35139: complete the new (b) parameter flag (2 days ago)

Note the stray '1'.

I added a group-name style and then the bug reproduced.  I also set
tag-order to make the example output shorter, however, the bug
reproduces without the tag-order setting too:

[[[
% zstyle ':completion:*' group-name '' 
% zstyle ':completion:*' tag-order 'commits commit-objects' '*'
% git checkout <TAB>
%D
fc8e719
4d591ef
27f99b8
d0fab2b
9c3bbbc
5c13371
4f46648
a2ed485
fcfd107
d52bf91
HEAD~10
HEAD~11
HEAD~12
HEAD~13
HEAD~14
HEAD~15
HEAD~16
HEAD~17
HEAD~18
HEAD~19
HEAD
HEAD^
HEAD^^
HEAD~3
HEAD~4
HEAD~5
HEAD~6
HEAD~7
HEAD~8
HEAD~9
32a448d
153a99d
0da0a0b
59a874f
e867201
15aa99b
63ffbab
55716ea
968c5ce
a1c1f68
-- git completion: offer HEAD~$n as completions in git, remove 34814 workaround (11 minutes ago)                                     
-- Fix _describe/compdescribe problem with unsorted groups (see 34814) (11 minutes ago)                                              
-- completion: git: add distance_from_head to __git_recent_commits (12 minutes ago)                                                  
-- completion: git: unique name for __git_recent_commits (12 minutes ago)                                                            
-- completion: git: add %cr to commit objects (all and recent) (12 minutes ago)                                                      
-- completion: git: __git_commit_objects: query 1000 commits (12 minutes ago)                                                        
-- completion: git: add __git_commit_objects_prefer_recent (12 minutes ago)                                                          
-- completion: git: use %D for %d, without the " (", ")" wrapping (12 minutes ago)                                                   
-- completion: git: __git_recent_commits: remove ' ->*' from heads (12 minutes ago)                                                  
-- 35155: cmdpop() could be called erroneously on error (33 hours ago)                                                               
-- users/20219: fix completion for git options (2 days ago)                                                                          
-- 35154: NEWS on arithmetic evaluation changes (2 days ago)                                                                         
-- 35153: nested math substitution (2 days ago)                                                                                      
-- 35151: improved check for parameter q and b flags (2 days ago)                                                                    
-- 35131: allow "[]" to match empty character set. (2 days ago)                                                                      
-- 35139: complete the new (b) parameter flag (2 days ago)                                                                           
-- Øystein Walle: 34841 (tweaked): allow grouping of thousands in printf format string (2 days ago)                                  
-- unposted: include .distfiles for new directory (2 days ago)                                                                       
-- 35062: __git_setup_revision_options includes __git_setup_diff_options (3 days ago)                                                
-- 35061: add __git_setup_diff_stage_options and use it with _git-diff-files and _git-diff explicitly (3 days ago)                   
... [snip: everything above this point, except for the %D, was repeated]
1                          4                          dot-zsh-3.1.5-pws-17       master                     zsh-4.0-patches
2                          \#CVSPS.NO.BRANCH          dot-zsh-3.1.5-pws-19       zsh                        zsh-4.2-patches
3                          dot-zsh-3.1.5-pws-14       interrupt_abort            zsh-3.1.5-pws-16-patches   
]]]

The literal %D at the start is an artifact of Daniel Hahler's 2/7 patch.
(My git doesn't support the %D expando, so git outputs "%D" verbatim and
_git interprets that as a head name and offers it as a completion.)

> That doesn't really look correct because it lists everything twice, but
> I get exactly the same thing with computil.c backed out.  If I back out
> the rest of 35127, I get:
> 
> torch% git checkout 1<TAB>
> torch% git checkout 15
> 153a99d  -- [HEAD^^]  35154: NEWS on arithmetic evaluation changes (2 days a
> 15aa99b  -- [HEAD~6]  35139: complete the new (b) parameter flag (2 days ago
> 153a99d  -- [HEAD^^]  35154: NEWS on arithmetic evaluation changes (2 days a
> 15aa99b  -- [HEAD~6]  35139: complete the new (b) parameter flag (2 days ago
> 

I get:

% git co 1<TAB>
1
153a99d  -- [HEAD~9]  35154: NEWS on arithmetic evaluation changes (2 days ago)
15aa99b  -- [HEAD~13] 35139: complete the new (b) parameter flag (2 days ago)
153a99d  -- [HEAD~9]  35154: NEWS on arithmetic evaluation changes (2 days ago)
15aa99b  -- [HEAD~13] 35139: complete the new (b) parameter flag (2 days ago)

There are two significant differences from your example: the stray '1'
output and the fact that '1<TAB>' does not become '15' for me.  The
difference in bracketed parts is insignificant (I used git-am to
actually apply Daniel Hahler's 7 patches as separate commits on top of
HEAD, whereas you apparently applied them as local mods on top of HEAD).

> Which I guess are not strictly duplicates because the descriptions differ.
> 

The descriptions you pasted do not differ: both 153a99d entries have the
same description as each other, as do both 15aa99b entries.

> If I then revert all of DH's patches (back to an empty 'git diff'):
> 
> torch% git checkout 1<TAB>
> torch% git checkout 15
> 153a99d  -- [153a99d] 35154: NEWS on arithmetic evaluation changes
> 15aa99b  -- [15aa99b] 35139: complete the new (b) parameter flag
> 
> The extra line for each of these is a side-effect from DH's patch 3/7 in
> workers/35101.
> 
> Incidentally _git_recent_objects is missing a "return ret" at the end (but
> repairing that doesn't change the duplication).

> 

Thanks very much for looking into this!  I know "apply nine patches"
isn't the world's friendliest reproduction recipe, but I had nothing
simpler and my head was getting flat from being hit against a wall for
too long...

Daniel