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

[phillip.lord@russet.org.uk (Phillip Lord)]

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

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> 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.
>
> I haven't had time to look in detail at this patch, so it's hard for me
> to give a strong agreement.  Maybe some explanations would help.


This stems from two reported bugs: one that point was not being
correctly restored after an insertion away from point, and another than
point was not being correctly restored after the first change in a
buffer.


> Could you give a verbosish "commit log" explaining the reason behind
> each hunk?

Yes. That is below with the final patch (including changes you have
requested).


>> @@ -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 ();
>>  }
>
> Not sure why/how this is related to the other changes, nor why this code
> was there in the first place.

It was a refactor since these two conditinals were always called
together. A mistake unfortunately (see below).

> But in any case, the comment before prepare_record needs to be updated,
> because it seems to describe neither the current behavior, nor the
> behavior after applying the above hunk.

Done.

> BTW, I notice that in the current code (emacs-25), there's one other
> call to record_first_change (in record_property_change), and it could be
> replaced with a call to prepare_record, so it begs the question whether
> the above hunk should also apply to record_property_change as well.

Don't understand. Think that the call to record_first_change is necessary.


>>  /* 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)));
>
> You said:
>
>     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.
>
> so maybe the computation of at_boundary above should be tweaked to
> ignore timestamps?


That's a possibility, but it appears more complex that re-ordering the
methods.

>>    /* 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)));
>
> I like this part.


Good!

>> -  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);
>
> And I like this part too.


Good!

Here is a hunk-by-hunk description. Comments come before the hunk they
apply to, and I've split some hunks where necessary. The final patch is
attached (it has some more doc, and another call to "prepare_record"
added) also.

Having read it though again, I cannot understand why the boundary is
pre-allocated at all, but that is a discussion for another time.

Sorry for the hassle this late in the development process.

Phil





`prepare_record` had been refactored out when undo.c was re-written. as the
two conditionals were being called together in many places. This turns out to
have been been a mistake, as we need to call something between these two
conditionals in one place (`record_point`).


#+begin_src diff
@@ -31,25 +31,21 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
    an undo-boundary.  */
 static Lisp_Object pending_boundary;
 
-/* Record point as it was at beginning of this command (if necessary)
-   and prepare the undo info for recording a change.
-   Prepare the undo info for recording a change. */
+/* Prepare the undo info for recording a change. */
 static void
 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 ();
 }
#+end_src


The semantics of record_point had been changed also, so that pt became the
point that was to be recorded, rather than the point that we shouldn't record.
The point we should record is available as global state
(`point_before_last_command_or_undo`) anyway.

#+begin_src diff
-/* 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
-   undo record that will be added just after this command terminates.  */
+/* Record point, if necessary, as it was at beginning of this command.
+   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)
#+end_src

Doc added


#+begin_src diff
@@ -57,16 +53,23 @@ record_point (ptrdiff_t pt)
 
   bool at_boundary;
 
+  /* Check whether we are at a boundary now, in case we record the
+  first change. */
   at_boundary = ! CONSP (BVAR (current_buffer, undo_list))
                 || NILP (XCAR (BVAR (current_buffer, undo_list)));
#+end_src

Record point is called from only two places, in both cases before immediately
after `prepare_record`, so we no longer call it here.

However, we must bring back the conditional statement because this is no
longer called in prepare_record, as in this case, it must be called after the
`at_boundary` check. The alternative, which is making the `at_boundary` check
accept "nil" then a timestamp as a boundary sounds more complex.

#+begin_src diff 
-  prepare_record ();
+  /* If this is the first change since save, then record this.*/
+  if (MODIFF <= SAVE_MODIFF)
+    record_first_change ();
#+end_src 

`record_point` is now called unconditionally in the places where it is called,
so we more incorporate the conditional checks here. Take the point to be
recorded from global state.


#+begin_src diff
-  /* 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 we are just after an undo boundary, and point was not at start
+     of deleted range, and the recorded point was in this buffer, then
+     record where it was.  */
+  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)));
 }
#+end_src
 

