bug#70077: An easier way to track buffer changes

[Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>]

>> You never want to use this call, it's only there to show in a concise
>> manner how BEG/END/BEFORE relate and what information they're supposed
>> to hold.
> My point is that it is hard to understand that docstring for someone
> reading it first. I did not mean that it does not make sense for someone
> who is already familiar with the rest of the code.
> I believe that it would help if you explain how to interpret the values
> when there is a chain of state1 -> next -> next -> nil.

How 'bout:

      "Object holding a description of a buffer state.
    A buffer state is described by a BEG/END/BEFORE triplet which say how to
    recover that state from the next state.  I.e. if the buffer's contents
    reflects the next state, you can recover the previous state by replacing
    the BEG..END region with the BEFORE string.
    
    NEXT is the next state object (i.e. a more recent state).
    If NEXT is nil it means it's the most recent state and it may be incomplete
    \(BEG/END/BEFORE may be nil), in which case those fields will take their
    values from `track-changes--before-(beg|end|before)' when the next
    state is created."

>> The second convention is used only when DISJOINT is non-nil, which is
>> why it's described where we document the effect of DISJOINT.
>> An alternative could be to make the `disjoint` arg hold the function to call
>> to signal disjoint changes.
>
> Sure. My point is that the current formulation of the docstring might
> lead to errors unless one is very careful when reading it.

The Texinfo doc is more upfront about the fact that it's called
sometimes with one and sometimes with 2 args, but I feel that the
function's docstring is already too long so I'd rather not
repeat that here.

I feel that

    These calls are distinguished from normal calls by calling SIGNAL with
    a second argument which ...

does make it sufficiently clear that it's different.  Also, any errors
should be caught pretty quickly, so I'm not worried about seeing a lot of
broken code with latent bugs because the SIGNAL function doesn't accept
the right number of arguments.

>>>> +  (unless nobefore
>>>> +    (setq track-changes--before-no nil)
>>>> +    (add-hook 'before-change-functions #'track-changes--before nil t))
>>>> +  (add-hook 'after-change-functions  #'track-changes--after  nil t)
>>> Note that all the changes made in indirect buffers will be missed.
>>> See bug#60333.
>> Yup.  And misuses of `inhibit-modification-hooks` or
>> `with-silent-modifications`.  🙁
> Indirect buffer is not at all a misuse.

I didn't claim the opposite here (tho I do admit to thinking that it's
an attractive nuisance).  I only reminded that cases where changes are
missed are things that can and do happen because of various problems in
various pieces of code and indirect buffers are just one of those
sources of problems.

> What I implied in my comment is that it would be nice to implement
> something like what whitespace.el does in bug#60333.

I'll reply the same as in bug#60333: it should be fixed at a lower level.
I don't see any reason why it should be difficult to make it work right.

> This point is important from the point of view of Org mode because Org
> mode uses a lot of indirect buffers both internally and as a recommended
> workflow (org-tree-to-indirect-buffer).

Then I encourage you to try and fix it: rename `make-indirect-buffer` to
`internal-make-indirect-buffer`, define a new `make-indirect-buffer` in
ELisp which registers *-change-functions to propagate the changes to all
the related buffers.

Personally all the problems with indirect buffers put me off.

> You are right. That sneaky (track-changes--before-clean nil) line is so
> sneaky that I'd like to see a comment highlighting that it exists ;)
> I simply missed it and thought that `track-changes--before-clean' only
> has two cond branches when I was reading the code.

OK, I made it stand out more. 🙂

>> During FUNC they're moved to BEG or END, and when we restore the
>> original state, well... the undo machinery has some support to restore
>> the markers where they were, but it's definitely not 100%.  🙁
>
> Interesting. I did not know that markers are recovered by undo.

That's what the following entries are for:

    An entry (MARKER . DISTANCE) indicates that the marker MARKER
    was adjusted in position by the offset DISTANCE (an integer).

> I had very bad experience with marker preservation when working with
> other low-level APIs like `replace-match'. And previous Org mode
> commiters had the same issues with kill/yank.

`replace-match`, `kill`, `yank`, etc... can't do very much because they
have no idea how the new text relates to the old one, so without
additional application-specific information, the only thing they can do
is move the inner markers to one of the ends of the change.
E.g. when you move a subtree, *you* know that the text you insert is the
same text that you previously deleted, but the `insdel.c` primitives
don't know that.
In some cases you can get the desired behavior "automatically" by
using text-properties instead of markers, tho.

Undo is different because we're going back to a state for which Emacs
can know where the markers should be because we've been there.

> May undo be trusted?

I never looked closely to see how reliable it is.
AFAIK for the markers for which we don't call `move-marker`
it should not be too hard to make it reliable (if it's not already).
But for those on which we do call `move-marker`, I don't think the
current form of `buffer-undo-list` provides sufficient info to
know whether the marker should be "moved to its previous position"
or should be left alone (if `move-marker` was called in the mean
time, the user probably doesn't want that marker to move back).

We know this because the `mark` is a marker we move all the time
(instead of creating a new marker each time) and we've had a bug report
about it.  🙁

> Or may it be more reliable to deploy a more manual
> approach explicitly saving the markers and restoring them via
> unwind-protect?

We don't have a primitive to fetch all the markers in a given region.
But if the FUNC in `track-changes--in-revert` doesn't move any of the
markers, then the `buffer-undo-list` should be good enough (in theory at
least, so it depends on whether we have bugs in there).

>>> Maybe first generate a random seed and then log it, so that the
>>> failing test can be repeated if necessary with seed assigned manually.
>>
>> Good idea.
>> But my attempt (see patch below) failed.
>> I'm not sure what I'm doing wrong, but
>>
>>     make test/lisp/emacs-lisp/track-changes-tests \
>>          EMACS_EXTRAOPT="--eval '(setq track-changes-tests--random-seed \"15216888\")'"
>>
>> gives a different score each time.  🙁
>
> May it be because you used different Emacs builds?

Nope, just repeated invocations of the same Emacs binary.
It was much more silly than that: I seeded the random number generator
too late (after generating the initial buffer content).  🙂
It works now, thank you.


        Stefan