Update our completing-read-multiple replacement/wrapper

Among other things we use crm to read revision ranges such as
"A..B" and "C...D".  As far as crm is concerned ".." and "..." are
interchangeable separators but to Git they mean different things.
We cannot use `completing-read-multiple' when reading ranges because
it returns a list of the read values, stripping the separators.

`magit-completing-read-multiple' was added in [1: 388a9a254] to avoid
some (additional upcoming) duplication between our two functions that
read ranges.

Giving it that name was a mistake because its function signature
is not compatible with that of `completing-read-multiple'.  In
[2: cb4d59190] `magit-completing-read-multiple*' was added to fix
that, but we did not port existing callers immediately because of
the involved risk.

Meanwhile we added more kludges to `magit-completing-read-multiple'
and `magit-completing-read-multiple*', addressing additional defects
and undesirable features of various completion frameworks.

Now the time has come to deprecate `magit-completing-read-multiple'
in favor of `magit-completing-read-multiple*'.  We should stop using
the former name for now, because of the differences in the function
signatures.  Eventually we would like to use the former name again,
but if we abruptly changing the signature of an existing function,
that would risk breaking third-party packages.  The use of an
intermediate function name minimizes the risk to third-parties.

The previous approach was to copied `completing-read-multiple' and
to modified the copy to our needs, but we then failed to incorporate
upstream changes into our copies, so now we use a wrapper instead.

Another reason to use a wrapper is that then we don't bypass any
advice that may have been added to the original.  For example the
`vertico' completion framework advices `completing-read-multiple'
but that advise obviously did not affect our copies.

1: 388a9a254f6ee985e67926d44a2d9ebad79af897
   completion: move common crm setup to a function

2: cb4d591900187b77e54ec00cac5f25b644153b5b
   magit-completing-read-multiple*: New function

Because this commit shuffles things around quite a bit, making history
inspection harder, I have added a code comment for each issue that our
wrapper addresses.

Co-authored-by: Miha Rihtaršič <miha@kamnitnik.top>
- Avoid changing the return value of split-string as doing that
  would break selectrum's crm function.
- Cosmetics.

5 files changed, 71 insertions, 51 deletions

diff --git a/Documentation/RelNotes/3.1.0.org b/Documentation/RelNotes/3.1.0.org
index c077291..40f425b 100644
--- a/Documentation/RelNotes/3.1.0.org
+++ b/Documentation/RelNotes/3.1.0.org
@@ -1,5 +1,11 @@
 * Magit v3.1.0 Release Notes (unreleased)
 ** Breaking changes
+
+- The function signature of ~magit-completing-read-multiple~ was not
+  compatible with that of ~completing-read-multiple~, so we deprecate
+  it.  Use the improved ~magit-completing-read-multiple*~ instead.
+  #4420
+
 ** Changes since v3.0.0
 
 - Added new command ~magit-log-move-to-revision~.  #4418