This record_point was simply missing -- point was never recorded after an
insertion, and this has been directly added to address the bug.

#+begin_src diff
@@ -85,6 +88,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)))
#+end_src

Refactor for completeness.

#+begin_src diff
@@ -120,9 +125,7 @@ record_marker_adjustments (ptrdiff_t from, ptrdiff_t to)
   register struct Lisp_Marker *m;
   register ptrdiff_t charpos, adjustment;
 
-  /* Allocate a cons cell to be the undo boundary after this command.  */
-  if (NILP (pending_boundary))
-    pending_boundary = Fcons (Qnil, Qnil);
+  prepare_record();
 
   for (m = BUF_MARKERS (current_buffer); m; m = m->next)
     {
#+end_src


Remove the conditionality, as this is now inside record_point. Change the
parameter to `record_point` to reflect the changed semantics of that method.

Move `prepare_record` call out of both sides of a conditional. The re-ordering
of `prepare_record` and `record_point` should not matter (famous last words).

#+begin_src diff
@@ -163,19 +168,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
#+end_src

Refactor for completeness.

#+begin_src diff
@@ -234,9 +237,7 @@ record_property_change (ptrdiff_t beg, ptrdiff_t length,
   if (EQ (BVAR (buf, undo_list), Qt))
     return;
 
-  /* Allocate a cons cell to be the undo boundary after this command.  */
-  if (NILP (pending_boundary))
-    pending_boundary = Fcons (Qnil, Qnil);
+  prepare_record();
 
   if (MODIFF <= SAVE_MODIFF)
     record_first_change ();
+end_src



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-missing-point-information-in-undo.patch --]
[-- Type: text/x-diff, Size: 6058 bytes --]

From c7e116d16299dfe3bb13a6574f9b03542cb72099 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.
  (record_property_change): Use prepare_record.
  (record_marker_adjustments): Use prepare_record.

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

diff --git a/src/undo.c b/src/undo.c
index be5b270..2676c3d 100644
--- a/src/undo.c
+++ b/src/undo.c
@@ -31,25 +31,21 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
    an undo-boundary.  */
 static Lisp_Object pending_boundary;
 
-/* Record point as it was at beginning of this command (if necessary)
-   and prepare the undo info for recording a change.
-   Prepare the undo info for recording a change. */
+/* Prepare the undo info for recording a change. */
 static void
 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
-   undo record that will be added just after this command terminates.  */
+/* Record point, if necessary, as it was at beginning of this command.
+   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)
@@ -57,16 +53,23 @@ record_point (ptrdiff_t pt)
 
   bool at_boundary;
 
+  /* Check whether we are at a boundary now, in case we record the
+  first change. */
   at_boundary = ! CONSP (BVAR (current_buffer, undo_list))
                 || NILP (XCAR (BVAR (current_buffer, undo_list)));
 
-  prepare_record ();
+  /* If this is the first change since save, then record this.*/
+  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 we are just after an undo boundary, and point was not at start
+     of deleted range, and the recorded point was in this buffer, then
+     record where it was.  */
+  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 +88,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)))
@@ -120,9 +125,7 @@ record_marker_adjustments (ptrdiff_t from, ptrdiff_t to)
   register struct Lisp_Marker *m;
   register ptrdiff_t charpos, adjustment;
 
-  /* Allocate a cons cell to be the undo boundary after this command.  */
-  if (NILP (pending_boundary))
-    pending_boundary = Fcons (Qnil, Qnil);
+  prepare_record();
 
   for (m = BUF_MARKERS (current_buffer); m; m = m->next)
     {
@@ -163,19 +166,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
@@ -234,9 +235,7 @@ record_property_change (ptrdiff_t beg, ptrdiff_t length,
   if (EQ (BVAR (buf, undo_list), Qt))
     return;
 
-  /* Allocate a cons cell to be the undo boundary after this command.  */
-  if (NILP (pending_boundary))
-    pending_boundary = Fcons (Qnil, Qnil);
+  prepare_record();
 
   if (MODIFF <= SAVE_MODIFF)
     record_first_change ();
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