bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Phillip Lord]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: phillip.lord@russet.org.uk (Phillip Lord)
>> Cc: Eli Zaretskii <eliz@gnu.org>,  23871@debbugs.gnu.org
>> Date: Fri, 01 Jul 2016 23:21:01 +0100
>> 
>> (let ((p (start-process "bc" (current-buffer) "bc")))
>>   (process-send-string p "2^10\n")
>>   (goto-char (point-max)))
>> 
>> The strange thing here is that it's not specifically an undo problem.
>> Point doesn't move after eval-defun.
>
> The problem, AFAIU, is in point movements _during_ eval-defun: they
> seem to not be recorded in buffer-undo-list.  The undo list I get
> after C-M-x is this:
>
>   (nil (117 . 122) (t 22391 27551 0 0))
>

So, I don't think this is a regression caused by my patch at all. It is
an bug that has been there since I altered undo.c last year.

The problem was caused because of undo only records point after a
boundary (or the first element). I'd changed things during slightly when
I update undo.c so that the timestamp list got added before checking
whether I was at a boundary, hence blocking addition of the point
restoration information.

This is why I found the problem so erratic to replicate; it only occurs
immediately the first change to a buffer.

I believe the following patch addresses the issue.

Phil

>From d4e9e44402fdf248ba4bc895e914d4cc5580f229 Mon Sep 17 00:00:00 2001
From: Phillip Lord <phillip.lord@russet.org.uk>
Date: Thu, 30 Jun 2016 22:06:00 +0100
Subject: [PATCH] Fix missing point information in undo

* src/undo.c (record_insert): Use record_point instead of
  prepare_record, and do so unconditionally.
  (prepare_record): Do not record first change.
  (record_point): Now conditional on state before the last command.
  (record_delete): Call record_point unconditionally.

Addresses Bug# 21722
---
 src/undo.c                    | 26 +++++++++++++-------------
 test/automated/simple-test.el | 19 ++++++++++++++++++-
 2 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/src/undo.c b/src/undo.c
index be5b270..f5a5ea1 100644
--- a/src/undo.c
+++ b/src/undo.c
@@ -40,16 +40,13 @@ prepare_record (void)
   /* Allocate a cons cell to be the undo boundary after this command.  */
   if (NILP (pending_boundary))
     pending_boundary = Fcons (Qnil, Qnil);
-
-  if (MODIFF <= SAVE_MODIFF)
-    record_first_change ();
 }
 
 /* Record point as it was at beginning of this command.
-   PT is the position of point that will naturally occur as a result of the
+   BEG is the position of point that will naturally occur as a result of the
    undo record that will be added just after this command terminates.  */
 static void
-record_point (ptrdiff_t pt)
+record_point (ptrdiff_t beg)
 {
   /* Don't record position of pt when undo_inhibit_record_point holds.  */
   if (undo_inhibit_record_point)
@@ -60,13 +57,16 @@ record_point (ptrdiff_t pt)
   at_boundary = ! CONSP (BVAR (current_buffer, undo_list))
                 || NILP (XCAR (BVAR (current_buffer, undo_list)));
 
-  prepare_record ();
+  if (MODIFF <= SAVE_MODIFF)
+    record_first_change ();
 
   /* If we are just after an undo boundary, and
      point wasn't at start of deleted range, record where it was.  */
-  if (at_boundary)
+  if (at_boundary
+      && point_before_last_command_or_undo != beg
+      && buffer_before_last_command_or_undo == current_buffer )
     bset_undo_list (current_buffer,
-                   Fcons (make_number (pt),
+                   Fcons (make_number (point_before_last_command_or_undo),
                           BVAR (current_buffer, undo_list)));
 }
 
@@ -85,6 +85,8 @@ record_insert (ptrdiff_t beg, ptrdiff_t length)
 
   prepare_record ();
 
+  record_point (beg);
+
   /* If this is following another insertion and consecutive with it
      in the buffer, combine the two.  */
   if (CONSP (BVAR (current_buffer, undo_list)))
@@ -163,19 +165,17 @@ record_delete (ptrdiff_t beg, Lisp_Object string, bool 
record_markers)
   if (EQ (BVAR (current_buffer, undo_list), Qt))
     return;
 
-  if (point_before_last_command_or_undo != beg
-      && buffer_before_last_command_or_undo == current_buffer)
-    record_point (point_before_last_command_or_undo);
+  prepare_record ();
+
+  record_point (beg);
 
   if (PT == beg + SCHARS (string))
     {
       XSETINT (sbeg, -beg);
-      prepare_record ();
     }
   else
     {
       XSETFASTINT (sbeg, beg);
-      prepare_record ();
     }
 
   /* primitive-undo assumes marker adjustments are recorded
diff --git a/test/automated/simple-test.el b/test/automated/simple-test.el
index 40cd1d2..c41d010 100644
--- a/test/automated/simple-test.el
+++ b/test/automated/simple-test.el
@@ -311,6 +311,7 @@ undo-test-point-after-forward-kill
       (undo-test-point-after-forward-kill))))
 
 (defmacro simple-test-undo-with-switched-buffer (buffer &rest body)
+  (declare (indent 1) (debug t))
   (let ((before-buffer (make-symbol "before-buffer")))
     `(let ((,before-buffer (current-buffer)))
        (unwind-protect
@@ -340,8 +341,24 @@ simple-test-undo-with-switched-buffer
        (point-min)
        (point-max))))))
 
+(ert-deftest missing-record-point-in-undo ()
+  "Check point is being restored correctly.
 
-
+See Bug#21722."
+  (should
+   (= 5
+      (with-temp-buffer
+       (generate-new-buffer " *temp*")
+       (emacs-lisp-mode)
+       (setq buffer-undo-list nil)
+       (insert "(progn (end-of-line) (insert \"hello\"))")
+       (beginning-of-line)
+       (forward-char 4)
+       (undo-boundary)
+       (eval-defun nil)
+       (undo-boundary)
+       (undo)
+       (point)))))
 
 (provide 'simple-test)
 ;;; simple-test.el ends here
-- 
2.9.0