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

>>>      node->otick = otick;
>>
>> Indeed, I saw later in the remove code that we do propagate offsets
>> without caring if they're up-to-date, and I think the logic behind that
>> is sound.  And indeed we can drop the test because the only property we
>> only ever care about for an `otick` is whether it's equal to
>> `tree->otick`, so if node->parent->otick != otick then performing the
>> assignment is equivalent to not performing it.
>
> The above isn't quite right:
> I was thinking of `node->otick = parent->otick`.

Hey Stefan, yep, I'll have to check out the changes you made today.

I had a different idea of tightening the otree invariant to this:

 1) A node's otick must be greater than or equal to its children's.
 2) There is no tree->otick, just tree->root->otick.  That is what is
    incremented when offsets are added.
 3) All downward tree traversal propagates offsets and otick.

This strikes me as conceptually simpler.  E.g. since the invariant is
defined recursively it might allow for more local reasoning, clearer
assertions, less places where "offset math" is needed, etc.  But you've
lived in this code a bit longer than me.  What do you think?  I have a
commit halfway worked up so maybe I can show that if it works out...

Anyway, thanks for fixing my "thinko" bug in the check_tree code -- I
had the fix but should have sent it faster.  I didn't actually intend
for that to be commited yet (pushed it to sr.ht by accident), but maybe
it helped you chase down the bugs you fixed today?

I have two patches, mostly to add more checks to the invariant checking
in check_tree.  Tests on noverlay are passing again as of your commits
today, which is great!

On https://git.sr.ht/~matta/emacs/log/feature/noverlay but also below.