bug#23785: Emacs 25: "Undo" overdoes things.

bug#23785: Emacs 25: "Undo" overdoes things.

[Alan Mackenzie]

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

Hello, Emacs.

Summary: `undo' is broken in Emacs 25.

In GNU Emacs 25.0.94.2 (x86_64-unknown-linux-gnu, GTK+ Version 3.18.7)
 of 2016-06-07 built on acm
Repository revision: 9d5ccebeba0506f7280662630f0ee85a52c8a327
Configured using:
 'configure --with-tiff=no --with-gif=no --with-gpm'

1. emacs -Q
2. C-x C-f decls-6.cc    ; file is attached.
3. Move point to BOL 17.
4. C-o, and insert the line "Type var = init;".  Call this line 16½.
5. M-x revert-buffer.
6. Move point to "[" on L16, and use C-M-k to delete "[3 * peq]".
7. C-_.  This restores "[3 * peq]" (correctly) but also reinserts line
   16½ (which is a bug).

[For reference, the pertinent part of the file looks like this.  In the
actual file "Type" is at column zero:

14.   Type var = init, x = Type();
15.   Type (*var) = init;
16.   Type var[3 * peq] = init;
16½.  Type var = init;         <======== inserted line.
17.   Type (var) = init;
18.   Type int = "int";               // int

].

-- 
Alan Mackenzie (Nuremberg, Germany).


[-- Attachment #2: decls-6.cc --]
[-- Type: text/x-c, Size: 1200 bytes --]

__INLINE__ FOO Type var, x;
__INLINE__ FOO Type *var;
__INLINE__ FOO Type var[3 * peq];

// This is an init paren that currently incorrectly causes the
// variable to be recognized as a function.
__INLINE__ FOO Type var (peq);

__INLINE__ FOO Type var = init, x = Type();
__INLINE__ FOO Type (*var) = init;
__INLINE__ FOO Type var[3 * peq] = init;
__INLINE__ FOO Type var int = "int"; // int

Type var = init, x = Type();
Type (*var) = init;
Type var[3 * peq] = init;
Type (var) = init;
Type int = "int";		// int

const Type var;
const Type (*var);
const Type var[3 * peq];
const Type (var);

Type (*foo) (Type *,
	     Type (*)[x],
	     Type (*var)[x],
	     // An incorrect one that gets "var" recorded as a type.
	     Type (var*)[x],
	     Type &);

Type2 var;
Type var;
Type (*var);			// Currently treated as function call.
Type (*var)[3];
Type var[3 * peq];
Type (var);			// Currently treated as function call.
Type (var)();
::Type var;

unsigned foo bar;
long long x;
long double y;
int x y;			// Invalid
int int y;			// Invalid

// This should be last to check a certain case.
#define low_assign_multiset_index(TO, NODE) do {			\
    struct svalue *_ms_index_to2_ = (TO);				\
  } while (0)

bug#23785: Emacs 25: "Undo" overdoes things.

[Eli Zaretskii]

> Date: Fri, 17 Jun 2016 15:02:45 +0000
> From: Alan Mackenzie <acm@muc.de>
> 
> Summary: `undo' is broken in Emacs 25.
> 
> In GNU Emacs 25.0.94.2 (x86_64-unknown-linux-gnu, GTK+ Version 3.18.7)
>  of 2016-06-07 built on acm
> Repository revision: 9d5ccebeba0506f7280662630f0ee85a52c8a327
> Configured using:
>  'configure --with-tiff=no --with-gif=no --with-gpm'
> 
> 1. emacs -Q
> 2. C-x C-f decls-6.cc    ; file is attached.
> 3. Move point to BOL 17.
> 4. C-o, and insert the line "Type var = init;".  Call this line 16½.
> 5. M-x revert-buffer.
> 6. Move point to "[" on L16, and use C-M-k to delete "[3 * peq]".
> 7. C-_.  This restores "[3 * peq]" (correctly) but also reinserts line
>    16½ (which is a bug).

Why does this minor issue deserve to declare 'undo' "broken"?  Looks
like an exaggeration to me.

bug#23785: Emacs 25: "Undo" overdoes things.

[Alan Mackenzie]

Hello, Eli.

On Fri, Jun 17, 2016 at 08:15:18PM +0300, Eli Zaretskii wrote:
> > Date: Fri, 17 Jun 2016 15:02:45 +0000
> > From: Alan Mackenzie <acm@muc.de>

> > Summary: `undo' is broken in Emacs 25.

> > In GNU Emacs 25.0.94.2 (x86_64-unknown-linux-gnu, GTK+ Version 3.18.7)
> >  of 2016-06-07 built on acm
> > Repository revision: 9d5ccebeba0506f7280662630f0ee85a52c8a327
> > Configured using:
> >  'configure --with-tiff=no --with-gif=no --with-gpm'

> > 1. emacs -Q
> > 2. C-x C-f decls-6.cc    ; file is attached.
> > 3. Move point to BOL 17.
> > 4. C-o, and insert the line "Type var = init;".  Call this line 16½.
> > 5. M-x revert-buffer.
> > 6. Move point to "[" on L16, and use C-M-k to delete "[3 * peq]".
> > 7. C-_.  This restores "[3 * peq]" (correctly) but also reinserts line
> >    16½ (which is a bug).

> Why does this minor issue deserve to declare 'undo' "broken"?  Looks
> like an exaggeration to me.

I don't think it's all that minor an issue.  `undo' can no longer be
depended upon to restore a buffer to its unchanged state.

