From 4f5bbb08d59d0b7eeda976c9136751215c5758a7 Mon Sep 17 00:00:00 2001 From: Redowan Delowar Date: Sun, 25 Oct 2026 10:56:45 +0100 Subject: [PATCH 1/8] perf: keep syntax colours between renders A sideways scroll renders the diff again, and each render highlighted every line again with chroma. diff.Cache keeps the highlighted lines, and the tui keeps one Cache for all renders. On a 5,000-line file, a scroll key takes 8 ms in place of 20 ms with the folds closed, and 11 ms in place of 180 ms in the Files tab. --- .gitignore | 2 + diff/cache_test.go | 20 ++++++++ diff/diff.go | 14 ++++++ diff/diff_test.go | 31 +++++++++++++ diff/file.go | 20 ++++++-- internal/VENDORED.md | 4 +- internal/diffview/cache_test.go | 53 +++++++++++++++++++++ internal/diffview/diffview.go | 81 +++++++++++++++++++++++---------- tui/bench_test.go | 61 +++++++++++++++++++++++++ tui/model.go | 1 + tui/render.go | 1 + 11 files changed, 259 insertions(+), 29 deletions(-) create mode 100644 diff/cache_test.go create mode 100644 internal/diffview/cache_test.go diff --git a/.gitignore b/.gitignore index f8be09c..d2fa71f 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,5 @@ /rexdiff /.pi/ /dist/ +*.test +*.out diff --git a/diff/cache_test.go b/diff/cache_test.go new file mode 100644 index 0000000..7554cfd --- /dev/null +++ b/diff/cache_test.go @@ -0,0 +1,20 @@ +package diff + +import ( + "strings" + "testing" +) + +// TestCacheKeepsFile checks that RenderFile uses the colours that the cache +// keeps for the same file, and colours a changed file again. +func TestCacheKeepsFile(t *testing.T) { + var c Cache + RenderFile("a.go", "x := 1\n", Options{Cache: &c}) + c.file.out = "kept" + if got := RenderFile("a.go", "x := 1\n", Options{Cache: &c}).Lines[0]; !strings.Contains(got, "kept") { + t.Errorf("the same file shows %q, want the kept colours", got) + } + if got := RenderFile("a.go", "x := 2\n", Options{Cache: &c}).Lines[0]; strings.Contains(got, "kept") { + t.Errorf("a changed file shows %q, the colours of the old file", got) + } +} diff --git a/diff/diff.go b/diff/diff.go index 1050453..14c6ea3 100644 --- a/diff/diff.go +++ b/diff/diff.go @@ -83,6 +83,17 @@ type Options struct { Signs bool // Theme sets the colours. Theme Theme + // Cache keeps the syntax colours between renders. If Cache is nil, the + // render keeps nothing. + Cache *Cache +} + +// Cache keeps the syntax colours of the lines, so that a new render of the +// same lines is fast, for example after a sideways scroll. The zero value +// is an empty Cache. A Cache is not safe for concurrent use. +type Cache struct { + lines diffview.Cache // the lines of Render + file struct{ path, src, out string } // the last file of RenderFile } // tabWidth is the number of spaces that replace a tab. @@ -110,6 +121,9 @@ func Render(path, old, new string, o Options) *Diff { Width(o.Width). TabWidth(tabWidth). XOffset(o.Offset) + if o.Cache != nil { + dv = dv.Cache(&o.Cache.lines) + } if o.Expanded != nil { dv = dv.Expand(func(f diffview.Fold) bool { return o.Expanded(foldOf(f)) }) } diff --git a/diff/diff_test.go b/diff/diff_test.go index f21da29..c963af7 100644 --- a/diff/diff_test.go +++ b/diff/diff_test.go @@ -348,3 +348,34 @@ func TestWrap(t *testing.T) { } } } + +// TestCache checks that a shared [diff.Cache] gives the same lines as no +// cache: for other offsets, for other languages, and for new content. +func TestCache(t *testing.T) { + const src = "def f(): # a comment\n\treturn 1\n" + renders := []struct { + path, old, new string + file bool // RenderFile, of new + offset int + }{ + {path: "a.go", old: src, new: src + "x\n"}, + {path: "a.go", old: src, new: src + "x\n", offset: 4}, + {path: "a.py", old: src, new: src + "x\n"}, + {path: "a.py", new: src, file: true}, + {path: "a.py", new: src + "y\n", file: true}, + {path: "a.go", new: src + "y\n", file: true}, + } + var c diff.Cache + for _, r := range renders { + render := func(c *diff.Cache) []string { + o := diff.Options{Width: 40, Offset: r.offset, Cache: c} + if r.file { + return diff.RenderFile(r.path, r.new, o).Lines + } + return diff.Render(r.path, r.old, r.new, o).Lines + } + if got, want := render(&c), render(nil); !slices.Equal(got, want) { + t.Errorf("%+v: with the cache\n%q\nwant\n%q", r, got, want) + } + } +} diff --git a/diff/file.go b/diff/file.go index 1d53ab2..22c780e 100644 --- a/diff/file.go +++ b/diff/file.go @@ -26,7 +26,7 @@ func RenderFile(path, src string, o Options) *Diff { "\r", "\u240d", "\t", strings.Repeat(" ", tabWidth), ).Replace(src) - hl := strings.Split(highlight(path, src), "\n") + hl := strings.Split(o.Cache.highlight(path, src), "\n") hl = hl[:min(len(hl), len(Lines(src)))] // chroma can add a line break at the end digits := len(strconv.Itoa(len(hl))) numWidth := digits + 2 // a space on each side of the number @@ -74,9 +74,21 @@ func fileLineParts(l string, code int, o Options) []string { return []string{ansi.TruncateLeft(l, o.Offset, "")} } -// highlight colours the whole file at once, so that tokens that span lines -// get the correct colour. If it cannot highlight src, it returns src with -// its control characters replaced. +// highlight returns highlight(path, src), and keeps it for the next call. +// A nil c keeps nothing. +func (c *Cache) highlight(path, src string) string { + if c == nil { + return highlight(path, src) + } + if f := &c.file; f.path != path || f.src != src { + f.path, f.src, f.out = path, src, highlight(path, src) + } + return c.file.out +} + +// highlight colours src, the whole file at path, at once, so that tokens +// that span lines get the correct colour. If it cannot highlight src, it +// returns src with its control characters replaced. func highlight(path, src string) string { it, err := diffview.Lexer(path, src).Tokenise(nil, src) if err != nil { diff --git a/internal/VENDORED.md b/internal/VENDORED.md index 89fca48..849abae 100644 --- a/internal/VENDORED.md +++ b/internal/VENDORED.md @@ -37,4 +37,6 @@ long runs of equal lines, as in herdr-reviewr; `Expand` shows a fold, and `Row.Fold` tells which lines a hunk header hides - the syntax cache key tells "no background" from black, so a black tint does not - leak onto equal lines + leak onto equal lines, and it has the lexer name + - the syntax cache is a `Cache` that views can share (`View.Cache`), so that a new + render does not highlight the same lines again; a `Cache` of 32 MB empties diff --git a/internal/diffview/cache_test.go b/internal/diffview/cache_test.go new file mode 100644 index 0000000..6725e2e --- /dev/null +++ b/internal/diffview/cache_test.go @@ -0,0 +1,53 @@ +package diffview + +import ( + "strings" + "testing" +) + +// TestCacheShared checks that a View uses the lines that another View +// highlighted into the same Cache. +func TestCacheShared(t *testing.T) { + render := func(c *Cache) string { + out, _, _ := New().Cache(c).Before("a.go", "x := 1\n").After("a.go", "x := 2\n").Render() + return out + } + c := &Cache{} + render(c) + if len(c.lines) == 0 { + t.Fatal("the cache is empty after a render") + } + for k := range c.lines { + c.lines[k] = "kept" + } + if got := render(c); strings.Count(got, "kept") != 2 { + t.Errorf("a View with the same cache shows\n%s\nwant the two kept lines", got) + } + if got := render(&Cache{}); strings.Contains(got, "kept") { + t.Errorf("a View with a new cache shows\n%s\nwant no kept lines", got) + } +} + +// TestCacheLimit checks that a full Cache empties, so that its memory stays +// small. +func TestCacheLimit(t *testing.T) { + var c Cache + line := strings.Repeat("x", 1<<20) + for i := range maxCacheSize >> 20 { + c.put(syntaxKey{r: uint32(i)}, line) + } + if got, want := len(c.lines), maxCacheSize>>20; got != want { + t.Errorf("the cache has %d lines, want %d", got, want) + } + c.put(syntaxKey{source: "a"}, "") + if len(c.lines) != 1 || c.size != 1 { + t.Errorf("the full cache has %d lines of %d bytes after a put, want 1 of 1", len(c.lines), c.size) + } +} + +// TestCacheNil checks that a View with a nil Cache renders. +func TestCacheNil(t *testing.T) { + if out, _, _ := New().Cache(nil).Before("a.go", "x\n").After("a.go", "y\n").Render(); out == "" { + t.Error("the render is empty") + } +} diff --git a/internal/diffview/diffview.go b/internal/diffview/diffview.go index 8387f48..6d76066 100644 --- a/internal/diffview/diffview.go +++ b/internal/diffview/diffview.go @@ -74,20 +74,52 @@ type View struct { // The View keeps the lexer, because to match a file name is slow. cachedLexer chroma.Lexer - // syntaxCache keeps the highlighted lines, so that the View highlights - // each equal line once. - syntaxCache map[syntaxKey]string + cache *Cache // the highlighted lines +} + +// Cache keeps highlighted lines, so that chroma highlights each line once. +// Views that share a Cache share the work. The zero value is an empty +// Cache. A Cache is not safe for concurrent use. +type Cache struct { + lines map[syntaxKey]string + size int // the bytes of the lines and their sources +} + +// maxCacheSize limits the memory of a Cache, in bytes. A full Cache +// empties. +const maxCacheSize = 32 << 20 + +// put keeps the highlighted line s for key k. +func (c *Cache) put(k syntaxKey, s string) { + // A source is part of the whole file. A copy lets the file go. + k.source = strings.Clone(k.source) + n := len(k.source) + len(s) + if c.lines == nil || c.size+n > maxCacheSize { + c.lines, c.size = make(map[syntaxKey]string), 0 + } + c.lines[k] = s + c.size += n } // New returns a View with the unified layout and a tab width of 8. func New() *View { return &View{ - layout: layoutUnified, - tabWidth: defaultTabWidth, - syntaxCache: make(map[syntaxKey]string), + layout: layoutUnified, + tabWidth: defaultTabWidth, + cache: &Cache{}, } } +// Cache sets the cache of the highlighted lines. A nil c gives the View a +// new Cache. +func (v *View) Cache(c *Cache) *View { + if c == nil { + c = &Cache{} + } + v.cache = c + return v +} + // Split sets the split layout: the old lines on the left, the new lines on // the right. func (v *View) Split() *View { @@ -109,11 +141,10 @@ func (v *View) After(path, content string) *View { return v } -// clearCaches clears the lexer, the syntax cache, and the diff. Call it when -// the content changes. +// clearCaches clears the lexer and the diff. Call it when the content +// changes. func (v *View) clearCaches() { v.cachedLexer = nil - clear(v.syntaxCache) v.isComputed = false } @@ -526,16 +557,17 @@ func hunkLineCounts(h *udiff.Hunk) (before, after int) { } // highlightCode returns source with syntax colours on the background -// bgColor. The syntax cache keeps the result, so highlightCode highlights -// each equal line once. If chroma fails, it returns source as it is. +// bgColor. The cache keeps the result. If chroma fails, it returns source +// as it is. func (v *View) highlightCode(source string, bgColor color.Color) string { // Chroma makes a lone CR a line break, so show it as a symbol first. source = strings.ReplaceAll(source, "\r", "\u240d") - key := newSyntaxKey(source, bgColor) - if cached, ok := v.syntaxCache[key]; ok { + lexer := v.chromaLexer() + key := newSyntaxKey(lexer.Config().Name, source, bgColor) + if cached, ok := v.cache.lines[key]; ok { return cached } - it, err := v.chromaLexer().Tokenise(nil, source) + it, err := lexer.Tokenise(nil, source) if err != nil { return source } @@ -545,22 +577,23 @@ func (v *View) highlightCode(source string, bgColor color.Color) string { return source } out := b.String() - v.syntaxCache[key] = out + v.cache.put(key, out) return out } -// syntaxKey is the key of the syntax cache: the source and its background -// colour. noColor is true for no background, because lipgloss.NoColor has -// the same RGBA value as black. +// syntaxKey is the key of a [Cache]: the lexer, the source, and its +// background colour. noColor is true for no background, because +// lipgloss.NoColor has the same RGBA value as black. type syntaxKey struct { - source string - noColor bool - r, g, b, a uint32 + lexer, source string + noColor bool + r, g, b, a uint32 } -// newSyntaxKey returns the cache key of source on the background bgColor. -func newSyntaxKey(source string, bgColor color.Color) syntaxKey { - k := syntaxKey{source: source} +// newSyntaxKey returns the cache key of source, highlighted with the lexer +// of that name on the background bgColor. +func newSyntaxKey(lexer, source string, bgColor color.Color) syntaxKey { + k := syntaxKey{lexer: lexer, source: source} if _, none := bgColor.(lipgloss.NoColor); bgColor == nil || none { k.noColor = true return k diff --git a/tui/bench_test.go b/tui/bench_test.go index 166f6d2..65df786 100644 --- a/tui/bench_test.go +++ b/tui/bench_test.go @@ -1,10 +1,15 @@ package tui import ( + "fmt" + "slices" "strings" "testing" tea "charm.land/bubbletea/v2" + + "github.com/rednafi/rexdiff/diff" + "github.com/rednafi/rexdiff/vcs" ) // BenchmarkReview reloads a large change and scrolls through it, as a user @@ -34,3 +39,59 @@ func BenchmarkReview(b *testing.B) { } } } + +// BenchmarkScroll scrolls a large change sideways: in the Changes tab with +// the folds closed and with the folds open, and in the Files tab. +// +// go test ./tui -run '^$' -bench Scroll +func BenchmarkScroll(b *testing.B) { + for _, name := range []string{"folded", "open", "file"} { + b.Run(name, func(b *testing.B) { + m := bigChange(b) + switch name { + case "open": + for { + i := slices.IndexFunc(m.changes.rows, func(r diff.Row) bool { return r.Fold.Lines > 0 }) + if i < 0 { + break + } + m.expandFold(m.changes.rows[i].Fold) + } + case "file": + m.setTab(tabFiles) + m.allSel = slices.IndexFunc(m.allFiles, func(e vcs.Entry) bool { return e.Path == "big.go" }) + m.renderViewer() + } + right := tea.KeyPressMsg{Code: tea.KeyRight} + left := tea.KeyPressMsg{Code: tea.KeyLeft} + for b.Loop() { + m = step(b, m, right) + _ = m.View() + m = step(b, m, left) + _ = m.View() + } + }) + } +} + +// bigChange returns a model that shows a change to big.go, a file of 5,000 +// long lines. The change edits each 100th line. +func bigChange(b *testing.B) model { + m, dir := newTestModel(b) + var old, cur strings.Builder + for i := range 5000 { + l := fmt.Sprintf("\tx%d := compute(%q, %d) // a long line with words\n", i, strings.Repeat("a", i%40), i*7) + old.WriteString(l) + if i%100 == 0 { + l = "\ty := 1\n" + } + cur.WriteString(l) + } + write(b, dir, "big.go", "package a\nfunc f() {\n"+old.String()+"}\n") + git(b, dir, "add", ".") + git(b, dir, "commit", "-qm", "big") + write(b, dir, "big.go", "package a\nfunc f() {\n"+cur.String()+"}\n") + m.reload() + m.pickFile(slices.IndexFunc(m.files, func(f changed) bool { return f.Path == "big.go" })) + return m +} diff --git a/tui/model.go b/tui/model.go index 2d902e0..2f331f0 100644 --- a/tui/model.go +++ b/tui/model.go @@ -220,6 +220,7 @@ type model struct { listHidden bool focusList bool // the keys j and k move in the file list listW int + cache diff.Cache // the syntax colours, so that a sideways scroll is fast comments diff.Comments editor textarea.Model diff --git a/tui/render.go b/tui/render.go index e3deab7..71a4b40 100644 --- a/tui/render.go +++ b/tui/render.go @@ -86,6 +86,7 @@ func (m *model) diffOptions(xoff int) diff.Options { Green: m.green, Red: m.red, }, + Cache: &m.cache, } } From a2e2412ef2f1dbcb83b1fece97922c0e3e9e16e6 Mon Sep 17 00:00:00 2001 From: Redowan Delowar Date: Sun, 25 Oct 2026 11:11:04 +0100 Subject: [PATCH 2/8] fix(tui): open the editor near a deleted line On a deleted line, e opened the file at line 1. It now opens at the first new line below the cursor, else the last new line above it. --- tui/comment.go | 24 +++++++++++++++++++----- tui/editor_test.go | 20 ++++++++++++++++++++ 2 files changed, 39 insertions(+), 5 deletions(-) diff --git a/tui/comment.go b/tui/comment.go index 7561a5d..7ad4981 100644 --- a/tui/comment.go +++ b/tui/comment.go @@ -1,6 +1,7 @@ package tui import ( + "slices" "strings" "charm.land/bubbles/v2/key" @@ -149,6 +150,23 @@ func (m *model) closeComment() { m.editor.Blur() } +// newLineNear returns the new line of row i. A row with no new line, for +// example a deleted line, gives the first new line below it, else the last +// new line above it, else 1. +func newLineNear(rows []diff.Row, i int) int { + for _, r := range rows[i:] { + if r.New > 0 { + return r.New + } + } + for _, r := range slices.Backward(rows[:i]) { + if r.New > 0 { + return r.New + } + } + return 1 +} + // editOrOpen edits the comment at the cursor. If there is no comment, it // opens the file in an editor at the cursor line. func (m *model) editOrOpen() tea.Cmd { @@ -159,9 +177,5 @@ func (m *model) editOrOpen() tea.Cmd { if p.path == "" { return nil } - line := 1 - if t := m.target(p.cur, diff.SideNew); t != nil && t.side == diff.SideNew { - line = t.line - } - return m.openEditor(p.path, line) + return m.openEditor(p.path, newLineNear(p.rows, p.cur)) } diff --git a/tui/editor_test.go b/tui/editor_test.go index 9261b64..795803e 100644 --- a/tui/editor_test.go +++ b/tui/editor_test.go @@ -8,6 +8,7 @@ import ( tea "charm.land/bubbletea/v2" + "github.com/rednafi/rexdiff/diff" "github.com/rednafi/rexdiff/rex" ) @@ -184,3 +185,22 @@ func TestEditorError(t *testing.T) { }) } } + +// TestNewLineNear checks the line that e opens for each kind of row. +func TestNewLineNear(t *testing.T) { + rows := []diff.Row{ + {Kind: diff.KindHunk}, + {Kind: diff.KindEqual, Old: 1, New: 1}, + {Kind: diff.KindDelete, Old: 2}, + {Kind: diff.KindInsert, New: 2}, + {Kind: diff.KindDelete, Old: 3}, + } + for i, want := range []int{1, 1, 2, 2, 2} { + if got := newLineNear(rows, i); got != want { + t.Errorf("newLineNear(row %d) = %d, want %d", i, got, want) + } + } + if got := newLineNear(rows[2:3], 0); got != 1 { + t.Errorf("newLineNear(a lone deleted line) = %d, want 1", got) + } +} From 77a63422bbee8d25748dcddd8b6485285b1a0404 Mon Sep 17 00:00:00 2001 From: Redowan Delowar Date: Sun, 25 Oct 2026 11:22:48 +0100 Subject: [PATCH 3/8] perf(diffview): keep the last lexer in the cache A file with an unknown name gets its lexer from the content, and that scan took 30 ms on each render of a 5,000-line file. Also simplify the cache comments and the benchmark. --- diff/diff.go | 9 ++++----- diff/file.go | 13 +++++++------ internal/VENDORED.md | 5 +++-- internal/diffview/cache_test.go | 20 ++++++++++++++++++++ internal/diffview/diffview.go | 30 +++++++++++++++++++++--------- tui/bench_test.go | 3 +-- tui/comment.go | 6 +++--- 7 files changed, 59 insertions(+), 27 deletions(-) diff --git a/diff/diff.go b/diff/diff.go index 14c6ea3..06712ad 100644 --- a/diff/diff.go +++ b/diff/diff.go @@ -83,14 +83,13 @@ type Options struct { Signs bool // Theme sets the colours. Theme Theme - // Cache keeps the syntax colours between renders. If Cache is nil, the - // render keeps nothing. + // Cache makes a new render of the same lines fast. Nil turns it off. Cache *Cache } -// Cache keeps the syntax colours of the lines, so that a new render of the -// same lines is fast, for example after a sideways scroll. The zero value -// is an empty Cache. A Cache is not safe for concurrent use. +// Cache keeps the syntax colours of the lines between renders, for example +// for a sideways scroll. The zero value is an empty Cache. A Cache is not +// safe for concurrent use. type Cache struct { lines diffview.Cache // the lines of Render file struct{ path, src, out string } // the last file of RenderFile diff --git a/diff/file.go b/diff/file.go index 22c780e..8ebd943 100644 --- a/diff/file.go +++ b/diff/file.go @@ -26,7 +26,7 @@ func RenderFile(path, src string, o Options) *Diff { "\r", "\u240d", "\t", strings.Repeat(" ", tabWidth), ).Replace(src) - hl := strings.Split(o.Cache.highlight(path, src), "\n") + hl := strings.Split(o.Cache.fileColours(path, src), "\n") hl = hl[:min(len(hl), len(Lines(src)))] // chroma can add a line break at the end digits := len(strconv.Itoa(len(hl))) numWidth := digits + 2 // a space on each side of the number @@ -74,16 +74,17 @@ func fileLineParts(l string, code int, o Options) []string { return []string{ansi.TruncateLeft(l, o.Offset, "")} } -// highlight returns highlight(path, src), and keeps it for the next call. -// A nil c keeps nothing. -func (c *Cache) highlight(path, src string) string { +// fileColours returns highlight(path, src), and keeps it for the next +// call. A nil c keeps nothing. +func (c *Cache) fileColours(path, src string) string { if c == nil { return highlight(path, src) } - if f := &c.file; f.path != path || f.src != src { + f := &c.file + if f.path != path || f.src != src { f.path, f.src, f.out = path, src, highlight(path, src) } - return c.file.out + return f.out } // highlight colours src, the whole file at path, at once, so that tokens diff --git a/internal/VENDORED.md b/internal/VENDORED.md index 849abae..2379316 100644 --- a/internal/VENDORED.md +++ b/internal/VENDORED.md @@ -37,6 +37,7 @@ long runs of equal lines, as in herdr-reviewr; `Expand` shows a fold, and `Row.Fold` tells which lines a hunk header hides - the syntax cache key tells "no background" from black, so a black tint does not - leak onto equal lines, and it has the lexer name + leak onto equal lines - the syntax cache is a `Cache` that views can share (`View.Cache`), so that a new - render does not highlight the same lines again; a `Cache` of 32 MB empties + render does not highlight the same lines again; its key has the lexer name, it keeps + the last lexer, and a `Cache` of 32 MB empties diff --git a/internal/diffview/cache_test.go b/internal/diffview/cache_test.go index 6725e2e..7bcc840 100644 --- a/internal/diffview/cache_test.go +++ b/internal/diffview/cache_test.go @@ -3,6 +3,8 @@ package diffview import ( "strings" "testing" + + "github.com/alecthomas/chroma/v2/lexers" ) // TestCacheShared checks that a View uses the lines that another View @@ -51,3 +53,21 @@ func TestCacheNil(t *testing.T) { t.Error("the render is empty") } } + +// TestCacheLexer checks that a View uses the last lexer of the Cache for +// the same file, and selects a new lexer for a changed file. +func TestCacheLexer(t *testing.T) { + c := &Cache{} + lexer := func(content string) string { + v := New().Cache(c).Before("a.zzq", content) + return v.chromaLexer().Config().Name + } + lexer("x\n") + c.lexer.l = lexers.Get("python") + if got := lexer("x\n"); got != "Python" { + t.Errorf("the same file has the lexer %q, want the kept Python", got) + } + if got := lexer("y\n"); got == "Python" { + t.Error("a changed file has the kept lexer") + } +} diff --git a/internal/diffview/diffview.go b/internal/diffview/diffview.go index 6d76066..11b7c44 100644 --- a/internal/diffview/diffview.go +++ b/internal/diffview/diffview.go @@ -74,15 +74,22 @@ type View struct { // The View keeps the lexer, because to match a file name is slow. cachedLexer chroma.Lexer - cache *Cache // the highlighted lines + cache *Cache } // Cache keeps highlighted lines, so that chroma highlights each line once. -// Views that share a Cache share the work. The zero value is an empty -// Cache. A Cache is not safe for concurrent use. +// Views that share a Cache reuse its lines and its last lexer. The zero +// value is an empty Cache. A Cache is not safe for concurrent use. type Cache struct { lines map[syntaxKey]string size int // the bytes of the lines and their sources + + // lexer is the last lexer, because to select a lexer from the content + // is slow. + lexer struct { + path, content string + l chroma.Lexer + } } // maxCacheSize limits the memory of a Cache, in bytes. A full Cache @@ -91,7 +98,7 @@ const maxCacheSize = 32 << 20 // put keeps the highlighted line s for key k. func (c *Cache) put(k syntaxKey, s string) { - // A source is part of the whole file. A copy lets the file go. + // The source is part of the file. A copy does not keep the file. k.source = strings.Clone(k.source) n := len(k.source) + len(s) if c.lines == nil || c.size+n > maxCacheSize { @@ -110,8 +117,8 @@ func New() *View { } } -// Cache sets the cache of the highlighted lines. A nil c gives the View a -// new Cache. +// Cache makes v keep its highlighted lines in c. A nil c gives v a new +// Cache. func (v *View) Cache(c *Cache) *View { if c == nil { c = &Cache{} @@ -590,8 +597,8 @@ type syntaxKey struct { r, g, b, a uint32 } -// newSyntaxKey returns the cache key of source, highlighted with the lexer -// of that name on the background bgColor. +// newSyntaxKey returns the key of source for the lexer named lexer, on +// bgColor. func newSyntaxKey(lexer, source string, bgColor color.Color) syntaxKey { k := syntaxKey{lexer: lexer, source: source} if _, none := bgColor.(lipgloss.NoColor); bgColor == nil || none { @@ -605,7 +612,12 @@ func newSyntaxKey(lexer, source string, bgColor color.Color) syntaxKey { // chromaLexer returns the lexer of the old file, and keeps it. func (v *View) chromaLexer() chroma.Lexer { if v.cachedLexer == nil { - v.cachedLexer = Lexer(v.before.path, v.before.content) + lx := &v.cache.lexer + if lx.l == nil || lx.path != v.before.path || lx.content != v.before.content { + lx.path, lx.content = v.before.path, v.before.content + lx.l = Lexer(lx.path, lx.content) + } + v.cachedLexer = lx.l } return v.cachedLexer } diff --git a/tui/bench_test.go b/tui/bench_test.go index 65df786..919df62 100644 --- a/tui/bench_test.go +++ b/tui/bench_test.go @@ -59,8 +59,7 @@ func BenchmarkScroll(b *testing.B) { } case "file": m.setTab(tabFiles) - m.allSel = slices.IndexFunc(m.allFiles, func(e vcs.Entry) bool { return e.Path == "big.go" }) - m.renderViewer() + m.pickFile(slices.IndexFunc(m.allFiles, func(e vcs.Entry) bool { return e.Path == "big.go" })) } right := tea.KeyPressMsg{Code: tea.KeyRight} left := tea.KeyPressMsg{Code: tea.KeyLeft} diff --git a/tui/comment.go b/tui/comment.go index 7ad4981..ccb964b 100644 --- a/tui/comment.go +++ b/tui/comment.go @@ -150,9 +150,9 @@ func (m *model) closeComment() { m.editor.Blur() } -// newLineNear returns the new line of row i. A row with no new line, for -// example a deleted line, gives the first new line below it, else the last -// new line above it, else 1. +// newLineNear returns the new line of row i. A row with no new line, such +// as a deleted line, gives the next new line below it. If there is none, it +// gives the last new line above it. If there is none, it gives 1. func newLineNear(rows []diff.Row, i int) int { for _, r := range rows[i:] { if r.New > 0 { From 4804407f330d5885ac3a2cab4bd242db450aa108 Mon Sep 17 00:00:00 2001 From: Redowan Delowar Date: Sun, 25 Oct 2026 11:26:49 +0100 Subject: [PATCH 4/8] docs: simplify the cache comments --- internal/VENDORED.md | 2 +- internal/diffview/diffview.go | 13 +++++++------ tui/editor_test.go | 3 ++- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/internal/VENDORED.md b/internal/VENDORED.md index 2379316..f11dc47 100644 --- a/internal/VENDORED.md +++ b/internal/VENDORED.md @@ -40,4 +40,4 @@ leak onto equal lines - the syntax cache is a `Cache` that views can share (`View.Cache`), so that a new render does not highlight the same lines again; its key has the lexer name, it keeps - the last lexer, and a `Cache` of 32 MB empties + the last lexer, and a `Cache` empties before its lines go above 32 MB diff --git a/internal/diffview/diffview.go b/internal/diffview/diffview.go index 11b7c44..36ed606 100644 --- a/internal/diffview/diffview.go +++ b/internal/diffview/diffview.go @@ -71,7 +71,7 @@ type View struct { afterNumDigits int heights []int // the number of screen lines of each row, for rows - // The View keeps the lexer, because to match a file name is slow. + // The View keeps the lexer. To match a file name is slow. cachedLexer chroma.Lexer cache *Cache @@ -84,21 +84,22 @@ type Cache struct { lines map[syntaxKey]string size int // the bytes of the lines and their sources - // lexer is the last lexer, because to select a lexer from the content - // is slow. + // lexer is the last lexer. To select a lexer from the content is + // slow. lexer struct { path, content string l chroma.Lexer } } -// maxCacheSize limits the memory of a Cache, in bytes. A full Cache -// empties. +// maxCacheSize limits the bytes of the lines and the sources in a Cache. +// A put that goes above it empties the Cache first. const maxCacheSize = 32 << 20 // put keeps the highlighted line s for key k. func (c *Cache) put(k syntaxKey, s string) { - // The source is part of the file. A copy does not keep the file. + // k.source is a part of the file string. A copy lets the cache keep + // the line without the whole file. k.source = strings.Clone(k.source) n := len(k.source) + len(s) if c.lines == nil || c.size+n > maxCacheSize { diff --git a/tui/editor_test.go b/tui/editor_test.go index 795803e..33f33ab 100644 --- a/tui/editor_test.go +++ b/tui/editor_test.go @@ -186,7 +186,8 @@ func TestEditorError(t *testing.T) { } } -// TestNewLineNear checks the line that e opens for each kind of row. +// TestNewLineNear checks the line that the editor opens for each kind of +// row. func TestNewLineNear(t *testing.T) { rows := []diff.Row{ {Kind: diff.KindHunk}, From d287692a44861ec793ee0e84217ec663e955e433 Mon Sep 17 00:00:00 2001 From: Redowan Delowar Date: Sun, 25 Oct 2026 11:49:51 +0100 Subject: [PATCH 5/8] refactor: move editor commands and terminal-safe text out of tui and diff - internal/editor builds the editor command, so tui only runs it. - internal/termtext has Plain and Quote, which diff exported before. - vcs.PullRequest removes control characters from the text itself, so every caller gets safe text. --- diff/comment.go | 6 +- diff/diff.go | 2 +- diff/fuzz_test.go | 21 ----- diff/hostile_test.go | 16 ---- diff/review.go | 37 ++------ internal/editor/editor.go | 133 ++++++++++++++++++++++++++ internal/editor/editor_test.go | 52 +++++++++++ internal/termtext/termtext.go | 31 +++++++ internal/termtext/termtext_test.go | 55 +++++++++++ tui/editor.go | 144 +++-------------------------- tui/editor_test.go | 46 --------- tui/model.go | 3 +- tui/nav.go | 3 +- tui/overlay.go | 3 +- tui/pr.go | 19 ---- tui/render.go | 5 +- tui/view.go | 5 +- vcs/pr.go | 29 +++++- vcs/pr_test.go | 24 +++++ 19 files changed, 356 insertions(+), 278 deletions(-) create mode 100644 internal/editor/editor.go create mode 100644 internal/editor/editor_test.go create mode 100644 internal/termtext/termtext.go create mode 100644 internal/termtext/termtext_test.go diff --git a/diff/comment.go b/diff/comment.go index c2786a7..469efb1 100644 --- a/diff/comment.go +++ b/diff/comment.go @@ -6,6 +6,8 @@ import ( "fmt" "slices" "strings" + + "github.com/rednafi/rexdiff/internal/termtext" ) // Comment is a review note on a range of lines on one side of a file. Use @@ -58,7 +60,7 @@ func NewComment(path string, s Side, rows []Row, old, new string) Comment { // A comment on the old side has " (removed)" at the end. The path is // quoted with [Quote]. func (c *Comment) Location() string { - loc := fmt.Sprintf("%s:%d", Quote(c.Path), c.Start) + loc := fmt.Sprintf("%s:%d", termtext.Quote(c.Path), c.Start) if c.End != c.Start { loc += fmt.Sprintf("-%d", c.End) } @@ -78,7 +80,7 @@ type Comments struct { // Len returns the number of comments. func (cs *Comments) Len() int { return len(cs.items) } -// Add adds a copy of c with a new ID, and returns the copy. +// Add adds c with a new ID, and returns the comment in the set. func (cs *Comments) Add(c Comment) *Comment { cs.last++ c.ID = cs.last diff --git a/diff/diff.go b/diff/diff.go index 06712ad..f1c434c 100644 --- a/diff/diff.go +++ b/diff/diff.go @@ -216,7 +216,7 @@ func Count(old, new string) (added, deleted int) { } // Lines splits s into lines, without the LF or CRLF line endings. Line n of -// s is Lines(s)[n-1]. A comment counts lines in this way. +// s is Lines(s)[n-1]. The lines of a [Comment] count in this way. func Lines(s string) []string { var lines []string for l := range strings.Lines(s) { diff --git a/diff/fuzz_test.go b/diff/fuzz_test.go index 05d9c28..df13474 100644 --- a/diff/fuzz_test.go +++ b/diff/fuzz_test.go @@ -7,7 +7,6 @@ import ( "strings" "testing" "unicode" - "unicode/utf8" "github.com/charmbracelet/x/ansi" @@ -203,26 +202,6 @@ func FuzzRenderFile(f *testing.F) { }) } -// FuzzPlain checks that Plain and Quote remove all unsafe control characters. -func FuzzPlain(f *testing.F) { - for _, s := range hostile { - f.Add(s) - } - f.Fuzz(func(t *testing.T, s string) { - p := diff.Plain(s) - if strings.ContainsFunc(p, isUnsafeControl) { - t.Errorf("Plain(%q) = %q, has a control character", s, p) - } - if pp := diff.Plain(p); pp != p { - t.Errorf("Plain is not idempotent: %q, then %q", p, pp) - } - q := diff.Quote(s) - if strings.ContainsFunc(q, unicode.IsControl) || !utf8.ValidString(q) { - t.Errorf("Quote(%q) = %q, has a control character", s, q) - } - }) -} - // FuzzCount checks that the counts of Count are in the range of the line // counts. func FuzzCount(f *testing.F) { diff --git a/diff/hostile_test.go b/diff/hostile_test.go index bd9498d..8a1d379 100644 --- a/diff/hostile_test.go +++ b/diff/hostile_test.go @@ -124,22 +124,6 @@ func TestRenderFileNarrow(t *testing.T) { } } -// TestQuote checks that Quote quotes only the paths that are not safe. -func TestQuote(t *testing.T) { - tests := []struct{ in, want string }{ - {"a.go", "a.go"}, - {"日本.go", "日本.go"}, - {"a\x1b]0;x\x07.go", `"a\x1b]0;x\a.go"`}, - {"a\n.go", `"a\n.go"`}, - {"a\x9b31m.go", `"a\x9b31m.go"`}, - } - for _, tt := range tests { - if got := diff.Quote(tt.in); got != tt.want { - t.Errorf("Quote(%q) = %s, want %s", tt.in, got, tt.want) - } - } -} - // TestReviewHostileComment checks that Review quotes a hostile path and // removes the control characters from the code and the text. func TestReviewHostileComment(t *testing.T) { diff --git a/diff/review.go b/diff/review.go index e00f640..ede0da5 100644 --- a/diff/review.go +++ b/diff/review.go @@ -1,10 +1,9 @@ package diff import ( - "strconv" "strings" - "unicode" - "unicode/utf8" + + "github.com/rednafi/rexdiff/internal/termtext" ) // Review returns the comments as herdr-reviewr text for a coding agent. It @@ -17,7 +16,7 @@ import ( // Merge these lines. func (cs *Comments) Review(prefix string) string { var parts []string - if p := strings.TrimRight(Plain(prefix), " \n\t"); p != "" { + if p := strings.TrimRight(termtext.Plain(prefix), " \n\t"); p != "" { parts = append(parts, p) } for _, c := range cs.All() { @@ -29,14 +28,14 @@ func (cs *Comments) Review(prefix string) string { // format returns c as text for [Comments.Review]. func format(c *Comment) string { var b strings.Builder - b.WriteString(Plain(c.Location())) + b.WriteString(termtext.Plain(c.Location())) if c.Stale { b.WriteString(" (stale: the code has changed since)") } // Write the rows as the diff shows them, so that the agent reads a real diff. if len(c.snippet) > 0 { for _, l := range c.snippet { - b.WriteString("\n" + Plain(l)) + b.WriteString("\n" + termtext.Plain(l)) } } else { mark := "+" @@ -44,11 +43,11 @@ func format(c *Comment) string { mark = "-" } for _, l := range c.code { - b.WriteString("\n" + mark + Plain(l)) + b.WriteString("\n" + mark + termtext.Plain(l)) } } var text []string - for l := range strings.SplitSeq(Plain(c.Text), "\n") { + for l := range strings.SplitSeq(termtext.Plain(c.Text), "\n") { if strings.TrimSpace(l) != "" { text = append(text, strings.TrimRight(l, " \t")) } @@ -57,28 +56,6 @@ func format(c *Comment) string { return b.String() } -// Plain returns s without control characters. This removes the escape -// sequences too, but keeps tabs and newlines. Use it on text from other -// people before you show or send it. -func Plain(s string) string { - return strings.Map(func(r rune) rune { - if r == '\t' || r == '\n' || !unicode.IsControl(r) { - return r - } - return -1 - }, s) -} - -// Quote returns s in Go quotes if it has control characters or is not valid -// UTF-8. git shows such a path the same way. Then s stays on one line and -// cannot change the terminal. -func Quote(s string) string { - if strings.ContainsFunc(s, unicode.IsControl) || !utf8.ValidString(s) { - return strconv.Quote(s) - } - return s -} - // snippet returns rows as unified diff lines. See [NewComment]. func snippet(rows []Row, old, new string) []string { oldLines, newLines := Lines(old), Lines(new) diff --git a/internal/editor/editor.go b/internal/editor/editor.go new file mode 100644 index 0000000..5582449 --- /dev/null +++ b/internal/editor/editor.go @@ -0,0 +1,133 @@ +// Package editor makes the sh command that opens a file at a line in a +// text editor. It also tells if the editor opens a window or draws in the +// terminal. +package editor + +import ( + "path/filepath" + "runtime" + "slices" + "strconv" + "strings" +) + +// Command is a command that opens a file at a line. +type Command struct { + Script string // the command, for sh -c + Window bool // the editor opens a window, not in the terminal +} + +// FromTemplate returns the command that opens file at line with template, +// a command that the user writes. The template gets file in place of +// {file}, and line in place of {line}. Without {file}, file goes at the +// end. +func FromTemplate(template, file string, line int) Command { + r := strings.NewReplacer("{file}", Quote(file), "{line}", strconv.Itoa(line)) + script := r.Replace(template) + if !strings.Contains(template, "{file}") { + script += " " + Quote(file) + } + return Command{script, dialectOf(template).window} +} + +// FromEditor returns the command that opens file at line with editor, for +// example the value of $EDITOR. FromEditor adds file and line in the form +// that the editor expects. +func FromEditor(editor, file string, line int) Command { + d := dialectOf(editor) + n, f := strconv.Itoa(line), Quote(file) + at := Quote(file + ":" + n) + var args string + switch d.line { + case argNone: + args = f + case argPlus: + args = "+" + n + " " + f + case argSuffix: + args = at + case argGoto: + args = "-g " + at + case argFlag: + args = "--line " + n + " " + f + } + return Command{editor + " " + args, d.window} +} + +// Quote quotes s as one word for sh. +func Quote(s string) string { return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" } + +// lineArg is how an editor takes the line to open at. +type lineArg int + +const ( + argNone lineArg = iota // file + argPlus // +42 file + argSuffix // file:42 + argGoto // -g file:42 + argFlag // --line 42 file +) + +// dialect tells how an editor takes the line, and if it opens a window. +type dialect struct { + line lineArg + window bool +} + +// dialects has the known editors, by program name. The table is the same +// as in herdr-reviewr. The table also lists fresh, open, and xdg-open. +var dialects = makeDialects() + +func makeDialects() map[string]dialect { + d := map[string]dialect{} + add := func(dl dialect, names ...string) { + for _, n := range names { + d[n] = dl + } + } + add(dialect{argPlus, false}, "vi", "vim", "nvim", "lvim", "vis", "joe", "nano", "micro", "kak") + // Emacs can open a window or not. In a pane, it works in both cases. + add(dialect{argPlus, false}, "emacs", "emacsclient") + add(dialect{argSuffix, false}, "hx", "helix", "fresh") + // MacVim and gVim open a window, unlike the other vi editors. + add(dialect{argPlus, true}, "mvim", "gvim", "bbedit", "gedit") + add(dialect{argGoto, true}, + "code", "code-insiders", "codium", "vscodium", "cursor", "windsurf", "positron") + add(dialect{argSuffix, true}, "subl", "sublime_text", "zed", "zeditor", "zedit") + add(dialect{argFlag, true}, "mate", "kate", "idea", "pycharm", "webstorm", "goland", + "clion", "phpstorm", "rubymine", "rider", "datagrip", "rustrover", "dataspell", "fleet") + add(dialect{argNone, true}, "open", "xdg-open") + if runtime.GOOS == "darwin" { + add(dialect{argFlag, true}, "xed") // the opener of Xcode + } + return d +} + +// dialectOf returns the dialect of the program of command. An unknown +// editor takes +line and draws in the terminal. Most terminal editors take +// +line. A window editor also works in a pane. A wait flag tells that the +// editor has a window. +func dialectOf(command string) dialect { + d := dialect{line: argPlus} + if known, ok := dialects[filepath.Base(program(command))]; ok { + d = known + } + words := strings.Fields(command) + if slices.Contains(words, "--wait") || slices.Contains(words, "--block") { + d.window = true + } + return d +} + +// program returns the first word of command. A word in quotes can have +// spaces, for example "/Applications/Visual Studio Code.app/.../code". +func program(command string) string { + command = strings.TrimSpace(command) + for _, q := range []string{`"`, "'"} { + if rest, ok := strings.CutPrefix(command, q); ok { + word, _, _ := strings.Cut(rest, q) + return word + } + } + word, _, _ := strings.Cut(command, " ") + return word +} diff --git a/internal/editor/editor_test.go b/internal/editor/editor_test.go new file mode 100644 index 0000000..b3a4512 --- /dev/null +++ b/internal/editor/editor_test.go @@ -0,0 +1,52 @@ +package editor_test + +import ( + "testing" + + "github.com/rednafi/rexdiff/internal/editor" +) + +// TestCommands checks the command and the kind of each editor. +func TestCommands(t *testing.T) { + const abs = "/r/a b.go" + tests := []struct { + template, editor string + script string + window bool + }{ + // The editor of the settings gets the placeholders. + {"/opt/homebrew/bin/nvim +{line} {file}", "", `/opt/homebrew/bin/nvim +12 '/r/a b.go'`, false}, + {"code -g {file}:{line}", "", `code -g '/r/a b.go':12`, true}, + {"zed {file}:{line}", "", `zed '/r/a b.go':12`, true}, + {"open -a Zed {file}", "", `open -a Zed '/r/a b.go'`, true}, + {"myeditor --at {line}", "", `myeditor --at 12 '/r/a b.go'`, false}, + // The editor of the environment gets the line as it expects it. + {"", "nvim", `nvim +12 '/r/a b.go'`, false}, + {"", "/usr/bin/vim -u NONE", `/usr/bin/vim -u NONE +12 '/r/a b.go'`, false}, + {"", "micro", `micro +12 '/r/a b.go'`, false}, + {"", "hx", `hx '/r/a b.go:12'`, false}, + {"", "fresh", `fresh '/r/a b.go:12'`, false}, + {"", "code --wait", `code --wait -g '/r/a b.go:12'`, true}, + {"", "subl", `subl '/r/a b.go:12'`, true}, + {"", "idea", `idea --line 12 '/r/a b.go'`, true}, + {"", "mvim", `mvim +12 '/r/a b.go'`, true}, + // A path in quotes can have spaces. + {"", `"/Apps/VS Code.app/bin/code" -w`, `"/Apps/VS Code.app/bin/code" -w -g '/r/a b.go:12'`, true}, + {"", `'/my apps/nvim'`, `'/my apps/nvim' +12 '/r/a b.go'`, false}, + // An unknown editor gets +line, and draws in the terminal. + {"", "vim.basic", `vim.basic +12 '/r/a b.go'`, false}, + // A wait flag tells that an unknown editor has a window. + {"", "myeditor --wait", `myeditor --wait +12 '/r/a b.go'`, true}, + } + for _, tt := range tests { + var got editor.Command + if tt.template != "" { + got = editor.FromTemplate(tt.template, abs, 12) + } else { + got = editor.FromEditor(tt.editor, abs, 12) + } + if want := (editor.Command{Script: tt.script, Window: tt.window}); got != want { + t.Errorf("editor %q%q = %+v, want %+v", tt.template, tt.editor, got, want) + } + } +} diff --git a/internal/termtext/termtext.go b/internal/termtext/termtext.go new file mode 100644 index 0000000..4f1e223 --- /dev/null +++ b/internal/termtext/termtext.go @@ -0,0 +1,31 @@ +// Package termtext makes text from other people safe to show in a terminal. +package termtext + +import ( + "strconv" + "strings" + "unicode" + "unicode/utf8" +) + +// Plain returns s without control characters. This removes the escape +// sequences too, but keeps tabs and newlines. Use it on text from other +// people before you show or send it. +func Plain(s string) string { + return strings.Map(func(r rune) rune { + if r == '\t' || r == '\n' || !unicode.IsControl(r) { + return r + } + return -1 + }, s) +} + +// Quote returns s in Go quotes if it has control characters or is not valid +// UTF-8. git shows such a path the same way. Then s stays on one line and +// cannot change the terminal. +func Quote(s string) string { + if strings.ContainsFunc(s, unicode.IsControl) || !utf8.ValidString(s) { + return strconv.Quote(s) + } + return s +} diff --git a/internal/termtext/termtext_test.go b/internal/termtext/termtext_test.go new file mode 100644 index 0000000..fa3f670 --- /dev/null +++ b/internal/termtext/termtext_test.go @@ -0,0 +1,55 @@ +package termtext_test + +import ( + "strings" + "testing" + "unicode" + "unicode/utf8" + + "github.com/rednafi/rexdiff/internal/termtext" +) + +// TestQuote checks that Quote quotes only the paths that are not safe. +func TestQuote(t *testing.T) { + tests := []struct{ in, want string }{ + {"a.go", "a.go"}, + {"日本.go", "日本.go"}, + {"a\x1b]0;x\x07.go", `"a\x1b]0;x\a.go"`}, + {"a\n.go", `"a\n.go"`}, + {"a\x9b31m.go", `"a\x9b31m.go"`}, + } + for _, tt := range tests { + if got := termtext.Quote(tt.in); got != tt.want { + t.Errorf("Quote(%q) = %s, want %s", tt.in, got, tt.want) + } + } +} + +// FuzzPlain checks that Plain and Quote remove all unsafe control characters. +func FuzzPlain(f *testing.F) { + for _, s := range []string{ + "a\x00b\n", + "\tx\r\n", + "\x1b[31mred\x1b[0m\n", + "\x1b]52;c;aGVsbG8=\x07\n", + "\u009b31m\u009d0;title\u009c\n", + "\x9b31m\xff\xfe\n", + "日本語😀👨‍👩‍👧\u202e\n", + } { + f.Add(s) + } + f.Fuzz(func(t *testing.T, s string) { + p := termtext.Plain(s) + unsafe := func(r rune) bool { return r != '\t' && r != '\n' && unicode.IsControl(r) } + if strings.ContainsFunc(p, unsafe) { + t.Errorf("Plain(%q) = %q, has a control character", s, p) + } + if pp := termtext.Plain(p); pp != p { + t.Errorf("Plain(Plain(%q)) = %q, want %q", s, pp, p) + } + q := termtext.Quote(s) + if strings.ContainsFunc(q, unicode.IsControl) || !utf8.ValidString(q) { + t.Errorf("Quote(%q) = %q, has a control character", s, q) + } + }) +} diff --git a/tui/editor.go b/tui/editor.go index 79884a1..632a5d2 100644 --- a/tui/editor.go +++ b/tui/editor.go @@ -8,164 +8,45 @@ import ( "os/exec" "path/filepath" "runtime" - "slices" - "strconv" "strings" "syscall" tea "charm.land/bubbletea/v2" -) - -// lineArg is how an editor takes the line to open at. -type lineArg int -const ( - argNone lineArg = iota // file - argPlus // +42 file - argSuffix // file:42 - argGoto // -g file:42 - argFlag // --line 42 file + "github.com/rednafi/rexdiff/internal/editor" ) -// dialect tells how an editor takes the line, and if it opens a window. -// An editor without a window draws in the terminal, so it needs a pane. -type dialect struct { - line lineArg - window bool -} - -// dialects has the editors that rexdiff knows, by program name. The table -// is the same as in herdr-reviewr. The table also lists fresh, open, and -// xdg-open. -var dialects = makeDialects() - -func makeDialects() map[string]dialect { - d := map[string]dialect{} - add := func(dl dialect, names ...string) { - for _, n := range names { - d[n] = dl - } - } - add(dialect{argPlus, false}, "vi", "vim", "nvim", "lvim", "vis", "joe", "nano", "micro", "kak") - // Emacs can open a window or not. In a pane, it works in both cases. - add(dialect{argPlus, false}, "emacs", "emacsclient") - add(dialect{argSuffix, false}, "hx", "helix", "fresh") - // MacVim and gVim open a window, unlike the other vi editors. - add(dialect{argPlus, true}, "mvim", "gvim", "bbedit", "gedit") - add(dialect{argGoto, true}, - "code", "code-insiders", "codium", "vscodium", "cursor", "windsurf", "positron") - add(dialect{argSuffix, true}, "subl", "sublime_text", "zed", "zeditor", "zedit") - add(dialect{argFlag, true}, "mate", "kate", "idea", "pycharm", "webstorm", "goland", - "clion", "phpstorm", "rubymine", "rider", "datagrip", "rustrover", "dataspell", "fleet") - add(dialect{argNone, true}, "open", "xdg-open") - if runtime.GOOS == "darwin" { - add(dialect{argFlag, true}, "xed") // the opener of Xcode - } - return d -} - -// dialectOf returns the dialect of the program of command. An editor that -// rexdiff does not know takes +line and draws in the terminal. Most -// terminal editors take +line. A window editor also works in a pane. A -// wait flag tells that the editor has a window. -func dialectOf(command string) dialect { - d := dialect{line: argPlus} - if known, ok := dialects[filepath.Base(program(command))]; ok { - d = known - } - words := strings.Fields(command) - if slices.Contains(words, "--wait") || slices.Contains(words, "--block") { - d.window = true - } - return d -} - -// program returns the first word of command. A word in quotes can have -// spaces, for example "/Applications/Visual Studio Code.app/.../code". -func program(command string) string { - command = strings.TrimSpace(command) - for _, q := range []string{`"`, "'"} { - if rest, ok := strings.CutPrefix(command, q); ok { - word, _, _ := strings.Cut(rest, q) - return word - } - } - word, _, _ := strings.Cut(command, " ") - return word -} - -// editorCommand is a command that opens a file at a line. -type editorCommand struct { - script string // the command, for sh -c - window bool // the editor opens a window, not in the terminal -} - -// templateCommand returns the command that opens the file abs at line with -// template, the editor of the settings. The template gets abs in place of -// {file}, and line in place of {line}. Without {file}, abs goes at the end. -func templateCommand(template, abs string, line int) editorCommand { - r := strings.NewReplacer("{file}", shellQuote(abs), "{line}", strconv.Itoa(line)) - script := r.Replace(template) - if !strings.Contains(template, "{file}") { - script += " " + shellQuote(abs) - } - return editorCommand{script, dialectOf(template).window} -} - -// envCommand returns the command that opens the file abs at line with -// editor, the editor of the environment or of git. rexdiff adds abs and -// line in the form that the editor expects. -func envCommand(editor, abs string, line int) editorCommand { - d := dialectOf(editor) - n, file := strconv.Itoa(line), shellQuote(abs) - at := shellQuote(abs + ":" + n) - var args string - switch d.line { - case argNone: - args = file - case argPlus: - args = "+" + n + " " + file - case argSuffix: - args = at - case argGoto: - args = "-g " + at - case argFlag: - args = "--line " + n + " " + file - } - return editorCommand{editor + " " + args, d.window} -} - // openEditor opens the file at path, at line. It uses the editor of the // settings, else $VISUAL, $EDITOR, core.editor, or vi. A terminal editor // opens in a new block beside rexdiff in Rex. Outside Rex, it takes the // screen until it stops. A window editor starts in the background. func (m *model) openEditor(path string, line int) tea.Cmd { abs := filepath.Join(m.repo.Root(), path) - var ed editorCommand + var ed editor.Command if m.cfg.Editor != "" { - ed = templateCommand(m.cfg.Editor, abs, line) + ed = editor.FromTemplate(m.cfg.Editor, abs, line) } else { - editor := cmp.Or( + e := cmp.Or( strings.TrimSpace(os.Getenv("VISUAL")), strings.TrimSpace(os.Getenv("EDITOR")), m.repo.Editor(m.ctx), "vi", ) - ed = envCommand(editor, abs, line) + ed = editor.FromEditor(e, abs, line) } - m.log.Debug("editor", "command", ed.script, "window", ed.window) - if ed.window { - return runWindowEditor(ed.script) + m.log.Debug("editor", "command", ed.Script, "window", ed.Window) + if ed.Window { + return runWindowEditor(ed.Script) } if m.rex != nil { - err := m.rex.Split(m.ctx, m.repo.Root(), "sh", "-c", ed.script) + err := m.rex.Split(m.ctx, m.repo.Root(), "sh", "-c", ed.Script) if err == nil { return nil } m.log.Debug("editor split", "err", err) } done := func(err error) tea.Msg { return editorDoneMsg{err} } - return tea.ExecProcess(exec.Command("sh", "-c", ed.script), done) + return tea.ExecProcess(exec.Command("sh", "-c", ed.Script), done) } // runWindowEditor starts a window editor. The editor gets its own session @@ -197,7 +78,7 @@ func runWindowEditor(script string) tea.Cmd { // the opener of the system. func openURL(cfg config, url string) error { if cfg.URLOpener != "" { - script := strings.ReplaceAll(cfg.URLOpener, "{url}", shellQuote(url)) + script := strings.ReplaceAll(cfg.URLOpener, "{url}", editor.Quote(url)) return start("sh", "-c", script) } opener := "xdg-open" @@ -217,6 +98,3 @@ func start(name string, args ...string) error { go cmd.Wait() return nil } - -// shellQuote quotes s as one word for sh. -func shellQuote(s string) string { return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" } diff --git a/tui/editor_test.go b/tui/editor_test.go index 33f33ab..fee041d 100644 --- a/tui/editor_test.go +++ b/tui/editor_test.go @@ -12,52 +12,6 @@ import ( "github.com/rednafi/rexdiff/rex" ) -// TestEditorCommands checks the command and the kind of each editor. -func TestEditorCommands(t *testing.T) { - const abs = "/r/a b.go" - tests := []struct { - template, editor string - script string - window bool - }{ - // The editor of the settings gets the placeholders. - {"/opt/homebrew/bin/nvim +{line} {file}", "", `/opt/homebrew/bin/nvim +12 '/r/a b.go'`, false}, - {"code -g {file}:{line}", "", `code -g '/r/a b.go':12`, true}, - {"zed {file}:{line}", "", `zed '/r/a b.go':12`, true}, - {"open -a Zed {file}", "", `open -a Zed '/r/a b.go'`, true}, - {"myeditor --at {line}", "", `myeditor --at 12 '/r/a b.go'`, false}, - // The editor of the environment gets the line as it expects it. - {"", "nvim", `nvim +12 '/r/a b.go'`, false}, - {"", "/usr/bin/vim -u NONE", `/usr/bin/vim -u NONE +12 '/r/a b.go'`, false}, - {"", "micro", `micro +12 '/r/a b.go'`, false}, - {"", "hx", `hx '/r/a b.go:12'`, false}, - {"", "fresh", `fresh '/r/a b.go:12'`, false}, - {"", "code --wait", `code --wait -g '/r/a b.go:12'`, true}, - {"", "subl", `subl '/r/a b.go:12'`, true}, - {"", "idea", `idea --line 12 '/r/a b.go'`, true}, - {"", "mvim", `mvim +12 '/r/a b.go'`, true}, - // A path in quotes can have spaces. - {"", `"/Apps/VS Code.app/bin/code" -w`, `"/Apps/VS Code.app/bin/code" -w -g '/r/a b.go:12'`, true}, - {"", `'/my apps/nvim'`, `'/my apps/nvim' +12 '/r/a b.go'`, false}, - // An unknown editor gets +line, and draws in the terminal. - {"", "vim.basic", `vim.basic +12 '/r/a b.go'`, false}, - // A wait flag tells that an unknown editor has a window. - {"", "myeditor --wait", `myeditor --wait +12 '/r/a b.go'`, true}, - } - for _, tt := range tests { - var got editorCommand - if tt.template != "" { - got = templateCommand(tt.template, abs, 12) - } else { - got = envCommand(tt.editor, abs, 12) - } - if got.script != tt.script || got.window != tt.window { - t.Errorf("editor %q%q = %q, window %v; want %q, window %v", - tt.template, tt.editor, got.script, got.window, tt.script, tt.window) - } - } -} - // writeScript writes an executable sh script with body to dir/name, and // returns its path. func writeScript(t *testing.T, dir, name, body string) string { diff --git a/tui/model.go b/tui/model.go index 2f331f0..f01e981 100644 --- a/tui/model.go +++ b/tui/model.go @@ -14,6 +14,7 @@ import ( uv "github.com/charmbracelet/ultraviolet" "github.com/rednafi/rexdiff/diff" + "github.com/rednafi/rexdiff/internal/termtext" "github.com/rednafi/rexdiff/rex" "github.com/rednafi/rexdiff/update" "github.com/rednafi/rexdiff/vcs" @@ -347,7 +348,7 @@ const flashTime = 6 * time.Second // flash shows s in the footer for a short time. The text can come from Rex // or from git, so flash quotes it if it has control characters. func (m *model) flash(s string) { - m.status, m.statusTill = diff.Quote(s), time.Now().Add(flashTime) + m.status, m.statusTill = termtext.Quote(s), time.Now().Add(flashTime) } // trees returns the two trees that the Changes tab compares in the scope. diff --git a/tui/nav.go b/tui/nav.go index 5b0bd31..2b0cdca 100644 --- a/tui/nav.go +++ b/tui/nav.go @@ -12,6 +12,7 @@ import ( "github.com/charmbracelet/x/ansi" "github.com/rednafi/rexdiff/diff" + "github.com/rednafi/rexdiff/internal/termtext" "github.com/rednafi/rexdiff/vcs" ) @@ -461,7 +462,7 @@ func (m *model) jumpTo(path string, s diff.Side, line int) { m.loadFiles() i := slices.IndexFunc(m.allFiles, func(e vcs.Entry) bool { return e.Path == path }) if i < 0 { - m.flash("no such file: " + diff.Quote(path)) + m.flash("no such file: " + termtext.Quote(path)) return } m.tab = tabFiles diff --git a/tui/overlay.go b/tui/overlay.go index 44e68fa..aba78ac 100644 --- a/tui/overlay.go +++ b/tui/overlay.go @@ -12,6 +12,7 @@ import ( "github.com/sahilm/fuzzy" "github.com/rednafi/rexdiff/diff" + "github.com/rednafi/rexdiff/internal/termtext" "github.com/rednafi/rexdiff/rex" "github.com/rednafi/rexdiff/vcs" ) @@ -509,7 +510,7 @@ func (m *model) overlayRows(inner int) (rows []string, start int) { if ov.kind == overlayCommits && ov.mark == ov.shown[i] { tag += " " + cursorStyle.Render("range start") } - text := diff.Quote(it.label) + " " + dimStyle.Render(diff.Quote(it.detail)) + tag + text := termtext.Quote(it.label) + " " + dimStyle.Render(termtext.Quote(it.detail)) + tag row := truncate(text, inner-2) if i == ov.cur { row = m.selStyle().Width(inner).Render("▶ " + row) diff --git a/tui/pr.go b/tui/pr.go index b4d7b7b..2410bdd 100644 --- a/tui/pr.go +++ b/tui/pr.go @@ -24,29 +24,10 @@ func (m *model) loadPR() tea.Cmd { } return func() tea.Msg { pr, err := repo.PullRequest(ctx, hosts) - if pr != nil { - cleanPR(pr) - } return prMsg{pr, err} } } -// cleanPR removes the control characters from the text of pr, because the -// text comes from other users. -func cleanPR(pr *vcs.PullRequest) { - texts := []*string{&pr.Title, &pr.URL, &pr.Author, &pr.Head, &pr.Base, &pr.Body} - for i := range pr.Checks { - texts = append(texts, &pr.Checks[i].Name) - } - for i := range pr.Comments { - c := &pr.Comments[i] - texts = append(texts, &c.Author, &c.When, &c.Body) - } - for _, s := range texts { - *s = diff.Plain(*s) - } -} - // prStateStyle returns the style that shows the state of a pull request. func prStateStyle(s vcs.State) lipgloss.Style { switch s { diff --git a/tui/render.go b/tui/render.go index 71a4b40..5908418 100644 --- a/tui/render.go +++ b/tui/render.go @@ -10,6 +10,7 @@ import ( "charm.land/glamour/v2" "github.com/rednafi/rexdiff/diff" + "github.com/rednafi/rexdiff/internal/termtext" "github.com/rednafi/rexdiff/vcs" ) @@ -115,7 +116,7 @@ func (m *model) renderChanges() { // A rename without changes, or a change of the file mode. note := "no change to the content" if f.From != "" { - note = "renamed from " + diff.Quote(f.From) + ", no change to the content" + note = "renamed from " + termtext.Quote(f.From) + ", no change to the content" } p.setNote(note) case m.rendered && isMarkdown(f.Path) && f.Status != vcs.StatusDeleted: @@ -205,7 +206,7 @@ func renderMarkdown(r *glamour.TermRenderer, src string) []string { // setMarkdown shows src as rendered markdown. The rendered lines take no // comments. Press m to see the source. func (m *model) setMarkdown(p *pane, src string) { - lines := renderMarkdown(m.markdown(m.codeWidth()-2), diff.Plain(src)) + lines := renderMarkdown(m.markdown(m.codeWidth()-2), termtext.Plain(src)) p.path = "" p.set(lines, make([]diff.Row, len(lines))) } diff --git a/tui/view.go b/tui/view.go index 08be596..c31b661 100644 --- a/tui/view.go +++ b/tui/view.go @@ -13,6 +13,7 @@ import ( "github.com/charmbracelet/x/ansi" "github.com/rednafi/rexdiff/diff" + "github.com/rednafi/rexdiff/internal/termtext" "github.com/rednafi/rexdiff/vcs" ) @@ -158,7 +159,7 @@ func (m *model) turnLabel() string { return dimStyle.Render("waiting for a turn") } ago := time.Since(m.turn.at).Round(time.Second) - app := diff.Quote(m.turn.app) // the app name comes from the agent + app := termtext.Quote(m.turn.app) // the app name comes from the agent if m.turn.running { return noteStyle.Render("● ") + app + dimStyle.Render(" working · "+ago.String()) } @@ -267,7 +268,7 @@ func (m *model) listStart(h int) int { return max(0, m.listSel()-(h-2)) } // fileRow returns " M name/of/file.go +3 -1". If the path is too long, it // removes the directory first, then the start of the name. func fileRow(mark, p, stat string, w int) string { - p = diff.Quote(p) + p = termtext.Quote(p) room := w - lipgloss.Width(stat) - 4 if n := ansi.StringWidth(p); n > room { if base := path.Base(p); ansi.StringWidth(base)+2 <= room { diff --git a/vcs/pr.go b/vcs/pr.go index 18b1c52..98eb4f1 100644 --- a/vcs/pr.go +++ b/vcs/pr.go @@ -7,6 +7,8 @@ import ( "errors" "fmt" "strings" + + "github.com/rednafi/rexdiff/internal/termtext" ) var ( @@ -92,7 +94,8 @@ type Comment struct { // PullRequest returns the pull request of the current branch. It selects the // forge from the URL of the remote "upstream", or else "origin". Then it runs // the command-line tool of the forge: gh, glab, or az. The tool must be -// installed and signed in. +// installed and signed in. The text of the pull request has no control +// characters. func (r *Repo) PullRequest(ctx context.Context, h Hosts) (*PullRequest, error) { remote := strings.ToLower(r.remoteURL(ctx)) // host names ignore case has := func(host string) bool { @@ -112,10 +115,30 @@ func (r *Repo) PullRequest(ctx context.Context, h Hosts) (*PullRequest, error) { default: return nil, errUnsupported } - if err == nil && pr.Title == "" { + if err != nil { + return nil, err + } + if pr.Title == "" { return nil, ErrNotFound } - return pr, err + cleanPR(pr) + return pr, nil +} + +// cleanPR removes the control characters from the text of pr, because the +// text comes from other users. +func cleanPR(pr *PullRequest) { + texts := []*string{&pr.Title, &pr.URL, &pr.Author, &pr.Head, &pr.Base, &pr.Body} + for i := range pr.Checks { + texts = append(texts, &pr.Checks[i].Name) + } + for i := range pr.Comments { + c := &pr.Comments[i] + texts = append(texts, &c.Author, &c.When, &c.Body) + } + for _, s := range texts { + *s = termtext.Plain(*s) + } } // decode runs a forge tool in the root directory and decodes its JSON diff --git a/vcs/pr_test.go b/vcs/pr_test.go index ea5dfe4..7d736f2 100644 --- a/vcs/pr_test.go +++ b/vcs/pr_test.go @@ -6,6 +6,7 @@ import ( "os/exec" "path/filepath" "reflect" + "slices" "testing" "github.com/rednafi/rexdiff/vcs" @@ -196,6 +197,29 @@ func TestPullRequestErrors(t *testing.T) { } } +// TestPullRequestHostile checks that the text of a pull request has no +// control characters. +func TestPullRequestHostile(t *testing.T) { + dir := t.TempDir() + fakePath(t, dir) + const osc = `\u001b]0;owned\u0007` + fakeTool(t, dir, "gh", map[string]string{ghArgs: `{ + "number": 7, "title": "Add` + osc + `", "state": "OPEN", + "body": "a` + osc + `\nb", "author": {"login": "ann` + osc + `"}, + "comments": [{"author": {"login": "bob"}, "body": "x` + osc + `"}], + "statusCheckRollup": [{"name": "t` + osc + `", "conclusion": "SUCCESS"}]}`}) + r, _ := repoAt(t, "https://github.com/o/r") + pr, err := r.PullRequest(t.Context(), vcs.Hosts{}) + if err != nil { + t.Fatal(err) + } + got := []string{pr.Title, pr.Body, pr.Author, pr.Comments[0].Body, pr.Checks[0].Name} + want := []string{"Add]0;owned", "a]0;owned\nb", "ann]0;owned", "x]0;owned", "t]0;owned"} + if !slices.Equal(got, want) { + t.Errorf("PullRequest texts = %q, want %q", got, want) + } +} + // fakePath sets PATH to dir and the system directories, and links git into // dir. Then PATH has the fake forge tools and no real ones. func fakePath(t *testing.T, dir string) { From 28998caae54b7dd478885bc76e0efba72a2abfb3 Mon Sep 17 00:00:00 2001 From: Redowan Delowar Date: Sun, 25 Oct 2026 11:50:24 +0100 Subject: [PATCH 6/8] style: name the fields of color.RGBA literals, and small Go idioms --- diff/diff_test.go | 12 ++++++------ internal/diffview/terminal.go | 2 +- tui/nav.go | 10 ++++------ 3 files changed, 11 insertions(+), 13 deletions(-) diff --git a/diff/diff_test.go b/diff/diff_test.go index c963af7..12b7d49 100644 --- a/diff/diff_test.go +++ b/diff/diff_test.go @@ -126,14 +126,14 @@ func TestRenderTabsAndOffset(t *testing.T) { // TestMarks checks the mark at the start of the code of each line, in the two // layouts. func TestMarks(t *testing.T) { - const old, new = "a\nb\nc\n", "a\nB\nc\n" + const before, after = "a\nb\nc\n", "a\nB\nc\n" // By default a bar marks a changed line, as in herdr-reviewr. Signs gives // + and -. The mark takes two cells in both cases, so the columns do not // move. theme := diff.Theme{ Background: color.Black, - Green: color.RGBA{0, 0xff, 0, 0xff}, - Red: color.RGBA{0xff, 0, 0, 0xff}, + Green: color.RGBA{G: 0xff, A: 0xff}, + Red: color.RGBA{R: 0xff, A: 0xff}, } tests := []struct { name string @@ -159,7 +159,7 @@ func TestMarks(t *testing.T) { } for _, tt := range tests { tt.o.Theme = theme - d := diff.Render("x.txt", old, new, tt.o) + d := diff.Render("x.txt", before, after, tt.o) if len(d.Lines) != 1+len(tt.lines) { t.Fatalf("%s: %d lines, want %d", tt.name, len(d.Lines), 1+len(tt.lines)) } @@ -182,8 +182,8 @@ func TestMarks(t *testing.T) { } // The review text of a comment uses + and - for both kinds of mark. for _, signs := range []bool{false, true} { - d := diff.Render("x.txt", old, new, diff.Options{Width: 40, Signs: signs}) - got := snippet(d.Rows[2:5], old, new) + d := diff.Render("x.txt", before, after, diff.Options{Width: 40, Signs: signs}) + got := snippet(d.Rows[2:5], before, after) if want := []string{"-b", "+B", " c"}; !slices.Equal(got, want) { t.Errorf("signs %v: snippet = %q, want %q", signs, got, want) } diff --git a/internal/diffview/terminal.go b/internal/diffview/terminal.go index c497f90..dc0278e 100644 --- a/internal/diffview/terminal.go +++ b/internal/diffview/terminal.go @@ -27,7 +27,7 @@ func mix(a, b color.Color, t float64) color.Color { br, bg, bb, _ := b.RGBA() // RGBA has 16 bits for each channel. Divide by 257 to get 8 bits. f := func(x, y uint32) uint8 { return uint8((float64(x)*(1-t) + float64(y)*t) / 257) } - return color.RGBA{f(ar, br), f(ag, bg), f(ab, bb), 255} + return color.RGBA{R: f(ar, br), G: f(ag, bg), B: f(ab, bb), A: 255} } // The amounts that TerminalStyle mixes into the background. diff --git a/tui/nav.go b/tui/nav.go index 2b0cdca..b609694 100644 --- a/tui/nav.go +++ b/tui/nav.go @@ -318,12 +318,10 @@ func hash(s string) uint64 { func (m *model) expanded(path string, comments []*diff.Comment, f diff.Fold) bool { // A fold that overlaps a range shows. An undo of a change next to a // range makes a larger fold, which shows as a whole. - if o := m.folds[path]; o != nil { - for _, r := range o.Ranges { - if f.Old <= r[1] && f.Old+f.Lines-1 >= r[0] { - return true - } - } + if o := m.folds[path]; o != nil && slices.ContainsFunc(o.Ranges, func(r [2]int) bool { + return f.Old <= r[1] && f.Old+f.Lines-1 >= r[0] + }) { + return true } for _, c := range comments { lo := f.New From 3eb689ea5ed144dd05c1688b729c1230742bc9b6 Mon Sep 17 00:00:00 2001 From: Redowan Delowar Date: Sun, 25 Oct 2026 12:05:30 +0100 Subject: [PATCH 7/8] docs: plain English comments and clear test failure messages Comments use the present tense, one idea in each sentence, and lines of 80 columns or fewer. Each test failure message tells what it got and what it wants. No code changes. --- cmd/rexdiff/install.go | 12 ++-- cmd/rexdiff/install_test.go | 20 +++--- cmd/rexdiff/main.go | 15 ++-- cmd/rexdiff/script_test.go | 4 +- diff/cache_test.go | 4 +- diff/comment.go | 10 +-- diff/comment_test.go | 46 +++++++------ diff/diff.go | 13 ++-- diff/diff_test.go | 36 +++++----- diff/file.go | 8 +-- diff/fold_test.go | 12 ++-- diff/fuzz_test.go | 56 ++++++++------- diff/hostile_test.go | 5 +- diff/review.go | 3 +- internal/diffview/ansi.go | 4 +- internal/diffview/cache_test.go | 12 ++-- internal/diffview/diffview.go | 48 +++++++------ internal/diffview/escape.go | 3 +- internal/diffview/fold.go | 6 +- internal/diffview/rows.go | 7 +- internal/diffview/terminal.go | 4 +- internal/editor/editor.go | 2 +- internal/termtext/termtext.go | 6 +- internal/termtext/termtext_test.go | 5 +- rex/rex.go | 16 +++-- rex/rex_test.go | 19 ++--- tui/bench_test.go | 7 +- tui/comment.go | 7 +- tui/config.go | 5 +- tui/editor.go | 8 ++- tui/editor_test.go | 15 ++-- tui/fold_test.go | 25 +++---- tui/keys.go | 4 +- tui/model.go | 66 +++++++++--------- tui/mouse.go | 10 +-- tui/nav.go | 26 +++---- tui/overlay.go | 18 +++-- tui/pr.go | 8 +-- tui/release.go | 4 +- tui/release_test.go | 54 ++++++++------- tui/render.go | 12 ++-- tui/resume.go | 4 +- tui/send.go | 20 +++--- tui/tui.go | 2 +- tui/tui_test.go | 107 ++++++++++++++++------------- tui/view.go | 20 +++--- update/update.go | 23 ++++--- update/update_test.go | 47 ++++++++----- vcs/hostile_test.go | 21 +++--- vcs/pr_test.go | 7 +- vcs/refs.go | 7 +- vcs/repo.go | 5 +- vcs/repo_test.go | 60 ++++++++-------- 53 files changed, 530 insertions(+), 438 deletions(-) diff --git a/cmd/rexdiff/install.go b/cmd/rexdiff/install.go index 9e8094c..55f536e 100644 --- a/cmd/rexdiff/install.go +++ b/cmd/rexdiff/install.go @@ -68,7 +68,7 @@ end -- runs_rexdiff returns true if the terminal block id runs rexdiff. rexdiff -- reports app=rexdiff to Rex with OSC 7501. Rex keeps an idle status after --- its program stops, so the block counts only while the process that +-- its program stops. Thus the block counts only while the process that -- reported the status is in the foreground. local function runs_rexdiff(id) for _, r in ipairs(call("program_status", id).records or {}) do @@ -92,8 +92,8 @@ local function reviewers() end -- check fails the action if rexdiff does not accept config, the settings --- as a JSON object. Here the user sees the error. If rexdiff stops at once --- in a new pane, the pane closes and the error is lost. +-- as a JSON object. The user sees an error of the action. If rexdiff stops +-- at once in a new pane, the pane closes and the user does not see the error. local function check(config) local r, err = rex.exec({ BIN, "--check", "--config", config }) if r == nil then error(err, 0) end @@ -213,7 +213,7 @@ func splitLua(path, content string) (settings, code string, err error) { } // newLua returns the content of rexdiff.lua with the code for bin. It -// keeps the settings of old, the content that the file has now. If old is +// keeps the settings of old, the current content of the file. If old is // empty, it uses the default settings. func newLua(path, old, bin string) (string, error) { if old == "" { @@ -236,8 +236,8 @@ func readFile(path string) (string, error) { } // executable returns the path of the rexdiff binary, for the Rex action. It -// keeps a symlink, such as the one from Homebrew. The target of the link -// changes at each upgrade. +// keeps a symlink, such as the one from Homebrew, because the target of the +// link changes at each upgrade. func executable() (string, error) { bin, err := os.Executable() if err != nil { diff --git a/cmd/rexdiff/install_test.go b/cmd/rexdiff/install_test.go index 264c082..ec91c29 100644 --- a/cmd/rexdiff/install_test.go +++ b/cmd/rexdiff/install_test.go @@ -21,7 +21,7 @@ func fakeRexDir(t *testing.T, log string) string { } // TestUpgrade checks that upgrade replaces the code of rexdiff.lua if it -// differs from the code of the binary, and keeps the settings. +// differs from the code of the binary. It keeps the settings. func TestUpgrade(t *testing.T) { dir := t.TempDir() reloads := filepath.Join(dir, "reloads") @@ -51,7 +51,7 @@ func TestUpgrade(t *testing.T) { t.Fatalf("upgrade without rexdiff.lua = %q, want no notice", n) } if _, err := os.Stat(lua); err == nil { - t.Fatal("upgrade made a rexdiff.lua") + t.Fatal("upgrade without rexdiff.lua: the file exists, want no file") } // If the file has old code for the same binary, upgrade replaces the @@ -62,7 +62,8 @@ func TestUpgrade(t *testing.T) { t.Fatalf("upgrade = %q, want no notice", n) } if got := read(); got != settings+luaCode(bin) { - t.Errorf("rexdiff.lua after upgrade:\n%s\nwant the settings and the new code", got) + t.Errorf("rexdiff.lua after upgrade got:\n%s\nwant:\n%s", + got, settings+luaCode(bin)) } if b, _ := os.ReadFile(reloads); string(b) != "config reload\n" { t.Errorf("rex calls = %q, want one config reload", b) @@ -89,23 +90,24 @@ func TestUpgrade(t *testing.T) { // tells what is wrong and what to do. noMark := strings.Replace(settings+old, codeMark+"\n", "", 1) if !strings.Contains(noMark, luaQuote(bin)) { - t.Fatal("the test file does not run bin") + t.Fatalf("the test file has no %s, want it", luaQuote(bin)) } write(noMark) n := upgrade(ctx, bin) for _, want := range []string{"cannot upgrade rexdiff.lua", lua, codeMark, "cannot tell your settings from its code", "Put that line back"} { if !strings.Contains(n, want) { - t.Errorf("notice %q does not have %q", n, want) + t.Errorf("notice = %q, want %q in it", n, want) } } - if read() != noMark { - t.Error("upgrade changed rexdiff.lua without the line above the code") + if got := read(); got != noMark { + t.Errorf("rexdiff.lua without the line above the code, after upgrade "+ + "got:\n%s\nwant:\n%s", got, noMark) } } -// TestLuaQuote checks that a path that Go would escape stays the same in -// Lua 5.1, which knows only a few escapes. +// TestLuaQuote checks that a path that Go escapes stays the same in Lua 5.1. +// Lua 5.1 knows only a few escapes. func TestLuaQuote(t *testing.T) { got := luaQuote("/a\u200bb/\"x\"\\y\nz") want := "\"/a\u200bb/\\\"x\\\"\\\\y\\nz\"" diff --git a/cmd/rexdiff/main.go b/cmd/rexdiff/main.go index 584039d..6bcea67 100644 --- a/cmd/rexdiff/main.go +++ b/cmd/rexdiff/main.go @@ -86,8 +86,8 @@ func run(ctx context.Context, args []string, stdout, stderr io.Writer) error { } defer closeLog() - // In Rex, upgrade replaces the code of the action if it differs from - // the code of this rexdiff. + // In Rex, upgrade replaces the code of the action if that code differs + // from the code of this rexdiff. client := rex.FromEnv() var notice string bin, binErr := executable() @@ -121,7 +121,9 @@ func run(ctx context.Context, args []string, stdout, stderr io.Writer) error { if err = os.Setenv(resumeEnv, resume); err == nil { err = update.Restart(bin, args) // returns only if it fails } - fmt.Print("\x1b[?1049l\x1b[?25h") // leave the screen that Run kept, show the cursor + // Run keeps the alternate screen for the restart. Leave that screen and + // show the cursor. + fmt.Print("\x1b[?1049l\x1b[?25h") return fmt.Errorf("start the new rexdiff: %w; your comments are saved in %s", err, resume) } @@ -130,12 +132,13 @@ func run(ctx context.Context, args []string, stdout, stderr io.Writer) error { // flag, so that the arguments of the user do not change. const resumeEnv = "REXDIFF_RESUME" -// resumeFile returns the file in resumeEnv, or "", and unsets the variable, +// resumeFile returns the file in resumeEnv, or "". It unsets the variable, // so that the programs that rexdiff starts, such as an editor, do not get -// it. +// the variable. func resumeFile() string { path := os.Getenv(resumeEnv) - _ = os.Unsetenv(resumeEnv) // fails only for a bad name, and resumeEnv is good + // Unsetenv fails only for a bad name. resumeEnv is a good name. + _ = os.Unsetenv(resumeEnv) return path } diff --git a/cmd/rexdiff/script_test.go b/cmd/rexdiff/script_test.go index fb4b519..74c6cd6 100644 --- a/cmd/rexdiff/script_test.go +++ b/cmd/rexdiff/script_test.go @@ -19,8 +19,8 @@ func TestMain(m *testing.M) { }) } -// fakeRex writes its arguments to $WORK/rex.log, so that a script can see -// what rexdiff asked Rex to do. It fails if $REX_FAIL is set. +// fakeRex writes its arguments to $WORK/rex.log. A script reads the log to +// see what rexdiff asks Rex to do. fakeRex fails if $REX_FAIL is set. func fakeRex() { log := filepath.Join(os.Getenv("WORK"), "rex.log") f, err := os.OpenFile(log, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644) diff --git a/diff/cache_test.go b/diff/cache_test.go index 7554cfd..6d7203e 100644 --- a/diff/cache_test.go +++ b/diff/cache_test.go @@ -6,7 +6,7 @@ import ( ) // TestCacheKeepsFile checks that RenderFile uses the colours that the cache -// keeps for the same file, and colours a changed file again. +// keeps for the same file. RenderFile colours a changed file again. func TestCacheKeepsFile(t *testing.T) { var c Cache RenderFile("a.go", "x := 1\n", Options{Cache: &c}) @@ -15,6 +15,6 @@ func TestCacheKeepsFile(t *testing.T) { t.Errorf("the same file shows %q, want the kept colours", got) } if got := RenderFile("a.go", "x := 2\n", Options{Cache: &c}).Lines[0]; strings.Contains(got, "kept") { - t.Errorf("a changed file shows %q, the colours of the old file", got) + t.Errorf("a changed file shows %q, want new colours", got) } } diff --git a/diff/comment.go b/diff/comment.go index 469efb1..43c3439 100644 --- a/diff/comment.go +++ b/diff/comment.go @@ -14,7 +14,7 @@ import ( // [NewComment] to make one. A comment keeps the text of its lines, for // [Comments.Relocate], and its diff rows, for [Comments.Review]. type Comment struct { - ID int // set by [Comments.Add] + ID int // [Comments.Add] sets it Path string Side Side // Start and End are the first and the last line of the range. They @@ -57,8 +57,8 @@ func NewComment(path string, s Side, rows []Row, old, new string) Comment { } // Location returns the path and the lines of c, for example "a.go:3-4". -// A comment on the old side has " (removed)" at the end. The path is -// quoted with [Quote]. +// A comment on the old side has " (removed)" at the end. [Quote] quotes +// the path. func (c *Comment) Location() string { loc := fmt.Sprintf("%s:%d", termtext.Quote(c.Path), c.Start) if c.End != c.Start { @@ -151,8 +151,8 @@ func (cs *Comments) Count(path string) int { // one. If the version does not have the text, the comment stays and // becomes stale. func (cs *Comments) Relocate(path, old, new string) { - // The lines of each side. Relocate splits a side only if a comment - // needs it. + // lines has the lines of each side. Relocate splits a side only if a + // comment needs that side. var lines [2][]string for _, c := range cs.items { if c.Path != path || len(c.code) == 0 { diff --git a/diff/comment_test.go b/diff/comment_test.go index 11dd157..b3a9e9d 100644 --- a/diff/comment_test.go +++ b/diff/comment_test.go @@ -58,10 +58,10 @@ func TestReviewMatchesHerdr(t *testing.T) { "c.go:1-2 (stale: the code has changed since)\n" + " ctx\n-\told[31m\n+\tnew\nfix]7501;state=idle this" if got != want { - t.Errorf("Review:\n%s\nwant:\n%s", got, want) + t.Errorf("Review got:\n%s\nwant:\n%s", got, want) } if got := new(diff.Comments).Review(""); got != "" { - t.Errorf("Review of nothing = %q", got) + t.Errorf("Review with no comments = %q, want \"\"", got) } } @@ -80,7 +80,8 @@ func TestReviewContinuedRows(t *testing.T) { // The comment has no diff lines, so Review writes the lines of its range. want := "a.go:1\n+" + strings.TrimSuffix(new, "\n") + "\nlong" if got := review(c); len(cont) == 0 || got != want { - t.Errorf("%d continued rows, Review:\n%s\nwant:\n%s", len(cont), got, want) + t.Errorf("%d continued rows, Review got:\n%s\nwant:\n%s", + len(cont), got, want) } } @@ -91,27 +92,30 @@ func TestComments(t *testing.T) { b := cs.Add(diff.Comment{Path: "a.go", Side: diff.SideOld, Start: 4, End: 4}) cs.Add(diff.Comment{Path: "b.go", Side: diff.SideNew, Start: 1, End: 1}) if cs.Len() != 3 || cs.Count("a.go") != 2 || a.ID == b.ID { - t.Fatalf("Len = %d, Count(a.go) = %d, IDs %d %d", + t.Fatalf("Len = %d, Count(a.go) = %d, IDs %d %d; "+ + "want 3, 2, two different IDs", cs.Len(), cs.Count("a.go"), a.ID, b.ID) } if c := cs.At("a.go", diff.SideNew, 3); c != a { - t.Errorf("At(a.go, New, 3) = %v", c) + t.Errorf("At(a.go, New, 3) = %v, want %v", c, a) } if c := cs.At("a.go", diff.SideNew, 5); c != nil { t.Errorf("At(a.go, New, 5) = %v, want nil", c) } if got := cs.EndingAt("a.go", diff.SideOld, 4); len(got) != 1 || got[0] != b { - t.Errorf("EndingAt(a.go, Old, 4) = %v", got) + t.Errorf("EndingAt(a.go, Old, 4) = %v, want [%v]", got, b) } if cs.Find(b.ID) != b { - t.Error("Find did not find b") + t.Errorf("Find(%d) = %v, want %v", b.ID, cs.Find(b.ID), b) } cs.Remove(b.ID) if cs.Find(b.ID) != nil || cs.Len() != 2 { - t.Error("Remove did not remove b") + t.Errorf("after Remove(%d): Find = %v, Len = %d; want nil, 2", + b.ID, cs.Find(b.ID), cs.Len()) } if c := cs.Add(diff.Comment{}); c.ID == a.ID || c.ID == b.ID { - t.Error("Add reused an ID") + t.Errorf("Add gives ID %d, want an ID other than %d and %d", + c.ID, a.ID, b.ID) } } @@ -136,7 +140,7 @@ func TestNewComment(t *testing.T) { } want += "\n a\n-b\n-c\n+B\n+C\n d\n" if got := review(c); got != want { - t.Errorf("split %v, side %d: Review:\n%s\nwant:\n%s", + t.Errorf("split %v, side %d: Review got:\n%s\nwant:\n%s", split, s, got, want) } // The comment follows the lines of its side. @@ -161,7 +165,8 @@ func TestNewComment(t *testing.T) { t.Errorf("split %v, the new lines: %+v, want lines 2-3", split, got) } if cs.Relocate("x.txt", "", "B\nC\n"); got.Start != 1 || got.Stale { - t.Errorf("split %v, the new lines did not follow B and C: %+v", split, got) + t.Errorf("split %v, the new lines after a move: %+v, want line 1", + split, got) } } // A hunk header has no line. @@ -183,26 +188,27 @@ func TestRelocateFollowsMovedCode(t *testing.T) { // Each comment looks in the version of its side. cs.Relocate("a.go", "x\nx\nx\nb\n", "new\nnew\na\nb\nc\n") if c.Start != 4 || c.End != 5 || c.Stale { - t.Errorf("moved: %+v", c) + t.Errorf("after a move: %+v, want lines 4-5", c) } if other.Start != 4 || other.Stale { - t.Errorf("the comment on the old side did not follow the old version: %+v", other) + t.Errorf("the comment on the old side after a move: %+v, want line 4", + other) } cs.Relocate("a.go", "b\n", "a\nB\nc\n") if !c.Stale || c.Start != 4 { - t.Errorf("not stale: %+v", c) + t.Errorf("after an edit: %+v, want stale at line 4", c) } cs.Relocate("a.go", "b\n", "b\nc\nx\nx\nx\nb\nc\n") if c.Stale || c.Start != 6 { - t.Errorf("did not go to the nearest copy: %+v", c) + t.Errorf("with two copies: %+v, want line 6", c) } - // The end of a file is not an empty line, and CRLF is the same as LF. + // The end of a file is not an empty line. CRLF is the same as LF. blank := cs.Add(commentOn("b.go", diff.SideNew, "x\n\n", 2, 2)) if cs.Relocate("b.go", "", "x\n"); !blank.Stale { - t.Errorf("a comment on a deleted blank line is not stale: %+v", blank) + t.Errorf("a comment on a deleted blank line: %+v, want stale", blank) } if cs.Relocate("b.go", "", "x\r\n\r\n"); blank.Stale || blank.Start != 2 { - t.Errorf("a comment on a CRLF blank line: %+v", blank) + t.Errorf("a comment on a CRLF blank line: %+v, want line 2", blank) } } @@ -216,7 +222,7 @@ func TestSnippetIsTheRealDiff(t *testing.T) { d := diff.Render("x.go", old, new, diff.Options{Split: split}) got := strings.Join(snippet(d.Rows, old, new), "\n") if got != want { - t.Errorf("split %v:\n%s\nwant:\n%s", split, got, want) + t.Errorf("split %v: snippet got:\n%s\nwant:\n%s", split, got, want) } } @@ -235,6 +241,6 @@ func TestSnippetIsTheRealDiff(t *testing.T) { out, _ := git.Output() _, body, _ := strings.Cut(string(out), "@@\n") if strings.TrimSuffix(body, "\n") != want { - t.Errorf("git diff:\n%s\nwant:\n%s", body, want) + t.Errorf("git diff got:\n%s\nwant:\n%s", body, want) } } diff --git a/diff/diff.go b/diff/diff.go index f1c434c..b8daf12 100644 --- a/diff/diff.go +++ b/diff/diff.go @@ -23,7 +23,7 @@ const ( KindEqual // a line that is the same on the two sides KindInsert // a line that only the new side has KindDelete // a line that only the old side has - KindChange // in the split layout, a deleted line beside an inserted line + KindChange // a deleted line beside an inserted line (split) ) // Side identifies one version of a file. @@ -37,7 +37,7 @@ const ( // Row tells which source lines one rendered line shows. Old and New are // line numbers that start at 1. A value of 0 means that the row has no line // on that side. Continued is true if the line continues the source lines of -// the line above, because [Options.Wrap] wrapped them. +// the line above, because [Options.Wrap] wraps them. // // A long run of equal lines folds into the hunk header below it. For a // hunk header, Fold is that run. @@ -48,9 +48,9 @@ type Row struct { Fold Fold } -// Fold is a run of equal lines that hides. Old and New are the line -// numbers of its first line. Lines is the number of lines. It is 0 if no -// line hides. +// Fold is a run of equal lines that the diff hides. Old and New are the +// line numbers of its first line. Lines is the number of lines. Lines is 0 +// if the diff hides no line. type Fold struct { Old, New, Lines int } @@ -83,7 +83,8 @@ type Options struct { Signs bool // Theme sets the colours. Theme Theme - // Cache makes a new render of the same lines fast. Nil turns it off. + // Cache makes a new render of the same lines fast. If Cache is nil, the + // render keeps nothing. Cache *Cache } diff --git a/diff/diff_test.go b/diff/diff_test.go index 12b7d49..9e62a72 100644 --- a/diff/diff_test.go +++ b/diff/diff_test.go @@ -90,11 +90,13 @@ func TestRenderRowsFollowLines(t *testing.T) { } } if len(d.Lines) != len(d.Rows) { - t.Fatalf("%s: %d lines for %d rows", name, len(d.Lines), len(d.Rows)) + t.Fatalf("%s: %d lines for %d rows, want one line for each row", + name, len(d.Lines), len(d.Rows)) } for i, l := range d.Lines { if w := ansi.StringWidth(l); w > width { - t.Errorf("%s: line %d is %d cells wide", name, i, w) + t.Errorf("%s: line %d is %d cells wide, want %d or fewer", + name, i, w, width) } } } @@ -114,12 +116,12 @@ func TestRenderSameContentIsEmpty(t *testing.T) { func TestRenderTabsAndOffset(t *testing.T) { d := diff.Render("x.go", "", "\tx := 1\n", diff.Options{Width: 80}) if got := ansi.Strip(d.Lines[1]); !strings.Contains(got, "▌ x := 1") { - t.Errorf("tab: line = %q", got) + t.Errorf("tab: line = %q, want %q in it", got, "▌ x := 1") } d = diff.Render("x.go", "", "\tx := 1\n", diff.Options{Width: 80, Offset: 6}) got := ansi.Strip(d.Lines[1]) if strings.Contains(got, "x :=") || !strings.Contains(got, "= 1") { - t.Errorf("offset 6: line = %q", got) + t.Errorf("offset 6: line = %q, want \"= 1\" and no \"x :=\"", got) } } @@ -138,7 +140,7 @@ func TestMarks(t *testing.T) { tests := []struct { name string o diff.Options - lines []string // the screen lines under the hunk header, without colours + lines []string // lines under the hunk header, no colours }{ {"unified bar", diff.Options{Width: 40}, []string{ " 1 1 a", " 2 ▌ b", " 2 ▌ B", " 3 3 c", @@ -275,18 +277,19 @@ func TestRenderFile(t *testing.T) { t.Fatalf("got %d lines and %d rows, want 5", len(d.Lines), len(d.Rows)) } if d.Rows[3] != (diff.Row{Kind: diff.KindEqual, Old: 4, New: 4}) { - t.Errorf("row 3 = %+v", d.Rows[3]) + t.Errorf("row 3 = %+v, want %+v", + d.Rows[3], diff.Row{Kind: diff.KindEqual, Old: 4, New: 4}) } if got := ansi.Strip(d.Lines[2]); got != " 3 func f() {" { - t.Errorf("line 2 = %q", got) + t.Errorf("line 2 = %q, want %q", got, " 3 func f() {") } if got := ansi.Strip(d.Lines[0]); got != " 1 package x" { - t.Errorf("line 0 = %q", got) + t.Errorf("line 0 = %q, want %q", got, " 1 package x") } // Control characters must not reach the terminal. d = diff.RenderFile("x.txt", "a\x1b[31mb\n", diff.Options{}) if strings.Contains(ansi.Strip(d.Lines[0]), "\x1b") { - t.Errorf("line 0 = %q, has an escape", d.Lines[0]) + t.Errorf("line 0 = %q, want no escape", d.Lines[0]) } } @@ -322,7 +325,8 @@ func TestWrap(t *testing.T) { var text strings.Builder for i, l := range d.Lines { if w := ansi.StringWidth(l); w > width { - t.Errorf("%s: line %d is %d cells wide", name, i, w) + t.Errorf("%s: line %d is %d cells wide, want %d or fewer", + name, i, w, width) } r := d.Rows[i] if r.New == 2 { @@ -332,25 +336,25 @@ func TestWrap(t *testing.T) { prev := d.Rows[max(i-1, 0)] sameLines := prev.Old == r.Old && prev.New == r.New if r.Continued && !sameLines { - t.Errorf("%s: continued row %d = %+v, the row above = %+v", + t.Errorf("%s: continued row %d = %+v, want the lines of %+v", name, i, r, prev) } } noSpace := func(s string) string { return strings.Join(strings.Fields(s), "") } if !strings.Contains(noSpace(text.String()), noSpace(long)) { - t.Errorf("%s: the wrapped lines do not show all of the line:\n%s", - name, text.String()) + t.Errorf("%s: the wrapped lines got:\n%s\nwant all of:\n%s", + name, text.String(), long) } // Wrap does not change the snippet of a comment. got, want := snippet(d.Rows, old, new), snippet(cut.Rows, old, new) if !slices.Equal(got, want) { - t.Errorf("%s: snippet with wrap %q, without %q", name, got, want) + t.Errorf("%s: snippet with wrap = %q, want %q", name, got, want) } } } // TestCache checks that a shared [diff.Cache] gives the same lines as no -// cache: for other offsets, for other languages, and for new content. +// cache. It tries other offsets, other languages, and new content. func TestCache(t *testing.T) { const src = "def f(): # a comment\n\treturn 1\n" renders := []struct { @@ -375,7 +379,7 @@ func TestCache(t *testing.T) { return diff.Render(r.path, r.old, r.new, o).Lines } if got, want := render(&c), render(nil); !slices.Equal(got, want) { - t.Errorf("%+v: with the cache\n%q\nwant\n%q", r, got, want) + t.Errorf("%+v: with the cache got:\n%q\nwant:\n%q", r, got, want) } } } diff --git a/diff/file.go b/diff/file.go index 8ebd943..e8bfaeb 100644 --- a/diff/file.go +++ b/diff/file.go @@ -11,8 +11,8 @@ import ( "github.com/rednafi/rexdiff/internal/diffview" ) -// lineNumberColor is the ANSI palette colour of the line numbers: bright -// black. +// lineNumberColor is the ANSI palette colour of the line numbers. Colour 8 +// is bright black. const lineNumberColor = "8" // RenderFile shows src, the content of the file at path, with line numbers @@ -27,12 +27,12 @@ func RenderFile(path, src string, o Options) *Diff { "\t", strings.Repeat(" ", tabWidth), ).Replace(src) hl := strings.Split(o.Cache.fileColours(path, src), "\n") - hl = hl[:min(len(hl), len(Lines(src)))] // chroma can add a line break at the end + hl = hl[:min(len(hl), len(Lines(src)))] // chroma can add a final newline digits := len(strconv.Itoa(len(hl))) numWidth := digits + 2 // a space on each side of the number numStyle := lipgloss.NewStyle().Foreground(lipgloss.Color(lineNumberColor)) d := &Diff{newNum: numWidth} - code := o.Width - numWidth - 1 // the width that the number and a space leave + code := o.Width - numWidth - 1 // the width after the number and a space blank := strings.Repeat(" ", numWidth+1) for i, l := range hl { for k, p := range fileLineParts(l, code, o) { diff --git a/diff/fold_test.go b/diff/fold_test.go index 9f4b3b5..89875d3 100644 --- a/diff/fold_test.go +++ b/diff/fold_test.go @@ -98,9 +98,9 @@ func TestFolds(t *testing.T) { } } - // The header tells how many lines hide. A fold at the end has no hunk - // header. In the split layout, the header goes on across the right - // side. No line is wider than the screen. + // The header gives the number of hidden lines. A fold at the end has no + // hunk header. In the split layout, the header continues across the + // right side. No line is wider than the screen. for _, tt := range []struct { o diff.Options want string @@ -114,11 +114,13 @@ func TestFolds(t *testing.T) { } { d := diff.Render("f.txt", old, new, tt.o) if got := headers(d); got != tt.want { - t.Errorf("split %v: headers:\n%s\nwant:\n%s", tt.o.Split, got, tt.want) + t.Errorf("split %v: headers got:\n%s\nwant:\n%s", + tt.o.Split, got, tt.want) } for i, l := range d.Lines { if w := ansi.StringWidth(l); w > tt.o.Width { - t.Errorf("split %v: line %d is %d cells wide", tt.o.Split, i, w) + t.Errorf("split %v: line %d is %d cells wide, want %d or fewer", + tt.o.Split, i, w, tt.o.Width) } } } diff --git a/diff/fuzz_test.go b/diff/fuzz_test.go index df13474..304682b 100644 --- a/diff/fuzz_test.go +++ b/diff/fuzz_test.go @@ -66,15 +66,17 @@ func fuzzOptions(layout, width, offset uint8, colored bool) diff.Options { func checkLines(t *testing.T, d *diff.Diff, o diff.Options) { t.Helper() if len(d.Lines) != len(d.Rows) { - t.Fatalf("%d lines for %d rows", len(d.Lines), len(d.Rows)) + t.Fatalf("got %d lines for %d rows, want one line for each row", + len(d.Lines), len(d.Rows)) } for i, l := range d.Lines { if w := ansi.StringWidth(l); o.Width > 0 && w > o.Width { - t.Errorf("line %d is %d cells wide, width is %d: %q", i, w, o.Width, l) + t.Errorf("line %d is %d cells wide, want %d or fewer: %q", + i, w, o.Width, l) } rest := sgr.ReplaceAllString(l, "") if strings.ContainsFunc(rest, unicode.IsControl) { - t.Errorf("line %d has a control character: %q", i, l) + t.Errorf("line %d = %q, want no control characters", i, l) } } } @@ -86,26 +88,28 @@ func checkRows(t *testing.T, rows []diff.Row, old, new string) { lastOld, lastNew := 0, 0 for i, r := range rows { if r.Old < 0 || r.Old > nOld || r.New < 0 || r.New > nNew { - t.Fatalf("row %d = %+v, the files have %d and %d lines", i, r, nOld, nNew) + t.Fatalf("row %d = %+v, want Old in 0-%d and New in 0-%d", + i, r, nOld, nNew) } if r.Continued { if i == 0 || rows[i-1].Old != r.Old || rows[i-1].New != r.New { - t.Fatalf("continued row %d = %+v does not continue the row above", i, r) + t.Fatalf("continued row %d = %+v, want the lines of the row above", + i, r) } continue } oldBack := r.Old != 0 && r.Old <= lastOld newBack := r.New != 0 && r.New <= lastNew if oldBack || newBack { - t.Fatalf("row %d = %+v is not after old line %d and new line %d", + t.Fatalf("row %d = %+v, want it after old line %d and new line %d", i, r, lastOld, lastNew) } lastOld, lastNew = max(lastOld, r.Old), max(lastNew, r.New) } } -// FuzzRender checks the invariants of Render, and that Count agrees with the -// snippet. +// FuzzRender checks the invariants of Render. It also checks that Count +// agrees with the snippet. func FuzzRender(f *testing.F) { for i, s := range hostile { f.Add(uint8(i), "a\nb\n", s, uint8(i), uint8(i*7), uint8(i%3), i%2 == 0) @@ -129,14 +133,15 @@ func FuzzRender(f *testing.F) { for i, l := range d.Lines { cut := strings.Contains(ansi.Strip(l), "…") if cut && d.Rows[i].Kind != diff.KindHunk { - t.Fatalf("line %d is cut with no width limit: %q", i, l) + t.Fatalf("line %d = %q, want no cut with no width limit", i, l) } } } added, deleted := diff.Count(old, new) if (added == 0 && deleted == 0) != (len(d.Lines) == 0) { - t.Fatalf("Count = %d, %d, but Render has %d lines", + t.Fatalf("Count = %d, %d and Render has %d lines, "+ + "want zero counts only for zero lines", added, deleted, len(d.Lines)) } var plus, minus int @@ -149,7 +154,7 @@ func FuzzRender(f *testing.F) { } } if plus != added || minus != deleted { - t.Fatalf("the snippet has +%d -%d, Count = +%d -%d", + t.Fatalf("the snippet has +%d -%d, want Count = +%d -%d", plus, minus, added, deleted) } }) @@ -167,7 +172,8 @@ func FuzzSnippetSameInLayouts(f *testing.F) { s := diff.Render("x.txt", old, new, o) got, want := snippet(s.Rows, old, new), snippet(u.Rows, old, new) if !slices.Equal(got, want) { - t.Fatalf("split snippet %q, unified snippet %q", got, want) + t.Fatalf("split snippet = %q, want the unified snippet %q", + got, want) } }) } @@ -197,7 +203,7 @@ func FuzzRenderFile(f *testing.F) { } } if want := len(diff.Lines(src)); n != want { - t.Fatalf("%d rows for a file of %d lines", n, want) + t.Fatalf("RenderFile gives %d rows, want %d", n, want) } }) } @@ -210,24 +216,27 @@ func FuzzCount(f *testing.F) { } f.Fuzz(func(t *testing.T, a, b string) { if added, deleted := diff.Count(a, a); added != 0 || deleted != 0 { - t.Errorf("Count(s, s) = %d, %d", added, deleted) + t.Errorf("Count(%q, %q) = %d, %d, want 0, 0", a, a, added, deleted) } na, nb := len(diff.Lines(a)), len(diff.Lines(b)) if added, deleted := diff.Count("", a); added != na || deleted != 0 { - t.Errorf("Count(\"\", %q) = %d, %d; want %d, 0", a, added, deleted, na) + t.Errorf("Count(\"\", %q) = %d, %d, want %d, 0", + a, added, deleted, na) } if added, deleted := diff.Count(a, ""); added != 0 || deleted != na { - t.Errorf("Count(%q, \"\") = %d, %d; want 0, %d", a, added, deleted, na) + t.Errorf("Count(%q, \"\") = %d, %d, want 0, %d", + a, added, deleted, na) } added, deleted := diff.Count(a, b) if added < 0 || deleted < 0 || added > nb || deleted > na { - t.Errorf("Count(%q, %q) = %d, %d", a, b, added, deleted) + t.Errorf("Count(%q, %q) = %d, %d, want 0-%d, 0-%d", + a, b, added, deleted, nb, na) } }) } -// FuzzComments checks that Relocate keeps a valid range, and that the review -// text has no control characters. +// FuzzComments checks that Relocate keeps a valid range. It also checks that +// the review text has no control characters. func FuzzComments(f *testing.F) { for i, s := range hostile { f.Add("a\nb\nc\n", s, "a.go", s, uint8(i), uint8(i%3)) @@ -257,18 +266,19 @@ func FuzzComments(f *testing.F) { n := len(diff.Lines(after)) badRange := c.Start < 1 || c.Start > c.End if badRange || (!c.Stale && c.End > n) { - t.Errorf("after Relocate: %+v, the file has %d lines", c, n) + t.Errorf("after Relocate: %+v, want 1 <= Start <= End <= %d", c, n) } if !c.Stale && !slices.Equal(diff.Lines(after)[c.Start-1:c.End], code) { - t.Errorf("after Relocate: lines %d-%d are not the code", c.Start, c.End) + t.Errorf("after Relocate: lines %d-%d = %q, want %q", + c.Start, c.End, diff.Lines(after)[c.Start-1:c.End], code) } r := cs.Review(text) if strings.ContainsFunc(r, isUnsafeControl) { - t.Errorf("Review has a control character: %q", r) + t.Errorf("Review = %q, want no control characters", r) } for _, c := range cs.All() { if l := c.Location(); strings.ContainsFunc(l, unicode.IsControl) { - t.Errorf("Location = %q, has a control character", l) + t.Errorf("Location = %q, want no control characters", l) } } }) diff --git a/diff/hostile_test.go b/diff/hostile_test.go index 8a1d379..149da81 100644 --- a/diff/hostile_test.go +++ b/diff/hostile_test.go @@ -78,7 +78,8 @@ func TestWrapWideText(t *testing.T) { } got := strings.Join(strings.Fields(text.String()), "") if !strings.Contains(got, line) { - t.Errorf("split %v: the wrapped lines lose text: %q", o.Split, got) + t.Errorf("split %v: the wrapped lines = %q, want all of %q", + o.Split, got, line) } } } @@ -146,7 +147,7 @@ func TestBlackTint(t *testing.T) { d := diff.Render("a.txt", "x\ny\nx\n", "y\nx\n", diff.Options{Theme: th}) for i, r := range d.Rows { if r.Kind == diff.KindEqual && strings.Contains(d.Lines[i], "48;2;0;0;0mx") { - t.Errorf("equal line %d has the background of a deleted line: %q", + t.Errorf("equal line %d = %q, want no background of a deleted line", r.New, d.Lines[i]) } } diff --git a/diff/review.go b/diff/review.go index ede0da5..4671b42 100644 --- a/diff/review.go +++ b/diff/review.go @@ -32,7 +32,8 @@ func format(c *Comment) string { if c.Stale { b.WriteString(" (stale: the code has changed since)") } - // Write the rows as the diff shows them, so that the agent reads a real diff. + // Write the rows as the diff shows them, so that the agent reads a real + // diff. if len(c.snippet) > 0 { for _, l := range c.snippet { b.WriteString("\n" + termtext.Plain(l)) diff --git a/internal/diffview/ansi.go b/internal/diffview/ansi.go index ef75c80..cebfc32 100644 --- a/internal/diffview/ansi.go +++ b/internal/diffview/ansi.go @@ -43,8 +43,8 @@ func ansiFor(t chroma.TokenType) (c string, italic, ok bool) { // ANSIFormatter returns a chroma formatter that colours the tokens with the // ANSI palette colours, on the background bgColor. It ignores the chroma -// style. If processValue is not nil, it changes the text of each token -// first. +// style. If processValue is not nil, ANSIFormatter calls it on the text of +// each token first. func ANSIFormatter(bgColor color.Color, processValue func(string) string) chroma.Formatter { format := func(w io.Writer, _ *chroma.Style, it chroma.Iterator) error { plain := lipgloss.NewStyle().Background(bgColor) diff --git a/internal/diffview/cache_test.go b/internal/diffview/cache_test.go index 7bcc840..2a24eb4 100644 --- a/internal/diffview/cache_test.go +++ b/internal/diffview/cache_test.go @@ -7,8 +7,8 @@ import ( "github.com/alecthomas/chroma/v2/lexers" ) -// TestCacheShared checks that a View uses the lines that another View -// highlighted into the same Cache. +// TestCacheShared checks that a View uses the lines that another View puts +// in the same Cache. func TestCacheShared(t *testing.T) { render := func(c *Cache) string { out, _, _ := New().Cache(c).Before("a.go", "x := 1\n").After("a.go", "x := 2\n").Render() @@ -17,7 +17,7 @@ func TestCacheShared(t *testing.T) { c := &Cache{} render(c) if len(c.lines) == 0 { - t.Fatal("the cache is empty after a render") + t.Fatal("the cache is empty after a render, want lines") } for k := range c.lines { c.lines[k] = "kept" @@ -50,12 +50,12 @@ func TestCacheLimit(t *testing.T) { // TestCacheNil checks that a View with a nil Cache renders. func TestCacheNil(t *testing.T) { if out, _, _ := New().Cache(nil).Before("a.go", "x\n").After("a.go", "y\n").Render(); out == "" { - t.Error("the render is empty") + t.Error(`Render with a nil Cache = "", want lines`) } } // TestCacheLexer checks that a View uses the last lexer of the Cache for -// the same file, and selects a new lexer for a changed file. +// the same file. It selects a new lexer for a changed file. func TestCacheLexer(t *testing.T) { c := &Cache{} lexer := func(content string) string { @@ -68,6 +68,6 @@ func TestCacheLexer(t *testing.T) { t.Errorf("the same file has the lexer %q, want the kept Python", got) } if got := lexer("y\n"); got == "Python" { - t.Error("a changed file has the kept lexer") + t.Errorf("a changed file has the lexer %q, want a new lexer", got) } } diff --git a/internal/diffview/diffview.go b/internal/diffview/diffview.go index 36ed606..46f9313 100644 --- a/internal/diffview/diffview.go +++ b/internal/diffview/diffview.go @@ -23,8 +23,9 @@ const ( ellipsis = "…" // shows where a line is cut ) -// The symbols at the start of the code of a line. A bar in the colour of the line -// marks each changed line. [View.Signs] shows + and - in place of the bar. +// These symbols are at the start of the code of a line. A bar in the colour +// of the line marks each changed line. [View.Signs] shows + and - in place +// of the bar. const ( symNone = " " // an equal line, a missing line, or a continued line symBar = "▌ " @@ -71,7 +72,7 @@ type View struct { afterNumDigits int heights []int // the number of screen lines of each row, for rows - // The View keeps the lexer. To match a file name is slow. + // The View keeps the lexer, because to match a file name is slow. cachedLexer chroma.Lexer cache *Cache @@ -84,8 +85,8 @@ type Cache struct { lines map[syntaxKey]string size int // the bytes of the lines and their sources - // lexer is the last lexer. To select a lexer from the content is - // slow. + // lexer keeps the last lexer, because to select a lexer from the + // content is slow. lexer struct { path, content string l chroma.Lexer @@ -93,7 +94,7 @@ type Cache struct { } // maxCacheSize limits the bytes of the lines and the sources in a Cache. -// A put that goes above it empties the Cache first. +// If a put goes above this size, put empties the Cache first. const maxCacheSize = 32 << 20 // put keeps the highlighted line s for key k. @@ -128,8 +129,8 @@ func (v *View) Cache(c *Cache) *View { return v } -// Split sets the split layout: the old lines on the left, the new lines on -// the right. +// Split sets the split layout. The old lines are on the left and the new +// lines are on the right. func (v *View) Split() *View { v.layout = layoutSplit return v @@ -361,8 +362,8 @@ func (v *View) resizeCodeWidth() { v.fullCodeWidth = v.codeWidth + leadingSymbolsSize } -// renderUnified renders the unified layout: the old and the new line -// numbers, then the code. +// renderUnified renders the unified layout. Each line has the old and the +// new line numbers, then the code. func (v *View) renderUnified() string { var b strings.Builder nums := func(ls lineStyle, before, after any) string { @@ -403,8 +404,8 @@ func (v *View) unifiedHeader(i int) string { div.code.Width(v.fullCodeWidth).Render(header) } -// renderSplit renders the split layout: the old lines on the left, the new -// lines on the right. +// renderSplit renders the split layout. The old lines are on the left and +// the new lines are on the right. func (v *View) renderSplit() string { var b strings.Builder afterWidth := v.codeWidth + btoi(v.extraColOnAfter) @@ -443,8 +444,8 @@ func (v *View) splitHeader(i int) string { } // splitSide renders line l on one side of the split layout, one string for -// each screen line. l is nil if that side has no line, and n is its line -// number. The fill function renders an empty screen line. Use it when the +// each screen line. l is nil if that side has no line. n is the line number +// of l. The fill function renders an empty screen line. Use fill when the // other side has more screen lines. func (v *View) splitSide( l *udiff.Line, @@ -506,7 +507,7 @@ func (v *View) lineNum(ls lineStyle, n any, digits int) string { return ls.lineNumber.Render(pad(n, digits)) } -// codeLines renders one source line: nums, sym, and the code in width +// codeLines renders one source line with nums, sym, and the code in width // cells. It cuts a long line. With wrap, the line continues on more screen // lines, which show blank in place of nums and no sym. func (v *View) codeLines( @@ -521,7 +522,8 @@ func (v *View) codeLines( w := max(width, 1) parts = strings.Split(ansi.Hardwrap(lipgloss.Wrap(code, w, ""), w, true), "\n") for i, p := range parts { - parts[i] = ansi.Truncate(p, w, "") // a wide character can be wider than w + // A wide character can be wider than w. + parts[i] = ansi.Truncate(p, w, "") } } else { code = ansi.GraphemeWidth.Cut(code, v.xOffset, len(code)) @@ -565,10 +567,11 @@ func hunkLineCounts(h *udiff.Hunk) (before, after int) { } // highlightCode returns source with syntax colours on the background -// bgColor. The cache keeps the result. If chroma fails, it returns source -// as it is. +// bgColor. The cache keeps the result. If chroma fails, highlightCode +// returns source as it is. func (v *View) highlightCode(source string, bgColor color.Color) string { - // Chroma makes a lone CR a line break, so show it as a symbol first. + // Chroma makes a lone CR a line break. Thus show each CR as a symbol + // first. source = strings.ReplaceAll(source, "\r", "\u240d") lexer := v.chromaLexer() key := newSyntaxKey(lexer.Config().Name, source, bgColor) @@ -589,8 +592,8 @@ func (v *View) highlightCode(source string, bgColor color.Color) string { return out } -// syntaxKey is the key of a [Cache]: the lexer, the source, and its -// background colour. noColor is true for no background, because +// syntaxKey is the key of a [Cache]. It has the lexer, the source, and the +// background colour of the source. noColor is true for no background, because // lipgloss.NoColor has the same RGBA value as black. type syntaxKey struct { lexer, source string @@ -639,7 +642,8 @@ func Lexer(path, content string) chroma.Lexer { } // processChromaValue escapes the text of a token for the terminal. A code -// line has no line break, so it removes the trailing ones. +// line has no line break. Thus processChromaValue removes the trailing line +// breaks. func processChromaValue(value string) string { return Escape(strings.TrimRight(value, "\n")) } diff --git a/internal/diffview/escape.go b/internal/diffview/escape.go index c865025..49bbe3f 100644 --- a/internal/diffview/escape.go +++ b/internal/diffview/escape.go @@ -10,7 +10,8 @@ import ( // The Unicode Control Pictures that Escape shows in place of a control // character. const ( - controlPictures = '\u2400' // the picture of U+0000, then the pictures of U+0001 to U+001F + // The picture of U+0000. The pictures of U+0001 to U+001F follow it. + controlPictures = '\u2400' delPicture = '\u2421' ) diff --git a/internal/diffview/fold.go b/internal/diffview/fold.go index 6f7319e..ec0e084 100644 --- a/internal/diffview/fold.go +++ b/internal/diffview/fold.go @@ -10,9 +10,9 @@ import ( // longer run of equal lines folds into one row, as in herdr-reviewr. const foldMargin = 3 -// Fold is a run of equal lines that hides. Before and After are the line -// numbers of its first line. Lines is the number of lines. It is 0 if no -// line hides. +// Fold is a run of equal lines that the view hides. Before and After are +// the line numbers of its first line. Lines is the number of lines. Lines +// is 0 if the view hides no line. type Fold struct { Before, After, Lines int } diff --git a/internal/diffview/rows.go b/internal/diffview/rows.go index 27b3b2e..13d82b2 100644 --- a/internal/diffview/rows.go +++ b/internal/diffview/rows.go @@ -11,13 +11,13 @@ const ( RowEqual // a line that is the same in the two versions RowInsert // an inserted line RowDelete // a deleted line - RowChange // in the split layout, a deleted line beside an inserted line + RowChange // a deleted line beside an inserted line (split) ) // Row tells which source lines one rendered line shows. Before and After are // line numbers from 1, or 0 if the row has no line on that side. Continued // is true for the second and the next screen lines of a wrapped line. -// For a hunk header, Fold is the run of lines that hides above it. +// For a hunk header, Fold is the run of lines that hides above the header. type Row struct { Kind RowKind Before, After int @@ -121,8 +121,7 @@ func splitKind(l *splitLine) RowKind { } // Geometry gives the columns of the line numbers in the output of -// [View.Render]. -// Use it to tell a line number from the code. +// [View.Render]. Use it to tell a line number from the code. type Geometry struct { BeforeNum int // the width of the old line-number column, in cells AfterNum int // the width of the new line-number column, in cells diff --git a/internal/diffview/terminal.go b/internal/diffview/terminal.go index dc0278e..96a9f28 100644 --- a/internal/diffview/terminal.go +++ b/internal/diffview/terminal.go @@ -34,7 +34,7 @@ func mix(a, b color.Color, t float64) color.Color { const ( tintDark = 0.16 // inserted and deleted lines on a dark background tintLight = 0.13 // inserted and deleted lines on a light background - tintNumber = 1.7 // multiplies the tint for the line numbers of those lines + tintNumber = 1.7 // multiplies the tint for the numbers of those lines shadeDivider = 0.07 // the hunk headers, toward the foreground shadeMissing = 0.03 // the empty side of a split row, toward the foreground ) @@ -45,7 +45,7 @@ const ( fallbackRed = "#f85149" ) -// TerminalStyle makes a diff style from the terminal colours: the +// TerminalStyle makes a diff style from the terminal colours. These are the // background, the foreground, and the ANSI green and red. The text colours // are ANSI palette indices, so the terminal selects the real colours. // TerminalStyle mixes the tints of inserted and deleted lines from the diff --git a/internal/editor/editor.go b/internal/editor/editor.go index 5582449..b7ba105 100644 --- a/internal/editor/editor.go +++ b/internal/editor/editor.go @@ -14,7 +14,7 @@ import ( // Command is a command that opens a file at a line. type Command struct { Script string // the command, for sh -c - Window bool // the editor opens a window, not in the terminal + Window bool // the editor opens a window and does not draw in the terminal } // FromTemplate returns the command that opens file at line with template, diff --git a/internal/termtext/termtext.go b/internal/termtext/termtext.go index 4f1e223..343a8fd 100644 --- a/internal/termtext/termtext.go +++ b/internal/termtext/termtext.go @@ -8,9 +8,9 @@ import ( "unicode/utf8" ) -// Plain returns s without control characters. This removes the escape -// sequences too, but keeps tabs and newlines. Use it on text from other -// people before you show or send it. +// Plain returns s without control characters and escape sequences. It keeps +// tabs and newlines. Use Plain on text from other people before you show or +// send the text. func Plain(s string) string { return strings.Map(func(r rune) rune { if r == '\t' || r == '\n' || !unicode.IsControl(r) { diff --git a/internal/termtext/termtext_test.go b/internal/termtext/termtext_test.go index fa3f670..e37ddd3 100644 --- a/internal/termtext/termtext_test.go +++ b/internal/termtext/termtext_test.go @@ -42,14 +42,15 @@ func FuzzPlain(f *testing.F) { p := termtext.Plain(s) unsafe := func(r rune) bool { return r != '\t' && r != '\n' && unicode.IsControl(r) } if strings.ContainsFunc(p, unsafe) { - t.Errorf("Plain(%q) = %q, has a control character", s, p) + t.Errorf("Plain(%q) = %q, want no control characters", s, p) } if pp := termtext.Plain(p); pp != p { t.Errorf("Plain(Plain(%q)) = %q, want %q", s, pp, p) } q := termtext.Quote(s) if strings.ContainsFunc(q, unicode.IsControl) || !utf8.ValidString(q) { - t.Errorf("Quote(%q) = %q, has a control character", s, q) + t.Errorf("Quote(%q) = %q, want valid UTF-8 with no control characters", + s, q) } }) } diff --git a/rex/rex.go b/rex/rex.go index f94eb55..bb0f60b 100644 --- a/rex/rex.go +++ b/rex/rex.go @@ -137,8 +137,8 @@ type State string const ( StateIdle State = "idle" // waits for the next instruction of the user StateWorking State = "working" // does a task - StateDone State = "done" // finished a task, and the user did not look yet - StateBlocked State = "blocked" // cannot continue until the user does something + StateDone State = "done" // waits for the user to see the done task + StateBlocked State = "blocked" // waits for an action of the user ) // statusEntry is a status as Rex sends it. A program can send one status @@ -184,7 +184,7 @@ func (c *Client) Cwd(ctx context.Context, block string) (string, error) { type process struct { Cwd string `json:"cwd"` // a file:// URL } - // The foreground process is the program that has the terminal now, for + // The foreground process is the program that holds the terminal, for // example an agent that runs in a shell. The child is the shell. var reply struct { Foreground process `json:"foreground"` @@ -201,7 +201,7 @@ func (c *Client) Cwd(ctx context.Context, block string) (string, error) { // Agent is a block whose program sends a status with an app name. type Agent struct { Block string // the ID of the block - Name string // the label of the block, or the app if the block has no label + Name string // the label of the block, or else the app name Window string // the label of the window of the block, or "" Cwd string // the working directory of the program Status @@ -268,7 +268,8 @@ type window struct { Label string `json:"label"` } -// windows returns the windows of the session. If rex fails, it returns nil. +// windows returns the windows of the session. If rex fails, windows returns +// nil. func (c *Client) windows(ctx context.Context) []window { var reply struct { Windows []window `json:"windows"` @@ -381,7 +382,7 @@ type Event struct { Block string // the ID of the block Closed bool // true if the block closed // Status is the new status of the program in the block. If the program - // removed its status, Status is the zero Status. + // removes its status, Status is the zero Status. Status Status } @@ -449,7 +450,8 @@ func parseEvent(line []byte) (Event, bool) { var msg struct { Payload struct { Event struct { - Type string `json:"type"` // "block_event" or "block_closed" + // Type is "block_event" or "block_closed". + Type string `json:"type"` Block blockEvent `json:"payload"` } `json:"event"` } `json:"payload"` diff --git a/rex/rex_test.go b/rex/rex_test.go index 8abe6ff..1e4ec55 100644 --- a/rex/rex_test.go +++ b/rex/rex_test.go @@ -97,7 +97,7 @@ func TestAgents(t *testing.T) { }, } if !slices.Equal(agents, want) { - t.Fatalf("Agents:\n%+v\nwant:\n%+v", agents, want) + t.Fatalf("Agents got:\n%+v\nwant:\n%+v", agents, want) } } @@ -106,14 +106,15 @@ func TestCommands(t *testing.T) { c, dir := fakeRex(t) ctx := t.Context() if id, err := c.Resolve(ctx, "agent"); err != nil || id != "block:pi" { - t.Errorf("Resolve(agent) = %q, %v", id, err) + t.Errorf("Resolve(agent) = %q, %v, want block:pi, nil", id, err) } if id, _ := c.Resolve(ctx, "block:x"); id != "block:x" { - t.Errorf("Resolve(block:x) = %q", id) + t.Errorf("Resolve(block:x) = %q, want block:x", id) } _, err := c.Resolve(ctx, "nosuch") if err == nil || err.Error() != "no block matches nosuch" { - t.Errorf("Resolve(nosuch): err = %v", err) + t.Errorf("Resolve(nosuch): err = %v, want %q", + err, "no block matches nosuch") } if err := c.Paste(ctx, "block:pi", "a\n\tb"); err != nil { t.Fatal(err) @@ -134,7 +135,7 @@ func TestCommands(t *testing.T) { `split -s S -b block:self --cwd /repo -- sh -c vi x`, } { if !strings.Contains(calls, want) { - t.Errorf("no call %s in:\n%s", want, calls) + t.Errorf("calls got:\n%s\nwant a call %s", calls, want) } } } @@ -176,7 +177,7 @@ func TestWatch(t *testing.T) { {Block: "block:pi"}, {Block: "block:pi", Closed: true}, } - // Stop Watch when it has reported all the events. + // Stop Watch when it reports all the events. ctx, cancel := context.WithTimeout(t.Context(), 5*time.Second) defer cancel() var got []rex.Event @@ -187,10 +188,10 @@ func TestWatch(t *testing.T) { } }) if !errors.Is(err, context.Canceled) { - t.Errorf("Watch returned %v, want context.Canceled", err) + t.Errorf("Watch = %v, want context.Canceled", err) } if !slices.Equal(got, want) { - t.Errorf("events:\n%+v\nwant:\n%+v", got, want) + t.Errorf("events got:\n%+v\nwant:\n%+v", got, want) } } @@ -242,7 +243,7 @@ func TestReloadConfigOutsideRex(t *testing.T) { } // TestErrorKeepsCause checks that the error of a failed rex is the message -// of rex, and still wraps the exit error of the process. +// of rex. The error also wraps the exit error of the process. func TestErrorKeepsCause(t *testing.T) { dir := t.TempDir() script := "#!/bin/sh\necho 'Error: no Rex server' >&2\nexit 3\n" diff --git a/tui/bench_test.go b/tui/bench_test.go index 919df62..4806a59 100644 --- a/tui/bench_test.go +++ b/tui/bench_test.go @@ -15,7 +15,8 @@ import ( // BenchmarkReview reloads a large change and scrolls through it, as a user // does. Use it to profile the program: // -// go test ./tui -run '^$' -bench Review -cpuprofile cpu.out -memprofile mem.out +// go test ./tui -run '^$' -bench Review \ +// -cpuprofile cpu.out -memprofile mem.out func BenchmarkReview(b *testing.B) { m, dir := newTestModel(b) // The benchmark makes 20 new files of 2,000 lines each. @@ -40,8 +41,8 @@ func BenchmarkReview(b *testing.B) { } } -// BenchmarkScroll scrolls a large change sideways: in the Changes tab with -// the folds closed and with the folds open, and in the Files tab. +// BenchmarkScroll scrolls a large change sideways. It scrolls in the Changes +// tab with the folds closed and open, and in the Files tab. // // go test ./tui -run '^$' -bench Scroll func BenchmarkScroll(b *testing.B) { diff --git a/tui/comment.go b/tui/comment.go index ccb964b..9ad53f0 100644 --- a/tui/comment.go +++ b/tui/comment.go @@ -17,8 +17,8 @@ type targetLine struct { } // target returns the line that a comment on row i of the current pane -// attaches to, or nil. If the row changes a line on side pref, it uses -// that side. Else it prefers the new side. So a comment on an unchanged +// attaches to, or nil. If the row changes a line on side pref, target uses +// that side. Else target uses the new side. Thus a comment on an unchanged // line is on the new side. func (m *model) target(i int, pref diff.Side) *targetLine { p := m.cur() @@ -53,7 +53,8 @@ func (m *model) contentFor(path string, s diff.Side) string { } return f.New } - content, _ := m.repo.ReadFile(path) // an unreadable file gives a comment no code + // An unreadable file gives a comment with no code. + content, _ := m.repo.ReadFile(path) return content } diff --git a/tui/config.go b/tui/config.go index b1e0403..1f4abe8 100644 --- a/tui/config.go +++ b/tui/config.go @@ -27,7 +27,7 @@ type Scope string const ( ScopeUncommitted Scope = "uncommitted" // HEAD ScopeBranch Scope = "branch" // the base branch - ScopeLastTurn Scope = "last-turn" // the start of the last turn of the agent + ScopeLastTurn Scope = "last-turn" // the start of the last agent turn ) // markdownView tells how the program shows a Markdown file. @@ -70,7 +70,8 @@ type config struct { MarkdownView markdownView `json:"markdown_view"` // FileList is the side of the screen that shows the list of files. FileList position `json:"file_list"` - // FileListWidth is in columns, minListWidth or more. + // FileListWidth is the width of the file list in columns. It is + // minListWidth or more. FileListWidth int `json:"file_list_width"` // DiffSigns marks a changed line with + or -, in place of a bar. DiffSigns bool `json:"diff_signs"` diff --git a/tui/editor.go b/tui/editor.go index 632a5d2..f305488 100644 --- a/tui/editor.go +++ b/tui/editor.go @@ -18,8 +18,9 @@ import ( // openEditor opens the file at path, at line. It uses the editor of the // settings, else $VISUAL, $EDITOR, core.editor, or vi. A terminal editor -// opens in a new block beside rexdiff in Rex. Outside Rex, it takes the -// screen until it stops. A window editor starts in the background. +// opens in a new block beside rexdiff in Rex. Outside Rex, a terminal +// editor takes the screen until the editor stops. A window editor starts in +// the background. func (m *model) openEditor(path string, line int) tea.Cmd { abs := filepath.Join(m.repo.Root(), path) var ed editor.Command @@ -89,7 +90,8 @@ func openURL(cfg config, url string) error { } // start starts a program and does not wait for it to stop. A goroutine -// waits for it, so that it does not stay a zombie process. +// waits for the program, so that the program does not stay a zombie +// process. func start(name string, args ...string) error { cmd := exec.Command(name, args...) if err := cmd.Start(); err != nil { diff --git a/tui/editor_test.go b/tui/editor_test.go index fee041d..ca3705f 100644 --- a/tui/editor_test.go +++ b/tui/editor_test.go @@ -70,7 +70,7 @@ func TestTerminalEditorInRex(t *testing.T) { t.Setenv("EDITOR", tt.env) log := logRex(t, &m) if _, cmd := pressE(t, m); cmd != nil { - t.Errorf("e returned a command %T, want none", cmd) + t.Errorf("e returns a command %T, want none", cmd) } b, err := os.ReadFile(log) if err != nil { @@ -97,10 +97,10 @@ func TestWindowEditor(t *testing.T) { log := logRex(t, &m) m, cmd := pressE(t, m) if cmd == nil { - t.Fatal("e returned no command") + t.Fatal("e returns no command, want a command") } if msg := cmd(); msg != nil { - t.Fatalf("the editor failed: %v", msg) + t.Fatalf("the editor command = %v, want nil", msg) } b, err := os.ReadFile(out) if err != nil { @@ -111,11 +111,12 @@ func TestWindowEditor(t *testing.T) { t.Errorf("the editor got\n%q\nwant\n%q", b, want) } if _, err := os.Stat(log); err == nil { - t.Error("a window editor opened a Rex block") + t.Errorf("rex log %s exists, want no Rex block for a window editor", + log) } } -// TestEditorError checks that the footer tells why an editor failed. +// TestEditorError checks that the footer tells why an editor fails. func TestEditorError(t *testing.T) { code := writeScript(t, t.TempDir(), "code", "echo 'code: no such file' >&2\nexit 3\n") tests := []struct { @@ -131,10 +132,10 @@ func TestEditorError(t *testing.T) { t.Setenv("PATH", tt.path) m, cmd := pressE(t, m) if cmd == nil { - t.Fatal("e returned no command") + t.Fatal("e returns no command, want a command") } if m = step(t, m, cmd()); !strings.Contains(view(m), tt.want) { - t.Errorf("the footer does not show %q:\n%s", tt.want, view(m)) + t.Errorf("view got:\n%s\nwant %q in the footer", view(m), tt.want) } }) } diff --git a/tui/fold_test.go b/tui/fold_test.go index 6690a29..bdceb09 100644 --- a/tui/fold_test.go +++ b/tui/fold_test.go @@ -57,8 +57,8 @@ func foldsOf(p *pane) []int { return out } -// TestExpandFold checks that → shows the lines of a fold and scrolls -// sideways on a line of code. +// TestExpandFold checks that the right arrow key shows the lines of a fold. +// On a line of code, the key scrolls sideways. func TestExpandFold(t *testing.T) { m := newFoldModel(t) p := &m.changes @@ -66,13 +66,14 @@ func TestExpandFold(t *testing.T) { t.Fatalf("folds = %v, want [6 13 7]", got) } if !shows(m, "⋯ 13 unmodified lines", "→ show the lines") { - t.Fatalf("the screen does not show the fold and its key:\n%s", view(m)) + t.Fatalf("view got:\n%s\nwant the fold and its key", view(m)) } m = press(t, m, "]") // the first change, line 10 if shows(m, "→ show the lines") { - t.Error("the footer tells about → on a line of code") + t.Error("on a line of code: the footer shows the → hint, want no hint") } - m = press(t, m, "]", "k", "k", "k", "k") // the second change, then up to its header + // Go to the second change, then up to its header. + m = press(t, m, "]", "k", "k", "k", "k") if r := m.changes.rows[m.changes.cur]; r.Fold.Lines != 13 || !shows(m, "→ show the lines") { t.Fatalf("the cursor is on %+v, want the middle fold", r) } @@ -86,10 +87,10 @@ func TestExpandFold(t *testing.T) { } for n := 14; n <= 26; n++ { if p.rowOf(diff.SideNew, n) < 0 { - t.Errorf("line %d does not show", n) + t.Errorf("line %d is hidden, want it shown", n) } } - // On a line of code, → scrolls. + // On a line of code, the right arrow key scrolls. m = step(t, m, tea.KeyPressMsg{Code: tea.KeyRight}) if m.changes.xoff == 0 || !slices.Equal(foldsOf(&m.changes), []int{6, 7}) { t.Errorf("→ on code: xoff %d, folds %v; want a scroll and no change", @@ -123,7 +124,7 @@ func TestExpandKeepsPlaces(t *testing.T) { m = step(t, m, tea.KeyPressMsg{Text: "line 3", Code: 'l'}) m = press(t, m, "enter") if !m.find.active() { - t.Fatal("the find has no hits") + t.Fatal("the find has no hits, want hits") } m.changes.cur = 0 // the fold at the top m = step(t, m, tea.KeyPressMsg{Code: tea.KeyRight}) @@ -135,7 +136,7 @@ func TestExpandKeepsPlaces(t *testing.T) { t.Errorf("the selection starts on %+v, want line 29", r) } if len(m.find.hits) == 0 { - t.Error("the find has no hits after the fold shows") + t.Error("the find has no hits after the fold shows, want hits") } for _, h := range m.find.hits { if l := strings.ToLower(ansi.Strip(p.lines[h])); !strings.Contains(l, "line 3") { @@ -224,7 +225,7 @@ func TestClickFold(t *testing.T) { t.Errorf("keys %v: after the click, folds = %v, want [6 13]", keys, got) } if m.changes.rowOf(diff.SideNew, 40) < 0 { - t.Errorf("keys %v: line 40 does not show", keys) + t.Errorf("keys %v: line 40 is hidden, want it shown", keys) } } } @@ -244,10 +245,10 @@ func TestFoldKeepsComment(t *testing.T) { } m = press(t, m, "b", "u") // the branch scope, then back if m.folds != nil { - t.Errorf("a new scope keeps the expanded folds %v", m.folds) + t.Errorf("after a new scope, expanded folds = %v, want nil", m.folds) } if m.changes.rowOf(diff.SideNew, 20) < 0 || !shows(m, "why") { - t.Errorf("the commented line 20 does not show:\n%s", view(m)) + t.Errorf("view got:\n%s\nwant the commented line 20", view(m)) } if got := foldsOf(&m.changes); !slices.Equal(got, []int{6, 7}) { t.Errorf("folds = %v, want [6 7]", got) diff --git a/tui/keys.go b/tui/keys.go index b1dbce8..404679e 100644 --- a/tui/keys.go +++ b/tui/keys.go @@ -130,8 +130,8 @@ type overlayKeyMap struct { Quit, Stay key.Binding } -// overlayKeys is the keys of the overlays. The settings do not change -// them. +// overlayKeys has the keys of the overlays. The settings do not change +// these keys. var overlayKeys = overlayKeyMap{ Choose: key.NewBinding(key.WithKeys("enter")), Close: key.NewBinding(key.WithKeys("esc")), diff --git a/tui/model.go b/tui/model.go index f01e981..9a75754 100644 --- a/tui/model.go +++ b/tui/model.go @@ -28,7 +28,7 @@ const ( tabPR ) -// scopeCommits compares the commits that the user picked. It is not a +// scopeCommits compares the commits that the user picks. It is not a // value of Config.DefaultScope. const scopeCommits Scope = "commits" @@ -47,25 +47,25 @@ const ( modeOverlay ) -// pane is one view that scrolls and takes comments: the rendered lines and -// the source lines that they show. +// pane is one view that scrolls and takes comments. It has the rendered +// lines and the source lines that they show. type pane struct { lines []string rows []diff.Row // rows[i] is the source line that lines[i] shows - diff *diff.Diff // the render, for mouse hits, or nil for markdown and the PR - path string // the file of the rows, or "" if the pane takes no comments + diff *diff.Diff // the render for mouse hits, or nil for Markdown or PR + path string // the file of the rows, or "" for a pane with no comments cur int // the row of the cursor top int // the first row on the screen anchor int // the start of the visual selection, or -1 xoff int // the sideways scroll, in columns - note string // shown instead of the lines, if there are no lines + note string // the text that shows if there are no lines // free is true after the wheel moves the view. Then the cursor can // be off the screen until the next key or click. free bool } -// set shows lines that show rows. It keeps the cursor, the top, and the -// selection in the new lines. +// set shows lines, and rows tells which source lines they show. It keeps +// the cursor, the top, and the selection in the new lines. func (p *pane) set(lines []string, rows []diff.Row) { p.lines, p.rows, p.diff = lines, rows, nil p.cur = min(p.cur, max(len(lines)-1, 0)) @@ -119,8 +119,8 @@ type changed struct { adds, dels int } -// turnState is the latest turn of an agent: a snapshot of the working tree -// when the agent started work. +// turnState is the latest turn of an agent. It keeps a snapshot of the +// working tree from the start of the turn. type turnState struct { block string app string // the name of the agent, for example "pi" @@ -142,14 +142,14 @@ func (f *findState) active() bool { return len(f.hits) > 0 } // dragState is a drag of the left mouse button in the main pane. type dragState struct { active bool // the button is down - moved bool // the cursor moved off the start row - gutter bool // the drag started on the line numbers - start int // the row where the drag started + moved bool // the cursor has left the start row + gutter bool // the drag starts on the line numbers + start int // the row where the drag starts pref diff.Side // the side of the line numbers or the code at the start } -// draft is the comment that the user writes: a new one, or an edit of the -// comment with ID editID. +// draft is the comment that the user writes. It is a new comment, or an +// edit of the comment with ID editID. type draft struct { diff.Comment editID int @@ -164,9 +164,9 @@ type prState struct { // agentState is what the program knows about the agent in a block. type agentState struct { - state rex.State // "" after the block closed + state rex.State // "" after the block closes cwd string // the working directory, or "" if unknown - hasCwd bool // the program asked Rex for cwd + hasCwd bool // the program has asked Rex for cwd } // model is the state of the program. It implements tea.Model. @@ -197,10 +197,11 @@ type model struct { tab tab scope Scope - folds map[string]*openFolds // the folds that the user expanded in the Changes tab, by path + // folds has the folds that the user expands in the Changes tab, by path. + folds map[string]*openFolds base string - from string // the commits scope: the oldest commit that the user picked - to string // the commits scope: the newest commit that the user picked + from string // the commits scope: the oldest commit that the user picks + to string // the commits scope: the newest commit that the user picks turn *turnState files []changed // the Changes tab @@ -221,7 +222,7 @@ type model struct { listHidden bool focusList bool // the keys j and k move in the file list listW int - cache diff.Cache // the syntax colours, so that a sideways scroll is fast + cache diff.Cache // the syntax colours, for a fast sideways scroll comments diff.Comments editor textarea.Model @@ -236,9 +237,9 @@ type model struct { statusTill time.Time agents map[string]*agentState // by block - sends int // the steps of sends that run now + sends int // the steps of the sends that run lastSent string // the block of the last send - beside string // the agent block that the pane opened beside + beside string // the agent block beside the pane } // newModel returns the model of the program with the options o and the @@ -290,7 +291,7 @@ func newModel(ctx context.Context, o Options, cfg config) model { return m } -// Messages. +// These are the messages of the program. type ( tickMsg struct{} agentsMsg struct { @@ -383,15 +384,15 @@ func (m *model) trees() (from, to, note string, err error) { return from, to, "", err } -// reload reads the changes of the scope again, even if the trees did not +// reload reads the changes of the scope again, even if the trees do not // change. func (m *model) reload() { m.shownTrees = "" // no trees are "", so reloadIfChanged reads them m.reloadIfChanged() } -// reloadIfChanged reads the changes of the scope again if the trees -// changed. +// reloadIfChanged reads the changes of the scope again if the trees differ +// from the shown trees. func (m *model) reloadIfChanged() { from, to, note, err := m.trees() trees := from + ".." + to @@ -451,7 +452,7 @@ func (m *model) reloadFiles() { m.refreshViewer() } -// Update handles a message. After an install, it restarts the program +// Update handles a message. After an install, Update restarts the program // when no work can get lost. func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { cmd := m.handle(msg) @@ -557,14 +558,15 @@ func (m *model) onGrep(msg grepMsg) { } } -// onPR shows the pull request that loadPR read. +// onPR shows the pull request that loadPR reads. func (m *model) onPR(msg prMsg) { m.pr = prState{pr: msg.pr} if msg.err != nil { m.pr.err = msg.err.Error() } m.renderPR() - if p := m.prPlace; p != nil && msg.err == nil && msg.pr != nil { // from before a restart + // prPlace is from before a restart. + if p := m.prPlace; p != nil && msg.err == nil && msg.pr != nil { m.prPlace = nil m.prPane.restore(*p) } @@ -589,7 +591,7 @@ const ( colorTicks = 3 // a request for the colours of the terminal ) -// onTick reloads the current tab, if it changed. +// onTick reloads the current tab if its content changes. func (m *model) onTick() tea.Cmd { m.ticks++ every := 1 @@ -602,7 +604,7 @@ func (m *model) onTick() tea.Cmd { m.reloadIfChanged() case tabFiles: if m.ticks%filesTicks == 0 { - m.reloadFiles() // it renders the file too + m.reloadFiles() // reloadFiles renders the file too } else { m.refreshViewer() } diff --git a/tui/mouse.go b/tui/mouse.go index 524db0f..25e250c 100644 --- a/tui/mouse.go +++ b/tui/mouse.go @@ -117,12 +117,12 @@ func (m *model) onRelease() tea.Cmd { return nil } switch { - case d.gutter: // a line or a range on the line numbers: comment + case d.gutter: // a drag on the line numbers opens the comment box if d.moved { p.anchor = d.start } return m.startComment(d.pref) - case d.moved: // a range on the code: copy it + case d.moved: // a drag on the code copies the range lo, hi := p.span(d.start, p.cur) out := m.sourceLines(lo, hi, d.pref) m.flash("copied " + plural(len(out), "line")) @@ -132,8 +132,8 @@ func (m *model) onRelease() tea.Cmd { } // sourceLines returns the source lines of rows lo to hi of the current -// pane. It uses side pref if they change it. A row with no source line -// gives its text on the screen. +// pane. It picks the side of each row as [model.target] does with pref. A +// row with no source line gives its text on the screen. func (m *model) sourceLines(lo, hi int, pref diff.Side) []string { p := m.cur() var out []string @@ -189,7 +189,7 @@ func (m *model) onWheel(mouse tea.Mouse) { } // clickOverlay handles a click while an overlay is open. A click outside -// the box closes it, but not the prompt before a quit. +// the box closes the overlay. The prompt before a quit does not close. func (m *model) clickOverlay(mouse tea.Mouse) tea.Cmd { box, x, y, listY := m.overlayBox() w, h := lipgloss.Width(box), lipgloss.Height(box) diff --git a/tui/nav.go b/tui/nav.go index b609694..88da59f 100644 --- a/tui/nav.go +++ b/tui/nav.go @@ -16,7 +16,7 @@ import ( "github.com/rednafi/rexdiff/vcs" ) -// Steps of the keys that move or resize, in columns. +// These are the steps of the keys that move or resize, in columns. const ( scrollStep = 8 // the sideways scroll listWidthStep = 4 // the width of the file list @@ -78,7 +78,7 @@ func (m *model) actionKey(msg tea.KeyPressMsg) tea.Cmd { } // scopeKey handles a key that changes the tab or the scope. It reports -// whether it took the key. +// whether it takes the key. func (m *model) scopeKey(msg tea.KeyPressMsg) (tea.Cmd, bool) { k := m.keys switch { @@ -110,7 +110,7 @@ func (m *model) scopeKey(msg tea.KeyPressMsg) (tea.Cmd, bool) { // moveKey handles a key that moves the cursor, the file selection, or // the sideways scroll. The wrap is here because it ends the sideways -// scroll. moveKey reports whether it took the key. +// scroll. moveKey reports whether it takes the key. func (m *model) moveKey(msg tea.KeyPressMsg) (tea.Cmd, bool) { k, p := m.keys, m.cur() last := max(len(p.lines)-1, 0) @@ -167,7 +167,7 @@ func (m *model) moveKey(msg tea.KeyPressMsg) (tea.Cmd, bool) { } // commentKey handles a key that adds, changes, or sends comments. It -// reports whether it took the key. +// reports whether it takes the key. func (m *model) commentKey(msg tea.KeyPressMsg) (tea.Cmd, bool) { k, p := m.keys, m.cur() switch { @@ -202,7 +202,7 @@ func (m *model) commentKey(msg tea.KeyPressMsg) (tea.Cmd, bool) { } // viewKey handles a key that searches, changes the view, or opens or -// reloads something. It reports whether it took the key. +// reloads something. It reports whether it takes the key. func (m *model) viewKey(msg tea.KeyPressMsg) (tea.Cmd, bool) { k := m.keys switch { @@ -296,11 +296,11 @@ func (m *model) onFold() bool { return m.tab == tabChanges && p.cur < len(p.rows) && p.rows[p.cur].Fold.Lines > 0 } -// openFolds has the folds that the user expanded in one file, as ranges of +// openFolds has the folds that the user expands in one file, as ranges of // old line numbers. The old side does not change while the agent edits the // file, so an edit keeps the lines of a range open. The ranges are for the // old content with the hash Old. A different old content, for example -// after a commit, closes them. +// after a commit, closes the ranges. type openFolds struct { Old uint64 `json:"old"` Ranges [][2]int `json:"ranges"` // the first and the last line of each range @@ -313,8 +313,8 @@ func hash(s string) uint64 { return h.Sum64() } -// expanded tells if the lines of fold f of the file at path show: the -// user expanded the fold, or one of comments is on one of its lines. +// expanded tells if the lines of fold f of the file at path show. They show +// if the user expands the fold, or if a comment is on one of them. func (m *model) expanded(path string, comments []*diff.Comment, f diff.Fold) bool { // A fold that overlaps a range shows. An undo of a change next to a // range makes a larger fold, which shows as a whole. @@ -488,7 +488,7 @@ func (p *pane) rowOf(s diff.Side, line int) int { // hit of the find. It wraps around. func (m *model) stepFind(d int) { f, p := &m.find, m.cur() - // A reload can make the pane shorter than the rows that matched. + // A reload can make the pane shorter than the rows of the hits. f.hits = slices.DeleteFunc(f.hits, func(h int) bool { return h >= len(p.lines) }) if len(f.hits) == 0 { m.find = findState{} @@ -499,7 +499,7 @@ func (m *model) stepFind(d int) { } // runFind finds q in the lines of the current pane, without case. It moves -// the cursor to the first hit at or below it. +// the cursor to the first hit at or below the cursor. func (m *model) runFind(q string) { p := m.cur() m.find = findState{query: q} @@ -523,8 +523,8 @@ func (m *model) runFind(q string) { p.cur = m.find.hits[m.find.i] } -// gotoLine moves the cursor to the line number in s: "0" is the first -// row, "$" the last row. +// gotoLine moves the cursor to the line number in s. If s is "0", the +// cursor goes to the first row. If s is "$", it goes to the last row. func (m *model) gotoLine(s string) { p := m.cur() switch s = strings.TrimSpace(s); s { diff --git a/tui/overlay.go b/tui/overlay.go index aba78ac..bdc1d3e 100644 --- a/tui/overlay.go +++ b/tui/overlay.go @@ -44,13 +44,15 @@ type pickItem struct { label string detail string // the dim text after the label tag string // a short mark at the end, for example "current" - // id is what the row picks: a path, a ref, a commit, or a block. + // id is what the row picks, for example a path, a ref, a commit, or a + // block. id string line int // the search: the line of a code match comment int // the comment list: the ID of the comment } -// overlay is a modal box: a list to pick from, a search, or a prompt. +// overlay is a modal box. It shows a list to pick from, a search, or a +// prompt. type overlay struct { kind overlayKind title string @@ -123,8 +125,8 @@ func (o *overlay) selected() *pickItem { func (o *overlay) down() { o.cur = min(o.cur+1, max(len(o.shown)-1, 0)) } func (o *overlay) up() { o.cur = max(o.cur-1, 0) } -// setSearch shows the search results: the files that match by name, then -// the matches in the code. +// setSearch shows the search results. It shows the files that match by +// name, then the matches in the code. func (o *overlay) setSearch(files []vcs.Entry, hits []vcs.Match) { q := o.input.Value() var items []pickItem @@ -241,7 +243,8 @@ func (m *model) openBasePicker() { // the commits scope. It lists the newest commits first. func (m *model) openCommitPicker() { ov := newOverlay(overlayCommits, "pick a commit, or space to start a range") - commits, _ := m.repo.Commits(m.ctx, maxCommits) // an error shows an empty picker + // An error shows an empty picker. + commits, _ := m.repo.Commits(m.ctx, maxCommits) var items []pickItem for _, c := range commits { items = append(items, pickItem{ @@ -454,7 +457,7 @@ func (o *overlay) agent(it *pickItem) (a rex.Agent, ok bool) { // baseRef returns the ref that the base picker picks, with the selected // item it. A typed ref, for example a tag, wins over the first match. A -// match that the user moved to wins over the typed text. If the ref is +// match that the user moves to wins over the typed text. If the ref is // not valid, ok is false. func (m *model) baseRef(it *pickItem) (ref string, ok bool) { ref = strings.TrimSpace(m.overlay.input.Value()) @@ -472,7 +475,8 @@ func (m *model) overlayBox() (box string, x, y, listY int) { ov := m.overlay w := min(max(m.w*2/3, 50), m.w-4) inner := w - 4 - lines := []string{truncate(headerStyle.Render(ov.title), inner)} // one line, for listY + // The title takes one line, for listY. + lines := []string{truncate(headerStyle.Render(ov.title), inner)} if ov.kind == overlayQuit { lines = append(lines, "", dimStyle.Render("q/enter quit · esc stay")) } else { diff --git a/tui/pr.go b/tui/pr.go index 2410bdd..bab2676 100644 --- a/tui/pr.go +++ b/tui/pr.go @@ -41,9 +41,9 @@ func prStateStyle(s vcs.State) lipgloss.Style { return dimStyle } -// checkSummary counts the checks that passed, failed, or are pending, and -// returns the names of the checks that failed. A skipped check counts as -// passed. +// checkSummary counts the checks that pass, fail, or are pending, and +// returns the names of the checks that fail. A skipped check counts as a +// pass. func checkSummary(checks []vcs.Check) (pass, fail, pend int, failed []string) { for _, c := range checks { switch c.Result { @@ -110,7 +110,7 @@ func prHeader(pr *vcs.PullRequest) []string { } // checkLines returns the counts of the checks, and the names of the -// checks that failed. With no checks, it returns no lines. +// checks that fail. With no checks, it returns no lines. func checkLines(checks []vcs.Check) []string { if len(checks) == 0 { return nil diff --git a/tui/release.go b/tui/release.go index 3e0ae7d..9fc6b3f 100644 --- a/tui/release.go +++ b/tui/release.go @@ -31,7 +31,7 @@ const ( phaseInstalled // rexdiff restarts when the user is ready ) -// installedMsg tells that the install of the release ended. +// installedMsg reports the end of the install of the release. type installedMsg struct{ err error } // checkRelease returns a command that asks for the latest release. Its @@ -69,7 +69,7 @@ func (m *model) install() tea.Cmd { return func() tea.Msg { return installedMsg{u.Install(ctx, r)} } } -// onInstalled prepares the restart, or tells why the install failed. After +// onInstalled prepares the restart, or tells why the install fails. After // a failure, the user can try again. func (m *model) onInstalled(msg installedMsg) { if msg.err != nil { diff --git a/tui/release_test.go b/tui/release_test.go index d2dfa42..faf5798 100644 --- a/tui/release_test.go +++ b/tui/release_test.go @@ -23,7 +23,7 @@ type fakeUpdater struct { newer bool err error // the error of Latest installErr error // the error of Install - installed []string // the tags that Install got + installed []string // the tags that Install gets } func (f *fakeUpdater) Latest(context.Context) (update.Release, bool, error) { @@ -55,7 +55,7 @@ func lastLine(m model) string { func run(t *testing.T, m model, cmd tea.Cmd) (model, tea.Cmd) { t.Helper() if cmd == nil { - t.Fatal("no command") + t.Fatal("run: no command, want a command") } next, cmd := m.Update(cmd()) return *next.(*model), cmd @@ -71,7 +71,7 @@ func TestUpdate(t *testing.T) { t.Fatalf("footer = %q, want %q", got, prompt) } - // The key U starts the install, and the footer tells about it. + // The key U starts the install. The footer shows the install. next, cmd := m.Update(tea.KeyPressMsg{Code: 'U', Text: "U"}) m = *next.(*model) if got := lastLine(m); !strings.Contains(got, "updating rexdiff to v0.2.0…") { @@ -84,12 +84,13 @@ func TestUpdate(t *testing.T) { t.Fatalf("Install got %v, want v0.2.0", u.installed) } if restart != nil || m.restartState != "" { - t.Fatal("rexdiff restarts while the user writes a comment") + t.Fatal("rexdiff restarts while the user writes a comment, " + + "want no restart") } // The restart waits until the user saves the comment. m = press(t, m, "f", "i", "x", "enter") if m.restartState == "" { - t.Fatal("no restart after the comment") + t.Fatal("no restart after the comment, want a restart") } t.Cleanup(func() { os.Remove(m.restartState) }) @@ -99,7 +100,7 @@ func TestUpdate(t *testing.T) { t.Fatal(err) } if _, err := os.Stat(m.restartState); !errors.Is(err, fs.ErrNotExist) { - t.Error("loadState did not remove the file") + t.Errorf("after loadState: Stat = %v, want fs.ErrNotExist", err) } m2 := newModel(t.Context(), Options{Repo: m.repo}, defaultConfig()) m2.restoreSettings(saved) @@ -113,7 +114,7 @@ func TestUpdate(t *testing.T) { m2.changes.cur, m2.scope, m2.tab, m.changes.cur, m.scope, m.tab) } if got, want := m2.comments.Review(""), m.comments.Review(""); got != want { - t.Errorf("the review after the restart:\n%s\nwant:\n%s", got, want) + t.Errorf("the review after the restart got:\n%s\nwant:\n%s", got, want) } } @@ -133,7 +134,7 @@ func TestUpdateClick(t *testing.T) { next, cmd := m.Update(tea.MouseClickMsg{Button: tea.MouseLeft, X: 3, Y: m.h - 1}) m = *next.(*model) if cmd == nil || m.phase != phaseInstalling { - t.Fatal("a click on the footer did not start the install") + t.Fatal("a click on the footer: no install, want the install") } } @@ -149,13 +150,13 @@ func TestUpdateFails(t *testing.T) { next, cmd := m.Update(tea.KeyPressMsg{Code: 'U', Text: "U"}) m, restart := run(t, *next.(*model), cmd) if restart != nil || m.restartState != "" { - t.Fatal("rexdiff restarts after a failed install") + t.Fatal("rexdiff restarts after a failed install, want no restart") } if !strings.Contains(lastLine(m), "update failed: brew: exit status 1: Error: another brew is running") { t.Errorf("footer = %q, want the reason", lastLine(m)) } if _, cmd := m.Update(tea.KeyPressMsg{Code: 'U', Text: "U"}); cmd == nil { - t.Error("U does not try again") + t.Error("U after a failed install: no command, want a new install") } } @@ -171,12 +172,13 @@ func TestNoUpdate(t *testing.T) { t.Errorf("updater %+v: footer %q, want no release", u, lastLine(m)) } if _, cmd := m.Update(tea.KeyPressMsg{Code: 'U', Text: "U"}); cmd != nil { - t.Errorf("updater %+v: U started a command", u) + t.Errorf("updater %+v: U returns a command, want none", u) } } m, _ := newTestModel(t) if cmd := m.Init(); m.updater != nil || cmd == nil { - t.Error("the test model has an updater") + t.Errorf("updater = %v, Init returns a command %v; want nil, true", + m.updater, cmd != nil) } } @@ -210,25 +212,25 @@ func TestRestartWaits(t *testing.T) { m.sends = 1 // a send runs while the install ends m = step(t, m, installedMsg{}) if m.restartState != "" { - t.Fatal("restart while a send runs") + t.Fatal("restart while a send runs, want no restart") } m = step(t, m, sentMsg{agent: rex.Agent{Name: "pi"}}) if m.restartState == "" { - t.Fatal("no restart after the send") + t.Fatal("no restart after the send, want a restart") } os.Remove(m.restartState) - // In the PR tab, the key r returns the command that loads the PR. Thus - // r does not restart, but a message without a command does. + // In the PR tab, the key r returns the command that loads the PR. So r + // does not restart. A message without a command starts the restart. m = updaterModel(t, u) m.tab = tabPR m.phase = phaseInstalled if next, cmd := m.Update(tea.KeyPressMsg{Code: 'r', Text: "r"}); cmd == nil || next.(*model).restartState != "" { - t.Fatal("restart on a message with a command") + t.Fatal("restart on a message with a command, want no restart") } m = step(t, m, tea.FocusMsg{}) if m.restartState == "" { - t.Fatal("no restart on a message without a command") + t.Fatal("no restart on a message without a command, want a restart") } os.Remove(m.restartState) } @@ -256,15 +258,15 @@ func TestUpdateClickRule(t *testing.T) { m := updaterModel(t, u) rule := bodyY + m.bodyHeight() if got := step(t, m, tea.MouseClickMsg{Button: tea.MouseLeft, X: 3, Y: rule}); got.phase == phaseInstalling { - t.Error("a click on the rule started the install") + t.Error("a click on the rule starts the install, want no install") } if got := step(t, m, tea.MouseClickMsg{Button: tea.MouseLeft, X: 3, Y: rule + 1}); got.phase != phaseInstalling { - t.Error("a click on the footer did not start the install") + t.Error("a click on the footer: no install, want the install") } } // TestRestartSaveFails checks that a failed save of the state tells the -// user and stops the restart, so that rexdiff continues with the old version. +// user and stops the restart. Then rexdiff continues with the old version. func TestRestartSaveFails(t *testing.T) { u := &fakeUpdater{latest: update.Release{Tag: "v0.2.0"}, newer: true} m := updaterModel(t, u) @@ -294,7 +296,7 @@ func TestRestartOnTick(t *testing.T) { next, cmd := m.Update(tickMsg{}) m = *next.(*model) if m.restartState == "" || cmd == nil { - t.Fatal("no restart on a tick") + t.Fatal("no restart on a tick, want a restart") } os.Remove(m.restartState) } @@ -352,9 +354,9 @@ func TestRunChecksOptions(t *testing.T) { } } -// TestSendKeepsEdits checks that a comment edited while its send runs -// stays after the send, whether the user saved the edit before the send -// ended or after it. +// TestSendKeepsEdits checks that a comment that the user edits while its +// send runs stays after the send. The user can save the edit before or after +// the send ends. func TestSendKeepsEdits(t *testing.T) { for _, saveFirst := range []bool{true, false} { m, _ := newTestModel(t) @@ -373,7 +375,7 @@ func TestSendKeepsEdits(t *testing.T) { t.Errorf("save first %v: comments after the send = %q, want the edited comment", saveFirst, got) } } - // The send removes a comment that the user did not change. + // The send removes a comment that the user does not change. m, _ := newTestModel(t) m = press(t, m, "j", "j", "j", "j", "c", "x", "enter") m.sends = 1 diff --git a/tui/render.go b/tui/render.go index 5908418..3a162bd 100644 --- a/tui/render.go +++ b/tui/render.go @@ -1,7 +1,7 @@ package tui -// This file turns files into the lines of a pane: diffs, the file viewer, -// and markdown. +// This file turns files into the lines of a pane. It renders diffs, the +// file viewer, and Markdown. import ( "errors" @@ -113,7 +113,7 @@ func (m *model) renderChanges() { case f.TooBig: p.setNote("file over 2 MB or 50,000 lines, not diffed") case f.Old == f.New: - // A rename without changes, or a change of the file mode. + // The change is a rename with no edit, or a change of the file mode. note := "no change to the content" if f.From != "" { note = "renamed from " + termtext.Quote(f.From) + ", no change to the content" @@ -139,14 +139,14 @@ func (p *pane) setNote(note string) { } // renderViewer shows the selected file in the Files tab, even if the file -// did not change. +// does not change. func (m *model) renderViewer() { m.viewer.path = "" // no file has the path "", so refreshViewer renders m.refreshViewer() } // refreshViewer shows the selected file in the Files tab if the file -// changed. +// changes. func (m *model) refreshViewer() { p := &m.viewer if m.allSel >= len(m.allFiles) { @@ -193,7 +193,7 @@ func (m *model) markdown(wrap int) *glamour.TermRenderer { } // renderMarkdown returns the lines of src, rendered with r. If r is nil or -// fails, it returns the lines of src as they are. +// fails, renderMarkdown returns the lines of src as they are. func renderMarkdown(r *glamour.TermRenderer, src string) []string { if r != nil { if out, err := r.Render(src); err == nil { diff --git a/tui/resume.go b/tui/resume.go index 8fdc554..14df7e4 100644 --- a/tui/resume.go +++ b/tui/resume.go @@ -1,8 +1,8 @@ package tui // After an update, rexdiff starts again in the same terminal. The old -// program saves the comments and the place of the user in a file, and the -// new one reads it, so that the user continues from the same place. +// program saves the comments and the place of the user in a file. The new +// program reads the file, so that the user continues from the same place. import ( "encoding/json/v2" diff --git a/tui/send.go b/tui/send.go index f0390f3..1f58798 100644 --- a/tui/send.go +++ b/tui/send.go @@ -32,8 +32,8 @@ func (m *model) listAgents(msg agentsMsg) tea.Cmd { // counts as in the repository. func (m *model) inRepo(dir string) bool { return dir == "" || m.repo.Contains(dir) } -// agent returns the state of the agent in block. It adds the block if it is -// new. +// agent returns the state of the agent in block. If the block is new, agent +// adds it. func (m *model) agent(block string) *agentState { a := m.agents[block] if a == nil { @@ -103,7 +103,7 @@ func (m *model) startTurn(block, app string) { } } -// onAgents keeps the states of the agents. For a send, it sends the +// onAgents keeps the states of the agents. For a send, onAgents sends the // review to the agent, or lets the user pick one. func (m *model) onAgents(msg agentsMsg) tea.Cmd { for _, a := range msg.agents { @@ -117,7 +117,8 @@ func (m *model) onAgents(msg agentsMsg) tea.Cmd { m.flashCopy(msg.err.Error()) return nil } - // Prefer the agents in this repository. Else use all of the session. + // Prefer the agents in this repository. Else use all the agents of the + // session. var here []rex.Agent for _, a := range msg.agents { if m.inRepo(a.Cwd) { @@ -135,7 +136,7 @@ func (m *model) onAgents(msg agentsMsg) tea.Cmd { return m.deliver(here[0]) case m.mode != modeNormal: // The user can open the comment box or an overlay while the agents - // load. A picker on top would hide the box and lose the comment. + // load. A picker on top can hide the box and lose the comment. m.flash("several agents: close the box, then press s to pick one") return nil } @@ -148,8 +149,8 @@ func (m *model) reviewText() string { return m.comments.Review(m.cfg.ReviewPrefix) } -// deliver returns a command that pastes the review into agent a. Before -// the paste, it checks the agent again. A prompt of the agent would take +// deliver returns a command that pastes the review into agent a. It checks +// the agent again before the paste, because a prompt of the agent can take // the text. func (m *model) deliver(a rex.Agent) tea.Cmd { m.sends++ @@ -176,14 +177,15 @@ func (m *model) deliver(a rex.Agent) tea.Cmd { return sentMsg{agent: a, err: err} } if focus { - _ = c.Focus(ctx, a.Block) // ignore the error, because the paste worked + // Ignore the error, because the paste works. + _ = c.Focus(ctx, a.Block) } return sentMsg{agent: a, ids: ids} } } // onSent removes the sent comments if the send worked. The comments that -// the user added or changed during the send stay. A saved edit gives a +// the user adds or changes during the send stay. A saved edit gives a // comment a new ID, and msg does not have that ID. func (m *model) onSent(msg sentMsg) { m.sends-- diff --git a/tui/tui.go b/tui/tui.go index 26107fa..4da59e9 100644 --- a/tui/tui.go +++ b/tui/tui.go @@ -78,7 +78,7 @@ func CheckConfig(text string) error { // Run shows the review program in the terminal. It returns when the user // quits or when ctx is done. Run returns an error before it starts if the // settings are not valid. It also returns an error if Repo is nil, or if -// it does not know the Layout or the Scope. After an update, Run returns +// Run does not know the Layout or the Scope. After an update, Run returns // the file of the saved state. The caller then runs the new rexdiff with // Options.Resume set to that file. func Run(ctx context.Context, o Options) (resume string, err error) { diff --git a/tui/tui_test.go b/tui/tui_test.go index 34f32ba..5b3b54a 100644 --- a/tui/tui_test.go +++ b/tui/tui_test.go @@ -155,11 +155,12 @@ func TestScopes(t *testing.T) { i := strings.Index(top, shortSHA(m.to)) m = step(t, m, tea.MouseClickMsg{Button: tea.MouseLeft, X: ansi.StringWidth(top[:i])}) if m.mode != modeOverlay || m.overlay.kind != overlayCommits { - t.Errorf("a click on the commit in the header did not open the commit picker") + t.Errorf("after a click on the commit in the header: mode %v, "+ + "want the commit picker", m.mode) } m = press(t, m, "esc") - // The turn scope shows only what changed during the turn of an agent. + // The turn scope shows only the changes of the turn of an agent. m = press(t, m, "t") if !strings.Contains(m.changes.note, "no agent turn") { t.Errorf("turn scope before a turn: note %q, want a note that no turn ran", m.changes.note) @@ -171,7 +172,7 @@ func TestScopes(t *testing.T) { t.Errorf("turn scope shows %v (running %v), want [c.go]", got, m.turn.running) } if v := view(m); !strings.Contains(v, "pi · ") { - t.Errorf("header does not name the agent of the turn:\n%s", v) + t.Errorf("view got:\n%s\nwant the agent of the turn in the header", v) } } @@ -183,7 +184,7 @@ func TestComment(t *testing.T) { // The unified layout uses three rows for the change. m = press(t, m, "]", "v", "j", "j", "c") if m.mode != modeComment { - t.Fatal("c did not open the comment box") + t.Fatalf("after c: mode %v, want the comment box", m.mode) } m = press(t, m, "h", "i", "enter") // The split layout uses two rows for the same change. @@ -195,10 +196,10 @@ func TestComment(t *testing.T) { } rows := "a.go:4-5\n-\treturn 1\n+\tx := 2\n+\treturn x\n" if got, want := m.comments.Review(""), rows+"hi\n\n"+rows+"ok"; got != want { - t.Errorf("Review:\n%s\nwant:\n%s", got, want) + t.Errorf("Review got:\n%s\nwant:\n%s", got, want) } if !shows(m, "💬 hi", "2 unsent comments") { - t.Errorf("view:\n%s\nwant the comments", view(m)) + t.Errorf("view got:\n%s\nwant the comments", view(m)) } m = press(t, m, "s") @@ -207,12 +208,13 @@ func TestComment(t *testing.T) { } next, cmd := m.Update(tea.KeyPressMsg{Code: 'y', Text: "y"}) if m = *next.(*model); cmd == nil || m.status != "copied 2 comments" { - t.Errorf("copy: status %q, command %v", m.status, cmd) + t.Errorf("copy: status %q, command %v; want %q, true", + m.status, cmd != nil, "copied 2 comments") } // Quit asks first, because the comments are not sent. if m = press(t, m, "q"); m.overlay == nil || m.overlay.kind != overlayQuit { - t.Error("q with unsent comments did not ask to confirm") + t.Error("q with unsent comments: no quit prompt, want a prompt") } } @@ -222,19 +224,20 @@ func TestTabs(t *testing.T) { m = press(t, m, "2") if !shows(m, "Files (2)", "package a") { - t.Errorf("Files tab:\n%s\nwant 2 files, with a.go shown", view(m)) + t.Errorf("Files tab got:\n%s\nwant 2 files, with a.go shown", view(m)) } // The repository has no remote, so the PR tab shows why. m = press(t, m, "3") m = step(t, m, m.loadPR()()) if v := view(m); !strings.Contains(v, "no git remote") { - t.Errorf("the PR tab does not say that there is no git remote:\n%s", v) + t.Errorf("the PR tab got:\n%s\nwant the note that there is no git remote", + v) } } // TestPRChecks checks that the PR tab shows the counts of the checks of a -// pull request, and the names of the checks that failed. +// pull request, and the names of the checks that fail. func TestPRChecks(t *testing.T) { m, _ := newTestModel(t) m = press(t, m, "3") @@ -250,7 +253,7 @@ func TestPRChecks(t *testing.T) { } m = step(t, m, prMsg{pr: pr}) if !shows(m, "checks: ✓ 2 ✗ 1 ◌ 1", "✗ build") { - t.Errorf("the PR tab does not show the checks:\n%s", view(m)) + t.Errorf("the PR tab got:\n%s\nwant the checks", view(m)) } } @@ -272,11 +275,13 @@ func TestSmallScreens(t *testing.T) { m = press(t, m, k) lines := strings.Split(view(m), "\n") if len(lines) > max(size.h, 1) { - t.Errorf("%dx%d after %q: %d lines", size.w, size.h, k, len(lines)) + t.Errorf("%dx%d after %q: %d lines, want %d or fewer", + size.w, size.h, k, len(lines), max(size.h, 1)) } for _, l := range lines { if w := ansi.StringWidth(l); w > max(size.w, 1) { - t.Errorf("%dx%d after %q: line width %d: %q", size.w, size.h, k, w, l) + t.Errorf("%dx%d after %q: line width %d, want %d or fewer: %q", + size.w, size.h, k, w, max(size.w, 1), l) break } } @@ -291,7 +296,7 @@ func TestDragAfterReload(t *testing.T) { m.drag = dragState{active: true, gutter: true, moved: true, start: 48} m = step(t, m, tea.MouseReleaseMsg{Button: tea.MouseLeft}) - m.changes.set(nil, nil) // a reload found no changes + m.changes.set(nil, nil) // a reload finds no changes m.drag = dragState{active: true} m = step(t, m, tea.MouseMotionMsg{Button: tea.MouseLeft, Y: m.h}) view(m) @@ -303,7 +308,7 @@ func screenY(t *testing.T, m model, i int) int { _, rowAt := m.mainLines(m.bodyHeight()) y := slices.Index(rowAt, i) if y < 0 { - t.Fatalf("row %d is not on the screen", i) + t.Fatalf("row %d is not on the screen, want it on the screen", i) } return bodyY + y } @@ -325,10 +330,10 @@ func TestClickOldNumber(t *testing.T) { m = click(t, m, m.mainX()+gutter, screenY(t, m, i)) m = press(t, m, "h", "i", "enter") if v := view(m); !strings.Contains(v, "💬 hi") { - t.Errorf("the comment does not show:\n%s", v) + t.Errorf("view got:\n%s\nwant the comment", v) } if m = press(t, m, "d"); m.comments.Len() != 0 { - t.Errorf("d did not delete the comment: %+v", m.comments.All()) + t.Errorf("after d: comments %+v, want none", m.comments.All()) } } @@ -418,7 +423,7 @@ func TestParseConfig(t *testing.T) { t.Fatal(err) } if _, err := parseConfig(string(all)); err != nil { - t.Errorf("every action: %v", err) + t.Errorf("parseConfig with every action: %v, want no error", err) } // An unknown key is an error. @@ -426,8 +431,8 @@ func TestParseConfig(t *testing.T) { {`{"layuot": "split"}`, `"layuot"`}, {`{"keybindings": {"next-fiel": ["x"]}}`, `"keybindings.next-fiel"`}, {`{"theme": "monokai"}`, `"theme"`}, - // Lua keys are case-sensitive, so a key in another case is unknown, - // and cannot skip the checks. + // Lua keys are case-sensitive. A key in another case is unknown and + // cannot skip the checks. {`{"EDITOR": " "}`, `"EDITOR"`}, {`{"keybindings": {"Send": ["x"]}}`, `"keybindings.Send"`}, } @@ -486,7 +491,7 @@ func TestParseConfig(t *testing.T) { }` c, err := parseConfig(good) if err != nil { - t.Fatalf("good config: %v", err) + t.Fatalf("parseConfig(good) = %v, want no error", err) } if c.focusAgent() || c.FileList != positionLeft || c.Layout != LayoutUnified || !c.DiffSigns { t.Errorf("good config = %+v, want no focus, the list on the left, "+ @@ -553,7 +558,8 @@ func TestConfigFlags(t *testing.T) { m = step(t, m, tea.WindowSizeMsg{Width: 120, Height: 40}) m = press(t, m, "X") if m.split { - t.Error("the key X of the configuration did not change the layout") + t.Errorf("after the key X of the configuration: split %v, want false", + m.split) } } @@ -574,15 +580,17 @@ func TestNotice(t *testing.T) { words := strings.Fields(v) for _, w := range strings.Fields(notice)[:30] { if !slices.Contains(words, w) { - t.Errorf("the view does not show %q of the notice:\n%s", w, v) + t.Errorf("view got:\n%s\nwant %q of the notice", v, w) } } if !strings.Contains(v, strings.Repeat("x", 80)) { - t.Errorf("the view does not show the start of a word wider than the screen") + t.Errorf("view got:\n%s\nwant the start of a word wider than the screen", + v) } for line := range strings.SplitSeq(v, "\n") { if ansi.StringWidth(line) > 80 { - t.Errorf("line wider than the screen: %q", line) + t.Errorf("line %q is %d cells wide, want 80 or fewer", + line, ansi.StringWidth(line)) } } } @@ -595,16 +603,17 @@ func TestPalette(t *testing.T) { // The tint is black, mixed 16% toward pure green. const tint = "48;2;0;40;0m" if strings.Contains(m.View().Content, tint) { - t.Fatal("the view has the tint before the terminal reports its green") + t.Fatal("the view has the tint before the terminal reports its green, " + + "want no tint") } m = step(t, m, uv.UnknownOscEvent("\x1b]4;2;rgb:0000/ffff/0000\x1b\\")) if !strings.Contains(m.View().Content, tint) { - t.Error("inserted lines are not tinted toward the green of the terminal") + t.Errorf("the view has no tint %q, want the green of the terminal", tint) } } -// wrapModel shows a change that makes line 4 of a.go too long for the -// screen, which is 80 columns wide. Wrap is off. +// wrapModel returns a model that shows a change that makes line 4 of a.go +// too long for the screen. The screen is 80 columns wide. Wrap is off. func wrapModel(t *testing.T) (m model, dir, long string) { t.Helper() m, dir = newTestModel(t) @@ -638,13 +647,14 @@ func TestWrap(t *testing.T) { m.wrap, len(m.changes.lines), cut) } if v := view(m); !strings.Contains(v, "x + 1") { - t.Errorf("the end of the long line does not show:\n%s", v) + t.Errorf("view got:\n%s\nwant the end of the long line", v) } // The cursor moves by source lines. m = press(t, m, "]") for range len(m.changes.lines) { if m.changes.cont(m.changes.cur) { - t.Fatalf("the cursor is on a continued row %d", m.changes.cur) + t.Fatalf("the cursor is on continued row %d, want a source line", + m.changes.cur) } m = press(t, m, "j") } @@ -662,12 +672,13 @@ func TestWrap(t *testing.T) { return strings.Contains(l, "💬 ok") }) if i < 1 || !strings.Contains(lines[i-1], "x + 1") { - t.Errorf("the comment is not below the end of the line:\n%s", view(m)) + t.Errorf("view got:\n%s\nwant the comment below the end of the line", + view(m)) } // The sideways scroll applies only without wrap. if m = step(t, m, tea.KeyPressMsg{Code: tea.KeyRight}); m.changes.xoff != 0 { - t.Errorf("→ with wrap scrolled to %d", m.changes.xoff) + t.Errorf("→ with wrap: xoff %d, want 0", m.changes.xoff) } if m = press(t, m, "w"); m.wrap || len(m.changes.lines) != cut { t.Errorf("w again: wrap %v, %d lines, want %d", m.wrap, len(m.changes.lines), cut) @@ -685,13 +696,13 @@ func TestWrapKeepsCursor(t *testing.T) { m, dir, long := wrapModel(t) m = press(t, m, "end", "w") if got := m.target(m.changes.cur, diff.SideNew).line; got != 5 { - t.Errorf("w moved the cursor from line 5 to line %d", got) + t.Errorf("after w: the cursor is on line %d, want 5", got) } m = twoHunks(t, m, dir, long) m.changes.cur = hunks(m)[1] if m = press(t, m, "w"); m.changes.cur != hunks(m)[1] { - t.Errorf("w moved the cursor from the second hunk header to row %+v", + t.Errorf("after w: the cursor is on row %+v, want the second hunk header", m.changes.rows[m.changes.cur]) } } @@ -722,17 +733,18 @@ func TestTallLine(t *testing.T) { m = step(t, m, tea.MouseWheelMsg{Button: b, X: 1, Y: 5}) m = step(t, m, tea.BlurMsg{}) // a later event keeps the view if m.changes.top == top { - t.Fatalf("the wheel stopped at row %d of %d", top, len(m.changes.lines)) + t.Fatalf("the wheel stops at row %d of %d, want a scroll", + top, len(m.changes.lines)) } } // A taller screen shows more rows, not blank space below the view. m = step(t, m, tea.WindowSizeMsg{Width: 30, Height: 40}) if last := max(len(m.changes.lines)-m.bodyHeight(), 0); m.changes.top > last { - t.Errorf("top %d of %d rows on a screen of %d rows", - m.changes.top, len(m.changes.lines), m.bodyHeight()) + t.Errorf("top %d of %d rows on a screen of %d rows, want %d or less", + m.changes.top, len(m.changes.lines), m.bodyHeight(), last) } if m = press(t, m, "j"); m.changes.top > m.changes.cur { - t.Errorf("j does not show the cursor: top %d, cursor %d", + t.Errorf("after j: top %d, cursor %d; want the cursor on the screen", m.changes.top, m.changes.cur) } @@ -744,7 +756,7 @@ func TestTallLine(t *testing.T) { m = press(t, m, "j") } if v := view(m); !strings.Contains(v, "return x") { - t.Errorf("the first rows of the line do not show:\n%s", v) + t.Errorf("view got:\n%s\nwant the first rows of the line", v) } m = press(t, m, "c", "o", "k", "enter", "c", "z", "z") lines := strings.Split(view(m), "\n") @@ -753,10 +765,12 @@ func TestTallLine(t *testing.T) { } i := slices.IndexFunc(lines, has("okzz")) if i < 0 || !slices.ContainsFunc(lines[:i], has("+ 1")) { - t.Errorf("the comment box is not below the end of the line:\n%s", view(m)) + t.Errorf("view got:\n%s\nwant the comment box below the end of the line", + view(m)) } if strings.Contains(view(m), "💬 ok") { - t.Errorf("the comment shows above the box that edits it:\n%s", view(m)) + t.Errorf("view got:\n%s\nwant no comment above the box that edits it", + view(m)) } } @@ -768,7 +782,7 @@ func TestHostileText(t *testing.T) { raw := func(where string, m model) { t.Helper() if strings.Contains(m.View().Content, osc) { - t.Errorf("%s: the screen has the escape sequence raw", where) + t.Errorf("%s: the screen has the raw escape sequence, want none", where) } } m, dir := newTestModel(t) @@ -854,12 +868,13 @@ func TestEmptyRepo(t *testing.T) { func TestFooterHints(t *testing.T) { m, _ := newTestModel(t) if m = press(t, m, "]", "v"); !strings.Contains(view(m), "extend") { - t.Errorf("select mode: the footer has no hint:\n%s", view(m)) + t.Errorf("select mode: view got:\n%s\nwant the hint extend", view(m)) } m = press(t, m, "esc") m = step(t, m, tea.KeyPressMsg{Code: 'f', Mod: tea.ModCtrl}) m = press(t, m, "r", "e", "t", "u", "r", "n", "enter") if !strings.Contains(view(m), "next/prev match") { - t.Errorf("find mode: the footer has no hint:\n%s", view(m)) + t.Errorf("find mode: view got:\n%s\nwant the hint next/prev match", + view(m)) } } diff --git a/tui/view.go b/tui/view.go index c31b661..9eed3c5 100644 --- a/tui/view.go +++ b/tui/view.go @@ -33,8 +33,8 @@ var ( // truncate cuts s to w columns, with an ellipsis. func truncate(s string, w int) string { return ansi.Truncate(s, max(w, 0), "…") } -// selStyle returns the style of a selected row: a tint of the terminal -// background. +// selStyle returns the style of a selected row. The style is a tint of the +// terminal background. func (m *model) selStyle() lipgloss.Style { if m.bg == nil { return lipgloss.NewStyle().Reverse(true) @@ -137,7 +137,7 @@ func (m *model) headerScopes(l *headerLine) { } } -// commitsLabel returns the name of the commits of the commits scope in the header. +// commitsLabel returns the header text for the commits of the commits scope. func (m *model) commitsLabel() string { if m.to == "" { return "pick ▾" @@ -199,7 +199,8 @@ func statusMark(s vcs.Status) string { return " " } -// listLines renders h lines of the file list: a title and the files. +// listLines renders h lines of the file list. The lines are a title and the +// files. func (m *model) listLines(h int) []string { w := m.listWidth() var title string @@ -265,8 +266,8 @@ func (m *model) selectedRow(r string, w int) string { // listStart returns the index of the first file that the list shows. func (m *model) listStart(h int) int { return max(0, m.listSel()-(h-2)) } -// fileRow returns " M name/of/file.go +3 -1". If the path is too long, it -// removes the directory first, then the start of the name. +// fileRow returns " M name/of/file.go +3 -1". If the path is too long, +// fileRow removes the directory first, then the start of the name. func fileRow(mark, p, stat string, w int) string { p = termtext.Quote(p) room := w - lipgloss.Width(stat) - 4 @@ -277,7 +278,8 @@ func fileRow(mark, p, stat string, w int) string { p = "…" + ansi.TruncateLeft(p, n-room+1, "") } } - pad := strings.Repeat(" ", max(room-ansi.StringWidth(p), 0)) // by width, not runes + // Pad by width, not by runes. + pad := strings.Repeat(" ", max(room-ansi.StringWidth(p), 0)) return " " + mark + " " + p + pad + " " + stat } @@ -431,7 +433,7 @@ func (m *model) ensureVisible() { if m.mode == modeComment { last, box = p.end(p.cur), lipgloss.Height(m.editor.View()) } - // Each row takes one line or more. So the last row cannot show if it is + // Each row takes one line or more. Thus the last row cannot show if it is // h rows or more below the top. p.top = max(p.top, last-h+1) p.top = min(last, p.top, max(len(p.lines)-1, 0)) @@ -456,7 +458,7 @@ func (m *model) footer() string { // wrapWords breaks s into lines of width w or less, at spaces only. // ansi.Wrap and lipgloss let a line end past w at a hyphen. Then the edge -// of the screen cuts off a "--". A word wider than w is cut, and a +// of the screen cuts off a "--". wrapWords cuts a word wider than w. A // character wider than w gets a line of its own. func wrapWords(s string, w int) string { w = max(w, 1) diff --git a/update/update.go b/update/update.go index 4446572..f1a8a88 100644 --- a/update/update.go +++ b/update/update.go @@ -59,8 +59,8 @@ const ( ) // Release is a release of rexdiff on GitHub. Get one from [Updater.Latest]. -// A Release that you make yourself has no files. Install can give it to -// Homebrew or go install, but it cannot download it. +// A Release that you make yourself has no files. Install can install such a +// Release with Homebrew or go install. Install cannot download it. type Release struct { Tag string // for example "v0.2.0" assets []asset @@ -107,8 +107,8 @@ func Version(stamped string) string { // rexdiff. If bin links into a Caskroom, it uses Homebrew. If stamped is "" // and the version comes from go install, it uses go install. Else it // downloads the release. If REXDIFF_LATEST_URL is set, the Updater gets the -// latest release from that address. Use it to test a release before it is -// out. +// latest release from that address. Use this variable to test a release +// before it is public. func Detect(bin, stamped string) *Updater { return detect(bin, Version(stamped), stamped == "") } @@ -160,10 +160,10 @@ func (u *Updater) Latest(ctx context.Context) (Release, bool, error) { return r, newer(r.Tag, u.version), nil } -// Install installs the release r, and checks that the binary of rexdiff is -// then the version of r. Install does not stop the running rexdiff. A -// cancel of ctx does not stop an install that runs, because a stopped brew -// can leave a broken cask. Install stops after five minutes. +// Install installs the release r. Then it checks that the binary of rexdiff +// has the version of r. Install does not stop the running rexdiff. Install +// ignores a cancel of ctx, because a brew that stops early can leave a broken +// cask. Install stops after five minutes. func (u *Updater) Install(ctx context.Context, r Release) error { ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), installTimeout) defer cancel() @@ -229,9 +229,10 @@ func (u *Updater) command(ctx context.Context, env []string, name string, args . return out, fmt.Errorf("%s: %w", filepath.Base(name), err) } -// tool returns the path of the program name in dirs. Else it returns name, -// and exec looks for it in PATH. tool looks in dirs first, because Rex -// starts panes with a PATH without Homebrew or Go. +// tool returns the path of the program name in dirs. If dirs do not have +// the program, tool returns name, and exec looks for name in PATH. tool +// looks in dirs first, because Rex starts panes with a PATH without +// Homebrew or Go. func tool(name string, dirs ...string) string { for _, dir := range dirs { path := filepath.Join(dir, name) diff --git a/update/update_test.go b/update/update_test.go index 60910eb..5577814 100644 --- a/update/update_test.go +++ b/update/update_test.go @@ -116,10 +116,10 @@ func TestLatest(t *testing.T) { t.Errorf("Latest of the same version: newer %v, %v; want false, nil", isNewer, err) } if _, _, err := answer(403, `rate limit`).Latest(t.Context()); err == nil || !strings.Contains(err.Error(), "403") { - t.Errorf("Latest after 403: %v, want the status", err) + t.Errorf("Latest after 403: %v, want an error with the status", err) } if _, _, err := answer(200, `not json`).Latest(t.Context()); err == nil { - t.Error("Latest of bad JSON: no error") + t.Error("Latest of bad JSON: no error, want an error") } } @@ -156,7 +156,7 @@ func TestInstallCommands(t *testing.T) { "/opt/homebrew/bin/rexdiff -version", } if !slices.Equal(brew.calls, want) { - t.Errorf("brew calls:\n%s\nwant:\n%s", strings.Join(brew.calls, "\n"), strings.Join(want, "\n")) + t.Errorf("brew calls got:\n%s\nwant:\n%s", strings.Join(brew.calls, "\n"), strings.Join(want, "\n")) } gorun := &fakeRun{version: "v0.2.0"} @@ -175,7 +175,8 @@ func TestInstallCommands(t *testing.T) { u = &Updater{bin: "/b/rexdiff", method: methodBrew, run: failing.run} err := u.Install(t.Context(), r) if err == nil || err.Error() != "brew: exit status 1: Error: another brew is running" { - t.Errorf("Install with a failing brew: %v", err) + t.Errorf("Install with a failing brew: %v, want %q", + err, "brew: exit status 1: Error: another brew is running") } // A new binary of another version is an error, so rexdiff does not @@ -183,7 +184,8 @@ func TestInstallCommands(t *testing.T) { stale := &fakeRun{version: "v0.1.0"} u = &Updater{bin: "/b/rexdiff", method: methodBrew, run: stale.run} if err := u.Install(t.Context(), r); err == nil || !strings.Contains(err.Error(), "left rexdiff v0.1.0, not v0.2.0") { - t.Errorf("Install that leaves the old version: %v", err) + t.Errorf("Install that leaves the old version: %v, want an error", + err) } } @@ -257,7 +259,7 @@ func TestDownload(t *testing.T) { t.Errorf("target file = %q, want the new binary", b) } if info, _ := os.Lstat(link); info.Mode()&os.ModeSymlink == 0 { - t.Error("the link is no longer a link") + t.Errorf("after Install: %s is not a symlink, want a symlink", link) } if info, _ := os.Stat(target); info.Mode().Perm() != 0o755 { t.Errorf("mode = %v, want 0755", info.Mode().Perm()) @@ -269,7 +271,8 @@ func TestDownload(t *testing.T) { // A file that does not match its checksum is not installed. file(t, target, "old binary") if err := u.Install(t.Context(), release("/bad")); err == nil || !strings.Contains(err.Error(), "does not match its checksum") { - t.Errorf("Install of a tampered archive: %v", err) + t.Errorf("Install of a tampered archive: %v, want a checksum error", + err) } if b, _ := os.ReadFile(target); string(b) != "old binary" { t.Errorf("target file after a bad download = %q, want the old binary", b) @@ -280,25 +283,28 @@ func TestDownload(t *testing.T) { stale := &fakeRun{version: "v0.1.9"} u.run = stale.run if err := u.Install(t.Context(), release("")); err == nil || !strings.Contains(err.Error(), "left rexdiff v0.1.9, not v0.2.0") { - t.Errorf("Install of a binary of another version: %v", err) + t.Errorf("Install of a binary of another version: %v, "+ + "want a version error", err) } if b, _ := os.ReadFile(target); string(b) != "old binary" { t.Errorf("target file after a failed check = %q, want the old binary", b) } if entries, _ := os.ReadDir(filepath.Dir(target)); len(entries) != 1 { - t.Errorf("a failed check left %d files, want only rexdiff", len(entries)) + t.Errorf("after a failed check: %d files, want only rexdiff", + len(entries)) } u.run = fake.run // A release without the archive of this system is an error. if err := u.Install(t.Context(), Release{Tag: "v0.2.0"}); err == nil || !strings.Contains(err.Error(), "has no "+name) { - t.Errorf("Install of a release without assets: %v", err) + t.Errorf("Install of a release without assets: %v, want an error", + err) } } // TestInstallOutlivesCancel checks that the cancel of the context of the -// program does not stop a brew that runs, because a stopped brew can leave -// a broken cask. +// program does not stop a brew that runs. A stopped brew can leave a broken +// cask. func TestInstallOutlivesCancel(t *testing.T) { ctx, cancel := context.WithCancel(t.Context()) var sawCancel bool @@ -312,7 +318,8 @@ func TestInstallOutlivesCancel(t *testing.T) { } u := &Updater{bin: "/b/rexdiff", method: methodBrew, run: run} if err := u.Install(ctx, Release{Tag: "v0.2.0"}); err != nil || sawCancel { - t.Errorf("Install after a cancel: %v; brew saw the cancel: %v", err, sawCancel) + t.Errorf("Install after a cancel = %v, brew sees the cancel %v; "+ + "want nil, false", err, sawCancel) } } @@ -321,15 +328,16 @@ func TestInstallOutlivesCancel(t *testing.T) { func TestBinaryTooLarge(t *testing.T) { big := strings.Repeat("x", maxBinary+1) if _, err := binaryOf(archive(t, big)); err == nil || !strings.Contains(err.Error(), "larger than") { - t.Errorf("binaryOf of a binary over the limit: %v", err) + t.Errorf("binaryOf of a binary over the limit: %v, want an error", + err) } if b, err := binaryOf(archive(t, "ok")); err != nil || string(b) != "ok" { - t.Errorf("binaryOf = %q, %v", b, err) + t.Errorf("binaryOf = %q, %v, want \"ok\", nil", b, err) } } -// TestVersion checks the version from the release build, and that a test -// binary, like a build for development, has none. +// TestVersion checks the version from the release build. A test binary has +// no version, like a build for development. func TestVersion(t *testing.T) { for stamped, want := range map[string]string{"0.2.0": "v0.2.0", "v0.2.0": "v0.2.0", "": ""} { if got := Version(stamped); got != want { @@ -341,12 +349,13 @@ func TestVersion(t *testing.T) { } t.Setenv("REXDIFF_LATEST_URL", "http://127.0.0.1:1/latest") if u := Detect("/bin/rexdiff", "0.2.0"); u == nil || u.latestURL != "http://127.0.0.1:1/latest" || u.method != methodDownload { - t.Errorf("Detect of a release build = %+v", u) + t.Errorf("Detect of a release build = %+v, "+ + "want a download from the test URL", u) } } // TestDetectBrew checks that a Homebrew install uses the brew beside its -// Caskroom, because Rex starts rexdiff with a PATH without Homebrew. +// Caskroom. Rex starts rexdiff with a PATH without Homebrew. func TestDetectBrew(t *testing.T) { prefix := t.TempDir() cask := file(t, filepath.Join(prefix, "Caskroom", "rexdiff", "0.2.0", "rexdiff"), "") diff --git a/vcs/hostile_test.go b/vcs/hostile_test.go index 5bf83fe..19b8e57 100644 --- a/vcs/hostile_test.go +++ b/vcs/hostile_test.go @@ -41,11 +41,11 @@ func TestRevIsNotAnOption(t *testing.T) { t.Errorf("Parent(%q) = %s, want an error", rev, id) } if _, err := r.Diff(ctx, rev, head(t, r)); err == nil { - t.Errorf("Diff(%q, HEAD): no error", rev) + t.Errorf("Diff(%q, HEAD): no error, want an error", rev) } } if b, _ := os.ReadFile(out); string(b) != "keep\n" { - t.Errorf("git changed the file of --output: %q", b) + t.Errorf("the file of --output = %q, want %q", b, "keep\n") } } @@ -63,7 +63,8 @@ func TestHostilePaths(t *testing.T) { for i, p := range paths { s := fmt.Sprintf("id%02d\n", i) if err := os.WriteFile(filepath.Join(dir, p), []byte(s), 0o644); err != nil { - t.Logf("skip %q: %v", p, err) // for example, macOS needs UTF-8 names + // For example, macOS needs UTF-8 names. + t.Logf("skip %q: %v", p, err) continue } want[p] = s @@ -77,7 +78,7 @@ func TestHostilePaths(t *testing.T) { got[f.Path] = f.New } if !reflect.DeepEqual(got, want) { - t.Errorf("Diff:\n%q\nwant:\n%q", got, want) + t.Errorf("Diff got:\n%q\nwant:\n%q", got, want) } es, err := r.Files(ctx) @@ -90,7 +91,7 @@ func TestHostilePaths(t *testing.T) { } for p, s := range want { if !files[p] { - t.Errorf("Files does not have %q", p) + t.Errorf("Files has no %q, want it", p) } if got, err := r.ReadFile(p); got != s || err != nil { t.Errorf("ReadFile(%q) = %q, %v; want %q", p, got, err, s) @@ -149,7 +150,7 @@ func TestDiffFileKinds(t *testing.T) { {Path: "type", Status: vcs.StatusModified, Old: "a file\n", New: "a.txt"}, } if got := changes(t, r, head(t, r), snapshot(t, r)); !reflect.DeepEqual(got, want) { - t.Errorf("Diff:\n%+v\nwant:\n%+v", got, want) + t.Errorf("Diff got:\n%+v\nwant:\n%+v", got, want) } } @@ -247,7 +248,8 @@ func TestGitWorktree(t *testing.T) { } st, _ := exec.Command("git", "-C", wt, "status", "--porcelain").Output() if string(st) != " M a.txt\nA staged.txt\n" { - t.Errorf("the index of the worktree changed:\n%s", st) + t.Errorf("git status in the worktree got:\n%s\nwant:\n%s", + st, " M a.txt\nA staged.txt\n") } } @@ -258,7 +260,8 @@ func TestDetachedHead(t *testing.T) { git(t, dir, "checkout", "-q", "--detach", "main") got := names(changes(t, r, head(t, r), snapshot(t, r))) if got != "A.gitignore Ma.txt Anew.txt" { - t.Errorf("Diff(Head, Snapshot) = %s", got) + t.Errorf("Diff(Head, Snapshot) = %s, want A.gitignore Ma.txt Anew.txt", + got) } if cs, err := r.Commits(ctx, 5); err != nil || len(cs) != 1 || cs[0].Subject != "init" { t.Errorf("Commits = %+v, %v; want init", cs, err) @@ -290,7 +293,7 @@ func TestGrepIsLiteral(t *testing.T) { for _, tt := range tests { ms, err := r.Grep(ctx, tt.q, 10) if err != nil { - t.Errorf("Grep(%q): %v", tt.q, err) + t.Errorf("Grep(%q) = %v, want no error", tt.q, err) continue } var got []string diff --git a/vcs/pr_test.go b/vcs/pr_test.go index 7d736f2..0b7271d 100644 --- a/vcs/pr_test.go +++ b/vcs/pr_test.go @@ -142,11 +142,12 @@ func TestPullRequest(t *testing.T) { r, _ := repoAt(t, tt.remote) pr, err := r.PullRequest(t.Context(), tt.hosts) if err != nil { - t.Errorf("PullRequest(%s): %v", tt.remote, err) + t.Errorf("PullRequest(%s) = %v, want no error", tt.remote, err) continue } if !reflect.DeepEqual(pr, tt.want) { - t.Errorf("PullRequest(%s):\n%+v\nwant:\n%+v", tt.remote, pr, tt.want) + t.Errorf("PullRequest(%s) got:\n%+v\nwant:\n%+v", + tt.remote, pr, tt.want) } } } @@ -188,7 +189,7 @@ func TestPullRequestErrors(t *testing.T) { _, err := r.PullRequest(t.Context(), vcs.Hosts{}) switch { case err == nil: - t.Errorf("PullRequest(%q): no error", tt.remote) + t.Errorf("PullRequest(%q): no error, want an error", tt.remote) case tt.want != nil && !errors.Is(err, tt.want): t.Errorf("PullRequest(%q): err = %v, want %v", tt.remote, err, tt.want) case tt.msg != "" && err.Error() != tt.msg: diff --git a/vcs/refs.go b/vcs/refs.go index 30334b3..803b8ce 100644 --- a/vcs/refs.go +++ b/vcs/refs.go @@ -137,7 +137,7 @@ func (r *Repo) Files(ctx context.Context) ([]Entry, error) { return es, nil } -// Match is a line that [Repo.Grep] found. +// Match is a line that [Repo.Grep] finds. type Match struct { Path string Line int // the line number, from 1 @@ -148,7 +148,8 @@ type Match struct { // that contain the text q. The search ignores case and binary files. func (r *Repo) Grep(ctx context.Context, q string, limit int) ([]Match, error) { if strings.Contains(q, "\n") { - // git reads each line as a pattern, but no line has a newline. + // git reads each line of q as a pattern. A match is one line, so no + // match can have the newline of q. return nil, nil } // --no-column overrides the git setting grep.column. That setting adds a @@ -163,7 +164,7 @@ func (r *Repo) Grep(ctx context.Context, q string, limit int) ([]Match, error) { return nil, err } // Each match is "path NUL line NUL text LF". Only the text ends with a - // newline, because the path can have one. + // newline, because the path can have a newline. var ms []Match for len(ms) < limit && out != "" { var path, num, text string diff --git a/vcs/repo.go b/vcs/repo.go index 66ea07e..60471d3 100644 --- a/vcs/repo.go +++ b/vcs/repo.go @@ -166,7 +166,8 @@ func (r *Repo) Parent(ctx context.Context, rev string) (string, error) { if err != nil { return "", err } - out, err := r.run(ctx, nil, "rev-parse", c+"^@") // the parents, one on each line + // The output has the parents, one on each line. + out, err := r.run(ctx, nil, "rev-parse", c+"^@") if err != nil { return "", err } @@ -206,7 +207,7 @@ func (r *Repo) Snapshot(ctx context.Context) (string, error) { env := []string{"GIT_INDEX_FILE=" + idx} _, err = r.run(ctx, env, "add", "-A", "--ignore-errors") if exitedWith(err, 1) { - err = nil // git could not add some files, but it added the others + err = nil // git cannot add some files, but it adds the others } if err != nil { return "", fmt.Errorf("snapshot: %w", err) diff --git a/vcs/repo_test.go b/vcs/repo_test.go index 1b17875..0cc53a4 100644 --- a/vcs/repo_test.go +++ b/vcs/repo_test.go @@ -132,10 +132,11 @@ func TestScopes(t *testing.T) { now := snapshot(t, r) unc := changes(t, r, head(t, r), now) if got := names(unc); got != "A.gitignore Ma.txt Anew.txt" { - t.Errorf("uncommitted: %s", got) + t.Errorf("uncommitted: %s, want A.gitignore Ma.txt Anew.txt", got) } if unc[1].Old != "one\n" || unc[1].New != "one\ntwo\n" { - t.Errorf("a.txt: %q -> %q", unc[1].Old, unc[1].New) + t.Errorf("a.txt: Old %q, New %q; want %q, %q", + unc[1].Old, unc[1].New, "one\n", "one\ntwo\n") } mb, err := r.MergeBase(ctx, "main") @@ -143,37 +144,37 @@ func TestScopes(t *testing.T) { t.Fatal(err) } if got := names(changes(t, r, mb, now)); got != "A.gitignore Ma.txt Ab.txt Anew.txt" { - t.Errorf("branch: %s", got) + t.Errorf("branch: %s, want A.gitignore Ma.txt Ab.txt Anew.txt", got) } if _, err := r.MergeBase(ctx, "nosuchbranch"); err == nil { - t.Error("MergeBase(nosuchbranch): no error") + t.Error("MergeBase(nosuchbranch): no error, want an error") } - // A turn: a snapshot, then the agent edits one file. + // In a turn, rexdiff takes a snapshot and then the agent edits a file. write(t, dir, "new.txt", "untracked\nmore\n") tf := changes(t, r, now, snapshot(t, r)) if got := names(tf); got != "Mnew.txt" || tf[0].New != "untracked\nmore\n" { - t.Errorf("turn: %s", got) + t.Errorf("turn: %s, want Mnew.txt with the new line", got) } // Snapshot must not change the real index. st, _ := exec.Command("git", "-C", dir, "status", "--porcelain").Output() if !strings.Contains(string(st), "?? new.txt") { - t.Errorf("the index changed: %s", st) + t.Errorf("git status after Snapshot = %q, want ?? new.txt", st) } cs, err := r.Commits(ctx, 5) if err != nil || len(cs) != 2 || cs[0].Subject != "b" || cs[1].Subject != "init" { - t.Fatalf("Commits = %+v, %v", cs, err) + t.Fatalf("Commits = %+v, %v, want b and init", cs, err) } tree, err := r.Tree(ctx, cs[0].SHA) if err != nil { t.Fatal(err) } if got := names(changes(t, r, parent(t, r, cs[0].SHA), tree)); got != "Ab.txt" { - t.Errorf("commit b: %s", got) + t.Errorf("commit b: %s, want Ab.txt", got) } - // The ID of the tree that has no files. + // emptyTree is the ID of the tree that has no files. const emptyTree = "4b825dc642cb6eb9a060e54bf8d69288fbee4904" if got := parent(t, r, cs[1].SHA); got != emptyTree { t.Errorf("parent of the root commit = %s, want the empty tree", got) @@ -183,13 +184,12 @@ func TestScopes(t *testing.T) { // TestMissingCommit checks that Head and Parent fail when a commit object // is missing. func TestMissingCommit(t *testing.T) { - // The empty tree is not a correct result, because it shows every file as - // added. + // The empty tree is not a correct result. It shows every file as added. r, dir := newRepo(t) ctx := t.Context() cs, err := r.Commits(ctx, 2) if err != nil || len(cs) != 2 { - t.Fatalf("Commits = %+v, %v", cs, err) + t.Fatalf("Commits = %+v, %v, want two commits", cs, err) } remove := func(sha string) { t.Helper() @@ -218,17 +218,19 @@ func TestDiffSkipsBinaryAndBigFiles(t *testing.T) { write(t, dir, "bin", "a\x00b") write(t, dir, "big", strings.Repeat("x\n", 50_001)) write(t, dir, "huge", strings.Repeat("x", 2<<20+1)) // over 2 MiB - write(t, dir, "small", "after\n") // read after the skipped content + // Diff reads this file after the skipped content. + write(t, dir, "small", "after\n") got := map[string]vcs.File{} for _, f := range changes(t, r, head(t, r), snapshot(t, r)) { got[f.Path] = f } if f := got["bin"]; !f.Binary || f.New != "" { - t.Errorf("bin: %+v", f) + t.Errorf("bin: %+v, want Binary with no New", f) } for _, p := range []string{"big", "huge"} { if f := got[p]; !f.TooBig || f.New != "" { - t.Errorf("%s: TooBig = %v, New has %d bytes", p, f.TooBig, len(f.New)) + t.Errorf("%s: TooBig = %v, New has %d bytes; want true, 0", + p, f.TooBig, len(f.New)) } } if f := got["small"]; f.New != "after\n" { @@ -260,7 +262,7 @@ func TestFilesInConflict(t *testing.T) { cmd := exec.Command("git", "merge", "other") cmd.Dir = dir if err := cmd.Run(); err == nil { - t.Fatal("the merge has no conflict") + t.Fatal("git merge other: no error, want a conflict") } r, err := vcs.Open(t.Context(), dir) if err != nil { @@ -305,20 +307,21 @@ func TestWorktree(t *testing.T) { ms, err := r.Grep(ctx, "TWO", 10) match := vcs.Match{Path: "a.txt", Line: 2, Text: "two"} if err != nil || len(ms) != 1 || ms[0] != match { - t.Errorf("Grep(TWO) = %+v, %v", ms, err) + t.Errorf("Grep(TWO) = %+v, %v, want [%+v], nil", ms, err, match) } write(t, dir, "a:é\n.txt", "x: two\n") // git quotes this name unless -z ms, err = r.Grep(ctx, "x: two", 10) match = vcs.Match{Path: "a:é\n.txt", Line: 1, Text: "x: two"} if err != nil || len(ms) != 1 || ms[0] != match { - t.Errorf("Grep(x: two) = %+v, %v", ms, err) + t.Errorf("Grep(x: two) = %+v, %v, want [%+v], nil", ms, err, match) } if ms, err := r.Grep(ctx, "nothing has this", 10); err != nil || len(ms) != 0 { - t.Errorf("Grep with no match = %+v, %v", ms, err) + t.Errorf("Grep with no match = %+v, %v, want none, nil", ms, err) } if s, err := r.ReadFile("a.txt"); err != nil || s != "one\ntwo\n" { - t.Errorf("ReadFile(a.txt) = %q, %v", s, err) + t.Errorf("ReadFile(a.txt) = %q, %v, want %q, nil", + s, err, "one\ntwo\n") } contains := []struct { path string @@ -340,13 +343,13 @@ func TestRefs(t *testing.T) { r, _ := newRepo(t) ctx := t.Context() if bs, err := r.Branches(ctx); err != nil || strings.Join(bs, " ") != "feat main" { - t.Errorf("Branches = %v, %v", bs, err) + t.Errorf("Branches = %v, %v, want [feat main], nil", bs, err) } if b := r.DefaultBase(ctx); b != "main" { - t.Errorf("DefaultBase = %q", b) + t.Errorf("DefaultBase = %q, want main", b) } if _, err := r.Resolve(ctx, "nosuchref"); err == nil { - t.Error("Resolve(nosuchref): no error") + t.Error("Resolve(nosuchref): no error, want an error") } } @@ -407,14 +410,14 @@ func TestDiffRenameAndSubmodule(t *testing.T) { }, } if !reflect.DeepEqual(got, want) { - t.Errorf("Diff:\n%+v\nwant:\n%+v", got, want) + t.Errorf("Diff got:\n%+v\nwant:\n%+v", got, want) } } // TestOpenOutsideARepo checks that Open fails outside a repository. func TestOpenOutsideARepo(t *testing.T) { if _, err := vcs.Open(t.Context(), t.TempDir()); err == nil { - t.Error("Open: no error outside a repository") + t.Error("Open outside a repository: no error, want an error") } } @@ -437,7 +440,7 @@ func TestNewRepo(t *testing.T) { // same second as the last write of the index. func TestSnapshotRacyEdit(t *testing.T) { // git checks such "racily clean" files by their content. It finds them by - // the modification time of the index, so the copy of the index must keep + // the modification time of the index. The copy of the index must keep // that time. r, dir := newRepo(t) git(t, dir, "config", "core.trustctime", "false") @@ -452,7 +455,8 @@ func TestSnapshotRacyEdit(t *testing.T) { } write(t, dir, "b.txt", "bee\n") touch("b.txt") - git(t, dir, "update-index", "--really-refresh") // the entry gets the time of b.txt + // The index entry gets the time of b.txt. + git(t, dir, "update-index", "--really-refresh") touch(".git/index") write(t, dir, "b.txt", "BEE\n") // the same size and time touch("b.txt") From af051b07448ca58292aa7cb7800187a889deafab Mon Sep 17 00:00:00 2001 From: Redowan Delowar Date: Sun, 25 Oct 2026 12:11:40 +0100 Subject: [PATCH 8/8] refactor: clearer editor names, and one lexer cache - editor.Template, editor.Program, and editor.ShellQuote replace FromTemplate, FromEditor, and Quote. - The View reads the lexer from its Cache only. - Fix a doc link to termtext.Quote. --- diff/comment.go | 4 ++-- internal/diffview/diffview.go | 23 ++++++++--------------- internal/editor/editor.go | 28 ++++++++++++++-------------- internal/editor/editor_test.go | 4 ++-- tui/editor.go | 6 +++--- update/update.go | 4 ++-- 6 files changed, 31 insertions(+), 38 deletions(-) diff --git a/diff/comment.go b/diff/comment.go index 43c3439..b301a91 100644 --- a/diff/comment.go +++ b/diff/comment.go @@ -57,8 +57,8 @@ func NewComment(path string, s Side, rows []Row, old, new string) Comment { } // Location returns the path and the lines of c, for example "a.go:3-4". -// A comment on the old side has " (removed)" at the end. [Quote] quotes -// the path. +// A comment on the old side has " (removed)" at the end. [termtext.Quote] +// quotes the path. func (c *Comment) Location() string { loc := fmt.Sprintf("%s:%d", termtext.Quote(c.Path), c.Start) if c.End != c.Start { diff --git a/internal/diffview/diffview.go b/internal/diffview/diffview.go index 46f9313..a9c54fe 100644 --- a/internal/diffview/diffview.go +++ b/internal/diffview/diffview.go @@ -72,9 +72,6 @@ type View struct { afterNumDigits int heights []int // the number of screen lines of each row, for rows - // The View keeps the lexer, because to match a file name is slow. - cachedLexer chroma.Lexer - cache *Cache } @@ -150,10 +147,8 @@ func (v *View) After(path, content string) *View { return v } -// clearCaches clears the lexer and the diff. Call it when the content -// changes. +// clearCaches clears the diff. Call it when the content changes. func (v *View) clearCaches() { - v.cachedLexer = nil v.isComputed = false } @@ -613,17 +608,15 @@ func newSyntaxKey(lexer, source string, bgColor color.Color) syntaxKey { return k } -// chromaLexer returns the lexer of the old file, and keeps it. +// chromaLexer returns the lexer of the old file, and keeps it in the +// cache. func (v *View) chromaLexer() chroma.Lexer { - if v.cachedLexer == nil { - lx := &v.cache.lexer - if lx.l == nil || lx.path != v.before.path || lx.content != v.before.content { - lx.path, lx.content = v.before.path, v.before.content - lx.l = Lexer(lx.path, lx.content) - } - v.cachedLexer = lx.l + lx := &v.cache.lexer + if lx.l == nil || lx.path != v.before.path || lx.content != v.before.content { + lx.path, lx.content = v.before.path, v.before.content + lx.l = Lexer(lx.path, lx.content) } - return v.cachedLexer + return lx.l } // Lexer returns the coalesced lexer for the file at path. If no lexer diff --git a/internal/editor/editor.go b/internal/editor/editor.go index b7ba105..db98b71 100644 --- a/internal/editor/editor.go +++ b/internal/editor/editor.go @@ -17,26 +17,26 @@ type Command struct { Window bool // the editor opens a window and does not draw in the terminal } -// FromTemplate returns the command that opens file at line with template, +// Template returns the command that opens file at line with template, // a command that the user writes. The template gets file in place of // {file}, and line in place of {line}. Without {file}, file goes at the // end. -func FromTemplate(template, file string, line int) Command { - r := strings.NewReplacer("{file}", Quote(file), "{line}", strconv.Itoa(line)) +func Template(template, file string, line int) Command { + r := strings.NewReplacer("{file}", ShellQuote(file), "{line}", strconv.Itoa(line)) script := r.Replace(template) if !strings.Contains(template, "{file}") { - script += " " + Quote(file) + script += " " + ShellQuote(file) } return Command{script, dialectOf(template).window} } -// FromEditor returns the command that opens file at line with editor, for -// example the value of $EDITOR. FromEditor adds file and line in the form -// that the editor expects. -func FromEditor(editor, file string, line int) Command { - d := dialectOf(editor) - n, f := strconv.Itoa(line), Quote(file) - at := Quote(file + ":" + n) +// Program returns the command that opens file at line with command, the +// command of an editor program such as the value of $EDITOR. Program adds +// file and line in the form that the editor expects. +func Program(command, file string, line int) Command { + d := dialectOf(command) + n, f := strconv.Itoa(line), ShellQuote(file) + at := ShellQuote(file + ":" + n) var args string switch d.line { case argNone: @@ -50,11 +50,11 @@ func FromEditor(editor, file string, line int) Command { case argFlag: args = "--line " + n + " " + f } - return Command{editor + " " + args, d.window} + return Command{command + " " + args, d.window} } -// Quote quotes s as one word for sh. -func Quote(s string) string { return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" } +// ShellQuote quotes s as one word for sh. +func ShellQuote(s string) string { return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" } // lineArg is how an editor takes the line to open at. type lineArg int diff --git a/internal/editor/editor_test.go b/internal/editor/editor_test.go index b3a4512..c65cde4 100644 --- a/internal/editor/editor_test.go +++ b/internal/editor/editor_test.go @@ -41,9 +41,9 @@ func TestCommands(t *testing.T) { for _, tt := range tests { var got editor.Command if tt.template != "" { - got = editor.FromTemplate(tt.template, abs, 12) + got = editor.Template(tt.template, abs, 12) } else { - got = editor.FromEditor(tt.editor, abs, 12) + got = editor.Program(tt.editor, abs, 12) } if want := (editor.Command{Script: tt.script, Window: tt.window}); got != want { t.Errorf("editor %q%q = %+v, want %+v", tt.template, tt.editor, got, want) diff --git a/tui/editor.go b/tui/editor.go index f305488..8da7f4f 100644 --- a/tui/editor.go +++ b/tui/editor.go @@ -25,7 +25,7 @@ func (m *model) openEditor(path string, line int) tea.Cmd { abs := filepath.Join(m.repo.Root(), path) var ed editor.Command if m.cfg.Editor != "" { - ed = editor.FromTemplate(m.cfg.Editor, abs, line) + ed = editor.Template(m.cfg.Editor, abs, line) } else { e := cmp.Or( strings.TrimSpace(os.Getenv("VISUAL")), @@ -33,7 +33,7 @@ func (m *model) openEditor(path string, line int) tea.Cmd { m.repo.Editor(m.ctx), "vi", ) - ed = editor.FromEditor(e, abs, line) + ed = editor.Program(e, abs, line) } m.log.Debug("editor", "command", ed.Script, "window", ed.Window) if ed.Window { @@ -79,7 +79,7 @@ func runWindowEditor(script string) tea.Cmd { // the opener of the system. func openURL(cfg config, url string) error { if cfg.URLOpener != "" { - script := strings.ReplaceAll(cfg.URLOpener, "{url}", editor.Quote(url)) + script := strings.ReplaceAll(cfg.URLOpener, "{url}", editor.ShellQuote(url)) return start("sh", "-c", script) } opener := "xdg-open" diff --git a/update/update.go b/update/update.go index f1a8a88..baa9370 100644 --- a/update/update.go +++ b/update/update.go @@ -107,8 +107,8 @@ func Version(stamped string) string { // rexdiff. If bin links into a Caskroom, it uses Homebrew. If stamped is "" // and the version comes from go install, it uses go install. Else it // downloads the release. If REXDIFF_LATEST_URL is set, the Updater gets the -// latest release from that address. Use this variable to test a release -// before it is public. +// latest release from that address. Set it to test a release before it +// is public. func Detect(bin, stamped string) *Updater { return detect(bin, Version(stamped), stamped == "") }