bug#70077: An easier way to track buffer changes

[Ihor Radchenko <yantar92@posteo.net>]

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

>> ...
>> This docstring is a bit confusing.
>> If a state object is not the most recent, how come 
>>
>>> +    (concat (buffer-substring (point-min) beg)
>>> +                before
>>> +                (buffer-substring end (point-max)))
>>
>> produces the previous content?
>
> Because it says "If the current buffer currently holds the content of
> the next state".
> [ "current ... currently" wasn't my best moment, admittedly.  ]
>
>> And if the state object is the most recent, "it may be incomplete"...
>> So, when is it safe to use the above (concat ... ) call?
>
> 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.

>> This is a bit confusing. The first paragraph says that SIGNAL is called
>> with a single argument, but that it appears that two arguments may be
>> passed. I'd rather tell the calling convention early in the docstring.
>
> 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.

>>> +  (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. It is a perfectly valid use.
What I implied in my comment is that it would be nice to implement
something like what whitespace.el does in bug#60333.

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

>> ...
>> such state will not pass (< beg end) assertion in
>> `track-changes--clean-state' called in `track-changes-fetch' immediately
>> after `track-changes--recover-from-error'
>
> I can't reproduce that.  Do you have a recipe?
>
> AFAICT all the (< beg end) tests in `track-changes--clean-state` are
> conditional on `track-changes--before-clean` being nil, whereas
> `track-changes--recover-from-error` sets that var to `unset`.

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.

>>> +      (atomic-change-group
>>> +        (goto-char end)
>>> +        (insert-before-markers before)
>>> +        (delete-region beg end)
>>
>> What happens if there are markers inside beg...end?
>
> 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.

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. We even have a dedicated
machinery in Org mode to save/restore markers in region. For example,
see `org-save-markers-in-region' `org-reinstall-markers-in-region'
(there are more in various places).

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

>> If you are using random values for edits, how can such test be
>> reproduced?
>
> Luck?

Such test is barely useful then, IMHO.

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

   Elisp manual 3.10 Random Numbers

   Sometimes you want the random number sequence to be repeatable. For
   example, when debugging a program whose behavior depends on the
   random number sequence, it is helpful to get the same behavior in
   each program run. To make the sequence repeat, execute ‘(random "")’.
   This sets the seed to a constant value for your particular Emacs
   executable (though it may differ for other Emacs builds). You can use
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   other strings to choose various seed values.

As an alternative, you can use `cl-make-random-state' + `cl-random'.
They are less random AFAIU, but it is not a big deal in this particular
use case.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>