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

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

[Daniel Hahler]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

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                                                         

With "-2V":

 -- commit object name --
- -- init                                                         
 -- commit object name --
b1fc0ca                          05a98c0

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.


Regards,
Daniel.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iD8DBQFVEI4jfAK/hT/mPgARAmF/AKDwkOg7vI6dxorebP0u7ykceLuV5gCcDBOj
0H8Vrng3rwCE8mmTU6tK4x0=
=5L0N
-----END PGP SIGNATURE-----

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

[Daniel Hahler]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

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
> 
> With "-2V":
> 
>  -- commit object name --
> -- init
>  -- commit object name --
> b1fc0ca                          05a98c0
> 
> 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:

diff --git i/Completion/Unix/Command/_git w/Completion/Unix/Command/_git
index 5524cb0..d1b3617 100644
- --- i/Completion/Unix/Command/_git
+++ w/Completion/Unix/Command/_git
@@ -5644,14 +5644,14 @@ __git_commit_objects () {
 __git_recent_commits () {
   local gitdir expl start
   declare -a descr tags heads commits
- -  local i j k
+  local i j k l
 
   # Careful: most %d will expand to the empty string.  Quote properly!
- -  : "${(A)commits::=${(@f)"$(_call_program commits git --no-pager log -20 --format='%h%n%d%n%s')"}}"
+  : "${(A)commits::=${(@f)"$(_call_program commits git --no-pager log -20 --format='%h%n%d%n%s%n%cd')"}}"
   __git_command_successful $pipestatus || return 1
 
- -  for i j k in "$commits[@]" ; do
- -    descr+=($i:$k)
+  for i j k l in "$commits[@]" ; do
+    descr+=($i:"$k ($l)")
     j=${${j# \(}%\)} # strip leading ' (' and trailing ')'
     for j in ${(s:, :)j}; do
       if [[ $j == 'tag: '* ]] ; then

It would be nice if this could be shortened, e.g. by removing the timezone offset and
removing the current year, month etc.

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


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.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iD8DBQFVEKrUfAK/hT/mPgARAhroAKCgOQca7N6owZ7WzlOfLVe5Y8D2hgCgg/Jf
hLLdL0+U0tqv23HgBXeTw+o=
=YFev
-----END PGP SIGNATURE-----

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

[Daniel Shahaf]

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

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

[Daniel Shahaf]

Daniel Shahaf wrote on Sun, Mar 29, 2015 at 05:47:53 +0000:
> > As a workaround I could imagine also adding the date, which is useful
> > by itself:
...
> Actually, I'm tempted to deduplicate it by throwing the hash into the
> description:

Another option: add the "HEAD~15" gitrevisions(7) identifier of the
commit to the description, which also uniquifies, isn't redundant, and
may be easier to type.

diff --git a/Completion/Unix/Command/_git b/Completion/Unix/Command/_git
index 5524cb0..00b124d 100644
--- a/Completion/Unix/Command/_git
+++ b/Completion/Unix/Command/_git
@@ -5645,13 +5645,27 @@ __git_recent_commits () {
   local gitdir expl start
   declare -a descr tags heads commits
   local i j k
+  integer distance_from_head
 
   # Careful: most %d will expand to the empty string.  Quote properly!
   : "${(A)commits::=${(@f)"$(_call_program commits git --no-pager log -20 --format='%h%n%d%n%s')"}}"
   __git_command_successful $pipestatus || return 1
 
   for i j k in "$commits[@]" ; do
-    descr+=($i:$k)
+    # Note: the after-the-colon part must be unique across the entire array;
+    # see workers/34768
+    if (( distance_from_head == 0 )); then
+      descr+=($i:"[HEAD]    $k")
+    elif (( distance_from_head == 1 )); then
+      descr+=($i:"[HEAD^]   $k")
+    elif (( distance_from_head == 2 )); then
+      descr+=($i:"[HEAD^^]  $k")
+    elif (( distance_from_head < 10 )); then
+      descr+=($i:"[HEAD~$distance_from_head]  $k")
+    else
+      descr+=($i:"[HEAD~$distance_from_head] $k")
+    fi
+    (( ++distance_from_head ))
     j=${${j# \(}%\)} # strip leading ' (' and trailing ')'
     for j in ${(s:, :)j}; do
       if [[ $j == 'tag: '* ]] ; then

I came up with this because copying the hex digits is annoying.  (It's
basically a captcha.)  I'm half seriously considering binding '=' to
a widget that inserts '=<TAB>HEAD~' if [[ $LBUFFER == *--fixup ]], to
allow me to type just '--fixup=12' to get '--fixup=HEAD~12'.

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

[Daniel Hahler]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 30.03.2015 20:21, Daniel Shahaf wrote:
> Daniel Shahaf wrote on Sun, Mar 29, 2015 at 05:47:53 +0000:
>>> As a workaround I could imagine also adding the date, which is useful
>>> by itself:
> ...
>> Actually, I'm tempted to deduplicate it by throwing the hash into the
>> description:
> 
> Another option: add the "HEAD~15" gitrevisions(7) identifier of the
> commit to the description, which also uniquifies, isn't redundant, and
> may be easier to type.
>
> ...
>
> I came up with this because copying the hex digits is annoying.  (It's
> basically a captcha.)  I'm half seriously considering binding '=' to
> a widget that inserts '=<TAB>HEAD~' if [[ $LBUFFER == *--fixup ]], to
> allow me to type just '--fixup=12' to get '--fixup=HEAD~12'.

I like this idea.  Have you experimented further with it?


Cheers,
Daniel.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iD8DBQFVU0ZsfAK/hT/mPgARApCpAKC1uGGr60H19PL3UVcG434OpuAtTACg453s
wncXHgtGuvzn3mJVU4hmR4M=
=n9hB
-----END PGP SIGNATURE-----

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

[Daniel Shahaf]

Daniel Hahler wrote on Wed, May 13, 2015 at 14:41:16 +0200:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 30.03.2015 20:21, Daniel Shahaf wrote:
> > Daniel Shahaf wrote on Sun, Mar 29, 2015 at 05:47:53 +0000:
> >>> As a workaround I could imagine also adding the date, which is useful
> >>> by itself:
> > ...
> >> Actually, I'm tempted to deduplicate it by throwing the hash into the
> >> description:
> > 
> > Another option: add the "HEAD~15" gitrevisions(7) identifier of the
> > commit to the description, which also uniquifies, isn't redundant, and
> > may be easier to type.
> >
> > ...
> >
> > I came up with this because copying the hex digits is annoying.  (It's
> > basically a captcha.)  I'm half seriously considering binding '=' to
> > a widget that inserts '=<TAB>HEAD~' if [[ $LBUFFER == *--fixup ]], to
> > allow me to type just '--fixup=12' to get '--fixup=HEAD~12'.
> 
> I like this idea.  Have you experimented further with it?

Nope, but here it is if anyone wants it:

equal_for_git_fixup() {
  zle .self-insert
  if [[ $LBUFFER == *--fixup= ]] ; then
    zle complete-word
    LBUFFER+="HEAD~"
  fi
}
zle -N equal_for_git_fixup
bindkey '=' equal_for_git_fixup

It'd be especially useful if "HEAD~15" were offered as completions (not
just as descriptions).  However, we can't simply add both the hash and
the HEAD~$n name of the commit to the 'descr' array, due to the bug in
'_describe -2V' when a description is repeated (workers/34814).

So we'd need to fix 34814 in order to add the HEAD~$n as completions.
I'll post about that separately.

Daniel

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

[Daniel Hahler]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 29.03.2015 07:47, Daniel Shahaf wrote:

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

I still like the date idea, especially for the recent commits.

I think using "%cr" for the relative committer date, after the subject is good.
It would not make it unique, but only add information.

I think for uniqueness, the "HEAD~15" gitrevisions(7) identifier would be useful
(instead of the hash).

I will post some patch(es) later in this regard.


Regards,
Daniel.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iD8DBQFVU0dAfAK/hT/mPgARAuG3AKCPP/naiQiqWxXbyrzQWh2jPKr8ewCgtTIi
WBInG/ASy3DqR0Jfir6Ru8o=
=rCF5
-----END PGP SIGNATURE-----

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

[Daniel Shahaf]

[-- Attachment #1: Type: text/plain, Size: 1854 bytes --]

Daniel Shahaf wrote on Sun, Mar 29, 2015 at 05:47:53 +0000:
> [ compdescribe question inside ]

This is the problem:

% autoload compinit
% compinit
% a=(dalton:bar charlie:bar bob:foo alice:foo)
% compdef '_describe -2 -V -x person a' f
% compdef '_describe -2 -J -x person a' g
% zstyle ':completion:*:descriptions' format '%b> %u%d%u%b'
% zstyle ':completion:*' group-name ''
% f <TAB>
> person
-- foo
-- bar
> person
alice   charlie   bob   dalton
% g <TAB>
> person
alice    bob     -- foo
charlie  dalton  -- bar

The expected output for f is to the same output as for g, but in reverse
order (the 'bar' lines should be first because they came first in the
array).

The executions on f and g diverge in begcmgroup() (callstack at that
point: addmatches(), bin_compadd(), Completion/Base/Utility/_describe).
When completing 'g', the 'alice' and 'bob' candidates get added to an
existing group that already contains an empty match; when completing
'f', that group exists, but isn't matched due to flags differences (the
CGF_NOSORT flag corresponds to -V), so those two matches get added to
a new group, resulting in the above output.

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.

In any case, I'm leaning towards waiting with these changes until after
5.0.8, to give them as much testing time as possible.

Cheers,

Daniel

¹ Those completions are generated by the function __git_commit_objects
in Completion/Unix/Command/_git.

[-- Attachment #2: 0001-Fix-_describe-compdescribe-problem-with-unsorted-gro.patch --]
[-- Type: text/x-patch, Size: 3377 bytes --]

>From 38c05e49a4beaa1dcd0398bbe6674294ab5c4a5c Mon Sep 17 00:00:00 2001
From: Daniel Shahaf <d.s@daniel.shahaf.name>
Date: Thu, 14 May 2015 11:04:19 +0000
Subject: [PATCH 1/3] Fix _describe/compdescribe problem with unsorted groups
 (see 34814)

---
 Src/Zle/compcore.c |  9 +++++----
 Src/Zle/computil.c | 40 +++++++++++++++++++++++++++++++++++++---
 2 files changed, 42 insertions(+), 7 deletions(-)

diff --git a/Src/Zle/compcore.c b/Src/Zle/compcore.c
index 000f9da..d4051bd 100644
--- a/Src/Zle/compcore.c
+++ b/Src/Zle/compcore.c
@@ -2996,9 +2996,9 @@ mod_export void
 begcmgroup(char *n, int flags)
 {
     if (n) {
-	Cmgroup p = amatches;
-
-	while (p) {
+	/* If a group named <n> already exists, reuse it. */
+	Cmgroup p;
+	for (p = amatches; p; p = p->next) {
 #ifdef ZSH_HEAP_DEBUG
 	    if (memory_validate(p->heap_id)) {
 		HEAP_ERROR(p->heap_id);
@@ -3016,9 +3016,10 @@ begcmgroup(char *n, int flags)
 
 		return;
 	    }
-	    p = p->next;
 	}
     }
+
+    /* Create a new group. */
     mgroup = (Cmgroup) zhalloc(sizeof(struct cmgroup));
 #ifdef ZSH_HEAP_DEBUG
     mgroup->heap_id = last_heap_id;
diff --git a/Src/Zle/computil.c b/Src/Zle/computil.c
index a81d1dd..27938c1 100644
--- a/Src/Zle/computil.c
+++ b/Src/Zle/computil.c
@@ -210,6 +210,25 @@ cd_calc()
     }
 }
 
+/* Return 1 if cd_state specifies unsorted groups, 0 otherwise. */
+static int
+cd_groups_want_sorting(void)
+{
+    Cdset set;
+    char *const *i;
+
+    for (set = cd_state.sets; set; set = set->next)
+        for (i = set->opts; *i; i++) {
+            if (!strncmp(*i, "-V", 2))
+                return 0;
+            else if (!strncmp(*i, "-J", 2))
+                return 1;
+        }
+
+    /* Sorted by default */
+    return 1;
+}
+
 static int
 cd_sort(const void *a, const void *b)
 {
@@ -283,7 +302,8 @@ cd_prep()
 	    unmetafy(s->sortstr, &dummy);
 	}
 
-        qsort(grps, preplines, sizeof(Cdstr), cd_sort);
+        if (cd_groups_want_sorting())
+            qsort(grps, preplines, sizeof(Cdstr), cd_sort);
 
         for (i = preplines, strp = grps; i > 1; i--, strp++) {
             strp2 = strp + 1;
@@ -441,7 +461,17 @@ cd_arrcat(char **a, char **b)
     }
 }
 
-/* Initialisation. Store and calculate the string and matches and so on. */
+/* Initialisation. Store and calculate the string and matches and so on.
+ *
+ * nam: argv[0] of the builtin
+ * hide: ???
+ * mlen: see max-matches-width style
+ * sep: see list-seperator style
+ * opts: options to (eventually) pass to compadd.
+ *       Returned via 2nd return parameter of 'compdescribe -g'.
+ * args: ??? (the positional arguments to 'compdescribe')
+ * disp: 1 if descriptions should be shown, 0 otherwise
+ */
 
 static int
 cd_init(char *nam, char *hide, char *mlen, char *sep,
@@ -699,8 +729,12 @@ cd_get(char **params)
             dpys[0] = ztrdup(run->strs->str);
             mats[1] = dpys[1] = NULL;
             opts = cd_arrdup(run->strs->set->opts);
+
+            /* Set -2V, possibly reusing the group name from an existing -J/-V
+             * flag. */
             for (dp = opts + 1; *dp; dp++)
-                if (dp[0][0] == '-' && dp[0][1] == 'J')
+                if ((dp[0][0] == '-' && dp[0][1] == 'J') ||
+		    (dp[0][0] == '-' && dp[0][1] == 'V'))
                     break;
             if (*dp) {
                 char *s = tricat("-2V", "", dp[0] + 2);
-- 
1.9.1


[-- Attachment #3: 0002-git-completion-offer-HEAD-n-as-completions-in-git-re.patch --]
[-- Type: text/x-patch, Size: 2152 bytes --]

>From 4fdb3036dae738df6c10e959db3acf5e7575b504 Mon Sep 17 00:00:00 2001
From: Daniel Shahaf <d.s@daniel.shahaf.name>
Date: Thu, 14 May 2015 11:04:51 +0000
Subject: [PATCH 2/3] git completion: offer HEAD~$n as completions in git,
 remove 34814 workaround

---
 Completion/Unix/Command/_git | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/Completion/Unix/Command/_git b/Completion/Unix/Command/_git
index 9304fbb..378dc4c 100644
--- a/Completion/Unix/Command/_git
+++ b/Completion/Unix/Command/_git
@@ -5658,9 +5658,7 @@ __git_commit_objects () {
   # Abort if the argument does not match a commit hash (including empty).
   _guard '[[:xdigit:]](#c,40)' || return 1
 
-  # Note: the after-the-colon part must be unique across the entire array;
-  # see workers/34768
-  : ${(A)commits::=${(f)"$(_call_program commits git --no-pager log -1000 --all --reflog --format='%h:\[%h\]\ %s\ \(%cr\)')"}}
+  : ${(A)commits::=${(f)"$(_call_program commits git --no-pager log -1000 --all --reflog --format='%h:%s\ \(%cr\)')"}}
   __git_command_successful $pipestatus || return 1
 
   _describe -V -t commits 'commit object name' commits
@@ -5679,18 +5677,21 @@ __git_recent_commits () {
   __git_command_successful $pipestatus || return 1
 
   for i j k in "$commits[@]" ; do
-    # Note: the after-the-colon part must be unique across the entire array;
-    # see workers/34768
     if (( distance_from_head == 0 )); then
-      descr+=($i:"[HEAD]    $k")
+      descr+=("HEAD:$k")
+      descr+=("$i:$k")
     elif (( distance_from_head == 1 )); then
-      descr+=($i:"[HEAD^]   $k")
+      descr+=("HEAD^:$k")
+      descr+=("$i:$k")
     elif (( distance_from_head == 2 )); then
-      descr+=($i:"[HEAD^^]  $k")
+      descr+=("HEAD^^:$k")
+      descr+=($i:"$k")
     elif (( distance_from_head < 10 )); then
-      descr+=($i:"[HEAD~$distance_from_head]  $k")
+      descr+=("HEAD~$distance_from_head:$k")
+      descr+=($i:"$k")
     else
-      descr+=($i:"[HEAD~$distance_from_head] $k")
+      descr+=($i:"$k")
+      descr+=("HEAD~$distance_from_head:$k")
     fi
     (( ++distance_from_head ))
 
-- 
1.9.1

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

[Daniel Shahaf]

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.  None of the other patches touch any C code or
anything remotely related to the _describe internals.

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

[Bart Schaefer]

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?

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?

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)

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

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

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

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

[Daniel Shahaf]

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

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

[Bart Schaefer]

On May 17, 11:40pm, Daniel Shahaf wrote:
} Subject: Re: [PATCH] compdescribe fix for unsorted groups (workers/34814) 
}
} Bart Schaefer wrote on Sat, May 16, 2015 at 21:07:35 -0700:
} > 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

OK, now starting from commit 34a1489f436d95bc2404f8e371130a469cbccebe, I
apply 35127 and:

torch% zstyle -L
zstyle ':completion:*:messages' format %S%d%s
zstyle ':completion:*:warnings' format 'No matches for %U%d%u'
zstyle ':completion:*' format '%SCompleting %U%d%u%s'
zstyle ':completion:*' group-name ''
zstyle ':completion:*' ignore-parents parent pwd ..
zstyle ':completion::*' insert-tab 'pending=1'
zstyle ':completion:*' menu 'yes=long' 'select=long-list'
zstyle ':completion:*' verbose true
torch% git checkout 1<TAB>
Completing recent commit object name
1d5b225  -- 35100: __git_recent_commits: massage ' ->*' from heads (3 hours 
153a99d  -- 35154: NEWS on arithmetic evaluation changes (3 days ago)
15aa99b  -- 35139: complete the new (b) parameter flag (3 days ago)
1d5b225  -- 35100: __git_recent_commits: massage ' ->*' from heads (3 hours 
153a99d  -- 35154: NEWS on arithmetic evaluation changes (3 days ago)
15aa99b  -- 35139: complete the new (b) parameter flag (3 days ago)

The duplication remains annoying -- again, this is coming from commit
454f079852deb0c704870c7d5a462485f1e65bbf (workers/35101) -- but I still
can't reproduce what you're seeing.  I even whittled down to:

torch% zstyle -L   
zstyle ':completion:*' group-name ''
zstyle ':completion:*' tag-order 'commits commit-objects' '*'
torch% git checkout 1<TAB>
1d5b225  -- 35100: __git_recent_commits: massage ' ->*' from heads (3 hours 
153a99d  -- 35154: NEWS on arithmetic evaluation changes (3 days ago)
15aa99b  -- 35139: complete the new (b) parameter flag (3 days ago)
1d5b225  -- 35100: __git_recent_commits: massage ' ->*' from heads (3 hours 
153a99d  -- 35154: NEWS on arithmetic evaluation changes (3 days ago)
15aa99b  -- 35139: complete the new (b) parameter flag (3 days ago)

Is there something else being found in your git tree(s) that I don't have?

Then you have this:

} 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   

Something is generating 1 2 3 4 as possible completions.  Do you perhaps
have a git alias for a command that's being invoked by _git, so that the
output is not as _git expects?  (The "stray" 1 in your output is the 1 on
the command line matching exactly the 1 in this listing.)

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

[Daniel Shahaf]

Bart Schaefer wrote on Sun, May 17, 2015 at 21:00:27 -0700:
> On May 17, 11:40pm, Daniel Shahaf wrote:
> } Subject: Re: [PATCH] compdescribe fix for unsorted groups (workers/34814) 
> }
> } Bart Schaefer wrote on Sat, May 16, 2015 at 21:07:35 -0700:
> } > 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
> 

> Then you have this:
> 
> } 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   
> 
> Something is generating 1 2 3 4 as possible completions.  Do you perhaps
> have a git alias for a command that's being invoked by _git, so that the
> output is not as _git expects?  (The "stray" 1 in your output is the 1 on
> the command line matching exactly the 1 in this listing.)

The "1 2 3 4" come from these remote branches:

% git for-each-ref | grep pr/
e1a7e686156e0e450164151dd1d3be192574bed3 commit	refs/remotes/github/pr/1
a1cf0253d8437aac4858e65b0a07c97243f15ec6 commit	refs/remotes/github/pr/2
14e5aa4b0cf69134b36dfde72c8dec19d99bf1ff commit	refs/remotes/github/pr/3
60e0f7163b9a06d8f7acd612db914ab335c65396 commit	refs/remotes/github/pr/4
% git config -l | grep remote.github
remote.github.url=https://github.com/zsh-users/zsh
remote.github.fetch=+refs/heads/*:refs/remotes/github/*
remote.github.fetch=+refs/pull/*/head:refs/remotes/github/pr/*
remote.github.skipfetchall=true

> OK, now starting from commit 34a1489f436d95bc2404f8e371130a469cbccebe, I
> apply 35127 and:
> 
> torch% zstyle -L
> zstyle ':completion:*:messages' format %S%d%s
> zstyle ':completion:*:warnings' format 'No matches for %U%d%u'
> zstyle ':completion:*' format '%SCompleting %U%d%u%s'
> zstyle ':completion:*' group-name ''
> zstyle ':completion:*' ignore-parents parent pwd ..
> zstyle ':completion::*' insert-tab 'pending=1'
> zstyle ':completion:*' menu 'yes=long' 'select=long-list'
> zstyle ':completion:*' verbose true
> torch% git checkout 1<TAB>
> Completing recent commit object name
> 1d5b225  -- 35100: __git_recent_commits: massage ' ->*' from heads (3 hours 
> 153a99d  -- 35154: NEWS on arithmetic evaluation changes (3 days ago)
> 15aa99b  -- 35139: complete the new (b) parameter flag (3 days ago)
> 1d5b225  -- 35100: __git_recent_commits: massage ' ->*' from heads (3 hours 
> 153a99d  -- 35154: NEWS on arithmetic evaluation changes (3 days ago)
> 15aa99b  -- 35139: complete the new (b) parameter flag (3 days ago)

I get the same, modulo the additional remote branch.

> Is there something else being found in your git tree(s) that I don't have?

You tried to complete 'git checkout 1<TAB>'.  The bug reproduces when
I do 'git checkout <TAB>', i.e., space-tab, not space-digitone-tab.

I can still reproduce the bug in the above setting (34a1489 plus 35127
plus either set of zstyles), even after I remove my ~/.gitconfig and cd
to a fresh clone of git://git.code.sf.net/p/zsh/code.

I haven't noticed this before, but after pressing space and tab I get
a "zsh: do you wish to see all 140 possibilities (124 lines)?" prompt
that I answer 'y' to.  The figure 140 is because there are 100
completions and 40 descriptions; that is, the descriptions are counted
towards the total number of offered completions.  Why would descriptions
be counted towards the total number of completions?

Cheers,

Daniel

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

[Bart Schaefer]

On May 19,  1:36am, Daniel Shahaf wrote:
} Subject: Re: [PATCH] compdescribe fix for unsorted groups (workers/34814) 
}
} You tried to complete 'git checkout 1<TAB>'.  The bug reproduces when
} I do 'git checkout <TAB>', i.e., space-tab, not space-digitone-tab.

OK; I was just trying to repeat what you said in 35127:
> 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

I now realize that you don't mean "a screenful of ######" but rather a
screenful of git checksums.  I've been looking all this time for some
sort of horrible memory scrambling that was filling your screen with
garbage.  Sigh.

So ... 35127 deletes/disregards this comment:

-    # Note: the after-the-colon part must be unique across the entire array;
-    # see workers/34768

It then proceeds to add two completions for each description.  The problem
described in 34768 kicks in, and you get the results you observed.

If I modify the _git hunk in 35127 to force one $k part of each pair of
_descr+=(...) assignments to differ, everything works as desired, except
that there are still another duplicate set of everything from 35101.

} I haven't noticed this before, but after pressing space and tab I get
} a "zsh: do you wish to see all 140 possibilities (124 lines)?" prompt
} that I answer 'y' to.  The figure 140 is because there are 100
} completions and 40 descriptions; that is, the descriptions are counted
} towards the total number of offered completions.  Why would descriptions
} be counted towards the total number of completions?

Side-effect of the problem discussed in 34768, and a clue if we'd noted
it sooner.

I think it's up to you to figure out how to make 34671 work with 35127,
as they are both your patches.

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

[Daniel Shahaf]

Bart Schaefer wrote on Mon, May 18, 2015 at 21:37:14 -0700:
> On May 19,  1:36am, Daniel Shahaf wrote:
> } Subject: Re: [PATCH] compdescribe fix for unsorted groups (workers/34814) 
> }
> } You tried to complete 'git checkout 1<TAB>'.  The bug reproduces when
> } I do 'git checkout <TAB>', i.e., space-tab, not space-digitone-tab.
> 
> OK; I was just trying to repeat what you said in 35127:
> > 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
> 
> I now realize that you don't mean "a screenful of ######" but rather a
> screenful of git checksums.  I've been looking all this time for some
> sort of horrible memory scrambling that was filling your screen with
> garbage.  Sigh.

Sorry about the miscommunication.  I see my original phrasing was
ambiguous.  I don't understand why 35169 didn't resolve the ambiguity
(that post includes a verbatim copy of the garbled output).

> So ... 35127 deletes/disregards this comment:
> 
> -    # Note: the after-the-colon part must be unique across the entire array;
> -    # see workers/34768
> 
> It then proceeds to add two completions for each description.  The problem
> described in 34768 kicks in, and you get the results you observed.

The second patch in 35127 deliberately triggers the condition the
being-removed comment states is buggy, to prove that the first patch in
35127 fixes that particular bug.

The issue is now resolved; I'll write a separate email with details.
(Spoiler: 35127 is correct when applied on top of 35216.)

Again, sorry for the miscommunication, and thanks for the help.

Daniel

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

[Bart Schaefer]

On May 19,  8:41pm, Daniel Shahaf wrote:
} Subject: Re: [PATCH] compdescribe fix for unsorted groups (workers/34814) 
}
} Bart Schaefer wrote on Mon, May 18, 2015 at 21:37:14 -0700:
} > I now realize that you don't mean "a screenful of ######" but rather a
} > screenful of git checksums.
} 
} Sorry about the miscommunication.  I see my original phrasing was
} ambiguous.  I don't understand why 35169 didn't resolve the ambiguity
} (that post includes a verbatim copy of the garbled output).

I thought they were two different things, one that occurred with nothing
after "checkout" and one that occurred when completing a partial string
after "checkout".

} The second patch in 35127 deliberately triggers the condition the
} being-removed comment states is buggy, to prove that the first patch in
} 35127 fixes that particular bug.

OK, so far I'm taking your word for it. :-)

-- 
Barton E. Schaefer

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

[Daniel Shahaf]

[-- Attachment #1: Type: text/plain, Size: 1735 bytes --]

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.  [... 'git checkout 1<TAB>' ...]  I'm not sure whether
> this indicates a bug in the first patch or an independent problem.

The 'git checkout <TAB>' issue is an independent problem.

Daniel Hahler has determined that the garbled output is due to _git's
calling __git_commit_objects twice (cf 35216).  The attached minimal
example demonstrates the problem.

So, in summary:

1. The first patch in 35127 fixes the problem introduced by 34671,
reported in 34768, and analysed in 34814.  The minimal example at the
top of 35127 produces garbled output without 35127 and correct output
with 35127.

2. The second patch in 35127 triggered garbled output in 'git checkout
<TAB>' because of a preexisting problem in _git, which 35216 fixes.
An example of the garbled output is in 35169, surrounded by triple
square brackets.

3. I propose the following documentation patch:

diff --git a/Doc/Zsh/compsys.yo b/Doc/Zsh/compsys.yo
index a081ea3..5043e7b 100644
--- a/Doc/Zsh/compsys.yo
+++ b/Doc/Zsh/compsys.yo
@@ -4197,6 +4197,9 @@ description will appear together in the list.
 
 tt(_describe) uses the tt(_all_labels) function to generate the matches, so
 it does not need to appear inside a loop over tag labels.
+
+tt(_describe) should not be called more than once in a single call to
+a completion function.
 )
 findex(_description)
 item(tt(_description) [ tt(-x) ] [ tt(-12VJ) ] var(tag) var(name) var(descr) [ var(spec) ... ])(

Unless objections, I'll commit 35127#1 (the first of the two patches
attached to that email).

Daniel

[-- Attachment #2: ftwice.txt --]
[-- Type: text/plain, Size: 242 bytes --]

a=(dalton:bar charlie:bar bob:foo alice:foo)
_ftwice() { _describe -2 -V -x person a; _describe -2 -V -x person a }; compdef _ftwice ftwice
% ftwice <TAB>
> person
charlie
alice
dalton
bob
-- bar
-- foo
charlie
alice
dalton
bob
-- bar
-- foo

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

[Daniel Shahaf]

Daniel Shahaf wrote on Tue, May 19, 2015 at 20:44:46 +0000:
> 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.  [... 'git checkout 1<TAB>' ...]  I'm not sure whether
> > this indicates a bug in the first patch or an independent problem.
> 
> The 'git checkout <TAB>' issue is an independent problem.
> 
> Daniel Hahler has determined that the garbled output is due to _git's
> calling __git_commit_objects twice (cf 35216).  The attached minimal
> example demonstrates the problem.
> 
> So, in summary:
> 
> 1. The first patch in 35127 fixes the problem introduced by 34671,
> reported in 34768, and analysed in 34814.  The minimal example at the
> top of 35127 produces garbled output without 35127 and correct output
> with 35127.
> 
> 2. The second patch in 35127 triggered garbled output in 'git checkout
> <TAB>' because of a preexisting problem in _git, which 35216 fixes.
> An example of the garbled output is in 35169, surrounded by triple
> square brackets.
> 

With both 35127 and 35169 applied, I get the :descriptions message
twice:

    > recent commit object name
    > recent commit object name
    cf83173  HEAD     - debug (12 minutes ago)
    ...

The duplicate goes away if I revert
ed3e5f521d1243981831d7e084225b03e205ae38 (35209), i.e., add back the -2.

I verified that __git_recent_commits is called only once, so 35169 is
not at fault here.  I'm not sure what the actual problem is: whether
35209 should be reverted (the -2 added back), or 35127#1 should be
smarter about how compdescribe calls compadd (cf empty matches discussed
in 34814).  I'll investigate later.

Cheers,

Daniel

> 3. I propose the following documentation patch:
> 
> diff --git a/Doc/Zsh/compsys.yo b/Doc/Zsh/compsys.yo
> index a081ea3..5043e7b 100644
> --- a/Doc/Zsh/compsys.yo
> +++ b/Doc/Zsh/compsys.yo
> @@ -4197,6 +4197,9 @@ description will appear together in the list.
>  
>  tt(_describe) uses the tt(_all_labels) function to generate the matches, so
>  it does not need to appear inside a loop over tag labels.
> +
> +tt(_describe) should not be called more than once in a single call to
> +a completion function.
>  )
>  findex(_description)
>  item(tt(_description) [ tt(-x) ] [ tt(-12VJ) ] var(tag) var(name) var(descr) [ var(spec) ... ])(
> 
> Unless objections, I'll commit 35127#1 (the first of the two patches
> attached to that email).
> 
> Daniel

> a=(dalton:bar charlie:bar bob:foo alice:foo)
> _ftwice() { _describe -2 -V -x person a; _describe -2 -V -x person a }; compdef _ftwice ftwice
> % ftwice <TAB>
> > person
> charlie
> alice
> dalton
> bob
> -- bar
> -- foo
> charlie
> alice
> dalton
> bob
> -- bar
> -- foo

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

[Bart Schaefer]

On May 19,  8:44pm, Daniel Shahaf wrote:
} Subject: Re: [PATCH] compdescribe fix for unsorted groups (workers/34814) 
}
} 3. I propose the following documentation patch:
} 
} +tt(_describe) should not be called more than once in a single call to
} +a completion function.

Is it really any time _describe is called twice, or is it just that
_describe should not be called more than once with the same (or an
overlapping?) set of descriptions?

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

[Daniel Shahaf]

Bart Schaefer wrote on Tue, May 19, 2015 at 15:23:00 -0700:
> On May 19,  8:44pm, Daniel Shahaf wrote:
> } Subject: Re: [PATCH] compdescribe fix for unsorted groups (workers/34814) 
> }
> } 3. I propose the following documentation patch:
> } 
> } +tt(_describe) should not be called more than once in a single call to
> } +a completion function.
> 
> Is it really any time _describe is called twice, or is it just that
> _describe should not be called more than once with the same (or an
> overlapping?) set of descriptions?

If you call _describe more than once, with the same tag parameter in
both calls (or with no -t parameter in both calls), and each of the two
calls (separately) has a pair of items with equal description, then the
display will be garbled.  This happens even if no flags (other than
possibly -t) are passed.

I'm not sure whether that's a complete characterization, i.e., whether
there are other circumstances that also trigger the problem.  I think
the trick might be to get cd_get() to pass -E to compadd (inserting an
empty match) twice during one completion operation.

While looking into this, I've also found a 100% CPU consumption bug; to
reproduce (in master):

    autoload compinit
    compinit
    _f() { d=(d1); a=(); compadd -XX -JJ -E1 -d d -a a;  compadd -XX -JJ -E1 -d d -a a }; compdef _f f
    f <TAB><TAB><TAB>

Backtrace:

    0x00007ffff60b7a5d in do_menucmp (lst=4) at compresult.c:1234
    1234		    } while (!(minfo.group)->mcount);
    (gdb) bt
    #0  0x00007ffff60b7a5d in do_menucmp (lst=4) at compresult.c:1234
    #1  0x00007ffff60a2aaf in before_complete (dummy=0x7ffff6516c30, lst=0x7fffffffdd5c) at compcore.c:478
    #2  0x000000000046870f in runhookdef (h=0x7ffff6516c30, d=0x7fffffffdd5c) at module.c:996
    #3  0x00007ffff62f7084 in docomplete (lst=4) at zle_tricky.c:622
    #4  0x00007ffff62f66b7 in expandorcomplete (args=0x7ffff6517188) at zle_tricky.c:315

It's spinning in the outer of the two do-while loops in there, with
minfo.cur being a dummy match.

Cheers,

Daniel

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

[Bart Schaefer]

On May 20, 11:51pm, Daniel Shahaf wrote:
}
} While looking into this, I've also found a 100% CPU consumption bug; to
} reproduce (in master):
} 
}     autoload compinit
}     compinit
}     _f() { d=(d1); a=(); compadd -XX -JJ -E1 -d d -a a;  compadd -XX -JJ -E1 -d d -a a }; compdef _f f
}     f <TAB><TAB><TAB>
} 
} It's spinning in the outer of the two do-while loops in there, with
} minfo.cur being a dummy match.

It reaches the state where it has to try amatches, but the "default"
group to which amatches points contains only a CMF_DUMMY, so it won't
display it; thus it tries to go to the next match, but there are no
more, so back to amatches we go again, and loop.

So the obvious thing would seem to be to stop the loop if we arrive at
the "examine amatches" state twice, which is easy enough to do; but
then on a subsequent (fourth, if repeating the example above) attempt,
*++(minfo.cur) runs off the end of memory and the shell crashes on the
subsequent access through (*minfo.cur).

Thus the less-obvious thing to do is, upon discovering the twice-amatches
case, force everything back to the state it was in the second-to-last
time do_menucmp() was called, so we just loop displaying the menu in a
stable way.  Trouble is, I'm not sure where that state can be found.

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

[Daniel Shahaf]

Daniel Shahaf wrote on Wed, May 20, 2015 at 23:51:58 +0000:
> While looking into this, I've also found a 100% CPU consumption bug; to
> reproduce (in master):

So, another summary...

- 35127#1 (fix for _describe -2V with duplicate descriptions): no reason
  to hold it back any longer; I'll push it.

- 35227 (duplicate :descriptions message): cosmetic issue.  The example
  given in that email requires 35127#2, which I'm not going to push yet,
  and no other example is known, so this issue is not blocking.

- 35246#tail (infinite loop): pre-existing problem, not related to my
  changes (it also happens in 5.0.7, which precedes my changes).  No
  known instances "in the wild".  I'm not currently looking into it.

- 35246#head (_describe output garbled): likewise, a pre-existing
  problem with no known instances.  Enclosed a patch to record its
  existence.

- Empty matches, probably added by cd_get(), are definitely related to
  the third issue and may be related to the second and fourth as well.

Is there better way to track all these issues than X-Seq numbers?

---

diff --git a/Completion/Base/Utility/_describe b/Completion/Base/Utility/_describe
index ab72005..76ab1d9 100644
--- a/Completion/Base/Utility/_describe
+++ b/Completion/Base/Utility/_describe
@@ -1,5 +1,12 @@
 #autoload
 
+# ### Note: Calling this function twice during one completion operation, such
+# ### that in each call there exists a pair of items having the same description
+# ### as each other, and the two calls specify the same $_type, currently leads
+# ### to garbled output; see workers/35229 (May 2015) and its thread (which also
+# ### discusses at least two other issues, that may or may not be related to
+# ### this one).
+
 # This can be used to add options or values with descriptions as matches.
 
 local _opt _expl _tmpm _tmpd _mlen _noprefix