bug#49700: 27.2; [PATCH] Refactor minibuffer aborting

[Alan Mackenzie <acm@muc.de>]

Hello, Miha.

On Fri, Jul 23, 2021 at 01:05:41 +0200, miha@kamnitnik.top wrote:
> The attached patch removes special handling of the 'exit tag from
> internal_catch.  This special handling was introduced by Alan in commit
> Sun Jan 10 20:32:40 2021 +0000
> (c7c154bb5756e0ae71d342c5d8aabf725877f186), hence me CC-ing him.

Thanks, that's appreciated.

I'm not sure I'm in favour of the change as a whole, since the proposed
code contains complexities (as does the code it would replace).  I find
the use of the closures difficult to understand.  But then again, I wrote
the old code, so I'm not in a position to judge whether the old or the
new is "better".

> It also exposes Vminibuffer_list to lisp through the new function
> Fminibuffer_alist.

Like Eli, I'm against this.  Indeed, when I was modifying the minibuffer
code, I took great care to avoid Vminibuffer_list becoming visible to
Lisp.  As a result, some of the current code is less elegant than it
might have been.  The idea of some Lisp looping through all existing
minibuffers doing something destructive didn't help me sleep well.

As a general point, I'm a bit worried you might not be distinguishing
between (minibuffer-depth) and (recursive-depth).  They are only the same
most of the time.  When (recursive-edit) gets called outside of the
minibuffer code, then these two values are different.  For example, in 
abort-minibuffers, you've got

> +      (when (yes-or-no-p
> +             (format "Abort %s minibuffer levels? "
> +                     (- (recursion-depth) minibuffer-level -1)))

..  minibuffer-level is confusingly a result of (recursion-depth), not
(minibuffer-depth), so the code isn't prompting with the number of
minibuffer levels to be aborted, but the number of recursive edits.

As a small point, the use of cl-decf:

> +        (cl-decf minibuffer-level)))

might be unwise.  Have you checked that it works in a bootstrap build?
My fear is that in a bootstrap, minibuffer.el might be compiled before
the CL files, and then cl-decf would be wrongly compiled as a function
call rather than a macro expansion.  But I haven't checked it myself.

I've also had a look a part of your patch from Tuesday (2021-07-20), and
am unhappy about some aspects of the change to the documentation on the
Elisp manual page Recursive Editing.  For example, the text no longer
says what happens on throwing a random value to 'exit (but it used to).
Also this text is generally a bit unclear; what does "a function value"
mean?  I would normally understand "the value returned by a function",
but here it just means the function.  But I think it would be better for
me to raise these issues in a different thread.

I haven't yet spent enough time looking at your patch.  Perhaps I'll
manage it in the next week.

-- 
Alan Mackenzie (Nuremberg, Germany).