detached 9d3b2c7

fix(core): copy style cache fields in Frame.CopyFrom

After merging the page style cache, CopyFrom left the destination's cache
pointing at a slot that now holds a different style, so a later Set with the
old style reused the wrong ID. CopyFrom now copies styleCache/ID/OK together
with the style table. Adds a stale-cache regression test, a reflect-based
guard that fails when cellPage gains a field CopyFrom does not handle, and
RowsEqualAt for comparing differently indexed rows of two frames.
commit
9d3b2c754fc0202b06b5684acd4a2a6a980785d5
author
brice · · 2026-10-02 09:45 UTC
parent
caaf0b1
signature
GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email
2 files · +107 -3
Split
UnifiedSplit
M core/frame_copy.go +12 -3
@@ -32,6 +32,10 @@ func (f *Frame) CopyFrom(src Frame) {
3232 dst.styles = append(dst.styles[:0], from.styles...)
3333 dst.freeStyles = append(dst.freeStyles[:0], from.freeStyles...)
3434 dst.styleCount = from.styleCount
35+ // The cache refers to a slot in the style table copied above, so copying it
36+ // keeps it valid. Leaving the destination's old cache would point at a slot
37+ // that may now hold a different style.
38+ dst.styleCache, dst.styleCacheID, dst.styleCacheOK = from.styleCache, from.styleCacheID, from.styleCacheOK
3539 if dst.styleIndex == nil {
3640 dst.styleIndex = make(map[Style]uint32, len(from.styleIndex))
3741 } else {
@@ -60,13 +64,18 @@ func (f *Frame) CopyFrom(src Frame) {
6064 // require the two frames to share ID tables. Frames of different width, an
6165 // out-of-range row, or inconsistent storage compare unequal.
6266 func RowsEqual(a, b Frame, y int) bool {
67+ return RowsEqualAt(a, y, b, y)
68+}
69+
70+// RowsEqualAt is RowsEqual for logical row ya of a against logical row yb of b.
71+func RowsEqualAt(a Frame, ya int, b Frame, yb int) bool {
6372 if a.Width != b.Width || a.Width <= 0 || a.page == nil || b.page == nil ||
64- y < 0 || y >= a.Height || y >= b.Height ||
65- y >= len(a.page.rows) || y >= len(b.page.rows) {
73+ ya < 0 || ya >= a.Height || yb < 0 || yb >= b.Height ||
74+ ya >= len(a.page.rows) || yb >= len(b.page.rows) {
6675 return false
6776 }
6877 width := a.Width
69- offA, offB := int(a.page.rows[y]), int(b.page.rows[y])
78+ offA, offB := int(a.page.rows[ya]), int(b.page.rows[yb])
7079 if offA+width > len(a.page.cells) || offB+width > len(b.page.cells) {
7180 return false
7281 }
@@ -32,6 +32,10 @@ func (f *Frame) CopyFrom(src Frame) {
32 dst.styles = append(dst.styles[:0], from.styles...)32 dst.styles = append(dst.styles[:0], from.styles...)
33 dst.freeStyles = append(dst.freeStyles[:0], from.freeStyles...)33 dst.freeStyles = append(dst.freeStyles[:0], from.freeStyles...)
34 dst.styleCount = from.styleCount34 dst.styleCount = from.styleCount
35+ // The cache refers to a slot in the style table copied above, so copying it
36+ // keeps it valid. Leaving the destination's old cache would point at a slot
37+ // that may now hold a different style.
38+ dst.styleCache, dst.styleCacheID, dst.styleCacheOK = from.styleCache, from.styleCacheID, from.styleCacheOK
35 if dst.styleIndex == nil {39 if dst.styleIndex == nil {
36 dst.styleIndex = make(map[Style]uint32, len(from.styleIndex))40 dst.styleIndex = make(map[Style]uint32, len(from.styleIndex))
37 } else {41 } else {
@@ -60,13 +64,18 @@ func (f *Frame) CopyFrom(src Frame) {
60 // require the two frames to share ID tables. Frames of different width, an64 // require the two frames to share ID tables. Frames of different width, an
61 // out-of-range row, or inconsistent storage compare unequal.65 // out-of-range row, or inconsistent storage compare unequal.
62 func RowsEqual(a, b Frame, y int) bool {66 func RowsEqual(a, b Frame, y int) bool {
67+ return RowsEqualAt(a, y, b, y)
68+}
69+
70+// RowsEqualAt is RowsEqual for logical row ya of a against logical row yb of b.
71+func RowsEqualAt(a Frame, ya int, b Frame, yb int) bool {
63 if a.Width != b.Width || a.Width <= 0 || a.page == nil || b.page == nil ||72 if a.Width != b.Width || a.Width <= 0 || a.page == nil || b.page == nil ||
64- y < 0 || y >= a.Height || y >= b.Height ||73+ ya < 0 || ya >= a.Height || yb < 0 || yb >= b.Height ||
65- y >= len(a.page.rows) || y >= len(b.page.rows) {74+ ya >= len(a.page.rows) || yb >= len(b.page.rows) {
66 return false75 return false
67 }76 }
68 width := a.Width77 width := a.Width
69- offA, offB := int(a.page.rows[y]), int(b.page.rows[y])78+ offA, offB := int(a.page.rows[ya]), int(b.page.rows[yb])
70 if offA+width > len(a.page.cells) || offB+width > len(b.page.cells) {79 if offA+width > len(a.page.cells) || offB+width > len(b.page.cells) {
71 return false80 return false
72 }81 }
M core/frame_copy_test.go +95 -0
@@ -1,10 +1,16 @@
11 package core
22
33 import (
4+ "fmt"
45 "reflect"
56 "testing"
67 )
78
9+// cellPageFieldCount is the number of cellPage fields CopyFrom handles. Adding
10+// a field to cellPage fails TestCopyFromHandlesEveryPageField until CopyFrom
11+// (and this count) are updated.
12+const cellPageFieldCount = 13
13+
814 func copyTestPayload(t *testing.T, grapheme, link string) CellPayload {
915 t.Helper()
1016 p, err := NewCellPayload(grapheme, link)
@@ -202,3 +208,92 @@ func TestRowsEqualDoesNotAllocate(t *testing.T) {
202208 t.Fatalf("RowsEqual allocated %v times", n)
203209 }
204210 }
211+
212+func TestCopyFromHandlesEveryPageField(t *testing.T) {
213+ typ := reflect.TypeOf(cellPage{})
214+ if typ.NumField() != cellPageFieldCount {
215+ t.Fatalf("cellPage has %d fields, CopyFrom handles %d: update CopyFrom and cellPageFieldCount", typ.NumField(), cellPageFieldCount)
216+ }
217+
218+ // Build a source that populates every field, and a destination whose every
219+ // field holds different, stale state.
220+ src := copyTestFrame(t, 9, 5, 'a')
221+ src.Set(0, 0, Cell{Rune: 'a', Style: Style{Bold: true, Foreground: 7, Background: -1}})
222+ dst := copyTestFrame(t, 9, 5, 'k')
223+ dst.Set(4, 4, Cell{Rune: 'f', Style: Style{Italic: true, Foreground: 1, Background: -1}})
224+ dst.Set(4, 4, Cell{Rune: 'f', Style: DefaultStyle()}) // frees a style slot
225+ dst.CopyFrom(src)
226+
227+ sp, dp := reflect.ValueOf(*src.page), reflect.ValueOf(*dst.page)
228+ populated := 0
229+ for i := range typ.NumField() {
230+ name := typ.Field(i).Name
231+ a, b := sp.Field(i), dp.Field(i)
232+ if !a.IsZero() {
233+ populated++
234+ }
235+ if (a.Kind() == reflect.Slice || a.Kind() == reflect.Map) && a.Len() == 0 && b.Len() == 0 {
236+ continue // nil and empty are equivalent
237+ }
238+ // Printing works on unexported fields and treats nil and empty alike;
239+ // fmt sorts map keys, so the rendering is deterministic.
240+ if got, want := fmt.Sprintf("%#v", b), fmt.Sprintf("%#v", a); got != want {
241+ t.Errorf("cellPage.%s = %s, want %s", name, got, want)
242+ }
243+ }
244+ if populated < typ.NumField()-2 { // freeStyles/freePayloads may legitimately be empty
245+ t.Errorf("source populates only %d of %d page fields; strengthen the test", populated, typ.NumField())
246+ }
247+}
248+
249+func TestCopyFromDropsStaleStyleCache(t *testing.T) {
250+ styleA := Style{Bold: true, Foreground: 1, Background: -1}
251+ styleB := Style{Italic: true, Foreground: 2, Background: -1}
252+
253+ // dst: style A lives in slot k and is cached by the last Set.
254+ dst := NewFrame(4, 2)
255+ dst.Set(0, 0, Cell{Rune: 'a', Style: styleA})
256+ k := dst.page.styleCacheID
257+ if !dst.page.styleCacheOK || dst.page.styles[k].style != styleA {
258+ t.Fatalf("setup: cache = (%v,%d)", dst.page.styleCacheOK, k)
259+ }
260+
261+ // src: a different style occupies the same slot k, and no cache.
262+ src := NewFrame(4, 2)
263+ src.Set(0, 0, Cell{Rune: 'b', Style: styleB})
264+ src.page.styleCacheOK = false
265+ if src.page.styleIndex[styleB] != k {
266+ t.Fatalf("setup: style B in slot %d, want %d", src.page.styleIndex[styleB], k)
267+ }
268+
269+ dst.CopyFrom(src)
270+ if dst.page.styleCacheOK && dst.page.styles[dst.page.styleCacheID].style != dst.page.styleCache {
271+ t.Fatal("CopyFrom left a stale style cache")
272+ }
273+ cell := Cell{Rune: 'c', Style: styleA}
274+ dst.Set(1, 1, cell)
275+ if got := dst.Cell(1, 1); !got.Equal(cell) {
276+ t.Fatalf("cell = %+v, want %+v", got, cell)
277+ }
278+ if got := dst.Cell(0, 0); got.Style != styleB {
279+ t.Fatalf("copied cell style = %+v, want %+v", got.Style, styleB)
280+ }
281+ if err := dst.CheckInvariants(); err != nil {
282+ t.Fatal(err)
283+ }
284+}
285+
286+func TestRowsEqualAt(t *testing.T) {
287+ a := copyTestFrame(t, 10, 5, 'a')
288+ b := a.Clone()
289+ // b row y+1 shifted: compare a row y+1 against b row y after scrolling b.
290+ b.ScrollUp(0, 4, 1)
291+ for y := range 4 {
292+ if !RowsEqualAt(a, y+1, b, y) {
293+ t.Fatalf("row %d of a != row %d of scrolled b", y+1, y)
294+ }
295+ }
296+ if RowsEqualAt(a, 0, b, 0) || RowsEqualAt(a, 0, b, 5) || RowsEqualAt(a, -1, b, 0) {
297+ t.Fatal("unequal or out-of-range rows reported equal")
298+ }
299+}
@@ -1,10 +1,16 @@
1 package core1 package core
2 2
3 import (3 import (
4+ "fmt"
4 "reflect"5 "reflect"
5 "testing"6 "testing"
6 )7 )
7 8
9+// cellPageFieldCount is the number of cellPage fields CopyFrom handles. Adding
10+// a field to cellPage fails TestCopyFromHandlesEveryPageField until CopyFrom
11+// (and this count) are updated.
12+const cellPageFieldCount = 13
13+
8 func copyTestPayload(t *testing.T, grapheme, link string) CellPayload {14 func copyTestPayload(t *testing.T, grapheme, link string) CellPayload {
9 t.Helper()15 t.Helper()
10 p, err := NewCellPayload(grapheme, link)16 p, err := NewCellPayload(grapheme, link)
@@ -202,3 +208,92 @@ func TestRowsEqualDoesNotAllocate(t *testing.T) {
202 t.Fatalf("RowsEqual allocated %v times", n)208 t.Fatalf("RowsEqual allocated %v times", n)
203 }209 }
204 }210 }
211+
212+func TestCopyFromHandlesEveryPageField(t *testing.T) {
213+ typ := reflect.TypeOf(cellPage{})
214+ if typ.NumField() != cellPageFieldCount {
215+ t.Fatalf("cellPage has %d fields, CopyFrom handles %d: update CopyFrom and cellPageFieldCount", typ.NumField(), cellPageFieldCount)
216+ }
217+
218+ // Build a source that populates every field, and a destination whose every
219+ // field holds different, stale state.
220+ src := copyTestFrame(t, 9, 5, 'a')
221+ src.Set(0, 0, Cell{Rune: 'a', Style: Style{Bold: true, Foreground: 7, Background: -1}})
222+ dst := copyTestFrame(t, 9, 5, 'k')
223+ dst.Set(4, 4, Cell{Rune: 'f', Style: Style{Italic: true, Foreground: 1, Background: -1}})
224+ dst.Set(4, 4, Cell{Rune: 'f', Style: DefaultStyle()}) // frees a style slot
225+ dst.CopyFrom(src)
226+
227+ sp, dp := reflect.ValueOf(*src.page), reflect.ValueOf(*dst.page)
228+ populated := 0
229+ for i := range typ.NumField() {
230+ name := typ.Field(i).Name
231+ a, b := sp.Field(i), dp.Field(i)
232+ if !a.IsZero() {
233+ populated++
234+ }
235+ if (a.Kind() == reflect.Slice || a.Kind() == reflect.Map) && a.Len() == 0 && b.Len() == 0 {
236+ continue // nil and empty are equivalent
237+ }
238+ // Printing works on unexported fields and treats nil and empty alike;
239+ // fmt sorts map keys, so the rendering is deterministic.
240+ if got, want := fmt.Sprintf("%#v", b), fmt.Sprintf("%#v", a); got != want {
241+ t.Errorf("cellPage.%s = %s, want %s", name, got, want)
242+ }
243+ }
244+ if populated < typ.NumField()-2 { // freeStyles/freePayloads may legitimately be empty
245+ t.Errorf("source populates only %d of %d page fields; strengthen the test", populated, typ.NumField())
246+ }
247+}
248+
249+func TestCopyFromDropsStaleStyleCache(t *testing.T) {
250+ styleA := Style{Bold: true, Foreground: 1, Background: -1}
251+ styleB := Style{Italic: true, Foreground: 2, Background: -1}
252+
253+ // dst: style A lives in slot k and is cached by the last Set.
254+ dst := NewFrame(4, 2)
255+ dst.Set(0, 0, Cell{Rune: 'a', Style: styleA})
256+ k := dst.page.styleCacheID
257+ if !dst.page.styleCacheOK || dst.page.styles[k].style != styleA {
258+ t.Fatalf("setup: cache = (%v,%d)", dst.page.styleCacheOK, k)
259+ }
260+
261+ // src: a different style occupies the same slot k, and no cache.
262+ src := NewFrame(4, 2)
263+ src.Set(0, 0, Cell{Rune: 'b', Style: styleB})
264+ src.page.styleCacheOK = false
265+ if src.page.styleIndex[styleB] != k {
266+ t.Fatalf("setup: style B in slot %d, want %d", src.page.styleIndex[styleB], k)
267+ }
268+
269+ dst.CopyFrom(src)
270+ if dst.page.styleCacheOK && dst.page.styles[dst.page.styleCacheID].style != dst.page.styleCache {
271+ t.Fatal("CopyFrom left a stale style cache")
272+ }
273+ cell := Cell{Rune: 'c', Style: styleA}
274+ dst.Set(1, 1, cell)
275+ if got := dst.Cell(1, 1); !got.Equal(cell) {
276+ t.Fatalf("cell = %+v, want %+v", got, cell)
277+ }
278+ if got := dst.Cell(0, 0); got.Style != styleB {
279+ t.Fatalf("copied cell style = %+v, want %+v", got.Style, styleB)
280+ }
281+ if err := dst.CheckInvariants(); err != nil {
282+ t.Fatal(err)
283+ }
284+}
285+
286+func TestRowsEqualAt(t *testing.T) {
287+ a := copyTestFrame(t, 10, 5, 'a')
288+ b := a.Clone()
289+ // b row y+1 shifted: compare a row y+1 against b row y after scrolling b.
290+ b.ScrollUp(0, 4, 1)
291+ for y := range 4 {
292+ if !RowsEqualAt(a, y+1, b, y) {
293+ t.Fatalf("row %d of a != row %d of scrolled b", y+1, y)
294+ }
295+ }
296+ if RowsEqualAt(a, 0, b, 0) || RowsEqualAt(a, 0, b, 5) || RowsEqualAt(a, -1, b, 0) {
297+ t.Fatal("unequal or out-of-range rows reported equal")
298+ }
299+}

Keyboard shortcuts

Single-key shortcuts are on. ctrl k and esc work whatever this says.

ctrl k :
go to a section, branch, tag, file or repository
?
this list
j k
move the row cursor down / up
enter
open the row under the cursor; on Files, a directory opens or closes in place
↑ ↓ ← → home end
in the file tree (focus on a row): previous / next row, close / open a directory or go to its parent / first entry, first / last row. They never open a file
l h
on Files: open a directory or a file / close a directory or go up to its parent, then to the parent page. Elsewhere: into the side panel (on a merge request, its files) / back to the content; there j k move between its controls
g g G
first / last row
/
filter the list; on Files, find a file anywhere in the repository (esc clears, enter opens the best match)
b
switch branch or tag
g s g f g c g b g m g i g r g a
tabs: summary, files, commits, branches & tags, merge requests, issues, releases, actions (when available)
g h
home
w
wide mode: use the whole width of the screen, or not (remembered)
y
copy a permalink pinned to the commit
d u x
diff: split / unified / collapse or expand all
e E
diff: the whole file in view / every file, with the lines the hunks hide
n
merge request: next note in the diff
esc
close a dialog, leave a field

Branches 2

Tags 13

↑↓ moveenter openesc close