[BUG] git checkout completion shows duplicated branch names

[BUG] git checkout completion shows duplicated branch names

[Marc Cornellà]

Steps to reproduce:

git init repo
cd repo
git commit --allow-empty -m 'Initial commit'
git checkout -b topic/foo
git checkout master

Now, when trying to complete to a branch name, here's what I see:

~/repo% git checkout <TAB>
e287376  -- [HEAD]    Initial commit (12 seconds ago)
topic/foo  master  -- Initial commit
HEAD                                                master
                                 topic/foo

Notice that branches appear twice. This only happens on versions newer
than 5.3.1, excluding this one.
A `git bisect` pointed me to commit e869952, which changed to using
the __git_recent_branches function,
which uses extracts branch names from the git reflog.

Furthermore, on versions prior to commit 4dddf3aa (< zsh-5.6), the
completion is even worse since a branch
is not fully completed even if there isn't any other branch with the
same prefix.

So typing this:

~/repo% git checkout topic/<TAB>
topic/foo  -- Initial commit
topic/foo

gets completed to `git checkout topic` instead of the full branch name.

So after version 5.6 and newer the bug is not that annoying because
the full branch name is indeed completed,
but there're still duplicated entries and I'd expect there to be
unique branch names.

If I knew enough about the completion system I would try to fix it,
but sometimes you have to let the hard stuff
to the pros.

Thanks,
Marc

Re: [BUG] git checkout completion shows duplicated branch names

[Daniel Shahaf]

Marc Cornellà wrote on Mon, 24 Sep 2018 14:51 +0000:
> Steps to reproduce:
> 
> git init repo
> cd repo
> git commit --allow-empty -m 'Initial commit'
> git checkout -b topic/foo
> git checkout master
> 
> Now, when trying to complete to a branch name, here's what I see:
> 
> ~/repo% git checkout <TAB>
> e287376  -- [HEAD]    Initial commit (12 seconds ago)
> topic/foo  master  -- Initial commit
> HEAD                                                master
>                                  topic/foo
> 
> Notice that branches appear twice. This only happens on versions newer
> than 5.3.1, excluding this one.
> A `git bisect` pointed me to commit e869952, which changed to using
> the __git_recent_branches function,
> which uses extracts branch names from the git reflog.
> 

The catch here is that they appear as part of different groups: the
first instance is part of the "recent branches" group and the second is
part of the "head" group.  You can see group names by setting «zstyle
':completion:*' format '> %d'».  A user can disable or hide individual
groups with the tag-order style.  If a user disables the "recent
branches" group, then topic/foo should still be offered under "head",
and vice-versa.

That is not to say that we can't make changes; I am just trying to
explain that avoiding the duplication is not as trivial as it may seem.

> Furthermore, on versions prior to commit 4dddf3aa (< zsh-5.6), the
> completion is even worse since a branch
> is not fully completed even if there isn't any other branch with the
> same prefix.
> 
> So typing this:
> 
> ~/repo% git checkout topic/<TAB>
> topic/foo  -- Initial commit
> topic/foo
> 
> gets completed to `git checkout topic` instead of the full branch name.
> 

With 'zsh -f' + 'autoload compinit; compinit', «git checkout t<TAB>»
gives me «git checkout topic/foo<CURSOR>» and a subsequent <TAB>
displays possible completions.  That is better than the behaviour you
get, but I agree it shouldn't be treating this situation as an ambiguous
completion.  I don't know offhand where the relevant piece of code is,
though.  Probably in the internals of bin_compdescribe/bin_compadd?

Here's a minimal example:

_f() {
  local a=( foo:FOO bar:BAR ) expl
  _describe -t lorem "lorem" a
  _wanted ipsum expl 'ipsum' compadd foo bar
} 
compdef _f f
f f<TAB>

> So after version 5.6 and newer the bug is not that annoying because
> the full branch name is indeed completed,
> but there're still duplicated entries and I'd expect there to be
> unique branch names.
> 
> If I knew enough about the completion system I would try to fix it,
> but sometimes you have to let the hard stuff
> to the pros.

Cheers,

Daniel

Re: [BUG] git checkout completion shows duplicated branch names

[Daniel Shahaf]

Daniel Shahaf wrote on Mon, Sep 24, 2018 at 16:04:23 +0000:
> Marc Cornellà wrote on Mon, 24 Sep 2018 14:51 +0000:
> > Furthermore, on versions prior to commit 4dddf3aa (< zsh-5.6), the
> > completion is even worse since a branch
> > is not fully completed even if there isn't any other branch with the
> > same prefix.
> > 
> > So typing this:
> > 
> > ~/repo% git checkout topic/<TAB>
> > topic/foo  -- Initial commit
> > topic/foo
> > 
> > gets completed to `git checkout topic` instead of the full branch name.
> > 
> 
> With 'zsh -f' + 'autoload compinit; compinit', «git checkout t<TAB>»
> gives me «git checkout topic/foo<CURSOR>» and a subsequent <TAB>
> displays possible completions.  That is better than the behaviour you
> get, but I agree it shouldn't be treating this situation as an ambiguous
> completion.  I don't know offhand where the relevant piece of code is,
> though.  Probably in the internals of bin_compdescribe/bin_compadd?
> 
> Here's a minimal example:
> 
> _f() {
>   local a=( foo:FOO bar:BAR ) expl
>   _describe -t lorem "lorem" a
>   _wanted ipsum expl 'ipsum' compadd foo bar
> } 
> compdef _f f
> f f<TAB>

