Additional commit default in minibuffer requires `M-n M-n M-p`

[paulapatience]

The key sequence required to reach the additional commit default in the minibuffer from magit-read-other-branch-or-commit should be M-n (see https://github.com/magit/magit/issues/4291), but it currently (with magit-version output of Magit c1fb53d3, Git 2.37.2, Emacs 29.0.50, gnu/linux) is M-n M-n M-p. To reproduce, put the cursor on a commit line in the recent commits section for which the commit has an associated branch, call magit-checkout, and press M-n M-n M-p.

Removing default from the magit-completing-read call in magit-read-other-branch-or-commit (i.e., letting fallback be nil) fixes the issue, but I don't know enough about that function and the other instances of magit-completing-read (for which I assume fallback needs to remain) to submit a PR.

[kyleam]

To reproduce, put the cursor on a commit line in the recent commits section for which the commit has an associated branch, call magit-checkout, and press M-n M-n M-p.

I haven't been able to reproduce. When I do that, M-n is all I need to pull the commit at point into the minibuffer. I tried with

• Emacs 29.0.50 built from 07c04da0101 (2022-08-20) and default completion
• Emacs 28.1 and default completion
• Emacs 28.1 and ivy (0.13.4)

That's all with the current tip of Magit's master (ffbbfa1e) and an otherwise vanilla configuration.

Removing default from the magit-completing-read call in magit-read-other-branch-or-commit (i.e., letting fallback be nil) fixes the issue

It doesn't change your observation, but note that default is being passed as the def argument, not fallback.

https://github.com/magit/magit/blob/ffbbfa1e2de3673703b32bb1e1246d5086eab33c/lisp/magit-git.el#L2527-L2535

---

What completion framework are you using?

[paulapatience]

I use Doom Emacs, with vertico, and that is the environment in which I discovered the problem. But I also tried with vanilla Emacs 29.0.50 by running the command returned from magit-emacs-Q-command (so with default completion) and I got the same problem. I also just tried with Magit v3.3.0-422-gffbbfa1e, Git 2.37.2, Emacs 28.1, gnu/linux, and again, same problem.

Of note, perhaps, is that I've installed Emacs through Guix, though Magit in the first two cases was installed through Doom Emacs (i.e., with straight.el), and in the last case Magit's dependencies through Guix and Magit directly from the Git repository (by running guix shell --pure emacs emacs-magit emacs-compat git and then emacs -Q -L path/to/magit/lisp).

I suppose my next step will be to try with the latest Emacs compiled from source.

Edit: I have discovered that the issue manifests itself only in some repositories. For example, in a freshly checked out copy of Magit, M-n correctly displays the commit hash; in some of my other repositories it doesn't. I will investigate further.

Edit 2: Here are the steps to reproduce the issue:

• Check out a fresh copy of Magit (or whatever other Git repository) somewhere.
• Navigate to it in Emacs and open magit-status.
• magit-checkout a commit with HEAD detached, e.g., the commit immediately preceding master.
• Open the l L log.
• Call magit-checkout on the commit line of master.
• Press M-n M-n M-p.

I think the problem comes down to having had a detached HEAD in the past. Maybe it duplicates the branch name in the hidden candidates list or something.

Edit 3: The issue manifests for me when magit-get-previous-branch returns non-nil. Another way I can reproduce the issue is by creating a new branch and then calling magit-checkout on another branch. The previous branch will be the newly created one. The issue isn't specific to having a detached HEAD.

Edit 4: The first time goto-history-element is called, i.e., when the first M-n is pressed, (funcall minibuffer-default-add-function) returns the branch name, and it's only the second time that goto-history-element is called that the same funcall returns the commit hash.

Edit 5: Actually, it's not that (funcall minibuffer-default-add-function) returns the branch name, it's that during the first goto-history-element, minibuffer-default-add-function is not called at all, because

(< nabs (- (if (listp minibuffer-default)
               (length minibuffer-default)
             1)))

returns nil.

[paulapatience]

I have determined the cause of the issue. If we provide def to magit-completing-read, goto-history-element thinks it doesn't need to add the special default from minibuffer-default-add-function (see the comparison with nabs in my previous message).

One way to fix the issue is to provide nothing for def, but ensure it still appears in the list of candidates by consing it to the collection provided to magit-completing-read, as in the following patch:

diff --git a/lisp/magit-git.el b/lisp/magit-git.el
index 297a63ea..4665e52d 100644
--- a/lisp/magit-git.el
+++ b/lisp/magit-git.el
@@ -2531,8 +2531,11 @@ and this option only controls what face is used.")
                       (and (not (equal current exclude)) current)
                       secondary-default
                       (magit-get-previous-branch))))
