bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Gregory Heytings <gregory@heytings.org>]

>> It would be possible to use whitelisting instead by renaming the 
>> current read-from-minibuffer to internal-read-from-minibuffer, which 
>> would be wrapped in a macro read-from-minibuffer.
>
> Indeed.  I think we need more data in order to figure out which of the 
> two options breaks less/more code out there.
>

Here is some (hopefully useful) data:

There are 213 calls to read-from-minibuffer in Emacs core.  AFAICS, the 
only call that uses minibuffer-completion-table is the one inside 
completing-read-default.  There's another one inside ido-read-internal, 
but it doesn't seem to use minibuffer-completion-table, AFAIU it replaces 
completing-read with its own processing in ido-exhibit.

There are 76 calls to read-from-minibuffer on ELPA.  AFAICS, the only call 
that uses minibuffer-completion-table is the one iside ivy-read.

There are 903 calls to read-from-minibuffer on MELPA.  AFAICS, only a 
handful of them (yatex, gams-mode, python-django, magit) use 
minibuffer-completion-table.  The case of Magit is interesting: it solves 
the current problem by defining two functions, 
magit-completing-read-multiple that let-binds minibuffer-completion-table 
to the appropriate value, and magit-read-string that let-binds 
minibuffer-completion-table to nil.

>
> I was working under the assumption that the only calls to 
> `read-from-minibuffer` which need the minibuffer to have a 
> `minibuffer-completion-table` are those coming from 
> `completing-read-default` (in which case the whitelisting is the better 
> approach since it requires a change only in `completing-read-default`), 
> but the fact that there's a `completing-read-function` is a strong hint 
> that this assumption is probably wrong.
>

I believe your assumption is mostly correct.  There are apparently only 
very few occurrences of read-from-minibuffer that use 
minibuffer-completion-table, and my understanding is that those who set 
completing-read-function to another value do not use 
minibuffer-completion-table anymore.

>> The change would be transparent, except for those places (e.g. 
>> completing-read-default) where what we actually want is to use 
>> internal-read-from-minibuffer.  But this change would be slightly more 
>> invasive than what follows.
>
> Actually, probably not very much, and it would be a lot cleaner.
>

I agree.

>>> Just like your approach, I think this is only a temporary fix until we 
>>> can solve the problem for real by making `minibuffer-completion-table` 
>>> buffer-local
>>
>> I'm not sure I fully understand why this is necessary, but is 
>> "Fmake_variable_buffer_local (Qminibuffer_completion_table);" just 
>> after "if ... specbind (Qminibuffer_completion_table, Qnil);" not 
>> enough for this?
>
> No, I meant that instead of using let-binding to set the var, we'd use 
> `setq-local`. This requires the code to run from within the minibuffer, 
> contrary to the current situation where the let-binding takes place 
> outside of the minibuffer.
>

Yes, I understood this, but the effect of let-binding the var and make it 
buffer-local after the minibuffer has been created but before the 
minibuffer-setup-hook is executed is the same.  Or am I missing something? 
If not, this would solve the problem without breaking anything (as the 
global value of minibuffer-completion-table would not change).