master 03ac16ece40: Correctly handle common prefixes in substring completion

[Stefan Monnier]

branch: master
commit 03ac16ece40ba3e3ba805d6a61cc457d84bf3792
Author: Spencer Baugh <sbaugh@janestreet.com>
Commit: Stefan Monnier <monnier@iro.umontreal.ca>

    Correctly handle common prefixes in substring completion
    
    Substring completion would previously not complete the longest common
    substring if that substring was a prefix of all the completion
    alternatives.  Now it does.  An explanation of this bug
    
    Substring completion is implemented by passing the `prefix' symbol as
    part of the pattern passed to completion-pcm--merge-completions.  This
    symbol is supposed to cause completion-pcm--merge-completions to
    "grow" a completion of a common substring only from the "right" of the
    symbol (a common suffix), not from the "left" of the symbol (a common
    prefix).  Yes, this is the opposite of what the name `prefix' would
    imply.
    
    When processing a symbolic element of the pattern,
    completion-pcm--merge-completions first finds the common prefix of all
    the completions in that part of the pattern (using try-completion).
    Then for `prefix' and other elements which want to complete a common
    suffix, the common prefix is removed from each element and then the
    common suffix is calculated with completion--common-suffix.
    
    If the common prefix covers the entirety of all the alternatives
    (i.e. when "unique" is true in the code), it's also a common suffix.
    In that case, the common suffix calculation (if it runs) is basically
    a no-op which will produce an empty string, since we removed the
    common prefix before running it.
    
    Before this change, `prefix' elements would unconditionally discard
    the common prefix, which produced the wrong result in the case that
    common prefix == common suffix.  For example:
    
      (completion-pcm--merge-completions '("ab" "ab") '(prefix "b"))
      -> ("b")
    
    Now we detect this situation and include the common prefix in this
    case for `prefix' elements.  Then we get:
    
      (completion-pcm--merge-completions '("ab" "ab") '(prefix "b"))
      -> ("b" "a")
    
    which is correct.
    
    * lisp/minibuffer.el (completion-pcm--merge-completions): Don't ignore
    a common suffix in a `prefix' pattern element when it's also a common
    prefix.
    * test/lisp/minibuffer-tests.el (completion-substring-test-5): Add a
    test.
---
 lisp/minibuffer.el            |  4 +++-
 test/lisp/minibuffer-tests.el | 13 +++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 5d7cbcedd41..52a286018a1 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -4025,7 +4025,9 @@ the same set of elements."
                      (unique (or (and (eq prefix t) (setq prefix fixed))
                                  (and (stringp prefix)
                                       (eq t (try-completion prefix comps))))))
-                (unless (or (eq elem 'prefix)
+                ;; if the common prefix is unique, it also is a common
+                ;; suffix, so we should add it for `prefix' elements
+                (unless (or (and (eq elem 'prefix) (not unique))
                             (equal prefix ""))
                   (push prefix res))
                 ;; If there's only one completion, `elem' is not useful
diff --git a/test/lisp/minibuffer-tests.el b/test/lisp/minibuffer-tests.el
index ff58d35eb3e..4f92d7f841c 100644
--- a/test/lisp/minibuffer-tests.el
+++ b/test/lisp/minibuffer-tests.el
@@ -298,6 +298,19 @@
                   "jab" '("dabjabstabby" "many") nil 3)))
            6)))
 
+(ert-deftest completion-substring-test-5 ()
+  ;; merge-completions needs to work correctly when
+  (should (equal
+           (completion-pcm--merge-completions '("ab" "sab") '(prefix "b"))
+           '("b" "a" prefix)))
+  (should (equal
+           (completion-pcm--merge-completions '("ab" "ab") '(prefix "b"))
+           '("b" "a")))
+  ;; substring completion should successfully complete the entire string
+  (should (equal
+           (completion-substring-try-completion "b" '("ab" "ab") nil 0)
+           '("ab" . 2))))
+
 (ert-deftest completion-flex-test-1 ()
   ;; Fuzzy match
   (should (equal