-    (or (magit-completing-read prompt (delete exclude (magit-list-refnames))
-                               nil nil nil 'magit-revision-history default)
+    (or (magit-completing-read prompt (thread-last (magit-list-refnames)
+                                                   (delete exclude)
+                                                   (delete default)
+                                                   (cons default))
+                               nil nil nil 'magit-revision-history)
         (user-error "Nothing selected"))))

 (defun magit-read-other-local-branch

This solution is better than just setting def to nil, which I suggested in my first post, because doing so causes the value of default to be missing from the candidates. I don't know if it's the ideal solution, however.

[kyleam]

@paulapatience I've got a longer message in the works.

[kyleam]

@paulapatience Thanks for narrowing down the conditions that trigger it. When I tried with your initial recipe ("Recent commits"), the commit at point was always being passed as default to magit-completing-read, which is why the behavior wasn't triggered. For context when reading this: I started preparing this after your edit 2 above, and haven't yet read your updates closely or made any attempt to reconcile what I say here with your thoughts.

My conclusion based on looking into it so far:

• The weird state where the commit at point isn't accessible with M-n is due to magit--minibuf-default-add-commit not wiring things up in the expected order. It's an interaction with minibuffer-default, so it only shows up when a non-nil default is specified to magit-completing-read and is not the same value as the commit at point.

• By using what I think is the right order, I can get the built-in completion to behave as I expect. That doesn't actually mean the commit at point is accessible with one M-n; it's either one or two depending on whether there's a default. That is, there's no weird state where you can only access it with M-p.

• ivy doesn't seem to play well with this. That makes me worried that we can't expect third-party completion frameworks to play well with a custom minibuffer-default-add-function, at least with how we're currently overriding the function. If I switch to using setting up magit--minibuf-default-add-commit via minibuffer-with-setup-hook, ivy seems happier, but I haven't tested that change much.

More details

8406b084 (Add additional default when reading branch or commit, 2021-08-29) wired up a minibuffer-default-add-function that looks like this:

(defun magit--minibuf-default-add-commit ()
  (let ((fn minibuffer-default-add-function))
    (lambda ()
      (if-let ((commit (with-selected-window (minibuffer-selected-window)
                         (magit-commit-at-point))))
          (cons commit (delete commit (funcall fn)))
        (funcall fn)))))

Compare that to the default minibuffer-default-add-function:

(defun minibuffer-default-add-completions ()
  "Return a list of all completions without the default value.
This function is used to add all elements of the completion table to
the end of the list of defaults just after the default value."
  (let ((def minibuffer-default)
        (all (all-completions ""
                              minibuffer-completion-table
                              minibuffer-completion-predicate)))
    (if (listp def)
        (append def all)
      (cons def (delete def all)))))

Notice that it considers minibuffer-default, and puts it in the front, where downstream code expects it.

So, one way to preserve the ordering in magit--minibuf-default-add-commit would be this:

diff --git a/lisp/magit-git.el b/lisp/magit-git.el
index 297a63ea..3848cf43 100644
--- a/lisp/magit-git.el
+++ b/lisp/magit-git.el
@@ -2420,7 +2420,12 @@ (defun magit--minibuf-default-add-commit ()
     (lambda ()
       (if-let ((commit (with-selected-window (minibuffer-selected-window)
                          (magit-commit-at-point))))
-          (cons commit (delete commit (funcall fn)))
+          (let ((rest (cons commit
+                            (delete commit (funcall fn))))
+                (def minibuffer-default))
+            (if (listp def)
+                (append def rest)
+              (cons def (delete def rest))))
         (funcall fn)))))

 (defun magit-read-branch (prompt &optional secondary-default)

With my testing, this works well with the built-in completion. If there's a default passed to magit-completing-read, I can first pull that default in with M-n and then I can pull in the commit at point from the "next history" added by magit--minibuf-default-add-commit with another M-n. (In this case, it's the second M-n that triggers the call of minibuffer-default-add-function.) If there's not a default or if the commit at point happens to be the default, it's a single M-n.

If I try with ivy (2051de5), it kind of works, but once I pull the commit at point in, I can't access any of the other "history" with M-n or M-p.

How Magit overrides minibuffer-default-add-function is a bit different than most spots in the Emacs code base (there are only a few). If I convert it over to using minibuffer-with-setup-hook, the history is no longer entirely overwritten.

minibuffer-with-setup-hook patch

```diff diff --git a/lisp/magit-git.el b/lisp/magit-git.el index 297a63ea..f79470bb 100644 --- a/lisp/magit-git.el +++ b/lisp/magit-git.el @@ -2416,12 +2416,15 @@ (defun magit-thingatpt--git-revision () (defvar magit-revision-history nil) (defun magit--minibuf-default-add-commit () - (let ((fn minibuffer-default-add-function)) - (lambda () - (if-let ((commit (with-selected-window (minibuffer-selected-window) - (magit-commit-at-point)))) - (cons commit (delete commit (funcall fn))) - (funcall fn))))) + (if-let ((commit (with-selected-window (minibuffer-selected-window) + (magit-commit-at-point)))) + (let ((rest (cons commit + (delete commit (minibuffer-default-add-completions)))) + (def minibuffer-default)) + (if (listp def) + (append def rest) + (cons def (delete def rest)))) + (minibuffer-default-add-completions))) (defun magit-read-branch (prompt &optional secondary-default) (magit-completing-read prompt (magit-list-branch-names) @@ -2431,7 +2434,10 @@ (defun magit-read-branch (prompt &optional secondary-default) (magit-get-current-branch)))) (defun magit-read-branch-or-commit (prompt &optional secondary-default) - (let ((minibuffer-default-add-function (magit--minibuf-default-add-commit))) + (minibuffer-with-setup-hook + (lambda () + (setq-local minibuffer-default-add-function + #'magit--minibuf-default-add-commit)) (or (magit-completing-read prompt (magit-list-refnames nil t) nil nil nil 'magit-revision-history (or (magit-branch-or-commit-at-point) @@ -2450,12 +2456,15 @@ (defun magit-read-range-or-commit (prompt &optional secondary-default) (magit-get-current-branch)))) (defun magit-read-range (prompt &optional default) - (let ((minibuffer-default-add-function (magit--minibuf-default-add-commit)) - (crm-separator "\\.\\.\\.?")) - (magit-completing-read-multiple* - (concat prompt ": ") - (magit-list-refnames) - nil nil nil 'magit-revision-history default nil t))) + (minibuffer-with-setup-hook + (lambda () + (setq-local minibuffer-default-add-function + #'magit--minibuf-default-add-commit)) + (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) @@ -2486,16 +2495,19 @@ (defun magit-read-local-branch (prompt &optional secondary-default) (magit-get-current-branch)))) (defun magit-read-local-branch-or-commit (prompt) - (let ((minibuffer-default-add-function (magit--minibuf-default-add-commit)) - (choices (nconc (magit-list-local-branch-names) - (magit-list-special-refnames))) - (commit (magit-commit-at-point))) - (when commit - (push commit choices)) - (or (magit-completing-read prompt choices - nil nil nil 'magit-revision-history - (or (magit-local-branch-at-point) commit)) - (user-error "Nothing selected")))) + (minibuffer-with-setup-hook + (lambda () + (setq-local minibuffer-default-add-function + #'magit--minibuf-default-add-commit)) + (let ((choices (nconc (magit-list-local-branch-names) + (magit-list-special-refnames))) + (commit (magit-commit-at-point))) + (when commit + (push commit choices)) + (or (magit-completing-read prompt choices + nil nil nil 'magit-revision-history + (or (magit-local-branch-at-point) commit)) + (user-error "Nothing selected"))))) (defun magit-read-local-branch-or-ref (prompt &optional secondary-default) (magit-completing-read prompt (nconc (magit-list-local-branch-names) @@ -2520,20 +2532,23 @@ (defun magit-read-other-branch (defun magit-read-other-branch-or-commit (prompt &optional exclude secondary-default) - (let* ((minibuffer-default-add-function (magit--minibuf-default-add-commit)) - (current (magit-get-current-branch)) - (atpoint (magit-branch-or-commit-at-point)) - (exclude (or exclude current)) - (default (or (and (not (equal atpoint exclude)) - (not (and (not current) - (magit-rev-equal atpoint "HEAD"))) - atpoint) - (and (not (equal current exclude)) current) - secondary-default - (magit-get-previous-branch)))) - (or (magit-completing-read prompt (delete exclude (magit-list-refnames)) - nil nil nil 'magit-revision-history default) - (user-error "Nothing selected")))) + (minibuffer-with-setup-hook + (lambda () + (setq-local minibuffer-default-add-function + #'magit--minibuf-default-add-commit)) + (let* ((current (magit-get-current-branch)) + (atpoint (magit-branch-or-commit-at-point)) + (exclude (or exclude current)) + (default (or (and (not (equal atpoint exclude)) + (not (and (not current) + (magit-rev-equal atpoint "HEAD"))) + atpoint) + (and (not (equal current exclude)) current) + secondary-default + (magit-get-previous-branch)))) + (or (magit-completing-read prompt (delete exclude (magit-list-refnames)) + nil nil nil 'magit-revision-history default) + (user-error "Nothing selected"))))) (defun magit-read-other-local-branch (prompt &optional exclude secondary-default no-require-match) ```

[kyleam]

One way to fix the issue is to provide nothing for def,

In my opinion, playing around with the default isn't really on the table. That's a very central thing to the completion behavior. On the other hand, the behavior being discussed here (using minibuffer-default-add-function to provide a "next history" item with the commit at point as a way to check out a detached head) is not. I think any tweaks to make this work are likely to come in the form of changing magit--minibuf-default-add-commit and how it is wired up.

[paulapatience]

In my opinion, playing around with the default isn't really on the table.

I agree with you. I hadn't actually tested my solution with the default completion. In Doom Emacs, with vertico, the default is still visible, and highlighted, in the list of candidates, so I can just press RET to get it. Or with one M-n I can get the commit hash. But with the default completion, where no candidates list appears below the minibuffer, of course the default should be prioritized.

I will test both your solutions on Doom Emacs and get back to you. Worst case, if the fix that gets incorporated into Magit still requires two M-n presses to get to the commit hash, I could submit a PR to Doom Emacs to adjust the behavior when vertico is enabled.

Edit: Both of your solutions behave as you described in Doom Emacs with vertico: first M-n to get the default, second to get the commit (unless the commit is the default). I don't think Magit should cater specifically to vertico (or any other completion package), so I'm not worried about having to press M-n twice to get to the commit. A tweak adjusting this behavior for vertico would not be out of place in Doom Emacs; we could even add it to Magit's wiki.

[kyleam]

@paulapatience

Both of your solutions behave as you described in Doom Emacs with vertico

Thank you for checking. I've gone with the first patch for now.