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

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.

From 38c05e49a4beaa1dcd0398bbe6674294ab5c4a5c Mon Sep 17 00:00:00 2001
From: Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>
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

From 4fdb3036dae738df6c10e959db3acf5e7575b504 Mon Sep 17 00:00:00 2001
From: Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>
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