bug#70077: An easier way to track buffer changes

[Stefan Monnier]

>> +  "Object holding a description of a buffer state.
>> +BEG..END is the area that was changed and BEFORE is its previous content.
>> +If the current buffer currently holds the content of the next state, you 
>> can get
>> +the contents of the previous state with:
>> +
>> +    (concat (buffer-substring (point-min) beg)
>> +                before
>> +                (buffer-substring end (point-max)))
>> +
>> +NEXT is the next state object (i.e. a more recent state).
>> +If NEXT is nil it means it's 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 create."
>
> 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.

>> +(defvar-local track-changes--before-beg (point-min)
>> +  "Beginning position of the remembered \"before string\".")
>> +(defvar-local track-changes--before-end (point-min)
>> +  "End position of the text replacing the \"before string\".")
> Why (point-min)? It will make the values dependent on the buffer
> narrowing that happens to be active when the library if first loaded.
> Which cannot be right.

The precise value should hopefully never matter, barring bugs.
I changed them to 0.

>> +(defvar-local track-changes--buffer-size nil
>> +  "Current size of the buffer, as far as this library knows.
>> +This is used to try and detect cases where buffer modifications are 
>> \"lost\".")
> Just looking at the buffer size may miss unregistered edits that do not
> change the total size of the buffer.  Although I do not know a better
> measure.  `buffer-chars-modified-tic' may lead to false-positives
> (Bug#51766).

Yup, hence the "try to".

>> +(cl-defun track-changes-register ( signal &key nobefore disjoint immediate)
>> +  "Register a new tracker and return a new tracker ID.
>> +SIGNAL is a function that will be called with one argument (the tracker ID)
>> +after the current buffer is modified, so that we can react to the change.
>> + ...
>> +If optional argument DISJOINT is non-nil, SIGNAL is called every time we are
>> +about to combine changes from \"distant\" parts of the buffer.
>> +This is needed when combining disjoint changes into one bigger change
>> +is unacceptable, typically for performance reasons.
>> +These calls are distinguished from normal calls by calling SIGNAL with
>> +a second argument which is the distance between the upcoming change and
>> +the previous changes.
>
> 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.

>> +  (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`.  🙁

>> +(defun track-changes-fetch (id func)
>> ...
>> +  (unless (equal track-changes--buffer-size (buffer-size))
>> +    (track-changes--recover-from-error))
>> +  (let ((beg nil)
>> +        (end nil)
>> +        (before t)
>> +        (lenbefore 0)
>> +        (states ()))
>> +    ;; Transfer the data from `track-changes--before-string'
>> +    ;; to the tracker's state object, if needed.
>> +    (track-changes--clean-state)
>
>> +(defun track-changes--recover-from-error ()
>> ...
>> +  (setq track-changes--state (track-changes--state)))
>
> This will create a dummy state with
>
>  (beg (point-max))
>  (end (point-min))
>
> 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`.

>> +(defun track-changes--in-revert (beg end before func)
>> ...
>> +      (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%.  🙁

>> +(defun track-changes-tests--random-word ()
>> +  (let ((chars ()))
>> +    (dotimes (_ (1+ (random 12)))
>> +      (push (+ ?A (random (1+ (- ?z ?A)))) chars))
>> +    (apply #'string chars)))
>
> If you are using random values for edits, how can such test be
> reproduced?

Luck?

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


        Stefan


diff --git a/test/lisp/emacs-lisp/track-changes-tests.el 
b/test/lisp/emacs-lisp/track-changes-tests.el
index cdccbe80299..eab9197030f 100644
--- a/test/lisp/emacs-lisp/track-changes-tests.el
+++ b/test/lisp/emacs-lisp/track-changes-tests.el
@@ -36,6 +36,11 @@ track-changes-tests--random-verbose
 (defun track-changes-tests--message (&rest args)
   (when track-changes-tests--random-verbose (apply #'message args)))
 
+(defvar track-changes-tests--random-seed
+  (let ((seed (number-to-string (random (expt 2 24)))))
+    (message "Random seed = %S" seed)
+    seed))
+
 (ert-deftest track-changes-tests--random ()
   ;; Keep 2 buffers in sync with a third one as we make random
   ;; changes to that 3rd one.
@@ -97,6 +102,8 @@ track-changes-tests--random
       (insert-into-buffer buf2)
       (should (equal (buffer-hash) (buffer-hash buf1)))
       (should (equal (buffer-hash) (buffer-hash buf2)))
+      (message "seeding with: %S" track-changes-tests--random-seed)
+      (random track-changes-tests--random-seed)
       (dotimes (_ 1000)
         (pcase (random 15)
           (0