Add the line-chunked diff (CRDT step 7) - #138
Open
pedersen wants to merge 1 commit into
Open
Conversation
Turns an externally edited file back into a minimal set of operations without handing a whole note to myersDiff. Minimality is the rule this exists to keep. Deleting everything and inserting the new text converges and passes a two-replica test, and is catastrophically wrong: it tombstones every element another device might concurrently be editing, so every concurrent remote insertion is discarded. Nothing about it looks broken until a second device exists, which is why the test that matters here builds two replicas and asserts B's insertion survives A's reconcile. But myersDiff cannot be handed the note either. It trims the common prefix and suffix and then runs with no size guard, snapshotting the frontier once per edit-distance step — memory proportional to the product of the edit distance and the text length. The trigger is a dispersed edit rather than a large note, and the ordinary one is a line-ending change: every line differs, prefix trimming stops at the first terminator, and the edit distance becomes the line count. So lines are aligned first and myersDiff is called only within a changed region. Lines are keyed on their content with the terminator excluded, which is what makes the motivating case cheap rather than merely smaller: a CRLF round-trip aligns every line, and each refinement then sees one line instead of the note. A 500-line file changing line endings produces a script that touches 499 characters. Line keys are assigned below the surrogate block, so a key is never half a pair, and they wrap rather than overflow — a collision costs alignment quality and never correctness, because every aligned pair is still compared in full and refined if it differs. The script is computed in full before the first mutation, and that ordering is what keeps a note from being left half edited. runInTransaction cannot provide it: its commit runs in a finally, so a throw part-way still flushes what was registered. What the transaction does provide is batching — the changes are created together at commit and surface as one update notification — and the tests now pin that rather than an atomicity the library does not offer. Attribution is honest by construction: synthesized operations carry this device's peerID, because we genuinely do not know who made the external edit. No user-facing change; docs/manual-test-plan.md is untouched, with no cases added, changed, or invalidated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Coverage after merging worktree-crdt-line-chunked-diff into main will be
Coverage Report |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Turns an externally edited file back into a minimal set of operations without
handing a whole note to
myersDiff. The plan asks for this one to be readclosely, so the two things I'd most want checked are called out below.
The algorithm
Lines are aligned first;
myersDiffis called only within a changed region.The part that makes the motivating case cheap rather than merely smaller:
lines are keyed on their content with the terminator excluded. A CRLF
round-trip then aligns every line, and each refinement sees one line instead of
the note. If lines were keyed with their terminators, every line would differ,
alignment would find nothing, and we would be back to a whole-note diff — the
6.4 GB case.
Concretely: a 500-line file changing line endings produces a script that
touches 499 characters, one per carriage return.
Line keys are assigned below the surrogate block, so a key is never half a
pair, and they wrap rather than overflow. A collision costs alignment quality
and never correctness — every aligned pair is compared in full and refined if
it differs, which is the same path a terminator change already takes.
A removal immediately followed by an insertion is treated as one changed region
and refined together, so a modified block does not become delete-all +
insert-all at region scale.
Two things worth your attention
1.
runInTransactiondoes not do what Decision 6 says it does.The design states: "Without it, reconciliation is not atomic, and a crash
partway through leaves a half-applied edit that Decision 5's write ordering
assumes cannot exist."
That isn't what the library provides.
TransactionManager.runcallscommit()in a
finally, so a throw part-way through the script still flushes theoperations already registered — there is no rollback. It also does not merge
the script into a single
Change; two replaced lines are still fouroperations.
What it does provide is batching: the changes are created together at commit
and surface as one update notification, so nothing downstream observes a
note mid-reconcile. The tests now pin that, and the doc comment says it
plainly.
The "never half applied" property is real, but it comes from computing the
whole script before the first mutation, not from the transaction. That's
tested directly.
I have not amended the design — it's
accepted, and this is your call. If youwant Decision 6's wording corrected to match, say so and I'll do it as a
separate docs PR.
2. The two-replica test is the one that matters.
device B's insertion is not discarded by A's reconcileis the test asingle-replica suite cannot write, and the one a replace-all implementation
passes convergence on while silently failing. There are three variants: an
appended line, an edit inside the same line, and a line-ending reconcile
running concurrently with an edit.
Tests
41 tests. Beyond the plan's named cases:
the script applied to the old text reproduces the new one exactly. Offsets
are the fragile part here: every edit is expressed against the old text, so a
wrong shift produces a plausible-looking script that reconstructs the wrong
string. This is what caught that class of bug during development.
trailing-whitespace stripping, asserted by counting the code units the script
touches rather than by timing.
a note that is entirely astral characters; and a 200-line emoji file changing
line endings, which is where a naive line key would split a surrogate pair.
insertion costs one character.
line_chunked_diff.dartat 100% line coverage; tree at 98.61%.No user-facing change, so
docs/manual-test-plan.mdis untouched: no casesadded, changed, or invalidated.
🤖 Generated with Claude Code