bug#45029: 27.1.50; Regression: Yanking into externally modified file with delete-selection-mode

bug#45029: 27.1.50; Regression: Yanking into externally modified file with delete-selection-mode

[Lars Ljung]

Hi,

I get an error when I do this, and the content of the buffer is
completely replaced by a single "y".

1. Start with "emacs -Q"
2. M-x delete-selection-mode
3. Open some text file (fundamental mode)
4. Modify the file externally
5. While a selection is active, yank some text
6. Answer yes to the question "<file> changed on disk..."

The content of the buffer is now completely replaced by a single "y".
The message "No catch for tag: exit, nil" is shown in the minibuffer.

I can't reproduce this on Emacs 26.3.

Kind regards,
Lars Ljung

bug#45029: 27.1.50; Regression: Yanking into externally modified file with delete-selection-mode

[Lars Ingebrigtsen]

Lars Ljung <lars@matholka.se> writes:

> I get an error when I do this, and the content of the buffer is
> completely replaced by a single "y".
>
> 1. Start with "emacs -Q"
> 2. M-x delete-selection-mode
> 3. Open some text file (fundamental mode)
> 4. Modify the file externally
> 5. While a selection is active, yank some text
> 6. Answer yes to the question "<file> changed on disk..."
>
> The content of the buffer is now completely replaced by a single "y".
> The message "No catch for tag: exit, nil" is shown in the minibuffer.

I can confirm that this bug is still present in Emacs 28.

The backtrace is included below, if that helps anybody.

Debugger entered--Lisp error: (no-catch exit nil)
  throw(exit nil)
  exit-minibuffer()
  read-char-from-minibuffer-insert-char()
  funcall-interactively(read-char-from-minibuffer-insert-char)
  call-interactively(read-char-from-minibuffer-insert-char nil nil)
  command-execute(read-char-from-minibuffer-insert-char)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#45029: 27.1.50; Regression: Yanking into externally modified file with delete-selection-mode

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Fri, 04 Dec 2020 11:18:29 +0100
> Cc: 45029@debbugs.gnu.org
> 
> Lars Ljung <lars@matholka.se> writes:
> 
> > I get an error when I do this, and the content of the buffer is
> > completely replaced by a single "y".
> >
> > 1. Start with "emacs -Q"
> > 2. M-x delete-selection-mode
> > 3. Open some text file (fundamental mode)
> > 4. Modify the file externally
> > 5. While a selection is active, yank some text
> > 6. Answer yes to the question "<file> changed on disk..."
> >
> > The content of the buffer is now completely replaced by a single "y".
> > The message "No catch for tag: exit, nil" is shown in the minibuffer.
> 
> I can confirm that this bug is still present in Emacs 28.
> 
> The backtrace is included below, if that helps anybody.
> 
> Debugger entered--Lisp error: (no-catch exit nil)
>   throw(exit nil)
>   exit-minibuffer()
>   read-char-from-minibuffer-insert-char()
>   funcall-interactively(read-char-from-minibuffer-insert-char)
>   call-interactively(read-char-from-minibuffer-insert-char nil nil)
>   command-execute(read-char-from-minibuffer-insert-char)

I'm guessing this is some unintended consequence of replacing
read-char-choice in userlock.el with read-char-from-minibuffer.

Juri, could you please look into this regression in Emacs 27?  I'd
like to try to fix this for 27.2.

Thanks.

bug#45029: 27.1.50; Regression: Yanking into externally modified file with delete-selection-mode

[Juri Linkov]