I've suffered several similar annoyances with `undo' in the emacs-25
branch.

Each buffer changing command is meant to have its own undo boundary
(with the exception of self-insert-command and the single character
deleting command).

And having to undo/redo command sequences by hand is _very_ irritating
when testing.

I think there's a case to be made for fixing this bug for Emacs 25.1.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#23785: Emacs 25: "Undo" overdoes things.

[Eli Zaretskii]

> Date: Fri, 17 Jun 2016 17:45:35 +0000
> Cc: 23785@debbugs.gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> > > 1. emacs -Q
> > > 2. C-x C-f decls-6.cc    ; file is attached.
> > > 3. Move point to BOL 17.
> > > 4. C-o, and insert the line "Type var = init;".  Call this line 16½.
> > > 5. M-x revert-buffer.
> > > 6. Move point to "[" on L16, and use C-M-k to delete "[3 * peq]".
> > > 7. C-_.  This restores "[3 * peq]" (correctly) but also reinserts line
> > >    16½ (which is a bug).
> 
> > Why does this minor issue deserve to declare 'undo' "broken"?  Looks
> > like an exaggeration to me.
> 
> I don't think it's all that minor an issue.

You are being unreasonably subjective on this one.

> `undo' can no longer be depended upon to restore a buffer to its
> unchanged state.

Undo is about undoing changes, and it is documented to lump several
changes together in some situations.  So your expectations are
unreasonably unrealistic to begin with.

> Each buffer changing command is meant to have its own undo boundary
> (with the exception of self-insert-command and the single character
> deleting command).

I don't think this is true, and in any case see no big difference
between several commands each deleting one character and a single
command deleting several characters at once.

> I think there's a case to be made for fixing this bug for Emacs 25.1.

It depends on what the fix will look like.  If it's simple and safe
enough, I'll agree.

bug#23785: Emacs 25: "Undo" overdoes things.

[Phillip Lord]

Alan Mackenzie <acm@muc.de> writes:

> Hello, Eli.
>
> On Fri, Jun 17, 2016 at 08:15:18PM +0300, Eli Zaretskii wrote:
>> > Date: Fri, 17 Jun 2016 15:02:45 +0000
>> > From: Alan Mackenzie <acm@muc.de>
>
>> > Summary: `undo' is broken in Emacs 25.
>
>> > In GNU Emacs 25.0.94.2 (x86_64-unknown-linux-gnu, GTK+ Version 3.18.7)
>> >  of 2016-06-07 built on acm
>> > Repository revision: 9d5ccebeba0506f7280662630f0ee85a52c8a327
>> > Configured using:
>> >  'configure --with-tiff=no --with-gif=no --with-gpm'
>
>> > 1. emacs -Q
>> > 2. C-x C-f decls-6.cc    ; file is attached.
>> > 3. Move point to BOL 17.
>> > 4. C-o, and insert the line "Type var = init;".  Call this line 16½.
>> > 5. M-x revert-buffer.
>> > 6. Move point to "[" on L16, and use C-M-k to delete "[3 * peq]".
>> > 7. C-_.  This restores "[3 * peq]" (correctly) but also reinserts line
>> >    16½ (which is a bug).
>
>> Why does this minor issue deserve to declare 'undo' "broken"?  Looks
>> like an exaggeration to me.
>
> I don't think it's all that minor an issue.  `undo' can no longer be
> depended upon to restore a buffer to its unchanged state.
>
> I've suffered several similar annoyances with `undo' in the emacs-25
> branch.

If you report them, then I will look at them, and I would appreciate if
you do report them. I changed undo in a way that *was* supposed to
change its semantics, and this may have had negative side effects. Or my
changes may have caused unexpected changes in semantics that I did not
intend.


> Each buffer changing command is meant to have its own undo boundary
> (with the exception of self-insert-command and the single character
> deleting command).


The problem in this case seems to be specific to revert-buffer. A much
simpler test case is as follows

1) Open a file with a single line in it
2) Add a new line at the start
3) M-x revert-buffer

Look at buffer-undo-list



*** Before undo changes

