Using consult-completing-read-multiple breaks magit-completing-read-multiple*

[iyefrat]

This issue was originally reported in minad/consult#358, but was found to be caused magit-completing-read-multiple* breaking the CRM abstraction by @minad, please read his comment here for more details.

Steps to reproduce:

• Open emacs -Q, install the newest versions of Consult and Magit
• (advice-add #'completing-read-multiple :override #'consult-completing-read-multiple)
• Go to some git repo and open the magit-status
• Do l o to open the log of another branch
• type in the name of any branch, press RET twice to select
• get the following error:

Debugger entered--Lisp error: (wrong-type-argument stringp nil)
  split-string(nil "[, ]" t)
  magit-log-read-revs()
  byte-code("\300 \301 B\207" [magit-log-read-revs magit-log-arguments] 2)
  #<subr call-interactively>(magit-log-other nil nil)
  apply(#<subr call-interactively> (magit-log-other nil nil))
  explain-pause--wrap-call-interactively(#<subr call-interactively> magit-log-other nil nil)
  apply(explain-pause--wrap-call-interactively #<subr call-interactively> (magit-log-other nil nil))
  call-interactively(magit-log-other nil nil)
  command-execute(magit-log-other)

[minad]

The issue should be pretty easy to resolve, consult-crm does not call split-string breaking the input retrieval hack of magit. Instead the return value should be used and concatenated.

[minad]

Maybe @jakanakaevangeli can help with this?

[jakanakaevangeli]

Magit uses crm for reading git revision ranges such as "master@{1}..master develop", where "master", "master" and "develop" are completion candidates, "@{1}.." and " " are separators and order is important. This is fundamentally incompatible with consult's crm function which works somewhat like a check-box to select a list of candidates. I brainstormed two ideas in case they are useful: A simple solution could be to expose a consult-inhibit-consult-crm variable which magit-crm* could bind to t. Though we would probably have to convert consult-crm from :override to :around advice for this to work. A more complex but IMO more complete solution could be for magit to expose a variable such as magit-crm*-avoid-crm. Users of consult-crm would set it to t. In this case, instead of calling completing-read-multiple, magit-crm* would use completing-read with a dynamic completion table similar to #'crm--collection-fn.

[minad]

Okay, so the point here for Magit is that the separators are preserved additionally to the candidates and their order? Order of selection itself is preserved by consult-completing-read-multiple, despite it acting like a checkbox.

I think it is kind of obvious to offer such a crm implementation as Consult does. There is also crm-custom, which calls cr in a loop. On the other hand the magit use of crm seems to be a pretty severe abuse. The correct way to implement the desired functionality in magit with compliant completion UIs like default completion, Icomplete, Vertico etc is to implement its own programmable completion table, which performs the desired completion of the revisions with the separators in between. If there is interest in adding such a feature here, I can try to contribute a patch. It would be even interesting to see how such a completion table would look. It is not trivial though, messing with all the completion boundaries etc.

A simple solution could be to expose a consult-inhibit-consult-crm
variable which magit-crm* could bind to t.

I think it would be easier to locally remove the consult-completing-read-multiple override in magit-crm* and fallback to the default implementation.

A more complex but IMO more complete solution could be for magit to
expose a variable such as magit-crm*-avoid-crm.

Agree.

The solutions in my order of preference:

1. Magit uses its own proper programmable completion table which is used with compliant UIs
2. Add a more general magit crm handler alist, which could be configured instead of the hardcoded list of hacks
3. Unbind the consult advice in magit-crm*, adding another hack to magit-crm*
4. Add some opt out mechanism to consult (consult-crm-opt-out alist), configured in consult (not setting some inhibit variable as you suggest on the side of the crm user)
5. Defer the configuration of consult-crm/magit-crm* to the user. The user can setup proper advices ensuring that the two functions don't interfere with each other

Regarding 4., the consult opt out mechanism: Such a mechanism may indeed be necessary since consult-crm works only correctly if the completion table passed to consult-crm is static. You cannot pass the file completion table to consult-crm for example. However one can also not pass the file completion table to the default crm implementation (throws an error), so it seems to me that dynamic tables are simply not used in conjunction with crm. The point is that dynamic completion tables passed to crm corresponds to recursive completion boundaries, I think this is simply not supported by the completion machinery. So given this, I believe that consult crm is mostly sufficient. Of course this does not include API abuses as in the magit case.

[minad]

Hmm, I think 1. is overkill. I would probably go with 3. or 5. for simplicity for now. Note that consult-crm is pretty new and experimental.

@iyefrat You could use something like this for now or decide to not use consult-crm if there are more scenarios where something breaks. Unfortunately not having something like consult-crm breaks use cases as the one in bibtex-actions.

(defun opt-out-of-consult-crm (&rest args)
  (if (advice-member-p #'consult-completing-read-multiple #'completing-read-multiple)
      (unwind-protect
          (progn
            (advice-remove #'completing-read-multiple #'consult-completing-read-multiple)
            (apply args))
        (advice-add #'completing-read-multiple :override #'consult-completing-read-multiple))
    (apply args)))

(advice-add #'magit-completing-read-multiple* :around #'opt-out-of-consult-crm)

[tarsius]

I do agree that Magit's "the separators are relevant data too" trick is abuse but the alternatives are not great either or as you say "overkill". But I am perfectly willing to cooperate on this. Right now I am a little busy though.

The "unbind the advice" approach sounds reasonable to me. I'm gonna add that now.

[minad]

The "unbind the advice" approach sounds reasonable to me. I'm gonna add that now.

Okay, I think the unbind advice is the lowest effort for now.

I do agree that Magit's "the separators are relevant data too" trick is abuse but the alternatives are not great either or as you say "overkill".

I think in the longer term, a proper completion table may be the way to go. And the good thing is - such a completion table already exists, it is called crm--collection-fn. For example this works as expected with Vertico:

(let ((crm-completion-table '("first" "second" "third")))
  (completing-read "CRM: " #'crm--collection-fn))

This way you reuse the underlying crm infrastructure and you still don't abuse the API. I believe at some point you also had something like this, basically a replica of the default completing-read-multiple from crm.el?

EDIT1: Actually it wasn't long ago c10e10c
EDIT2: See also cb4d591

[tarsius]

EDIT1: Actually it wasn't long ago c10e10c

I was just gonna post that. 😀

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.

So yeah, this was actually done so that we would automatically benefit from changes made by completion frameworks. Of course there is also the risk of things automatically breaking as a result as we have just seen, but then we just have to find a fix.

[tarsius]

By the way, consults crm interface is really nice.

[tarsius]

Okay then... football. 🍿 ⚽

[minad]

By the way, consults crm interface is really nice.

Thanks! Have fun!

[iyefrat]

Thanks for the fix! Just checking, did you leave this issue open on purpose?

[tarsius]

I am usually quick to close issues and then once I don't ...

[minad]

@tarsius consult-completing-read-multiple has been removed again from Consult due to technical issues. So you can also remove the workaround from Magit again in a while, after I released the next Consult version. See minad/consult#567.

[tarsius]

Thanks for the heads up!

[tarsius]

Done.