[-- Attachment #1: Type: text/plain, Size: 1544 bytes --]

> I'm guessing this is some unintended consequence of replacing
> read-char-choice in userlock.el with read-char-from-minibuffer.
>
> Juri, could you please look into this regression in Emacs 27?  I'd
> like to try to fix this for 27.2.

Here is a brief trace that explains the cause of the problem:

1. C-y (yank) calls delete-selection-pre-hook from pre-command-hook
2. delete-selection-helper calls delete-active-region
3. this activates ask-user-about-supersession-threat on a modified file
4. it calls read-char-from-minibuffer ('this-command' is still 'yank')
5. read-char-from-minibuffer waits when user types 'y'
   (bound to the command 'read-char-from-minibuffer-insert-char')
6. after typing 'y', read-char-from-minibuffer returns 'y', but
   now 'this-command' is 'read-char-from-minibuffer-insert-char'
7. when delete-selection-pre-hook finishes, due to 'this-command'
   'read-char-from-minibuffer-insert-char' is called again,
   and this time operates on the buffer as if it's the minibuffer

It seems there is a fundamental problem when read-from-minibuffer
is used from pre-command-hook it modifies the value of this-command.
But I wonder why this problem doesn't occur in other places.
Maybe other places doesn't try to operate on the buffer as on
the minibuffer?

Anyway, here is a patch with two fixes:
1. Guards read-char-from-minibuffer-insert-char against inadvertent
   operating on the non-minibuffer buffer;
2. Prevents read-char-from-minibuffer from changing the value of
   'this-command' by read-from-minibuffer:


[-- Attachment #2: protect-this-command.patch --]
[-- Type: text/x-diff, Size: 3261 bytes --]

diff --git a/lisp/subr.el b/lisp/subr.el
index 4b75268c04..7f1450d4a2 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -2745,20 +2746,22 @@ read-char-from-minibuffer-insert-char
   "Insert the character you type in the minibuffer and exit.
 Discard all previous input before inserting and exiting the minibuffer."
   (interactive)
-  (delete-minibuffer-contents)
-  (insert last-command-event)
-  (exit-minibuffer))
+  (when (minibufferp)
+    (delete-minibuffer-contents)
+    (insert last-command-event)
+    (exit-minibuffer)))
 
 (defun read-char-from-minibuffer-insert-other ()
   "Handle inserting of a character other than allowed.
 Display an error on trying to insert a disallowed character.
 Also discard all previous input in the minibuffer."
   (interactive)
-  (delete-minibuffer-contents)
-  (ding)
-  (discard-input)
-  (minibuffer-message "Wrong answer")
-  (sit-for 2))
+  (when (minibufferp)
+    (delete-minibuffer-contents)
+    (ding)
+    (discard-input)
+    (minibuffer-message "Wrong answer")
+    (sit-for 2)))
 
 (defvar empty-history)
 
@@ -2802,6 +2805,8 @@ read-char-from-minibuffer
                                  map read-char-from-minibuffer-map-hash)
                         map))
                 read-char-from-minibuffer-map))
+         ;; Protect this-command when called from pre-command-hook (bug#45029)
+         (this-command this-command)
          (result
           (read-from-minibuffer prompt nil map nil
                                 (or history 'empty-history)))
@@ -2856,28 +2861,31 @@ y-or-n-p-insert-y
   "Insert the answer \"y\" and exit the minibuffer of `y-or-n-p'.
 Discard all previous input before inserting and exiting the minibuffer."
   (interactive)
-  (delete-minibuffer-contents)
-  (insert "y")
-  (exit-minibuffer))
+  (when (minibufferp)
+    (delete-minibuffer-contents)
+    (insert "y")
+    (exit-minibuffer)))
 
 (defun y-or-n-p-insert-n ()
   "Insert the answer \"n\" and exit the minibuffer of `y-or-n-p'.
 Discard all previous input before inserting and exiting the minibuffer."
   (interactive)
-  (delete-minibuffer-contents)
-  (insert "n")
-  (exit-minibuffer))
+  (when (minibufferp)
+    (delete-minibuffer-contents)
+    (insert "n")
+    (exit-minibuffer)))
 
 (defun y-or-n-p-insert-other ()
   "Handle inserting of other answers in the minibuffer of `y-or-n-p'.
 Display an error on trying to insert a disallowed character.
 Also discard all previous input in the minibuffer."
   (interactive)
-  (delete-minibuffer-contents)
-  (ding)
-  (discard-input)
-  (minibuffer-message "Please answer y or n")
-  (sit-for 2))
+  (when (minibufferp)
+    (delete-minibuffer-contents)
+    (ding)
+    (discard-input)
+    (minibuffer-message "Please answer y or n")
+    (sit-for 2)))
 
 (defvar empty-history)
 
@@ -2955,6 +2963,8 @@ y-or-n-p
                              (let ((help-form msg)) ; lexically bound msg
                                (help-form-show)))))
                        map))
