This is an Engineering Notebook post (notes to myself) concerning
https://github.com/leo-editor/leo-editor/issues/149
"Clones can revert changes made by git discard even with --no-cache"
You can safely ignore this post unless you are one of Leo's core developers.
===== Background
This bug is extremely serious. In unusual situations Leo can revert
changes made by git discard without any warning of the reversion, except
that expected "recovered nodes" warnings are missing. If the user ignores
the "dog that didn't bark", the Leo file will contain a hidden time bomb.
The next time Leo writes the affected external file, the reversions will
undo the git discard operation.
The section << Detecting clone conflicts >> discusses the general ideas
behind giving "recovered nodes" warnings. Two ivars are involved:
- **v.tempBodyList**: accumulates lines of body text during reading.
- **v.tempBodyString**: contains the previous value of v.b, if any.
Alas, the code is too complicated. It must be *radically* simplified. I'm
pretty sure I know how to do this, but it will require a detailed plan of
action, discussed later.
===== Proximate cause of the bug
at.readPostPass contains mistaken code. In effect, this code uses
v.tempBodyString before it is defined. No crash happens because the code
uses hasattr to test for the existence of v.tempBodyString.
In effect, the code assumes that v.tempBodyString has already been
computed, when in fact at.terminateBody computes v.tempBodyString. Usually,
at.readPostPass calls at.terminateNode, which calls at.terminateBody and
all is well.
Alas, the guard statement::
if not hasString and not hasList:
continue # Bug fix 2010/07/06: do nothing!
prevents the call to at.terminateNode in precisely bug 149's special case.
As the comment indicates, it's not clear that this guard can be removed
without greatly changing the code.
===== Complexity is the culprit
The "real" cause of the bug is that the code involving
v.tempBodyString/List is far too complicated. During investigation, I used
clone-find-all-flatted, yielding::
Found: (flattened) terminateNode
Found: (flattened) tempBodyList
Found: (flattened) tempBodyString
The code is too complex to fully understand, much less remember, even with
these summaries and even when using Leo's clones to their full extent.
===== Simplifications
The fundamental problem is that Leo inits and deletes the v.tempBodyString
and v.tempBodyList attributes "irregularly". As a result, the code
contains a myriad of guards and special cases. All such code must go.
Part 1: v.tempBodyString.
This ivar is supposed to *exist* if an only if a previous version of v.b
exists. It contains that previous version.
The only reasonable thing is to init v.tempBodyString in the code that
actually creates (or finds) vnodes. Doh!
The code that allocates/finds vnodes is complex. It is in
at.createNewThinNode and various helpers. Happily, the new idea is simple:
Create/update v.tempBodyString if the vnode exists.
v is a clone and there *is* a previous value of p.b.
Do not create v.tempBodyString if the vnode does not exist.
v is not (yet) a clone, and *no* previous value of p.b exists.
With this scheme, v.tempBodyString should exist "forever", that is, all
file-related work completes. fc.readExternalFiles is one possible place to
delete all these ivars, but it would be better encapsulation to delete this
ivar in an atFile method, say in at.readAll. We shall see...
**Important**: Leo creates vnodes in two places: in the sax code that reads
.leo files, and in the code that reads external files. Both places must
create v.tempBodyString as shown above.
Part 2: v.tempBodyString.
Conceptually, v.tempBodyList is more temporary than v.tempBodyString. At
present, it is created only if body text actually exists, and it can be
deleted as soon as v.b is correctly computed.
Whether or not v.tempBodyList is created for empty bodies is a nit. What
*does* matter is that program logic should *never* depend on
hasattr(v,'tempBodyList').
===== Plan of action
A big collapse in complexity will surely result. However, simplifying the
code promises to be fraught with dangers. As a result, I shall do the work
in the following stages:
Stage 0: Revert to rev 5c6cb97
This should be a completely safe starting point. It contains no "real"
changes to the code, except for renaming methods and moving nodes to be
children of at.readPostPass.
This stage involves reverting all of yesterdays investigations. No big
loss.
Stage 1: Init v.tempBodyString when vnodes are created/cloned.
No program logic will be changed. The idea is to make this momentous change
with as little impact as possible.
I may use a new_clone_conflicts switch to *mark* the changes, and to revert
in true emergencies. However, this switch will be removed soon after all
work is completed.
Stage 2: revise at.readPostPass.
This method must be drastically simplified.
The question is whether removing the guard will work. It had better ;-)
Stage 3: Collapsing the code
This stage will make all further simplifications. This will be picky work,
but it should result in beautiful code in the end.
===== Testing and pushing code
Naturally, I'll test each stage as thoroughly as possible before committing
the code.
The question is, when should I *push* the code to Leo's repo? Perhaps I
should wait until all stages are complete. We shall see...
Edward
--
You received this message because you are subscribed to the Google Groups
"leo-editor" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/leo-editor.
For more options, visit https://groups.google.com/d/optout.