This seems to fix it:

[[[
diff --git a/Src/Zle/compcore.c b/Src/Zle/compcore.c
index 8eca39447..6118c62c1 100644
--- a/Src/Zle/compcore.c
+++ b/Src/Zle/compcore.c
@@ -3160,9 +3160,7 @@ matcheq(Cmatch a, Cmatch b)
 	matchstreq(a->ppre, b->ppre) &&
 	matchstreq(a->psuf, b->psuf) &&
 	matchstreq(a->suf, b->suf) &&
-	((!a->disp && !b->disp && matchstreq(a->str, b->str)) ||
-	 (a->disp && b->disp && !strcmp(a->disp, b->disp) &&
-	  matchstreq(a->str, b->str)));
+	  matchstreq(a->str, b->str);
 }
 
 /* Make an array from a linked list. The second argument says whether *
]]]

Can anyone think of a reason to consider matches different if their display
strings both exist, and differ; or if one of them has a display string and one
does not?

Marc, thanks for asking — this has been annoying me, too. ☺

Cheers,

Daniel

Re: [BUG] git checkout completion shows duplicated branch names

[Bart Schaefer]

On Mon, Sep 24, 2018 at 11:00 AM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>
> Can anyone think of a reason to consider matches different if their display
> strings both exist, and differ; or if one of them has a display string and one
> does not?

Perhaps not  in the latter case, but if the items really should be in
two different groups (and the groups are going to be displayed
separately) or the display string is providing info to the user that
explains why one might wish to select that completion, then it's
potentially helpful to include it both times.

Re: [BUG] git checkout completion shows duplicated branch names

[Oliver Kiddle]

Daniel Shahaf wrote:
> Can anyone think of a reason to consider matches different if their display
> strings both exist, and differ; or if one of them has a display string and one
> does not?

They might have different matching control rules.

Oliver

Re: [BUG] git checkout completion shows duplicated branch names

[Daniel Shahaf]

Oliver Kiddle wrote on Tue, 25 Sep 2018 02:46 +0200:
> Daniel Shahaf wrote:
> > Can anyone think of a reason to consider matches different if their display
> > strings both exist, and differ; or if one of them has a display string and one
> > does not?
> 
> They might have different matching control rules.

The *display* strings don't affect matching control rules, do they?
Those only care about the trial completion and about the command-line
string.  Even if matcheq() should compare matching control rules
as well, I think that's orthogonal to the upthread patch.

Re: [BUG] git checkout completion shows duplicated branch names

[Daniel Shahaf]

Bart Schaefer wrote on Mon, 24 Sep 2018 17:24 -0700:
> On Mon, Sep 24, 2018 at 11:00 AM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> >
> > Can anyone think of a reason to consider matches different if their display
> > strings both exist, and differ; or if one of them has a display string and one
> > does not?
> 
> Perhaps not  in the latter case, but if the items really should be in
> two different groups (and the groups are going to be displayed
> separately) or the display string is providing info to the user that
> explains why one might wish to select that completion, then it's
> potentially helpful to include it both times.

I tried with:

_g() { local a=( foo:FOO bar:BAR ) b=( foo:1 bar:2 ) expl ; _describe -t lorem lorem a; _describe -t ipsum ipsum b; }

_h() { local a=( FOO BAR ) ; compadd -V v -ld a foo bar ; compadd -J j -ld a foo bar }

and in both cases, "FOO" showed twice, and 'g fo<TAB>' (respectively 'h
fo<TAB>') did not treat the completion as ambiguous.  I conclude that
the relaxation of matcheq() does not prevent duplicate matches from
being listed twice.

However, I can't confirm that conclusion by code inspection.  I went
through the callsites of matcheq() but they are not terribly well-documented.
Does anyone know what permmatches() does?  What about  makearray()?  The
docstrings aren't that clear.

Should I test any other cases?

Cheers,

Daniel

Re: [BUG] git checkout completion shows duplicated branch names

[Bart Schaefer]

On Tue, Sep 25, 2018 at 9:43 AM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> Oliver Kiddle wrote on Tue, 25 Sep 2018 02:46 +0200:
>> They might have different matching control rules.
>
> The *display* strings don't affect matching control rules, do they?

I think not, but display strings do affect the use of list-colors.

Re: [BUG] git checkout completion shows duplicated branch names

[Daniel Shahaf]

Bart Schaefer wrote on Tue, 25 Sep 2018 10:15 -0700:
> On Tue, Sep 25, 2018 at 9:43 AM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > Oliver Kiddle wrote on Tue, 25 Sep 2018 02:46 +0200:
> >> They might have different matching control rules.
> >
> > The *display* strings don't affect matching control rules, do they?
> 
> I think not, but display strings do affect the use of list-colors.

Thanks.  As per my other replay, I can't provoke fewer display strings
being shown with this patch than without.

Shall I commit this and wait for bug reports?