(nil
 ("\n" . -1)
 (#<marker at 1 in simple-example.txt> . -1)
 (#<marker at 1 in simple-example.txt> . -1)
 (#<marker in no buffer> . -1)
 nil
 (1 . 2)
 (t 22372 26717 127527 392000))

*** After undo changes

(("\n" . -1)
 (#<marker at 1 in simple-example.txt> . -1)
 (#<marker at 1 in simple-example.txt> . -1)
 nil
 (1 . 2)
 (t 22372 26717 127527 392000))


So, the issue seems to be specific to M-x revert-buffer. After my
changes, the list no longer has a undo-boundary as its first element.
Got to be honest, I am surprised that M-x revert-buffer maintains the
undo-list; I'd have expected it to blitz the whole list, but it doesn't.

No idea why, although I suspect that it's the same issue Stefan found
with viper -- undo-boundary no longer gets called after all commands
only those that change the buffer.

I will investigate.

Phil

bug#23785: Emacs 25: "Undo" overdoes things.

[Phillip Lord]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> I think there's a case to be made for fixing this bug for Emacs 25.1.
>
> It depends on what the fix will look like.  If it's simple and safe
> enough, I'll agree.


The following patch addresses the issue in my hands. I'd welcome
confirmation.

Stefan what do you think? You suggested something like this for
Bug#22295 also. I think that the problem is the same; if I read
insert-file-contents correctly, it disables collection of undo
information. I may well be reading it wrong though.

Given that this has turned up in two places now, it may be a wider
problem, and the more general solution might be the better one. It might
also invalidate the necessity for 12e009e52, but I haven't tested that
yet.

Alternatively, revert-buffer could just force undo-boundary.

Phil


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-missing-undo-boundary-after-revert-buffer.patch --]
[-- Type: text/x-diff, Size: 1158 bytes --]

From 2ab1f314ad6fe0e68420cc510445495467d82b8f Mon Sep 17 00:00:00 2001
From: Phillip Lord <phillip.lord@russet.org.uk>
Date: Fri, 17 Jun 2016 22:34:50 +0100
Subject: [PATCH] Fix missing undo-boundary after revert-buffer

* lisp/simple.el (undo-auto--boundaries): Ensure an undo-boundary after
  every command whether it (apparently) changes the buffer or not.

Addresses Bug#23785
---
 lisp/simple.el | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lisp/simple.el b/lisp/simple.el
index b66827d..3110430 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -2875,6 +2875,10 @@ undo-auto--boundaries
   "Check recently changed buffers and add a boundary if necessary.
 REASON describes the reason that the boundary is being added; see
 `undo-last-boundary' for more information."
+  ;; (Bug #23785) All commands should ensure that there is an undo
+  ;; boundary whether they have changed the current buffer or not.
+  (when (eq cause 'command)
+    (add-to-list 'undo-auto--undoably-changed-buffers (current-buffer)))
   (dolist (b undo-auto--undoably-changed-buffers)
           (when (buffer-live-p b)
             (with-current-buffer b
-- 
2.8.4

bug#23785: Emacs 25: "Undo" overdoes things.

[Óscar Fuentes]

Alan Mackenzie <acm@muc.de> writes:

> I don't think it's all that minor an issue.  `undo' can no longer be
> depended upon to restore a buffer to its unchanged state.
>
> I've suffered several similar annoyances with `undo' in the emacs-25
> branch.

Me too, although I'm not sure about Emacs being the culprit. I use Evil
and it adds some heuristics on top of Emacs'.

> Each buffer changing command is meant to have its own undo boundary
> (with the exception of self-insert-command and the single character
> deleting command).
>
> And having to undo/redo command sequences by hand is _very_ irritating
> when testing.

Only when testing? :-)

I've observed cases where sequences of char inserts were undone one at a
time and cases where entire edit sessions across multiple areas of the
file vanished with one `undo'.

> I think there's a case to be made for fixing this bug for Emacs 25.1.

FWIW, this is the change that enabled keeping the undo log after
revert-buffer (a feature I asked for, BTW):


commit 22513e526eba97bd1014e4bacde0a8649fbe7870
Author: Stefan Monnier <monnier@iro.umontreal.ca>
Date:   Tue May 28 21:07:53 2013 -0400

    * src/fileio.c (Finsert_file_contents): Preserve undo info when reverting
    a buffer.
    
    Fixes: debbugs:8447

bug#23785: Emacs 25: "Undo" overdoes things.

[Stefan Monnier]

> Stefan what do you think? You suggested something like this for
> Bug#22295 also. I think that the problem is the same; if I read
> insert-file-contents correctly, it disables collection of undo
> information. I may well be reading it wrong though.

Looks OK to me, yes.

> Given that this has turned up in two places now, it may be a wider
> problem, and the more general solution might be the better one.  It
> might also invalidate the necessity for 12e009e52, but I haven't
> tested that yet.

It probably makes 12e009e52 unnecessary, indeed, but I'd argue that
it is wrong of viper-adjust-undo to remove the topmost undo-boundary, so
IMO we should keep 12e009e52 (if not in emacs-25 at least in master).

Similarly I think it's still a bug that revert-buffer removes
a pre-existing undo-boundary (even if that bug ends up hidden by adding
current-buffer unconditionally in undo-auto--boundaries).


        Stefan

bug#23785: Emacs 25: "Undo" overdoes things.

[Eli Zaretskii]

> From: phillip.lord@russet.org.uk (Phillip Lord)
> Cc: Alan Mackenzie <acm@muc.de>,  23785@debbugs.gnu.org
> Date: Fri, 17 Jun 2016 22:47:20 +0100
> 
> The following patch addresses the issue in my hands. I'd welcome
> confirmation.
> 
> Stefan what do you think? You suggested something like this for
> Bug#22295 also. I think that the problem is the same; if I read
> insert-file-contents correctly, it disables collection of undo
> information. I may well be reading it wrong though.
> 
> Given that this has turned up in two places now, it may be a wider
> problem, and the more general solution might be the better one. It might
> also invalidate the necessity for 12e009e52, but I haven't tested that
> yet.
> 
> Alternatively, revert-buffer could just force undo-boundary.
> 
> >From 2ab1f314ad6fe0e68420cc510445495467d82b8f Mon Sep 17 00:00:00 2001
> From: Phillip Lord <phillip.lord@russet.org.uk>
> Date: Fri, 17 Jun 2016 22:34:50 +0100
> Subject: [PATCH] Fix missing undo-boundary after revert-buffer
> 
> * lisp/simple.el (undo-auto--boundaries): Ensure an undo-boundary after
>   every command whether it (apparently) changes the buffer or not.
> 
> Addresses Bug#23785
> ---
>  lisp/simple.el | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/lisp/simple.el b/lisp/simple.el
> index b66827d..3110430 100644
> --- a/lisp/simple.el
> +++ b/lisp/simple.el
> @@ -2875,6 +2875,10 @@ undo-auto--boundaries
>    "Check recently changed buffers and add a boundary if necessary.
>  REASON describes the reason that the boundary is being added; see
>  `undo-last-boundary' for more information."
> +  ;; (Bug #23785) All commands should ensure that there is an undo
> +  ;; boundary whether they have changed the current buffer or not.
> +  (when (eq cause 'command)
> +    (add-to-list 'undo-auto--undoably-changed-buffers (current-buffer)))
>    (dolist (b undo-auto--undoably-changed-buffers)
>            (when (buffer-live-p b)
>              (with-current-buffer b

I'd indeed suggest to have on emacs-25 a change that only affects
revert-buffer, and have the above on master.  It sounds too scary to
make a change that affects all commands this late in the pretest.

Thanks.

bug#23785: Emacs 25: "Undo" overdoes things.

[Alan Mackenzie]

Hello, Phil.

On Fri, Jun 17, 2016 at 10:23:51PM +0100, Phillip Lord wrote:
> Alan Mackenzie <acm@muc.de> writes:

> > I don't think it's all that minor an issue.  `undo' can no longer be
> > depended upon to restore a buffer to its unchanged state.

> > I've suffered several similar annoyances with `undo' in the emacs-25
> > branch.

> If you report them, then I will look at them, and I would appreciate if
> you do report them.

For example, at times I've deleted a character, moved point, then deleted
another character.  Undo has undone both deletions as a single operation,
despite the point movement between them.

> I changed undo in a way that *was* supposed to change its semantics,
> and this may have had negative side effects. Or my changes may have
> caused unexpected changes in semantics that I did not intend.

The lack of configurability seems to be a problem.  There have been
times, testing times, when I've felt that each deletion of a character is
its own atomic operation.  Yet I can't configure undo to respect this
wish.

[ .... ]

> I will investigate.

> Phil

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#23785: Emacs 25: "Undo" overdoes things.

[Stefan Monnier]

> I'd indeed suggest to have on emacs-25 a change that only affects
> revert-buffer, and have the above on master.  It sounds too scary to
> make a change that affects all commands this late in the pretest.

FWIW, this change you say is scary brings the behavior much closer
to that of Emacs<25, so I think it's the safer choice.


        Stefan

bug#23785: Emacs 25: "Undo" overdoes things.

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: phillip.lord@russet.org.uk (Phillip Lord),  acm@muc.de,  23785@debbugs.gnu.org
> Date: Sat, 18 Jun 2016 14:42:07 -0400
> 
> > I'd indeed suggest to have on emacs-25 a change that only affects
> > revert-buffer, and have the above on master.  It sounds too scary to
> > make a change that affects all commands this late in the pretest.
> 
> FWIW, this change you say is scary brings the behavior much closer
> to that of Emacs<25, so I think it's the safer choice.

My concern is not with the behavior the proposed change intends to get
us, the concern is with the unintended consequences of the change.  At
this late stage, I'd like to keep the risk of unintended consequences
to the minimum, unless we want this change so badly we agree to extend
the pretesting by another month or two.

bug#23785: Emacs 25: "Undo" overdoes things.

[Stefan Monnier]

> My concern is not with the behavior the proposed change intends to get
> us, the concern is with the unintended consequences of the change.  At
> this late stage, I'd like to keep the risk of unintended consequences
> to the minimum, unless we want this change so badly we agree to extend
> the pretesting by another month or two.

Yes, that's the other side of the coin, indeed.


        Stefan

bug#23785: Emacs 25: 'Undo' overdoes things.

[Phillip Lord]

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

On Sat, June 18, 2016 8:52 pm, Stefan Monnier wrote:
>> My concern is not with the behavior the proposed change intends to get
>> us, the concern is with the unintended consequences of the change.  At
>> this late stage, I'd like to keep the risk of unintended consequences to
>> the minimum, unless we want this change so badly we agree to extend the
>> pretesting by another month or two.
>
> Yes, that's the other side of the coin, indeed.


Attached are another two patches which also solve the issue.

One adds an undo boundary specifically in revert-buffer. Works but is
questionable in that revert-buffer is pluggable -- it can call any
function, other than the default.

The other fiddles with insert-file-contents and adds an undo. It is this
function that has specialized handling for the undo list that is causing
the problem. My patch in this case is questionable in that I have randomly
pushed a call to undo-boundary near the end. It should probably be
somewere better.

Another possibility would be to have insert-file-contents call
"undo-auto--undoable-change" -- this is the root cause of the problem.
Changes are happening.

Also attached is a test case which demonstrates the problem.

So many possibilities!



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-missing-undo-boundary-after-revert-buffer.patch --]
[-- Type: text/x-patch; name="0001-Fix-missing-undo-boundary-after-revert-buffer.patch", Size: 1006 bytes --]

From 4250d150cfce6cd461dc6475bac08d7587842e84 Mon Sep 17 00:00:00 2001
From: Phillip Lord <phillip.lord@russet.org.uk>
Date: Sun, 19 Jun 2016 21:34:58 +0100
Subject: [PATCH] Fix missing undo-boundary after revert-buffer

 - lisp/files.el (revert-buffer): Ensure an undo-boundary after
   completion.

Addresses Bug#23785
---
 lisp/files.el | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lisp/files.el b/lisp/files.el
index 1f97fa5..c5d2bab 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -5533,7 +5533,10 @@ revert-buffer
   (let ((revert-buffer-in-progress-p t)
         (revert-buffer-preserve-modes preserve-modes))
     (funcall (or revert-buffer-function #'revert-buffer--default)
-             ignore-auto noconfirm)))
+             ignore-auto noconfirm)
+    ;; Ensure that we have a undo-boundary after reversion (see Bug
+    ;; #23785)
+    (undo-boundary)))
 
 (defun revert-buffer--default (ignore-auto noconfirm)
   "Default function for `revert-buffer'.
-- 
2.9.0

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-Fix-missing-undo-boundary-on-revert.patch --]
[-- Type: text/x-patch; name="0001-Fix-missing-undo-boundary-on-revert.patch", Size: 858 bytes --]

From 1b2cd06f4b6f993052632e24c50209816314c004 Mon Sep 17 00:00:00 2001
From: Phillip Lord <phillip.lord@russet.org.uk>
Date: Sun, 19 Jun 2016 23:33:22 +0100
Subject: [PATCH] Fix missing undo-boundary on revert

But badly, because I have pushed the undo_boundary call into a fairly
random place in a very long function.
---
 src/fileio.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/fileio.c b/src/fileio.c
index b11f923..e01045e 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -4500,6 +4500,10 @@ by calling `format-decode', which see.  */)
                              current_buffer->newline_cache,
                              PT - BEG, Z - PT - inserted);
 
+  /* Random push an undo boundary here because I don't know where else
+     to put this call */
+  Fundo_boundary();
+
   if (read_quit)
     Fsignal (Qquit, Qnil);
 
-- 
2.9.0

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: revert-buffer-tests.el --]
[-- Type: text/x-emacs-lisp; name="revert-buffer-tests.el", Size: 338 bytes --]

(require 'ert)

(ert-deftest undo-after-revert-buffer ()
  (should-not
   (unwind-protect
       (progn
         (find-file "simple-file.txt")
         (goto-char (point-min))
         (insert "\n")
         (revert-buffer t t)
         (undo-auto--needs-boundary-p))
     (kill-buffer
      (get-file-buffer
       "simple-file.txt")))))

[-- Attachment #5: simple-file.txt --]
[-- Type: text/plain, Size: 9 bytes --]

line one

bug#23785: Emacs 25: 'Undo' overdoes things.

[Stefan Monnier]

> The other fiddles with insert-file-contents and adds an undo. It is this
> function that has specialized handling for the undo list that is causing
> the problem. My patch in this case is questionable in that I have randomly
> pushed a call to undo-boundary near the end. It should probably be
> somewere better.

Indeed, the problem is not that insert-file-contents fails to add
a boundary, but rather that it removes an existing one, so we should
figure out where/why it removes it and change the code to push one back
after removing it.

> Another possibility would be to have insert-file-contents call
> "undo-auto--undoable-change" -- this is the root cause of the problem.

Figuring out why undo-auto--undoable-change isn't called for it would be
good as well, indeed.  After all, it runs after-change-functions, so
it should call undo-auto--undoable-change.


        Stefan

bug#23785: Emacs 25: "Undo" overdoes things.

[Phillip Lord]

Óscar Fuentes <ofv@wanadoo.es> writes:

> Alan Mackenzie <acm@muc.de> writes:
>
>> I don't think it's all that minor an issue.  `undo' can no longer be
>> depended upon to restore a buffer to its unchanged state.
>>
>> I've suffered several similar annoyances with `undo' in the emacs-25
>> branch.
>
> Me too, although I'm not sure about Emacs being the culprit. I use Evil
> and it adds some heuristics on top of Emacs'.


Do let me know if you find any. It would be nice it evil worked out of
the box with the new release!

Phil

bug#23785: Emacs 25: 'Undo' overdoes things.

[Phillip Lord]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> The other fiddles with insert-file-contents and adds an undo. It is this
>> function that has specialized handling for the undo list that is causing
>> the problem. My patch in this case is questionable in that I have randomly
>> pushed a call to undo-boundary near the end. It should probably be
>> somewere better.
>
> Indeed, the problem is not that insert-file-contents fails to add
> a boundary, but rather that it removes an existing one, so we should
> figure out where/why it removes it and change the code to push one back
> after removing it.

I don't understand why you say this. AFAICT, the problem is that the
buffer-undo-list doesn't have a nil after the command has happened.


>> Another possibility would be to have insert-file-contents call
>> "undo-auto--undoable-change" -- this is the root cause of the problem.
>
> Figuring out why undo-auto--undoable-change isn't called for it would be
> good as well, indeed.  After all, it runs after-change-functions, so
> it should call undo-auto--undoable-change.

I think that it does -- it calls "insert_from_buffer" which then calls
"prepare_to_modify_buffer".

I *think* what is happening is that prepare_to_modify_buffer is being
called when buffer-undo-list is specbound to t -- hence the change does
not register as undoable change.

I am somewhat guessing here.

Phil

bug#23785: Emacs 25: 'Undo' overdoes things.

[Stefan Monnier]

> I don't understand why you say this. AFAICT, the problem is that the
> buffer-undo-list doesn't have a nil after the command has happened.

Hmm... I thought I had seen a trace showing that there was a nil before
the revert-buffer that then disappeared.  But it looks like
I misremember.  Sorry.

> I think that it does -- it calls "insert_from_buffer" which then calls
> "prepare_to_modify_buffer".
> I *think* what is happening is that prepare_to_modify_buffer is being
> called when buffer-undo-list is specbound to t -- hence the change does
> not register as undoable change.

That would be a good explanation, indeed.  So I guess your patch is the
right fix.  Thank you.


        Stefan

bug#23785: Emacs 25: 'Undo' overdoes things.

[Phillip Lord]

"Phillip Lord" <phillip.lord@russet.org.uk> writes:

> On Sat, June 18, 2016 8:52 pm, Stefan Monnier wrote:
>>> My concern is not with the behavior the proposed change intends to get
>>> us, the concern is with the unintended consequences of the change.  At
>>> this late stage, I'd like to keep the risk of unintended consequences to
>>> the minimum, unless we want this change so badly we agree to extend the
>>> pretesting by another month or two.
>>
>> Yes, that's the other side of the coin, indeed.
>
> So many possibilities!


So, final patch possibility -- this one, I think directly fixes the
problem -- it ensures that the changes in the buffer result in an
undoable change.

It's probably less efficient (cause the undo-list will be created, then
dumped), but is the most minimal change.

My suggestion: this patch goes to Emacs-25. And the previous patch
(which automatically adds an undo boundary to current-buffer regardless
of changes), goes to master.

Phil

bug#23785: Emacs 25: 'Undo' overdoes things.

[Eli Zaretskii]

> From: phillip.lord@russet.org.uk (Phillip Lord)
> Cc: "Eli Zaretskii" <eliz@gnu.org>,  acm@muc.de,  23785@debbugs.gnu.org
> Date: Mon, 20 Jun 2016 16:03:35 +0100
> 
> My suggestion: this patch goes to Emacs-25.

What patch?

bug#23785: Emacs 25: 'Undo' overdoes things.

[Phillip Lord]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: phillip.lord@russet.org.uk (Phillip Lord)
>> Cc: "Eli Zaretskii" <eliz@gnu.org>,  acm@muc.de,  23785@debbugs.gnu.org
>> Date: Mon, 20 Jun 2016 16:03:35 +0100
>> 
>> My suggestion: this patch goes to Emacs-25.
>
> What patch?

Eli, do pay attention! The one that I didn't attach obviously.

Phil


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Ensure-undo-boundary-after-insert-file-contents.patch --]
[-- Type: text/x-diff, Size: 1189 bytes --]

From a77a8683cddf918869b63fc3242ae594b186cc94 Mon Sep 17 00:00:00 2001
From: Phillip Lord <phillip.lord@russet.org.uk>
Date: Mon, 20 Jun 2016 14:26:02 +0100
Subject: [PATCH] Ensure undo-boundary after insert-file-contents.

* src/fileio.c: Record undoable change during insert-file-contents.

Addresses Bug #23785.
---
 src/fileio.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/fileio.c b/src/fileio.c
index b11f923..9fc1bd8 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -4047,8 +4047,16 @@ by calling `format-decode', which see.  */)
 	 being called in insert_from_buffer (via in
 	 prepare_to_modify_buffer).  */
       specbind (intern ("buffer-file-name"), Qnil);
+
+      /*
+        Temporarily enable the undo-buffer to ensure that the change
+        is marked as an undoable one. Bug #23785.
+       */
+      bset_undo_list(current_buffer,Qnil);
       insert_from_buffer (XBUFFER (conversion_buffer),
 			  same_at_start_charpos, inserted_chars, 0);
+      bset_undo_list(current_buffer,Qt);
+
       /* Set `inserted' to the number of inserted characters.  */
       inserted = PT - temp;
       /* Set point before the inserted characters.  */
-- 
2.9.0

bug#23785: Emacs 25: 'Undo' overdoes things.

[Eli Zaretskii]

> From: phillip.lord@russet.org.uk (Phillip Lord)
> Cc: monnier@iro.umontreal.ca,  acm@muc.de,  23785@debbugs.gnu.org
> Date: Mon, 20 Jun 2016 18:12:29 +0100
> 
> >> From: phillip.lord@russet.org.uk (Phillip Lord)
> >> Cc: "Eli Zaretskii" <eliz@gnu.org>,  acm@muc.de,  23785@debbugs.gnu.org
> >> Date: Mon, 20 Jun 2016 16:03:35 +0100
> >> 
> >> My suggestion: this patch goes to Emacs-25.
> >
> > What patch?
> 
> Eli, do pay attention! The one that I didn't attach obviously.

Thanks, please push to the emacs-25 branch, with one change:

> +      /*
> +        Temporarily enable the undo-buffer to ensure that the change
> +        is marked as an undoable one. Bug #23785.
> +       */

Please format this comment according to our style.

bug#23785: Emacs 25: 'Undo' overdoes things.

[Eli Zaretskii]

> From: phillip.lord@russet.org.uk (Phillip Lord)
> Cc: "Eli Zaretskii" <eliz@gnu.org>,  acm@muc.de,  23785@debbugs.gnu.org
> Date: Mon, 20 Jun 2016 16:03:35 +0100
> 
> My suggestion: this patch goes to Emacs-25. And the previous patch
> (which automatically adds an undo boundary to current-buffer regardless
> of changes), goes to master.

So can you please show the "previous" patch?  There were too many of
them.

bug#23785: Emacs 25: 'Undo' overdoes things.

[Phillip Lord]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: phillip.lord@russet.org.uk (Phillip Lord)
>> Cc: "Eli Zaretskii" <eliz@gnu.org>,  acm@muc.de,  23785@debbugs.gnu.org
>> Date: Mon, 20 Jun 2016 16:03:35 +0100
>> 
>> My suggestion: this patch goes to Emacs-25. And the previous patch
>> (which automatically adds an undo boundary to current-buffer regardless
>> of changes), goes to master.
>
> So can you please show the "previous" patch?  There were too many of
> them.


This one, to master.

>From 2ab1f314ad6fe0e68420cc510445495467d82b8f Mon Sep 17 00:00:00 2001
From: Phillip Lord <phillip.lord@russet.org.uk>
Date: Fri, 17 Jun 2016 22:34:50 +0100
Subject: [PATCH] Fix missing undo-boundary after revert-buffer

* lisp/simple.el (undo-auto--boundaries): Ensure an undo-boundary after
  every command whether it (apparently) changes the buffer or not.

Addresses Bug#23785
---
 lisp/simple.el | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lisp/simple.el b/lisp/simple.el
index b66827d..3110430 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -2875,6 +2875,10 @@ undo-auto--boundaries
   "Check recently changed buffers and add a boundary if necessary.
 REASON describes the reason that the boundary is being added; see
 `undo-last-boundary' for more information."
+  ;; (Bug #23785) All commands should ensure that there is an undo
+  ;; boundary whether they have changed the current buffer or not.
+  (when (eq cause 'command)
+    (add-to-list 'undo-auto--undoably-changed-buffers (current-buffer)))
   (dolist (b undo-auto--undoably-changed-buffers)
           (when (buffer-live-p b)
             (with-current-buffer b
-- 
2.8.4

bug#23785: Emacs 25: 'Undo' overdoes things.

[Phillip Lord]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: phillip.lord@russet.org.uk (Phillip Lord)
>> Cc: monnier@iro.umontreal.ca,  acm@muc.de,  23785@debbugs.gnu.org
>> Date: Mon, 20 Jun 2016 18:12:29 +0100
>> 
>> >> From: phillip.lord@russet.org.uk (Phillip Lord)
>> >> Cc: "Eli Zaretskii" <eliz@gnu.org>,  acm@muc.de,  23785@debbugs.gnu.org
>> >> Date: Mon, 20 Jun 2016 16:03:35 +0100
>> >> 
>> >> My suggestion: this patch goes to Emacs-25.
>> >
>> > What patch?
>> 
>> Eli, do pay attention! The one that I didn't attach obviously.
>
> Thanks, please push to the emacs-25 branch, with one change:
>
>> +      /*
>> +        Temporarily enable the undo-buffer to ensure that the change
>> +        is marked as an undoable one. Bug #23785.
>> +       */
>
> Please format this comment according to our style.

Okay. Stefan, can you have a quick look as well. This function is well
out of my depth of understanding.

Phil

bug#23785: Emacs 25: 'Undo' overdoes things.

[Eli Zaretskii]

> From: phillip.lord@russet.org.uk (Phillip Lord)
> Cc: monnier@iro.umontreal.ca,  acm@muc.de,  23785@debbugs.gnu.org
> Date: Tue, 21 Jun 2016 15:29:47 +0100
> 
> This one, to master.
> 
> >From 2ab1f314ad6fe0e68420cc510445495467d82b8f Mon Sep 17 00:00:00 2001
> From: Phillip Lord <phillip.lord@russet.org.uk>
> Date: Fri, 17 Jun 2016 22:34:50 +0100
> Subject: [PATCH] Fix missing undo-boundary after revert-buffer
> 
> * lisp/simple.el (undo-auto--boundaries): Ensure an undo-boundary after
>   every command whether it (apparently) changes the buffer or not.
> 
> Addresses Bug#23785
> ---
>  lisp/simple.el | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/lisp/simple.el b/lisp/simple.el
> index b66827d..3110430 100644
> --- a/lisp/simple.el
> +++ b/lisp/simple.el
> @@ -2875,6 +2875,10 @@ undo-auto--boundaries
>    "Check recently changed buffers and add a boundary if necessary.
>  REASON describes the reason that the boundary is being added; see
>  `undo-last-boundary' for more information."
> +  ;; (Bug #23785) All commands should ensure that there is an undo
> +  ;; boundary whether they have changed the current buffer or not.
> +  (when (eq cause 'command)
> +    (add-to-list 'undo-auto--undoably-changed-buffers (current-buffer)))
>    (dolist (b undo-auto--undoably-changed-buffers)
>            (when (buffer-live-p b)
>              (with-current-buffer b

OK.

bug#23785: Emacs 25: 'Undo' overdoes things.

[Stefan Monnier]

> +      /*
> +        Temporarily enable the undo-buffer to ensure that the change
> +        is marked as an undoable one. Bug #23785.
> +       */
> +      bset_undo_list(current_buffer,Qnil);

Additionally to Eli's remark about the shape of your comments, please
also put spaces before open parens and after commas.

>        insert_from_buffer (XBUFFER (conversion_buffer),
>  			  same_at_start_charpos, inserted_chars, 0);
> +      bset_undo_list(current_buffer,Qt);

Instead of two bset_undo_list, you could use a single specbind since the
above code is almost immediately followed by unbind_to.

But more seriously, I'm wondering: where is undo-list set to t (and
hence causing the problem we're seeing)?
Searching for "undo" in that function gives m the impression that
undo-0list won't be set to t during the call to insert_from_buffer.
What am I missing?


        Stefan

bug#23785: Emacs 25: 'Undo' overdoes things.

[Phillip Lord]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> +      /*
>> +        Temporarily enable the undo-buffer to ensure that the change
>> +        is marked as an undoable one. Bug #23785.
>> +       */
>> +      bset_undo_list(current_buffer,Qnil);
>
> Additionally to Eli's remark about the shape of your comments, please
> also put spaces before open parens and after commas.

Oh, dear, I took Eli's okay and commited. I always was over
enthusiastic. I think I better revert.


>
>>        insert_from_buffer (XBUFFER (conversion_buffer),
>>  			  same_at_start_charpos, inserted_chars, 0);
>> +      bset_undo_list(current_buffer,Qt);
>
> Instead of two bset_undo_list, you could use a single specbind since the
> above code is almost immediately followed by unbind_to.


I tried that

      specbind (intern("buffer-undo-list"), Qnil);


> But more seriously, I'm wondering: where is undo-list set to t (and
> hence causing the problem we're seeing)?
> Searching for "undo" in that function gives m the impression that
> undo-0list won't be set to t during the call to insert_from_buffer.
> What am I missing?

It's set in several places to Qt, then restored at the end here.

      if (!empty_undo_list_p)
	{
	  bset_undo_list (current_buffer, old_undo);
	  if (CONSP (old_undo) && inserted != old_inserted)
	    {
	      /* Adjust the last undo record for the size change during
		 the format conversion.  */
	      Lisp_Object tem = XCAR (old_undo);
	      if (CONSP (tem) && INTEGERP (XCAR (tem))
		  && INTEGERP (XCDR (tem))
		  && XFASTINT (XCDR (tem)) == PT + old_inserted)
		XSETCDR (tem, make_number (PT + inserted));
	    }
	}

At least that was my theory; I tested it by adding print statements to
run_undoable_change which running, but returning before the call0.

run_undoable_change (void)
{
  if (EQ (BVAR (current_buffer, undo_list), Qt))
    return;

  call0 (Qundo_auto__undoable_change);
}

The code is convoluted enough, though, that I am worried that I may have
got this wrong.


Anyway, I've just testing emacs-25 after my change, the patch seems to
be doing very bad things -- i.e. leaving buffer-undo-list as Qt. So, I
think I really sure revert, then worry about it tomorrow.

Phil

bug#23785: Emacs 25: 'Undo' overdoes things.

[Lars Ingebrigtsen]

This was a pretty long bug report with many suggested patches, some of
which were applied and then reverted.

But this one was applied and is still in Emacs:

commit c98bc9821f4a402d5fda67fe141ed34622c50e4f
Author:     Phillip Lord <phillip.lord@russet.org.uk>
AuthorDate: Fri Jun 17 22:34:50 2016 +0100

    Ensure undo-boundary after all commands
    
I may be misreading the thread, but it sounded like this fixed the
reported problem?  So I'm closing this bug report; if there's more to be
worked on there, please respond to the debbugs address and we'll reopen.

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

bug#23785: Emacs 25: 'Undo' overdoes things.

[Alan Mackenzie]

Hello, Lars.

On Fri, Sep 04, 2020 at 16:03:41 +0200, Lars Ingebrigtsen wrote:
> This was a pretty long bug report with many suggested patches, some of
> which were applied and then reverted.

> But this one was applied and is still in Emacs:

> commit c98bc9821f4a402d5fda67fe141ed34622c50e4f
> Author:     Phillip Lord <phillip.lord@russet.org.uk>
> AuthorDate: Fri Jun 17 22:34:50 2016 +0100

>     Ensure undo-boundary after all commands
    
> I may be misreading the thread, but it sounded like this fixed the
> reported problem?  So I'm closing this bug report; if there's more to be
> worked on there, please respond to the debbugs address and we'll reopen.

Whatever it was that caused me to submit that bug report hasn't annoyed
me in a long, long time.  So I think that patch which remains has fixed
the bug.

So I agree that closing it is the right thing to do.

Thanks!

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

-- 
Alan Mackenzie (Nuremberg, Germany).