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

[Stefan Monnier]

>> 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.
Could you give a verbosish "commit log" explaining the reason behind
each hunk?

> @@ -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.

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.

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.

>  /* 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?

>    /* 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.

> -  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.


        Stefan