diff --git a/lisp/magit-git.el b/lisp/magit-git.el
index 4c7041c..f7ecd6d 100644
--- a/lisp/magit-git.el
+++ b/lisp/magit-git.el
@@ -2194,10 +2194,11 @@ and this option only controls what face is used.")
        (magit-get-current-branch))))
 
 (defun magit-read-range (prompt &optional default)
-  (magit-completing-read-multiple prompt
-                                  (magit-list-refnames)
-                                  "\\.\\.\\.?"
-                                  default 'magit-revision-history))
+  (let ((crm-separator "\\.\\.\\.?"))
+    (magit-completing-read-multiple*
+     (concat prompt ": ")
+     (magit-list-refnames)
+     nil nil nil 'magit-revision-history default nil t)))
 
 (defun magit-read-remote-branch
     (prompt &optional remote default local-branch require-match)
diff --git a/lisp/magit-log.el b/lisp/magit-log.el
index 05da238..19e57fb 100644
--- a/lisp/magit-log.el
+++ b/lisp/magit-log.el
@@ -595,16 +595,17 @@ the upstream isn't ahead of the current branch) show."
 
 (defun magit-log-read-revs (&optional use-current)
   (or (and use-current (--when-let (magit-get-current-branch) (list it)))
-      (let ((collection (magit-list-refnames nil t)))
-        (split-string
-         (magit-completing-read-multiple "Log rev,s" collection
-                                         "\\(\\.\\.\\.?\\|[, ]\\)"
-                                         (or (magit-branch-or-commit-at-point)
-                                             (unless use-current
-                                               (magit-get-previous-branch)))
-                                         'magit-revision-history
-                                         magit-log-read-revs-map)
-         "[, ]" t))))
+      (let ((crm-separator "\\(\\.\\.\\.?\\|[, ]\\)")
+            (crm-local-completion-map magit-log-read-revs-map))
+        (split-string (magit-completing-read-multiple*
+                       "Log rev,s: "
+                       (magit-list-refnames nil t)
+                       nil nil nil 'magit-revision-history
+                       (or (magit-branch-or-commit-at-point)
+                           (unless use-current
+                             (magit-get-previous-branch)))
+                       nil t)
+                      "[, ]" t))))
 
 (defun magit-log-read-pattern (option)
   "Read a string from the user to pass as parameter to OPTION."
diff --git a/lisp/magit-push.el b/lisp/magit-push.el
index 2a98591..3710e4e 100644
--- a/lisp/magit-push.el
+++ b/lisp/magit-push.el
@@ -217,11 +217,10 @@ only available for the part before the colon, or when no colon
 is used."
   (interactive
    (list (magit-read-remote "Push to remote")
-         (split-string (magit-completing-read-multiple
-                        "Push refspec,s"
-                        (cons "HEAD" (magit-list-local-branch-names))
-                        nil nil 'magit-push-refspecs-history)
-                       crm-default-separator t)
+         (magit-completing-read-multiple*
+          "Push refspec,s: "
+          (cons "HEAD" (magit-list-local-branch-names))
+          nil nil nil 'magit-push-refspecs-history)
          (magit-push-arguments)))
   (run-hooks 'magit-credential-hook)
   (magit-run-git-async "push" "-v" args remote refspecs))
diff --git a/lisp/magit-utils.el b/lisp/magit-utils.el
index 24a0815..d7f1862 100644
--- a/lisp/magit-utils.el
+++ b/lisp/magit-utils.el
@@ -567,6 +567,7 @@ When KEYMAP is nil, it defaults to `crm-local-completion-map'.
 
 Unlike `completing-read-multiple', the return value is not split
 into a list."
+  (declare (obsolete magit-completing-read-multiple* "Magit 3.1.0"))
   (let* ((crm-separator (or sep crm-default-separator))
          (crm-completion-table (magit--completion-table choices))
          (choose-completion-string-functions
@@ -592,40 +593,52 @@ into a list."
 
 (defun magit-completing-read-multiple*
     (prompt table &optional predicate require-match initial-input
-            hist def inherit-input-method)
+            hist def inherit-input-method
+            no-split)
   "Read multiple strings in the minibuffer, with completion.
 Like `completing-read-multiple' but don't mess with order of
-TABLE.  Also bind `helm-completion-in-region-default-sort-fn'
-to nil."
-  (unwind-protect
-      (cl-letf (((symbol-function 'completion-pcm--all-completions)))
-        (when (< emacs-major-version 26)
-          (fset 'completion-pcm--all-completions
-                'magit-completion-pcm--all-completions))
-        (add-hook 'choose-completion-string-functions
-                  'crm--choose-completion-string)
-        (let* ((minibuffer-completion-table #'crm--collection-fn)
-               (minibuffer-completion-predicate predicate)
-               ;; see completing_read in src/minibuf.c
-               (minibuffer-completion-confirm
-                (unless (eq require-match t) require-match))
-               (crm-completion-table (magit--completion-table table))
-               (map (if require-match
-                        crm-local-must-match-map
-                      crm-local-completion-map))
-               (helm-completion-in-region-default-sort-fn nil)
-               (ivy-sort-matches-functions-alist nil)
-               ;; If the user enters empty input, `read-from-minibuffer'
-               ;; returns the empty string, not DEF.
-               (input (read-from-minibuffer
-                       prompt initial-input map
-                       nil hist def inherit-input-method)))
-          (when (and def (string-equal input ""))
-            (setq input (if (consp def) (car def) def)))
-          ;; Remove empty strings in the list of read strings.
-          (split-string input crm-separator t)))
-    (remove-hook 'choose-completion-string-functions
-                 'crm--choose-completion-string)))
+TABLE and take an additional argument NO-SPLIT, which causes
+the user input to be returned as a single unmodified string.
+Also work around various misfeatures of various third-party
+completion frameworks."
+  (cl-letf*
+      (;; To implement NO-SPLIT we have to manipulate the respective
+       ;; `split-string' invocation.  We cannot simply advice it to
+       ;; return the input string because `SELECTRUM' would choke on
+       ;; it that string.  Use a variable to pass along the raw user
+       ;; input string. aa5f098ab
+       (input nil)
+       (split-string (symbol-function 'split-string))
+       ((symbol-function 'split-string)
+        (lambda (string &optional separators omit-nulls trim)
+          (when (and no-split
+                     (equal separators crm-separator)
+                     (equal omit-nulls t))
+            (setq input string))
+          (funcall split-string string separators omit-nulls trim)))
+       ;; In Emacs 25 this function has a bug, so we use a copy of the
+       ;; version from Emacs 26. bef9c7aa3
+       ((symbol-function 'completion-pcm--all-completions)
+        (if (< emacs-major-version 26)
+            'magit-completion-pcm--all-completions
+          (symbol-function 'completion-pcm--all-completions)))
+       ;; Prevent `BUILT-IN' completion from messing up our existing
+       ;; order of the completion candidates. aa5f098ab
+       (table (magit--completion-table table))
+       ;; Prevent `IVY' from messing up our existing order. c7af78726
+       (ivy-sort-matches-functions-alist nil)
+       ;; Prevent `HELM' from messing up our existing order.  6fcf994bd
+       (helm-completion-in-region-default-sort-fn nil)
+       ;; Prevent `HELM' from automatically appending the separator,
+       ;; which is counterproductive when NO-SPLIT is non-nil and/or
+       ;; when reading commit ranges. 798aff564
+       (helm-crm-default-separator
+        (if no-split nil helm-crm-default-separator))
+       ;; And now, the moment we have all been waiting for...
+       (values (completing-read-multiple
+                prompt table predicate require-match initial-input
+                hist def inherit-input-method)))
+    (if no-split input values)))
 
 (defun magit-ido-completing-read
     (prompt choices &optional predicate require-match initial-input hist def)