From d1621dca89d49b047348971fb9a682b232e19a26 Mon Sep 17 00:00:00 2001 From: Eugene Blikh Date: Wed, 5 Aug 2026 04:23:08 +0300 Subject: [PATCH] fix(prosediff): an equal run's separator comes from whichever side has one (spec-by6.5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit spans() read Span.Space off toks[0], and for an equal run those tokens come from the old side. An equal run exists in both revisions at once, and the two sides can disagree about what precedes it: a block that gains words at its head has nothing before its first old token and the insertion before its first new one. The script then said Space=false and a renderer joining the spans wrote "{+in practice+}the storage layer" with the words run together — a defect the reader would read as the author's, because nothing in the output says a separator went missing. Not reachable from the review page today: web/diffrows.go's sideSpans already carries a dropped span's separator onto the next kept one, and the per-line merge never joins two spans across that boundary. It was wrong in the data all the same, and the next consumer of the script would have inherited it. Passing the flag into emit rather than deriving it also drops the write-back that reached into out[len(out)-1] to clear a substitution's separator, which would have edited the wrong span had emit ever skipped an empty run. --- prosediff/token.go | 26 +++++++++++++++++--------- prosediff/token_test.go | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 9 deletions(-) diff --git a/prosediff/token.go b/prosediff/token.go index e65c69cf647b8f058be9b80e813ad96ac08befb7..dc2f4eb85cfbe8781e14b1d7d2c55efcb64dda09 100644 --- a/prosediff/token.go +++ b/prosediff/token.go @@ -199,7 +199,15 @@ func deletesFirst(script []edit) []edit { // spans walks an edit script and materializes it into renderable runs. func spans(script []edit, a, b []Token) []Span { var out []Span - emit := func(op Op, toks []Token) { + // space is passed in rather than read off toks[0] because an equal run + // exists on both sides at once and the two sides can disagree about the + // separator in front of it. A block that gains words at its head has + // Space=false on the old side's first token — nothing precedes it there — + // and Space=true on the new side's, where the inserted words do. Reading + // only the old side emitted "{+two words+}the rest" with the words run + // together; a renderer has no way to recover the separator, because the new + // side's flag never reached it. + emit := func(op Op, toks []Token, space bool) { if len(toks) == 0 { return } @@ -210,7 +218,7 @@ func spans(script []edit, a, b []Token) []Span { } sb.WriteString(t.Text) } - out = append(out, Span{Op: op, Text: sb.String(), Space: toks[0].Space}) + out = append(out, Span{Op: op, Text: sb.String(), Space: space}) } i, j := 0, 0 @@ -220,23 +228,23 @@ func spans(script []edit, a, b []Token) []Span { // the whitespace the deletion already carried, or every one-word // substitution renders as "[-old-] {+new+}". if e.op == OpInsert && k > 0 && script[k-1].op == OpDelete { - emit(OpInsert, b[j:j+e.n]) - if len(out) > 0 { - out[len(out)-1].Space = false - } + emit(OpInsert, b[j:j+e.n], false) j += e.n continue } switch e.op { case OpEqual: - emit(OpEqual, a[i:i+e.n]) + // Either side's separator is reason enough to emit one: the run is + // rendered once, between whatever precedes it on the old side and + // whatever precedes it on the new. + emit(OpEqual, a[i:i+e.n], a[i].Space || b[j].Space) i += e.n j += e.n case OpDelete: - emit(OpDelete, a[i:i+e.n]) + emit(OpDelete, a[i:i+e.n], a[i].Space) i += e.n case OpInsert: - emit(OpInsert, b[j:j+e.n]) + emit(OpInsert, b[j:j+e.n], b[j].Space) j += e.n } } diff --git a/prosediff/token_test.go b/prosediff/token_test.go index 439e388553b8db519ff8b21f610620d80a5d727f..375cd636b7d971e518ef698c8d764a19d2582dc2 100644 --- a/prosediff/token_test.go +++ b/prosediff/token_test.go @@ -128,6 +128,38 @@ func TestDiffWordsAbsorbsShredding(t *testing.T) { assert.Equal(t, "agents emit, operators approve", spans[1].Text) } +// TestEqualRunAfterAHeadInsertionKeepsItsSeparator checks the one place the two +// sides of a diff disagree about whitespace. An equal run exists in both +// revisions at once; when words are inserted before it, the old side's first +// token has nothing in front of it and the new side's has the insertion. Reading +// only the old side dropped the separator, and a renderer joining the spans +// wrote "{+two words+}the rest" with no space — a defect the reader would read +// as the author's, since nothing in the output says a space went missing. +func TestEqualRunAfterAHeadInsertionKeepsItsSeparator(t *testing.T) { + spans := DiffWords( + "the storage layer keeps every revision", + "in practice the storage layer keeps every revision", + ) + require.Len(t, spans, 2) + assert.Equal(t, OpInsert, spans[0].Op) + assert.Equal(t, "in practice", spans[0].Text) + assert.False(t, spans[0].Space, "nothing precedes the first span") + assert.Equal(t, OpEqual, spans[1].Op) + assert.True(t, spans[1].Space, "the inserted words are separated from the rest") +} + +// TestSubstitutionStillCarriesNoDoubleSeparator guards the rule the fix above +// must not undo: an insertion directly replacing a deletion inherits the +// deletion's separator rather than announcing its own. +func TestSubstitutionStillCarriesNoDoubleSeparator(t *testing.T) { + spans := DiffWords("the quick brown fox", "the quick red fox") + require.Len(t, spans, 4) + assert.Equal(t, OpDelete, spans[1].Op) + assert.True(t, spans[1].Space) + assert.Equal(t, OpInsert, spans[2].Op) + assert.False(t, spans[2].Space) +} + func TestDiffLinesIsWhitespaceSensitive(t *testing.T) { old := []string{"func main() {", "\tfmt.Println(1)", "}"} nw := []string{"func main() {", " fmt.Println(1)", "}"}