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

[Stefan Monnier]

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

In record_property_change we have (among other things) the exact same
code as in prepare_record (i.e. there's code duplication).  So we
could/should replace those 4 lines of code with a call to
prepare_record.  But if we do, then your (previous) patch changes the
behavior of record_property_change, whereas if we don't, your (previous)
patch doesn't change its behavior.
Anyway, your new patch addresses this.

> That's a possibility, but it appears more complex than re-ordering the
> methods.
[...]
> 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.

More complex but more robust.  I think it'd be worthwhile to put a FIXME
comment in there, at least.  E.g. the above explanation should be put
inside the code.

> +  /* 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)));
>  }

I think the comment should explain the intention better.
It is currently too close to a simple paraphrase of the code.
I suggest "If it's the first change since the last boundary, and the
upcoming undo record wouldn't restore point correctly, then record where
it was".

Other than that it looks good, thank you for the detailed explanation.


        Stefan