+             ;; Protect this-command when called from pre-command-hook (bug#45029)
+             (this-command this-command)
              (str (read-from-minibuffer
                    prompt nil keymap nil
                    (or y-or-n-p-history-variable 'empty-history))))

bug#45029: 27.1.50; Regression: Yanking into externally modified file with delete-selection-mode

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: Lars Ingebrigtsen <larsi@gnus.org>,  lars@matholka.se,
>   45029@debbugs.gnu.org
> Date: Sat, 05 Dec 2020 21:42:10 +0200
> 
> Anyway, here is a patch with two fixes:
> 1. Guards read-char-from-minibuffer-insert-char against inadvertent
>    operating on the non-minibuffer buffer;
> 2. Prevents read-char-from-minibuffer from changing the value of
>    'this-command' by read-from-minibuffer:

Thanks!

bug#45029: 27.1.50; Regression: Yanking into externally modified file with delete-selection-mode

[Juri Linkov]

tags 45029 fixed
close 45029 27.2
quit

>> I'd like to try to fix this for 27.2.

So I pushed the patch to master, and backported it to the emacs-27 branch
(separate commits were needed to avoid merge conflicts).

bug#45029: 27.1.50; Regression: Yanking into externally modified file with delete-selection-mode

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: Lars Ingebrigtsen <larsi@gnus.org>,  lars@matholka.se,
>   45029@debbugs.gnu.org
> Date: Sun, 06 Dec 2020 23:12:36 +0200
> 
> >> I'd like to try to fix this for 27.2.
> 
> So I pushed the patch to master, and backported it to the emacs-27 branch
> (separate commits were needed to avoid merge conflicts).

Thanks.

bug#45029: 27.1.50; Regression: Yanking into externally modified file with delete-selection-mode

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Stefan Monnier [2023-09-13 13:36:54] wrote:

> Here is a brief trace that explains the cause of the problem:
>
> 1. C-y (yank) calls delete-selection-pre-hook from pre-command-hook
> 2. delete-selection-helper calls delete-active-region
> 3. this activates ask-user-about-supersession-threat on a modified file
> 4. it calls read-char-from-minibuffer ('this-command' is still 'yank')
> 5. read-char-from-minibuffer waits when user types 'y'
>    (bound to the command 'read-char-from-minibuffer-insert-char')
> 6. after typing 'y', read-char-from-minibuffer returns 'y', but
>    now 'this-command' is 'read-char-from-minibuffer-insert-char'
> 7. when delete-selection-pre-hook finishes, due to 'this-command'
>    'read-char-from-minibuffer-insert-char' is called again,
>    and this time operates on the buffer as if it's the minibuffer

Yuck!
And thanks for the investigation.

> Anyway, here is a patch with two fixes:
> 1. Guards read-char-from-minibuffer-insert-char against inadvertent
>    operating on the non-minibuffer buffer;

I suspect this will/would just hide a problem (such as the current one)
under the rug.

> 2. Prevents read-char-from-minibuffer from changing the value of
>    'this-command' by read-from-minibuffer:

Right, but I think that this needs to apply to all recursive edits
rather than only `read-char-from-minibuffer`.


         Stefan

bug#45029: 27.1.50; Regression: Yanking into externally modified file with delete-selection-mode

[Juri Linkov]

>> Here is a brief trace that explains the cause of the problem:
>>
>> 1. C-y (yank) calls delete-selection-pre-hook from pre-command-hook
>> 2. delete-selection-helper calls delete-active-region
>> 3. this activates ask-user-about-supersession-threat on a modified file
>> 4. it calls read-char-from-minibuffer ('this-command' is still 'yank')
>> 5. read-char-from-minibuffer waits when user types 'y'
>>    (bound to the command 'read-char-from-minibuffer-insert-char')
>> 6. after typing 'y', read-char-from-minibuffer returns 'y', but
>>    now 'this-command' is 'read-char-from-minibuffer-insert-char'
>> 7. when delete-selection-pre-hook finishes, due to 'this-command'
>>    'read-char-from-minibuffer-insert-char' is called again,
>>    and this time operates on the buffer as if it's the minibuffer
>
> Yuck!
> And thanks for the investigation.
>
>> Anyway, here is a patch with two fixes:
>> 1. Guards read-char-from-minibuffer-insert-char against inadvertent
>>    operating on the non-minibuffer buffer;
>
> I suspect this will/would just hide a problem (such as the current one)
> under the rug.

I admit this is a workaround, but I have no idea what would be a proper fix.

>> 2. Prevents read-char-from-minibuffer from changing the value of
>>    'this-command' by read-from-minibuffer:
>
> Right, but I think that this needs to apply to all recursive edits
> rather than only `read-char-from-minibuffer`.

I tried to test recursive commands at the read-char-from-minibuffer's
prompt, but can't reproduce the same problem.

bug#45029: 27.1.50; Regression: Yanking into externally modified file with delete-selection-mode

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>>> Anyway, here is a patch with two fixes:
>>> 1. Guards read-char-from-minibuffer-insert-char against inadvertent
>>>    operating on the non-minibuffer buffer;
>> I suspect this will/would just hide a problem (such as the current one)
>> under the rug.
> I admit this is a workaround, but I have no idea what would be a proper fix.

The let-binding of `this-command` fixes the problem, doesn't it?

>>> 2. Prevents read-char-from-minibuffer from changing the value of
>>>    'this-command' by read-from-minibuffer:
>> Right, but I think that this needs to apply to all recursive edits
>> rather than only `read-char-from-minibuffer`.
> I tried to test recursive commands at the read-char-from-minibuffer's
> prompt, but can't reproduce the same problem.

No, I mean other uses of a recursive edit from `pre-command-hook` would
cause the same problem, so the let-binding should be placed deeper than
in `read-char-from-minibuffer`, e.g. inside `recursive_edit_1`.


        Stefan

bug#45029: 27.1.50; Regression: Yanking into externally modified file with delete-selection-mode

[Juri Linkov]

>>>> Anyway, here is a patch with two fixes:
>>>> 1. Guards read-char-from-minibuffer-insert-char against inadvertent
>>>>    operating on the non-minibuffer buffer;
>>> I suspect this will/would just hide a problem (such as the current one)
>>> under the rug.
>> I admit this is a workaround, but I have no idea what would be a proper fix.
>
> The let-binding of `this-command` fixes the problem, doesn't it?

But might other uses of a recursive edit break it again?
I guess `when (minibufferp)` can't be removed
from `read-char-from-minibuffer-insert-other`
until all cases below are fixed.

>>>> 2. Prevents read-char-from-minibuffer from changing the value of
>>>>    'this-command' by read-from-minibuffer:
>>> Right, but I think that this needs to apply to all recursive edits
>>> rather than only `read-char-from-minibuffer`.
>> I tried to test recursive commands at the read-char-from-minibuffer's
>> prompt, but can't reproduce the same problem.
>
> No, I mean other uses of a recursive edit from `pre-command-hook` would
> cause the same problem, so the let-binding should be placed deeper than
> in `read-char-from-minibuffer`, e.g. inside `recursive_edit_1`.

I see, so this problem is more general and not specific to
`read-char-from-minibuffer`?  It would be nice to find
a test case for other commands to confirm that a change in
`recursive_edit_1` fixes them.

bug#45029: 27.1.50; Regression: Yanking into externally modified file with delete-selection-mode

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> The let-binding of `this-command` fixes the problem, doesn't it?
> But might other uses of a recursive edit break it again?

What do you mean by `it`?

There's only one recursive edit associated with
`read-char-from-minibuffer-insert-char`, so no definitely not that
(unless the user binds (or somehow uses) that command in keymaps used in
other recursive edits, of course).

And AFAICT, the bug see is that `delete-selection-mode` ends up running
a command (originally run inside a recursive edit) a second time.
IOW it's not "recursive edit broke `read-char-from-minibuffer-insert-char`"
but "recursive edit broke `delete-selection-mode`".
This can/will cause problems with loads of commands, not just
`read-char-from-minibuffer-insert-char`.

So yes, other uses of recursive edits can break `delete-selection-mode`
as well, but no amount of changing
`read-char-from-minibuffer-insert-char` will hide the problem because
those other uses will use other commands.

OTOH, the let-binding of `this-command` should fix the problem once and
for all uses of recursive edit *if* it's placed deep enough that it
affects all recursive edits.

> I guess `when (minibufferp)` can't be removed from
> `read-char-from-minibuffer-insert-other` until all cases below
> are fixed.

I don't see the "cases below" you're referring to.

> I see, so this problem is more general and not specific to
> `read-char-from-minibuffer`?

Exactly.  Any recursive edit happening from the `pre-command-hook`.

> It would be nice to find a test case for other commands to confirm
> that a change in `recursive_edit_1` fixes them.

Even without trying to reproduce the specific problem that caused this
bug report, think of this scenario:

   - User causes COMMAND to be run.
   - `this-command` is thus set to COMMAND.
   - We run the `pre-command-hook`.
   - One of the functions on `pre-command-hook` use a recursive edit.
   - `this-command` is now set to whichever command was run last in the
     recursive edit, which is just plain wrong.

I remember seeing places in the code where we try and circumvent this
problem, but this bug report makes it clear what is the real source of
the problem.


        Stefan

bug#45029: 27.1.50; Regression: Yanking into externally modified file with delete-selection-mode

[Juri Linkov]

>>> The let-binding of `this-command` fixes the problem, doesn't it?
>> But might other uses of a recursive edit break it again?
>
> What do you mean by `it`?

Thanks for explanations.  I thought it is specific to
`delete-selection-mode` and `read-char-from-minibuffer`.
But since a combination of `pre-command-hook` and a recursive command
is a general problem, and OTOH `read-char-from-minibuffer` is already
safeguarded by the let-binding of `this-command`, so probably
`when (minibufferp)` could be removed from
`read-char-from-minibuffer-insert-other`.

> OTOH, the let-binding of `this-command` should fix the problem once and
> for all uses of recursive edit *if* it's placed deep enough that it
> affects all recursive edits.

Frankly speaking, I'm afraid of making such change at great depths ;-)