From bc7d95b4a30d5f15c36d42a6119ed0bf182b9065 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Tue, 18 Aug 2026 17:03:39 +0200 Subject: [PATCH 01/12] Demonstrate that a wrapped selection is reported by segment The lines a selection covers are asked for by view line, which counts the segments a wrapping view breaks a line into, and then used to index the content, whose lines are unwrapped. The two agree only for a view whose content doesn't wrap. Co-authored-by: Claude Opus 5 (1M context) --- pkg/gocui/view_test.go | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/pkg/gocui/view_test.go b/pkg/gocui/view_test.go index 3ffd56e7c..adbfb952b 100644 --- a/pkg/gocui/view_test.go +++ b/pkg/gocui/view_test.go @@ -820,3 +820,32 @@ func TestApplySelTextColor(t *testing.T) { }) } } + +// A view that wraps draws one line of its content as several view lines, and the +// cursor and the range anchor count those. What is asked about a selection is +// which lines of the content it covers, so those are what it has to be reported +// in. +func TestSelectedLinesOfWrappedContent(t *testing.T) { + v := NewView("name", 0, 0, 11, 10, OutputNormal) // InnerWidth 10 + v.Wrap = true + v.Highlight = true + + // "a line that wraps" takes two view lines, so the four lines of content are + // drawn as five: "one", "two", "a line th", "at wraps", "four". + v.writeString("one\ntwo\na line that wraps\nfour\n") + assert.Equal(t, 5, v.ViewLinesHeight()) + + // The cursor on the wrapped line's second half is on that line. + v.FocusPoint(0, 3, false) + /* EXPECTED: + assert.Equal(t, "a line that wraps", v.SelectedLine()) + ACTUAL: */ + assert.Equal(t, "four", v.SelectedLine()) + + // A range over both halves of the wrapped line covers one line of content. + v.SetRangeSelectStart(2) + /* EXPECTED: + assert.Equal(t, []string{"a line that wraps"}, v.SelectedLines()) + ACTUAL: */ + assert.Equal(t, []string{"a line that wraps", "four"}, v.SelectedLines()) +} From 3b0cf1e1820a62bb17657cd35b40a450df8fe6cb Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Tue, 18 Aug 2026 17:05:49 +0200 Subject: [PATCH 02/12] Report a wrapped selection by the lines of content it covers A test asks which lines of a view are selected; a wrapping view's cursor and range anchor answer in view lines, which count the segments each line is drawn as. Going through the segment-to-line mapping keeps the answer in the terms the question was asked in, and a line the selection covers several segments of is reported once. Co-authored-by: Claude Opus 5 (1M context) --- pkg/gocui/view.go | 29 ++++++++++++++++++++++++++--- pkg/gocui/view_test.go | 6 ------ 2 files changed, 26 insertions(+), 9 deletions(-) diff --git a/pkg/gocui/view.go b/pkg/gocui/view.go index 6fbbecdb6..7bb0b7ca5 100644 --- a/pkg/gocui/view.go +++ b/pkg/gocui/view.go @@ -1985,11 +1985,12 @@ func (v *View) SelectedLine() string { v.writeMutex.Lock() defer v.writeMutex.Unlock() - if len(v.buf.lines) == 0 { + idx, ok := v.bufferLineForViewLine(v.SelectedLineIdx()) + if !ok { return "" } - return v.lineContentAtIdx(v.SelectedLineIdx()) + return v.lineContentAtIdx(idx) } // expected to only be used in tests @@ -2004,8 +2005,17 @@ func (v *View) SelectedLines() []string { startIdx, endIdx := v.SelectedLineRange() lines := make([]string, 0, endIdx-startIdx+1) + previous := -1 for i := startIdx; i <= endIdx; i++ { - lines = append(lines, v.lineContentAtIdx(i)) + // The selection is in view lines, which count the segments a wrapped line + // is drawn as; a line the selection covers several segments of is still + // the one line it is. + idx, ok := v.bufferLineForViewLine(i) + if !ok || idx == previous { + continue + } + previous = idx + lines = append(lines, v.lineContentAtIdx(idx)) } return lines @@ -2015,6 +2025,19 @@ func (v *View) lineContentAtIdx(idx int) string { return v.buf.lines[idx].cells.String() } +// bufferLineForViewLine maps a view line index, which counts the wrapped +// segments of the lines it draws, to the index of the line of content it is a +// segment of. Only call this with a lock on writeMutex. +func (v *View) bufferLineForViewLine(y int) (int, bool) { + v.refreshViewLinesIfNeeded() + + if y < 0 || y >= len(v.viewLines) { + return 0, false + } + + return v.viewLines[y].linesY, true +} + func (v *View) SelectedPoint() (int, int) { cx, cy := v.Cursor() ox, oy := v.Origin() diff --git a/pkg/gocui/view_test.go b/pkg/gocui/view_test.go index adbfb952b..2b2d50fd2 100644 --- a/pkg/gocui/view_test.go +++ b/pkg/gocui/view_test.go @@ -837,15 +837,9 @@ func TestSelectedLinesOfWrappedContent(t *testing.T) { // The cursor on the wrapped line's second half is on that line. v.FocusPoint(0, 3, false) - /* EXPECTED: assert.Equal(t, "a line that wraps", v.SelectedLine()) - ACTUAL: */ - assert.Equal(t, "four", v.SelectedLine()) // A range over both halves of the wrapped line covers one line of content. v.SetRangeSelectStart(2) - /* EXPECTED: assert.Equal(t, []string{"a line that wraps"}, v.SelectedLines()) - ACTUAL: */ - assert.Equal(t, []string{"a line that wraps", "four"}, v.SelectedLines()) } From 9c8c1cb479c1b66d98f143f2e4a2840edbbc1fae Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Tue, 18 Aug 2026 17:22:54 +0200 Subject: [PATCH 03/12] Demonstrate that resizing a wrapping view moves its selection The scroll offset, the cursor and a range's anchor are all view lines, which count the segments each line of the content is wrapped into. A change of width wraps the content differently, so every one of them ends up on a different line than the one it was put on. Co-authored-by: Claude Opus 5 (1M context) --- pkg/gocui/view_test.go | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/pkg/gocui/view_test.go b/pkg/gocui/view_test.go index 2b2d50fd2..6fc33bbbd 100644 --- a/pkg/gocui/view_test.go +++ b/pkg/gocui/view_test.go @@ -843,3 +843,33 @@ func TestSelectedLinesOfWrappedContent(t *testing.T) { v.SetRangeSelectStart(2) assert.Equal(t, []string{"a line that wraps"}, v.SelectedLines()) } + +// Resizing a view throws away the wrapping of its content and wraps it again for +// the new width, which moves every line of it to a different view line. The +// positions into the view count view lines, so they all have to come along. +func TestResizingAWrappingViewKeepsItsPlaceInTheContent(t *testing.T) { + g := &Gui{} + v, _ := g.SetView("name", 0, 0, 11, 10, 0) // InnerWidth 10 + v.Wrap = true + v.Highlight = true + + // Two wrapping lines, with a single line between them: eight view lines for + // five lines of content. + v.writeString("one\na line that wraps\ntwo\nanother wrapping line\nthree\n") + assert.Equal(t, 8, v.ViewLinesHeight()) + + // A range over the whole of the second wrapping line, which is drawn as view + // lines 4 to 6. + v.SetRangeSelectStart(4) + v.FocusPoint(0, 6, false) + assert.Equal(t, []string{"another wrapping line"}, v.SelectedLines()) + + // Widen the view so that nothing wraps any more. + _, _ = g.SetView("name", 0, 0, 31, 10, 0) // InnerWidth 30 + assert.Equal(t, 5, v.ViewLinesHeight()) + + /* EXPECTED: + assert.Equal(t, []string{"another wrapping line"}, v.SelectedLines()) + ACTUAL: */ + assert.Equal(t, []string{"three"}, v.SelectedLines()) +} From f04c561b2e2d857dfade2461685c5faf726a770a Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Tue, 18 Aug 2026 17:49:22 +0200 Subject: [PATCH 04/12] Keep a resized view's place in the content it wraps Wrapping the content at another width moves every line of it to a different view line. The positions into the view are all view lines: the scroll offset, the cursor, a range's anchor. Each of them is then left pointing at a line it was never on. Committing the last of the staged changes widens the main view by half a screen, and that moves a selected hunk somewhere else entirely. Carry the positions through the lines of content they were on. A position always meant a line of content rather than a view line. The line the cursor is on keeps the row it was drawn on, so it stays in front of the user rather than the view scrolling under it; a view with no cursor on screen keeps its own place instead. A range's ends go on the outermost segments of their lines, since a range covers lines of content and not the segments those lines are drawn as. Co-authored-by: Claude Opus 5 (1M context) --- pkg/gocui/gui.go | 4 +- pkg/gocui/view.go | 122 ++++++++++++++++++++++++++++++++++++++--- pkg/gocui/view_test.go | 3 - 3 files changed, 116 insertions(+), 13 deletions(-) diff --git a/pkg/gocui/gui.go b/pkg/gocui/gui.go index 12968fa95..5af5a7cca 100644 --- a/pkg/gocui/gui.go +++ b/pkg/gocui/gui.go @@ -438,7 +438,7 @@ func (g *Gui) SetView(name string, x0, y0, x1, y1 int, overlaps byte) (*View, er v.y1 = y1 if sizeChanged { - v.ClearViewLines() + v.RewrapContent() if v.Editable { cursorX, cursorY := v.TextArea.GetCursorXY() @@ -1582,7 +1582,7 @@ func (g *Gui) flush() error { // if GUI's size has changed, we need to redraw all views if maxX != g.maxX || maxY != g.maxY { for _, v := range g.views { - v.ClearViewLines() + v.RewrapContent() } } g.maxX, g.maxY = maxX, maxY diff --git a/pkg/gocui/view.go b/pkg/gocui/view.go index 7bb0b7ca5..9be969991 100644 --- a/pkg/gocui/view.go +++ b/pkg/gocui/view.go @@ -251,23 +251,129 @@ type pos struct { x, y int } -// call this in the event of a view resize, or if you want to render new content -// without the chance of old content still appearing, or if you want to remove -// a line from the existing content +// call this if you want to render new content without the chance of old content +// still appearing, or if you want to remove a line from the existing content. For +// a view whose size has changed, whose content is the same but has to be wrapped +// afresh, call RewrapContent instead. func (v *View) clearViewLines() { v.tainted = true v.viewLines = nil v.clearHover() } -// ClearViewLines is clearViewLines guarded by writeMutex. It's for callers on -// the UI thread (the layout pass) that touch a view whose content a task -// goroutine may be writing concurrently: viewLines/tainted/hover are all -// buffer state that writeMutex protects. -func (v *View) ClearViewLines() { +// RewrapContent wraps the view's content for the size the view has now, and puts +// the positions into that content — the scroll offset, the cursor, a range's +// anchor — back on the lines they were on. They are all view lines, which count +// the segments each line is wrapped into, so wrapping the content at another +// width leaves every one of them pointing at a different line. +// +// Call it on the UI thread whenever the view's size changes; a task goroutine may +// be writing the content concurrently, and all of this is state writeMutex +// protects. +func (v *View) RewrapContent() { v.writeMutex.Lock() defer v.writeMutex.Unlock() + + v.refreshViewLinesIfNeeded() + origin := v.contentPosOf(v.oy) + cursor := v.contentPosOf(v.oy + v.cy) + anchor := v.contentPosOf(v.rangeSelectStartY) + cursorRow := v.cy + v.clearViewLines() + v.refreshViewLinesIfNeeded() + + if !origin.ok { + return + } + + cursorLine, cursorOk := v.viewLineOf(cursor) + if anchorLine, ok := v.viewLineOf(anchor); ok { + v.rangeSelectStartY = anchorLine + if cursorOk { + // A range covers lines of content, not the wrapped segments those + // lines are drawn as, so its ends go back on the outermost segments + // of their lines: a line that was covered whole stays covered whole. + cursorLine = v.viewLineOfRangeEnd(cursor, anchor) + v.rangeSelectStartY = v.viewLineOfRangeEnd(anchor, cursor) + } + } + + // The line the cursor is on keeps the row it was drawn on, so that it doesn't + // move under the user; with no cursor on screen the view keeps its own place + // in the content instead. + if v.Highlight && cursorOk && cursorRow >= 0 && cursorRow < v.InnerHeight() { + v.SetOriginY(cursorLine - cursorRow) + } else if originLine, ok := v.viewLineOf(origin); ok { + v.SetOriginY(originLine) + } + if cursorOk { + v.cy = cursorLine - v.oy + } +} + +// contentPos is a position in a view's content in terms that survive the content +// being wrapped again: which line of it, and which of that line's segments. +type contentPos struct { + line, segment int + ok bool +} + +// contentPosOf returns where the given view line sits in the content. Only call +// this with a lock on writeMutex, and with the view lines up to date. +func (v *View) contentPosOf(viewLine int) contentPos { + if viewLine < 0 || viewLine >= len(v.viewLines) { + return contentPos{} + } + return contentPos{ + line: v.viewLines[viewLine].linesY, + segment: v.viewLines[viewLine].linesX, + ok: true, + } +} + +// viewLineOf returns the view line drawing the given position in the content, +// on the nearest segment its line still has. Only call this with a lock on +// writeMutex, and with the view lines up to date. +func (v *View) viewLineOf(pos contentPos) (int, bool) { + first, last, ok := v.segmentSpanOf(pos) + if !ok { + return 0, false + } + return min(first+pos.segment, last), true +} + +// viewLineOfRangeEnd returns the view line for one end of a range selection: the +// outermost segment of its line, so that the range covers that line whole. other +// is the range's other end, which says which way is outward. Both ends have to be +// positions whose lines are drawn, which viewLineOf answers. +func (v *View) viewLineOfRangeEnd(pos contentPos, other contentPos) int { + first, last, _ := v.segmentSpanOf(pos) + if pos.line <= other.line { + return first + } + return last +} + +// segmentSpanOf returns the first and last view line drawing the given position's +// line of the content. ok is false when the position was never taken, or its line +// isn't drawn at all. +func (v *View) segmentSpanOf(pos contentPos) (int, int, bool) { + if !pos.ok { + return 0, 0, false + } + first, last := -1, -1 + for i, vline := range v.viewLines { + if vline.linesY == pos.line { + if first == -1 { + first = i + } + last = i + } else if first != -1 { + break + } + } + return first, last, first != -1 } type searcher struct { diff --git a/pkg/gocui/view_test.go b/pkg/gocui/view_test.go index 6fc33bbbd..930602657 100644 --- a/pkg/gocui/view_test.go +++ b/pkg/gocui/view_test.go @@ -868,8 +868,5 @@ func TestResizingAWrappingViewKeepsItsPlaceInTheContent(t *testing.T) { _, _ = g.SetView("name", 0, 0, 31, 10, 0) // InnerWidth 30 assert.Equal(t, 5, v.ViewLinesHeight()) - /* EXPECTED: assert.Equal(t, []string{"another wrapping line"}, v.SelectedLines()) - ACTUAL: */ - assert.Equal(t, []string{"three"}, v.SelectedLines()) } From 43a4ba366623ff69b2f7ab58628b61d68349f43f Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sun, 9 Aug 2026 12:57:50 +0200 Subject: [PATCH 05/12] Add an old-file counterpart to Patch.LineNumberOfLine Identifying a change line of a diff by its file line number needs both sides: two consecutive deletions sit at the same new-file position, so only their old-file line numbers tell them apart. LineNumberOfLine only answers for the new file, which leaves deletions ambiguous. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/commands/patch/patch.go | 31 ++++++++++++++++++ pkg/commands/patch/patch_test.go | 54 ++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+) diff --git a/pkg/commands/patch/patch.go b/pkg/commands/patch/patch.go index fbbf3c935..8cff4d1fa 100644 --- a/pkg/commands/patch/patch.go +++ b/pkg/commands/patch/patch.go @@ -114,6 +114,37 @@ func (self *Patch) LineNumberOfLine(idx int) int { return hunk.newStart + offset } +// Takes a line index in the patch and returns the line number in the old file. +// This is the old-file counterpart of LineNumberOfLine; for a deletion it gives +// the line's position in the old file (additions get the position they sit at). +// If the line is a header line, returns 1. +// If the line is a hunk header line, returns the first old-file line number in that hunk. +// If the line is out of range below, returns the last old-file line number in the last hunk. +func (self *Patch) OldLineNumberOfLine(idx int) int { + if idx < len(self.header) || len(self.hunks) == 0 { + return 1 + } + + hunkIdx := self.HunkContainingLine(idx) + // cursor out of range, return last file line number + if hunkIdx == -1 { + lastHunk := self.hunks[len(self.hunks)-1] + return lastHunk.oldStart + lastHunk.oldLength() - 1 + } + + hunk := self.hunks[hunkIdx] + hunkStartIdx := self.HunkStartIdx(hunkIdx) + idxInHunk := idx - hunkStartIdx + + if idxInHunk == 0 { + return hunk.oldStart + } + + lines := hunk.bodyLines[:idxInHunk-1] + offset := nLinesWithKind(lines, []PatchLineKind{DELETION, CONTEXT}) + return hunk.oldStart + offset +} + // Returns hunk index containing the line at the given patch line index func (self *Patch) HunkContainingLine(idx int) int { for hunkIdx, hunk := range self.hunks { diff --git a/pkg/commands/patch/patch_test.go b/pkg/commands/patch/patch_test.go index 4f84041d6..9b2474bd1 100644 --- a/pkg/commands/patch/patch_test.go +++ b/pkg/commands/patch/patch_test.go @@ -120,6 +120,20 @@ index 9320895..6d79956 100644 lemon ` +// Two deletions with no line between them: they share a new-file line number +// (both sit at the same new-file position), so only their old-file line numbers +// tell them apart. +const consecutiveDeletions = `diff --git a/filename b/filename +index 9320895..6d79956 100644 +--- a/filename ++++ b/filename +@@ -1,4 +1,2 @@ + apple +-grape +-pear + lemon +` + const newFile = `diff --git a/newfile b/newfile new file mode 100644 index 0000000..4e680cc @@ -682,6 +696,46 @@ func TestLineNumberOfLine(t *testing.T) { } } +func TestOldLineNumberOfLine(t *testing.T) { + type scenario struct { + testName string + patchStr string + indexes []int + expecteds []int + } + + scenarios := []scenario{ + { + testName: "twoChangesInOneHunk", + patchStr: twoChangesInOneHunk, + indexes: []int{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 1000}, + expecteds: []int{1, 1, 1, 1, 1, 1, 2, 3, 3, 4, 5, 5, 5}, + }, + { + testName: "consecutiveDeletions", + patchStr: consecutiveDeletions, + indexes: []int{0, 1, 2, 3, 4, 5, 6, 7, 8, 1000}, + expecteds: []int{1, 1, 1, 1, 1, 1, 2, 3, 4, 4}, + }, + { + testName: "renameWithModificationDiff", + patchStr: renameWithModificationDiff, + indexes: []int{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 1000}, + expecteds: []int{1, 1, 1, 1, 1, 1, 1, 1, 1, 2, 3, 3, 4, 5, 5}, + }, + } + + for _, s := range scenarios { + t.Run(s.testName, func(t *testing.T) { + for i, idx := range s.indexes { + patch := Parse(s.patchStr) + result := patch.OldLineNumberOfLine(idx) + assert.Equal(t, s.expecteds[i], result) + } + }) + } +} + func TestGetNextStageableLineIndex(t *testing.T) { type scenario struct { testName string From db6a04b8a50b4fdd5aebd11e985e2af128955c0b Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sun, 9 Aug 2026 13:23:26 +0200 Subject: [PATCH 06/12] Add a well-formedness check for a parsed patch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Parse is lenient: it takes any text and reads a diff out of it, which is what we want when we hand it a diff, but it has no way to say "this isn't one". We're about to parse the *rendered* contents of a diff view, which a diff renderer is free to restructure — putting the line numbers in a gutter, say, shifts the +/- marker off the start of each body line, so every line reads as context and the parse silently lies about which lines are changes. Comparing each hunk's body against the lengths its header declares catches exactly that, without teaching us anything about any particular renderer's layout: a faithful unified diff agrees with its headers, a restructured one doesn't. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/commands/patch/hunk.go | 5 ++ pkg/commands/patch/parse.go | 37 +++++++---- pkg/commands/patch/patch.go | 38 ++++++++++++ pkg/commands/patch/patch_test.go | 102 +++++++++++++++++++++++++++++++ 4 files changed, 171 insertions(+), 11 deletions(-) diff --git a/pkg/commands/patch/hunk.go b/pkg/commands/patch/hunk.go index 568b312a7..e539f3a47 100644 --- a/pkg/commands/patch/hunk.go +++ b/pkg/commands/patch/hunk.go @@ -16,6 +16,11 @@ type Hunk struct { newStart int // the context at the end of the header line (' func (f *CommitFile) Description() string {' in the above example) headerContext string + // the lengths declared in the header line ('2' and '3' in the above example), + // kept so that we can check the parsed body against them (see + // Patch.IsWellFormed). Only set by Parse. + declaredOldLength int + declaredNewLength int // the body of the hunk, excluding the header line bodyLines []*PatchLine } diff --git a/pkg/commands/patch/parse.go b/pkg/commands/patch/parse.go index fee7d2918..a26332869 100644 --- a/pkg/commands/patch/parse.go +++ b/pkg/commands/patch/parse.go @@ -7,7 +7,9 @@ import ( "github.com/jesseduffield/lazygit/pkg/utils" ) -var hunkHeaderRegexp = regexp.MustCompile(`(?m)^@@ -(\d+)[^\+]+\+(\d+)[^@]+@@(.*)$`) +// Captures, in order: the old start, the old length (omitted by git when it is +// 1), the new start, the new length (likewise), and the trailing context. +var hunkHeaderRegexp = regexp.MustCompile(`(?m)^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@(.*)$`) func Parse(patchStr string) *Patch { // ignore trailing newline. @@ -19,13 +21,15 @@ func Parse(patchStr string) *Patch { var currentHunk *Hunk for _, line := range lines { if strings.HasPrefix(line, "@@") { - oldStart, newStart, headerContext := headerInfo(line) + oldStart, oldLength, newStart, newLength, headerContext := headerInfo(line) currentHunk = &Hunk{ - oldStart: oldStart, - newStart: newStart, - headerContext: headerContext, - bodyLines: []*PatchLine{}, + oldStart: oldStart, + newStart: newStart, + declaredOldLength: oldLength, + declaredNewLength: newLength, + headerContext: headerContext, + bodyLines: []*PatchLine{}, } hunks = append(hunks, currentHunk) } else if currentHunk != nil { @@ -41,14 +45,25 @@ func Parse(patchStr string) *Patch { } } -func headerInfo(header string) (int, int, string) { +func headerInfo(header string) (oldStart int, oldLength int, newStart int, newLength int, headerContext string) { match := hunkHeaderRegexp.FindStringSubmatch(header) - oldStart := utils.MustConvertToInt(match[1]) - newStart := utils.MustConvertToInt(match[2]) - headerContext := match[3] + oldStart = utils.MustConvertToInt(match[1]) + oldLength = declaredLength(match[2]) + newStart = utils.MustConvertToInt(match[3]) + newLength = declaredLength(match[4]) + headerContext = match[5] - return oldStart, newStart, headerContext + return oldStart, oldLength, newStart, newLength, headerContext +} + +// declaredLength parses a length capture of a hunk header, which git omits when +// it is 1 (e.g. "@@ -0,0 +1 @@"). +func declaredLength(match string) int { + if match == "" { + return 1 + } + return utils.MustConvertToInt(match) } func newHunkLine(line string) *PatchLine { diff --git a/pkg/commands/patch/patch.go b/pkg/commands/patch/patch.go index 8cff4d1fa..32c4788fb 100644 --- a/pkg/commands/patch/patch.go +++ b/pkg/commands/patch/patch.go @@ -79,6 +79,44 @@ func (self *Patch) HunkEndIdx(hunkIndex int) int { return self.HunkStartIdx(hunkIndex) + self.hunks[hunkIndex].lineCount() - 1 } +// IsWellFormed reports whether every hunk's body matches the lengths declared in +// its header. A faithful unified diff always satisfies this; a rendering that +// restructured the diff body does not — a diff renderer that puts line numbers in +// a gutter, say, shifts the +/- marker off the start of each line, so every body +// line reads as context and the computed lengths no longer match the header. That +// makes this the test for whether a rendered diff can be parsed as a unified diff +// at all, rather than trusting a mis-parse. Only meaningful for patches produced +// by Parse, which is where the declared lengths come from. +func (self *Patch) IsWellFormed() bool { + return self.isWellFormed(false) +} + +// IsWellFormedSoFar is IsWellFormed for a patch parsed from a diff we have only the +// beginning of. Its last hunk holds the first lines of a body that hasn't all arrived, +// so every hunk but the last has to match its header exactly, as before, while the last +// one only has to fit within what its header declares. +// +// The check exists to tell a faithful rendering from a restructured one, and it still +// does that. A rendering that moves the +/- marker off the start of the line makes us +// read a change as context, and a context line counts towards both lengths, so such a +// hunk comes out longer than its header declares rather than shorter. +func (self *Patch) IsWellFormedSoFar() bool { + return self.isWellFormed(true) +} + +func (self *Patch) isWellFormed(lastHunkMayBeIncomplete bool) bool { + for i, hunk := range self.hunks { + if lastHunkMayBeIncomplete && i == len(self.hunks)-1 { + return hunk.oldLength() <= hunk.declaredOldLength && + hunk.newLength() <= hunk.declaredNewLength + } + if hunk.oldLength() != hunk.declaredOldLength || hunk.newLength() != hunk.declaredNewLength { + return false + } + } + return true +} + func (self *Patch) ContainsChanges() bool { return lo.SomeBy(self.hunks, func(hunk *Hunk) bool { return hunk.containsChanges() diff --git a/pkg/commands/patch/patch_test.go b/pkg/commands/patch/patch_test.go index 9b2474bd1..1d925d399 100644 --- a/pkg/commands/patch/patch_test.go +++ b/pkg/commands/patch/patch_test.go @@ -696,6 +696,108 @@ func TestLineNumberOfLine(t *testing.T) { } } +func TestIsWellFormed(t *testing.T) { + // The body of a diff as rendered with the +/- markers moved out of the text + // and into a gutter: every body line now reads as context, so the lengths no + // longer match the header. + const gutterMangled = `diff --git a/filename b/filename +index 9320895..6d79956 100644 +--- a/filename ++++ b/filename +@@ -1,4 +1,2 @@ + apple + grape + pear + lemon +` + + scenarios := []struct { + testName string + patchStr string + expected bool + }{ + {"simpleDiff", simpleDiff, true}, + {"renameWithModificationDiff", renameWithModificationDiff, true}, + {"addNewlineToEndOfFile", addNewlineToEndOfFile, true}, + {"twoHunks", twoHunks, true}, + {"consecutiveDeletions", consecutiveDeletions, true}, + {"newFile", newFile, true}, + {"deletedFile", deletedFile, true}, + {"addNewlineToPreviouslyEmptyFile", addNewlineToPreviouslyEmptyFile, true}, + {"exampleHunk", exampleHunk, true}, + {"gutterMangled", gutterMangled, false}, + } + + for _, s := range scenarios { + t.Run(s.testName, func(t *testing.T) { + assert.Equal(t, s.expected, Parse(s.patchStr).IsWellFormed()) + }) + } +} + +func TestIsWellFormedSoFar(t *testing.T) { + // A diff read only as far as the middle of its second hunk. + const cutShort = `diff --git a/filename b/filename +index e48a11c..b2ab81b 100644 +--- a/filename ++++ b/filename +@@ -1,5 +1,5 @@ + apple +-grape ++orange + ... + ... + ... +@@ -8,6 +8,8 @@ grape + ... + ... +` + + // The same diff cut short in its first hunk, so that the second is missing + // entirely rather than short. + const cutShortInTheFirstHunk = `diff --git a/filename b/filename +index e48a11c..b2ab81b 100644 +--- a/filename ++++ b/filename +@@ -1,5 +1,5 @@ + apple +-grape +` + + // A rendering with the +/- markers moved into a gutter, cut short: reading the + // changes as context makes the hunk longer than its header declares, not shorter, + // so it doesn't pass for a diff we only have the beginning of. + const gutterMangledAndCutShort = `diff --git a/filename b/filename +index 9320895..6d79956 100644 +--- a/filename ++++ b/filename +@@ -1,4 +1,2 @@ + apple + grape + pear + lemon + melon +` + + scenarios := []struct { + testName string + patchStr string + expected bool + }{ + {"simpleDiff", simpleDiff, true}, + {"twoHunks", twoHunks, true}, + {"cutShort", cutShort, true}, + {"cutShortInTheFirstHunk", cutShortInTheFirstHunk, true}, + {"gutterMangledAndCutShort", gutterMangledAndCutShort, false}, + } + + for _, s := range scenarios { + t.Run(s.testName, func(t *testing.T) { + assert.Equal(t, s.expected, Parse(s.patchStr).IsWellFormedSoFar()) + }) + } +} + func TestOldLineNumberOfLine(t *testing.T) { type scenario struct { testName string From 26e5f773cb100d0d940ce6f4f749dcb02123bb49 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sun, 9 Aug 2026 13:38:30 +0200 Subject: [PATCH 07/12] Let callers map between view lines and buffer lines MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Everything about a diff's content — which file and line a row belongs to, whether it's a change — is a property of the unwrapped buffer line, while the cursor, clicks and the range selection all speak in view lines, which count wrapped segments. Reading the content under the cursor therefore needs the mapping in both directions, and doing it outside gocui isn't possible: the wrapping is internal, and the caller couldn't take the view's lock across the lookup and the read. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/gocui/view.go | 51 ++++++++++++++++++++++++++++++++++++++- pkg/gocui/view_test.go | 55 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 105 insertions(+), 1 deletion(-) diff --git a/pkg/gocui/view.go b/pkg/gocui/view.go index 9be969991..0485f2e48 100644 --- a/pkg/gocui/view.go +++ b/pkg/gocui/view.go @@ -362,9 +362,18 @@ func (v *View) segmentSpanOf(pos contentPos) (int, int, bool) { if !pos.ok { return 0, 0, false } + return v.viewLineSpanOfBufferLine(pos.line) +} + +// viewLineSpanOfBufferLine returns the first and last view line drawing the given +// buffer line, i.e. the first and last segment it is wrapped into. Both are the +// same view line when the line doesn't wrap. ok is false when the line isn't drawn +// at all. Only call this with a lock on writeMutex, and with the view lines up to +// date. +func (v *View) viewLineSpanOfBufferLine(bufferLine int) (int, int, bool) { first, last := -1, -1 for i, vline := range v.viewLines { - if vline.linesY == pos.line { + if vline.linesY == bufferLine { if first == -1 { first = i } @@ -1851,6 +1860,46 @@ func (v *View) BufferLines() []string { return lines } +// BufferLineForViewLine maps a view line index (which counts wrapped lines) to +// the index of the corresponding line in the unwrapped internal buffer (as +// returned by BufferLines). Several view lines map to the same buffer line when +// that line wraps. Returns false if the view line is out of range. +func (v *View) BufferLineForViewLine(y int) (int, bool) { + v.writeMutex.Lock() + defer v.writeMutex.Unlock() + + return v.bufferLineForViewLine(y) +} + +// ViewLineForBufferLine maps an unwrapped buffer line index to the index of the +// first view line that renders it — the inverse of BufferLineForViewLine, for +// turning a line found by examining the buffer into a line to scroll to or +// select. Returns false if the buffer line isn't rendered into any view line. +func (v *View) ViewLineForBufferLine(bufferLineIdx int) (int, bool) { + v.writeMutex.Lock() + defer v.writeMutex.Unlock() + + v.refreshViewLinesIfNeeded() + + first, _, ok := v.viewLineSpanOfBufferLine(bufferLineIdx) + return first, ok +} + +// LastViewLineForBufferLine maps an unwrapped buffer line index to the index of +// the last view line that renders it, which for a line that doesn't wrap is the +// same as the first. It is where the far end of a range goes: a range is over +// buffer lines, so it has to cover the last one of them to its final segment +// rather than stopping where that line begins. +func (v *View) LastViewLineForBufferLine(bufferLineIdx int) (int, bool) { + v.writeMutex.Lock() + defer v.writeMutex.Unlock() + + v.refreshViewLinesIfNeeded() + + _, last, ok := v.viewLineSpanOfBufferLine(bufferLineIdx) + return last, ok +} + // Buffer returns a string with the contents of the view's internal // buffer. func (v *View) Buffer() string { diff --git a/pkg/gocui/view_test.go b/pkg/gocui/view_test.go index 930602657..493e83f30 100644 --- a/pkg/gocui/view_test.go +++ b/pkg/gocui/view_test.go @@ -204,6 +204,61 @@ func TestViewLinesTruncatedByShorterRender(t *testing.T) { assert.Equal(t, []string{"aaa", "bbb", "ccc"}, v.ViewBufferLines()) } +func TestBufferLineForViewLine(t *testing.T) { + v := NewView("name", 0, 0, 10, 10, OutputNormal) // InnerWidth is 9 + v.Wrap = true + + // Buffer line 0 is short (view line 0); buffer line 1 wraps into three view + // lines (1, 2, 3); buffer line 2 is short again (view line 4). + v.writeString("short\n" + strings.Repeat("b", 27) + "\nlast") + + for viewLine, wantBufferLine := range []int{0, 1, 1, 1, 2} { + bufferLine, ok := v.BufferLineForViewLine(viewLine) + assert.True(t, ok) + assert.Equal(t, wantBufferLine, bufferLine) + } + + _, ok := v.BufferLineForViewLine(5) + assert.False(t, ok) + + _, ok = v.BufferLineForViewLine(-1) + assert.False(t, ok) +} + +func TestViewLineForBufferLine(t *testing.T) { + v := NewView("name", 0, 0, 10, 10, OutputNormal) // InnerWidth is 9 + v.Wrap = true + + // A wrapped buffer line maps to the first of the view lines it spans. + v.writeString("short\n" + strings.Repeat("b", 27) + "\nlast") + + for bufferLine, wantViewLine := range []int{0, 1, 4} { + viewLine, ok := v.ViewLineForBufferLine(bufferLine) + assert.True(t, ok) + assert.Equal(t, wantViewLine, viewLine) + } + + _, ok := v.ViewLineForBufferLine(3) + assert.False(t, ok) +} + +func TestLastViewLineForBufferLine(t *testing.T) { + v := NewView("name", 0, 0, 10, 10, OutputNormal) // InnerWidth is 9 + v.Wrap = true + + // A wrapped buffer line maps to the last of the view lines it spans. + v.writeString("short\n" + strings.Repeat("b", 27) + "\nlast") + + for bufferLine, wantViewLine := range []int{0, 3, 4} { + viewLine, ok := v.LastViewLineForBufferLine(bufferLine) + assert.True(t, ok) + assert.Equal(t, wantViewLine, viewLine) + } + + _, ok := v.LastViewLineForBufferLine(3) + assert.False(t, ok) +} + // While an async re-render loads, it swaps in only a partially-filled buffer at // its first paint and keeps appending lines afterwards. The scrollbar must keep // using the pre-load height until the load ends, so the thumb doesn't shrink and From d317ff1824db739bdc32f89796b2cd0f072ce9cd Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Mon, 21 Sep 2026 09:39:49 +0200 Subject: [PATCH 08/12] Demonstrate that overwriting lines after a pending newline lands a line low A view holds back the newline that ends a write until more content arrives, so that it doesn't end in an empty line. OverwriteLines moves the write cursor to the line it is given without letting go of that pending newline, so the write that follows advances first and lands on the line below. Nothing in lazygit overwrites lines right after such a write today. Co-Authored-By: Claude Fable 5.1 --- pkg/gocui/view_test.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/pkg/gocui/view_test.go b/pkg/gocui/view_test.go index 493e83f30..58b98e01d 100644 --- a/pkg/gocui/view_test.go +++ b/pkg/gocui/view_test.go @@ -113,6 +113,20 @@ func TestWriteString(t *testing.T) { } } +func TestOverwriteLinesAfterContentEndingInANewline(t *testing.T) { + v := NewView("name", 0, 0, 20, 10, OutputNormal) + // The trailing newline is held back until more content arrives, so that the + // view doesn't end in an empty line. + v.writeString("a\nb\n") + + v.OverwriteLines(0, "x") + + /* EXPECTED: + assert.Equal(t, []string{"x", "b"}, v.BufferLines()) + ACTUAL: */ + assert.Equal(t, []string{"a", "x"}, v.BufferLines()) +} + func TestUpdatedCursorAndOrigin(t *testing.T) { tests := []struct { prevOrigin int From caa20f20a04ab2c16b5de936663e49c26031d873 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Mon, 21 Sep 2026 09:41:31 +0200 Subject: [PATCH 09/12] Clear a pending newline when overwriting lines in place OverwriteLines is asked for a line and writes the one below it when the write before it ended in a newline. The view holds such a newline back until more content arrives, so that it doesn't end in an empty line, and OverwriteLines moved the write cursor without letting go of it, so the write that followed advanced to the next line first. Move the cursor through SetWritePos, which drops the pending newline along with it. Co-Authored-By: Claude Fable 5.1 --- pkg/gocui/view.go | 3 +-- pkg/gocui/view_test.go | 3 --- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/pkg/gocui/view.go b/pkg/gocui/view.go index 0485f2e48..10e620cd4 100644 --- a/pkg/gocui/view.go +++ b/pkg/gocui/view.go @@ -2263,8 +2263,7 @@ func (v *View) ClearTextArea() { func (v *View) overwriteLines(y int, content string) { // break by newline, then for each line, write it, then add that erase command - v.buf.wx = 0 - v.buf.wy = y + v.SetWritePos(0, y) v.clearViewLines() lines := strings.ReplaceAll(content, "\n", "\x1b[K\n") diff --git a/pkg/gocui/view_test.go b/pkg/gocui/view_test.go index 58b98e01d..f559e562b 100644 --- a/pkg/gocui/view_test.go +++ b/pkg/gocui/view_test.go @@ -121,10 +121,7 @@ func TestOverwriteLinesAfterContentEndingInANewline(t *testing.T) { v.OverwriteLines(0, "x") - /* EXPECTED: assert.Equal(t, []string{"x", "b"}, v.BufferLines()) - ACTUAL: */ - assert.Equal(t, []string{"a", "x"}, v.BufferLines()) } func TestUpdatedCursorAndOrigin(t *testing.T) { From d5c19a83f30e9e1aa00e60df0e7fcac93713ac86 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Mon, 21 Sep 2026 08:46:49 +0200 Subject: [PATCH 10/12] Keep a view's lines as they were written, beside their cells A reader that parses a view's content rather than showing it wants the text the writer wrote, and the cells don't always spell it. A tab is expanded into the spaces it fills, so a line read back from the cells ends in one to four spaces where the writer put a tab. A carriage return moves the write cursor back to the start of the line, so the text written after it overwrites what came before. The parser of the main view's diff, which the next commit adds, meets the first case in every header of a file whose path contains a space. git terminates the path field of a "---" or "+++" line with a tab then, and a parser reading the cells takes the spaces the tab became for part of the path. That path names a file that doesn't exist, so the file's lines can't be acted on. Keep the text as written per line, from the first character on that the cells spell differently, so that a line without a tab or a carriage return costs nothing. LinesAsWritten hands it out the way BufferLines hands out the cells' text. Co-Authored-By: Claude Fable 5.1 --- pkg/gocui/view.go | 86 +++++++++++++++++++++++++++++++++++++++--- pkg/gocui/view_test.go | 77 +++++++++++++++++++++++++++++++++++++ 2 files changed, 158 insertions(+), 5 deletions(-) diff --git a/pkg/gocui/view.go b/pkg/gocui/view.go index 10e620cd4..2961447f6 100644 --- a/pkg/gocui/view.go +++ b/pkg/gocui/view.go @@ -5,6 +5,7 @@ package gocui import ( + "bytes" "fmt" "io" "slices" @@ -693,6 +694,23 @@ type lineType struct { // matches the current width; nil means nothing is cached yet. wrappedCells [][]cell wrappedColumns int + + // asWritten is the text of the line as its writer wrote it, escape sequences + // left out, kept from the first character on that the cells spell differently: + // a tab, which the cells hold as the spaces it fills, or a carriage return, + // which they hold as the overwrite it caused. nil while the cells spell the + // line as it was written, as they do for most lines. + asWritten []byte +} + +// textAsWritten returns the line's text as its writer wrote it, escape sequences +// left out. A reader that parses a view's content rather than showing it wants +// this form; the cells' text is for showing. +func (l *lineType) textAsWritten() string { + if l.asWritten != nil { + return string(l.asWritten) + } + return l.cells.String() } // trailingFillAttributes describes the fg/bg colors that draw() should @@ -950,8 +968,7 @@ func (v *View) SetWritePos(x, y int) { y = 0 } - v.buf.wx = x - v.buf.wy = y + v.buf.seekWrite(x, y) // Changing the write position makes a pending newline obsolete v.buf.pendingNewline = false @@ -1035,6 +1052,35 @@ func (b *viewBuffer) writeCells(cells []cell) { b.wx += len(cells) } +// seekWrite moves the write cursor to (x, y). Writing there starts the line over, +// so whatever it kept of its text as written is dropped; the text a line keeps is +// the text written to it from its start. A carriage return continues a line +// instead, and moves the cursor without this (see write). +func (b *viewBuffer) seekWrite(x, y int) { + b.wx = x + b.wy = y + if y < len(b.lines) { + b.lines[y].asWritten = nil + } +} + +// startAsWritten begins keeping the current line's text as written (see +// lineType.asWritten), at the first character the cells won't spell the same way. +// Up to here they spell it exactly, so their text is what was written so far. +func (b *viewBuffer) startAsWritten() { + if line := &b.lines[b.wy]; line.asWritten == nil { + line.asWritten = append([]byte{}, line.cells.String()...) + } +} + +// noteAsWritten records text the writer wrote to the current line, once the line +// keeps its text as written at all. +func (b *viewBuffer) noteAsWritten(text []byte) { + if line := &b.lines[b.wy]; line.asWritten != nil { + line.asWritten = append(line.asWritten, text...) + } +} + // Write appends a byte slice into the view's internal buffer. Because // View implements the io.Writer interface, it can be passed as parameter // of functions like fmt.Fprintf, fmt.Fprintln, io.Copy, etc. Clear must @@ -1081,8 +1127,7 @@ func (b *viewBuffer) write(v *View, p []byte) { } advanceToNextLine := func() { - b.wx = 0 - b.wy++ + b.seekWrite(0, b.wy+1) if b.wy >= len(b.lines) { b.lines = append(b.lines, lineType{}) } @@ -1115,6 +1160,10 @@ func (b *viewBuffer) write(v *View, p []byte) { b.ei.notifyRowAdvance() case characterEquals(chr, '\r'): finishLine() + // The cells will hold what follows as an overwrite of what came + // before; the text as written keeps the return itself. + b.startAsWritten() + b.noteAsWritten(chr) b.wx = 0 b.ei.notifyColumnReset() default: @@ -1231,7 +1280,8 @@ func (b *viewBuffer) parseInput(v *View, ch []byte, width int, x int, _ int) (bo isEscape, err := b.ei.parseOne(ch) if err != nil { - for _, chr := range b.ei.characters() { + characters := b.ei.characters() + for _, chr := range characters { c := cell{ fgColor: v.FgColor, bgColor: v.BgColor, @@ -1240,6 +1290,7 @@ func (b *viewBuffer) parseInput(v *View, ch []byte, width int, x int, _ int) (bo } cells = append(cells, c) } + b.noteAsWritten([]byte(strings.Join(characters, ""))) b.ei.reset() } else { repeatCount := 1 @@ -1265,10 +1316,15 @@ func (b *viewBuffer) parseInput(v *View, ch []byte, width int, x int, _ int) (bo repeatCount = cf.n ch = []byte{' '} width = 1 + b.noteAsWritten(bytes.Repeat(ch, repeatCount)) } else if isEscape { // do not output anything return truncateLine, nil } else if characterEquals(ch, '\t') { + // The cells hold a tab as the spaces it fills; the text as written + // keeps the tab itself. + b.startAsWritten() + b.noteAsWritten(ch) // fill tab-sized space tabWidth := v.TabWidth if tabWidth < 1 { @@ -1277,6 +1333,8 @@ func (b *viewBuffer) parseInput(v *View, ch []byte, width int, x int, _ int) (bo ch = []byte{' '} width = 1 repeatCount = tabWidth - (x % tabWidth) + } else { + b.noteAsWritten(ch) } c := cell{ fgColor: b.ei.curFgColor, @@ -1860,6 +1918,24 @@ func (v *View) BufferLines() []string { return lines } +// LinesAsWritten returns the lines of the view's internal buffer as their writer +// wrote them, escape sequences left out, where BufferLines returns them as the +// cells spell them. The two differ where the cells can't spell what was written: +// a tab, which they hold as the spaces it fills, and a carriage return, which +// they hold as the overwrite it caused. A reader that parses the content rather +// than showing it wants this form. git, for one, terminates a path containing a +// space with a tab in a diff header, and a parser of the diff has to see the tab. +func (v *View) LinesAsWritten() []string { + v.writeMutex.Lock() + defer v.writeMutex.Unlock() + + lines := make([]string, len(v.buf.lines)) + for i := range v.buf.lines { + lines[i] = v.buf.lines[i].textAsWritten() + } + return lines +} + // BufferLineForViewLine maps a view line index (which counts wrapped lines) to // the index of the corresponding line in the unwrapped internal buffer (as // returned by BufferLines). Several view lines map to the same buffer line when diff --git a/pkg/gocui/view_test.go b/pkg/gocui/view_test.go index f559e562b..57d18abde 100644 --- a/pkg/gocui/view_test.go +++ b/pkg/gocui/view_test.go @@ -124,6 +124,83 @@ func TestOverwriteLinesAfterContentEndingInANewline(t *testing.T) { assert.Equal(t, []string{"x", "b"}, v.BufferLines()) } +func TestLinesAsWritten(t *testing.T) { + tests := []struct { + name string + stringsToWrite []string + expectedShown []string + expectedAsWritten []string + }{ + { + name: "a line the cells spell as written", + stringsToWrite: []string{"abc\n"}, + expectedShown: []string{"abc"}, + expectedAsWritten: []string{"abc"}, + }, + { + name: "a tab is kept rather than the spaces it fills", + stringsToWrite: []string{"a\tb\n"}, + expectedShown: []string{"a b"}, + expectedAsWritten: []string{"a\tb"}, + }, + { + name: "a carriage return is kept rather than the overwrite it causes", + stringsToWrite: []string{"abc\rde\n"}, + expectedShown: []string{"dec"}, + expectedAsWritten: []string{"abc\rde"}, + }, + { + // git writes a CRLF file's lines as "+foo\r", the color reset, "\n". + name: "escape sequences are left out", + stringsToWrite: []string{"\x1b[32m+foo\r\x1b[m\n"}, + expectedShown: []string{"+foo"}, + expectedAsWritten: []string{"+foo\r"}, + }, + { + // ConPTY writes a run of spaces as a cursor-forward escape. + name: "a cursor-forward escape stands for the spaces it skips", + stringsToWrite: []string{"\ta\x1b[2Cb\n"}, + expectedShown: []string{" a b"}, + expectedAsWritten: []string{"\ta b"}, + }, + { + name: "a line written in two parts", + stringsToWrite: []string{"a\t", "b\n"}, + expectedShown: []string{"a b"}, + expectedAsWritten: []string{"a\tb"}, + }, + { + name: "only the lines with a tab or a return are kept separately", + stringsToWrite: []string{"x\n", "y\tz\n", "w\n"}, + expectedShown: []string{"x", "y z", "w"}, + expectedAsWritten: []string{"x", "y\tz", "w"}, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + v := NewView("name", 0, 0, 20, 10, OutputNormal) + for _, s := range test.stringsToWrite { + v.writeString(s) + } + assert.Equal(t, test.expectedShown, v.BufferLines()) + assert.Equal(t, test.expectedAsWritten, v.LinesAsWritten()) + }) + } +} + +func TestLinesAsWrittenOfAnOverwrittenLine(t *testing.T) { + v := NewView("name", 0, 0, 20, 10, OutputNormal) + v.writeString("a\tb\nc\td") + + // Overwriting a line starts it over: what it kept of its earlier text goes, + // and the line below is left alone. + v.OverwriteLines(0, "xy") + + assert.Equal(t, []string{"xy", "c d"}, v.BufferLines()) + assert.Equal(t, []string{"xy", "c\td"}, v.LinesAsWritten()) +} + func TestUpdatedCursorAndOrigin(t *testing.T) { tests := []struct { prevOrigin int From 2a190fd1e4a5e59558d9fd30c544eb17c58496c3 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sun, 9 Aug 2026 13:44:07 +0200 Subject: [PATCH 11/12] Recover the identity of a diff line from the rendered diff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit To act on the line the user is pointing at in a diff view — to stage it, to open it in an editor, to keep the cursor on it while the diff is regenerated — we need to know which file it belongs to and where it sits in that file. Only the diff the view was rendered from knows that, and by the time it's on screen all we have is text. So parse it back: the view's contents are (usually) a unified diff, and running them through the patch parser recovers each row's file, kind and line numbers. "Usually" is why this goes behind a seam, and why it answers "I don't know" rather than guessing: a diff renderer is free to restructure what it prints, and a wrong answer here means acting on the wrong line of the wrong file. Parsing a whole buffer is a separate entry point from parsing a single line, because a caller resolving every row of a large diff must not re-parse a file's section once per line of it. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/gui/controllers.go | 2 + .../controllers/helpers/diff_line_helper.go | 55 ++++ .../controllers/helpers/diff_line_parser.go | 286 ++++++++++++++++++ .../helpers/diff_line_parser_test.go | 266 ++++++++++++++++ pkg/gui/controllers/helpers/helpers.go | 2 + pkg/gui/types/diff_line_info.go | 42 +++ 6 files changed, 653 insertions(+) create mode 100644 pkg/gui/controllers/helpers/diff_line_helper.go create mode 100644 pkg/gui/controllers/helpers/diff_line_parser.go create mode 100644 pkg/gui/controllers/helpers/diff_line_parser_test.go create mode 100644 pkg/gui/types/diff_line_info.go diff --git a/pkg/gui/controllers.go b/pkg/gui/controllers.go index f21fb607f..dc5a77834 100644 --- a/pkg/gui/controllers.go +++ b/pkg/gui/controllers.go @@ -68,6 +68,7 @@ func (gui *Gui) resetHelpersAndControllers() { searchHelper, ) diffHelper := helpers.NewDiffHelper(helperCommon) + diffLineHelper := helpers.NewDiffLineHelper(helperCommon) cherryPickHelper := helpers.NewCherryPickHelper( helperCommon, rebaseHelper, @@ -110,6 +111,7 @@ func (gui *Gui) resetHelpersAndControllers() { SuspendResume: helpers.NewSuspendResumeHelper(helperCommon), Snake: helpers.NewSnakeHelper(helperCommon), Diff: diffHelper, + DiffLine: diffLineHelper, Repos: reposHelper, RecordDirectory: recordDirectoryHelper, Update: helpers.NewUpdateHelper(helperCommon, gui.Updater), diff --git a/pkg/gui/controllers/helpers/diff_line_helper.go b/pkg/gui/controllers/helpers/diff_line_helper.go new file mode 100644 index 000000000..de4a60960 --- /dev/null +++ b/pkg/gui/controllers/helpers/diff_line_helper.go @@ -0,0 +1,55 @@ +package helpers + +import ( + "path/filepath" + + "github.com/jesseduffield/lazygit/pkg/gocui" + "github.com/jesseduffield/lazygit/pkg/gui/types" +) + +type DiffLineHelper struct { + c *HelperCommon +} + +func NewDiffLineHelper(c *HelperCommon) *DiffLineHelper { + return &DiffLineHelper{c: c} +} + +// GetDiffLineInfo recovers the identity — file, kind, and old/new line number — +// of the diff row at the given (wrapped) view line of the given view. It is the +// seam every consumer of a diff row goes through, so that how we recover that +// identity can change without them noticing: today the only way is to parse the +// view's contents as a unified diff, which works for the renderings that keep a +// diff's structure (no renderer, `git diff --color`, a renderer that only +// colorizes) and fails for the ones that restructure it. +// +// ok is false when the row's identity can't be recovered, in which case the +// caller must not act on the line at all. +func (self *DiffLineHelper) GetDiffLineInfo(view *gocui.View, viewLineIdx int) (types.DiffLineInfo, bool) { + // The cursor and clicks land on a view line, which counts wrapped segments; + // the contents are indexed by unwrapped buffer line. + bufferLineIdx, ok := view.BufferLineForViewLine(viewLineIdx) + if !ok { + return types.DiffLineInfo{}, false + } + + // The lines as written, not as shown: git ends the path field of a diff header + // with a tab when the path contains a space, and the view shows a tab as spaces. + parsed, ok := parseDiffLineFromBuffer(view.LinesAsWritten(), bufferLineIdx) + if !ok { + return types.DiffLineInfo{}, false + } + + return self.diffLineInfoFromParsed(parsed), true +} + +// diffLineInfoFromParsed turns the parser's repo-relative result into the +// absolute-path identity consumers work with. +func (self *DiffLineHelper) diffLineInfoFromParsed(parsed parsedDiffLine) types.DiffLineInfo { + return types.DiffLineInfo{ + Path: filepath.Join(self.c.Git().RepoPaths.WorktreePath(), parsed.RelPath), + Type: parsed.Type, + NewLine: parsed.NewLine, + OldLine: parsed.OldLine, + } +} diff --git a/pkg/gui/controllers/helpers/diff_line_parser.go b/pkg/gui/controllers/helpers/diff_line_parser.go new file mode 100644 index 000000000..c9abcf5c2 --- /dev/null +++ b/pkg/gui/controllers/helpers/diff_line_parser.go @@ -0,0 +1,286 @@ +package helpers + +import ( + "regexp" + "strings" + + "github.com/jesseduffield/lazygit/pkg/commands/patch" + "github.com/jesseduffield/lazygit/pkg/gui/types" +) + +// diffFilePrefix marks the start of a file's section in a (possibly multi-file) +// unified diff. +const diffFilePrefix = "diff --git " + +// submodulePrefix opens the line a submodule's section of a diff starts with, which +// stands in for the "diff --git" header a file of the repo gets. +const submodulePrefix = "Submodule " + +// submoduleSectionPattern matches that line and captures the submodule's path. git +// writes one of two kinds: the commit the submodule is checked out at has moved +// ("Submodule sub a32f27c..2d9f921:", with "..." in place of ".." where the move is +// no fast-forward, " (rewind)" where it goes backwards, and a message in brackets in +// place of the colon where the two commits can't both be read), or its working tree +// is dirty ("Submodule sub contains untracked content"). +// +// The path is captured greedily: git writes it unquoted, so a path that itself ends +// in something reading like a range of commits is told apart by taking the last such +// range on the line. +var submoduleSectionPattern = regexp.MustCompile( + `^Submodule (.+) (?:contains (?:untracked|modified) content|[0-9a-f]+\.{2,3}[0-9a-f]+(?: \(.*\))?:?)$`) + +// parsedDiffLine is what the parser recovers about a row of a rendered diff. +// RelPath is the path as the diff header spells it, i.e. relative to the repo +// root; the caller turns it into the absolute path of types.DiffLineInfo. +type parsedDiffLine struct { + RelPath string + Type types.DiffLineType + NewLine int + OldLine int +} + +// bufferLineParse is the parser's result for one buffer line: the recovered +// identity, and whether the line could be resolved at all (false for a line in +// an unparseable section, or outside any file section). +type bufferLineParse struct { + parsed parsedDiffLine + ok bool +} + +// parseDiffLineFromBuffer recovers the identity of a row of a rendered diff by +// parsing the view's decolorized contents. +// +// bufferLines is the full unwrapped view buffer; targetIdx is the buffer line to +// resolve. A commit's diff spans several files, so we isolate the file section +// containing targetIdx and parse just that one (see parseFileSection). Use this +// for a single line, e.g. the one under the cursor; to resolve every line of a +// buffer, use parseAllDiffLinesFromBuffer, which parses each section only once. +// +// ok is false when the buffer isn't a parseable unified diff at targetIdx, +// because the diff renderer restructured it, so that the caller can fall back. +func parseDiffLineFromBuffer(bufferLines []string, targetIdx int) (parsedDiffLine, bool) { + if targetIdx < 0 || targetIdx >= len(bufferLines) { + return parsedDiffLine{}, false + } + start, end := fileSectionBounds(bufferLines, targetIdx) + if start == -1 { + return parsedDiffLine{}, false + } + r := parseFileSection(bufferLines[start:end], end == len(bufferLines))[targetIdx-start] + return r.parsed, r.ok +} + +// parseAllDiffLinesFromBuffer resolves every line of a (possibly multi-file) +// diff buffer in one pass, parsing each file section exactly once. It is the +// batch form of parseDiffLineFromBuffer, for callers that scan a whole buffer: +// resolving line by line would re-parse a section once per line of it — O(n²) on +// a large single-file diff — whereas this is O(n). The result is indexed 1:1 +// with bufferLines; a line in an unparseable section, or above the first one, is +// left ok=false. +func parseAllDiffLinesFromBuffer(bufferLines []string) []bufferLineParse { + result := make([]bufferLineParse, len(bufferLines)) + for i := 0; i < len(bufferLines); { + if !startsFileSection(bufferLines[i]) { + i++ // in no file section; leave it unresolved + continue + } + end := fileSectionEnd(bufferLines, i) + copy(result[i:end], parseFileSection(bufferLines[i:end], end == len(bufferLines))) + i = end + } + return result +} + +// fileSectionBounds returns the half-open range [start, end) of the file section +// containing targetIdx: the nearest line starting a section at or above it, up to +// where that section ends. start is -1 when targetIdx is above the first file +// section, or below the end of the last one that begins above it. +func fileSectionBounds(bufferLines []string, targetIdx int) (start, end int) { + for start = targetIdx; start >= 0; start-- { + if !startsFileSection(bufferLines[start]) { + continue + } + if end = fileSectionEnd(bufferLines, start); targetIdx < end { + return start, end + } + return -1, -1 + } + return -1, -1 +} + +// fileSectionEnd returns the line the file section beginning at start ends before. +// +// A file's section runs to the next one, since every line between them is part of +// its diff. A submodule's runs only as far as what git writes for it — the line +// naming it, and the log of the commits it moved over — because the lines after +// that need not belong to any section at all. A diff renderer's output has no +// "diff --git" line to stop at, and the rows it puts between one file and the next +// belong to neither. +func fileSectionEnd(bufferLines []string, start int) int { + if submodulePath(bufferLines[start]) != "" { + end := start + 1 + for end < len(bufferLines) && isSubmoduleLogLine(bufferLines[end]) { + end++ + } + return end + } + + for i := start + 1; i < len(bufferLines); i++ { + if startsFileSection(bufferLines[i]) { + return i + } + } + return len(bufferLines) +} + +// startsFileSection reports whether the line opens a section of a diff: git's header +// for a file of the repo, or the line a submodule's section begins with. +func startsFileSection(line string) bool { + return strings.HasPrefix(line, diffFilePrefix) || submodulePath(line) != "" +} + +// isSubmoduleLogLine reports whether the line is one of the commits git lists under +// a submodule's header, which it writes as two spaces, the direction the commit was +// moved in, and the commit's subject. +func isSubmoduleLogLine(line string) bool { + return strings.HasPrefix(line, " > ") || strings.HasPrefix(line, " < ") +} + +// submodulePath returns the submodule whose section the given line opens, and "" for +// every other line. +// +// A submodule gets no "diff --git" header and no hunks: git states which commits it +// moved between and lists them, so that one line is all there is to take the path +// from. The prefix is tested first so that the pattern is run over next to no lines +// of a diff. +func submodulePath(line string) string { + if !strings.HasPrefix(line, submodulePrefix) { + return "" + } + if match := submoduleSectionPattern.FindStringSubmatch(line); match != nil { + return match[1] + } + return "" +} + +// parseFileSection parses one file's diff section (fileLines, starting at the line +// that opens it) a single time and returns the identity of each of its +// lines, indexed 1:1 with fileLines. patch.Parse's line indices line up with the +// section's buffer lines, so the type and the old/new line numbers fall out of +// the patch arithmetic. A submodule's section has no hunks at all, so every row of +// it comes out as a header of the submodule, which is what they are: what git states +// there is which commits it moved between, not lines of a file. +// +// Every line is left ok=false when the section has no +// recoverable path or isn't a well-formed unified diff — the rendering +// restructured it, and acting on a mis-parse would land us on the wrong line, so +// the caller should fall back. +// +// endsTheBuffer says the section runs to the end of what we were given. That is +// where a diff we have only part of breaks off. A long one is read a screenful +// at a time and the rest as the user scrolls, so its last hunk holds fewer lines +// than its header declares until the reading is done. Insisting on the whole +// hunk there would leave every line of the file unresolved while the diff is the +// one on screen, so a section in that position is held to what has arrived. +func parseFileSection(fileLines []string, endsTheBuffer bool) []bufferLineParse { + result := make([]bufferLineParse, len(fileLines)) + + relPath := pathFromDiffHeader(fileLines) + if relPath == "" { + return result + } + p := patch.Parse(strings.Join(fileLines, "\n")) + isWellFormed := p.IsWellFormed + if endsTheBuffer { + isWellFormed = p.IsWellFormedSoFar + } + if !isWellFormed() { + return result + } + patchLines := p.Lines() + for i := range fileLines { + if i >= len(patchLines) { + break + } + parsed := parsedDiffLine{ + RelPath: relPath, + Type: diffLineTypeForKind(patchLines[i].Kind), + NewLine: p.LineNumberOfLine(i), + } + if parsed.Type == types.DiffLineDeleted { + parsed.OldLine = p.OldLineNumberOfLine(i) + } + result[i] = bufferLineParse{parsed, true} + } + return result +} + +func diffLineTypeForKind(kind patch.PatchLineKind) types.DiffLineType { + switch kind { + case patch.PATCH_HEADER: + return types.DiffLineFileHeader + case patch.HUNK_HEADER: + return types.DiffLineHunkHeader + case patch.ADDITION: + return types.DiffLineAdded + case patch.DELETION: + return types.DiffLineDeleted + case patch.CONTEXT: + return types.DiffLineContext + default: + return types.DiffLineOther + } +} + +// pathFromDiffHeader extracts the new-file path of a single diff section. A +// submodule's section states its path in the line it opens with. For a file of the +// repo the path comes from the "+++ b/" line, falling back to "--- a/" +// when the new path is /dev/null (a deleted file), and to the "diff --git" line when +// there are no such lines at all (a pure rename, which has no hunks). +func pathFromDiffHeader(fileLines []string) string { + if path := submodulePath(fileLines[0]); path != "" { + return path + } + + var oldPath, newPath string + for _, line := range fileLines { + if strings.HasPrefix(line, "@@") { + break // past the header + } + switch { + case strings.HasPrefix(line, "+++ "): + newPath = stripDiffPathPrefix(strings.TrimPrefix(line, "+++ ")) + case strings.HasPrefix(line, "--- "): + oldPath = stripDiffPathPrefix(strings.TrimPrefix(line, "--- ")) + } + } + + if newPath != "" && newPath != "/dev/null" { + return newPath + } + if oldPath != "" && oldPath != "/dev/null" { + return oldPath + } + return pathFromDiffGitLine(fileLines[0]) +} + +// stripDiffPathPrefix removes the a/ or b/ prefix git puts on the paths in a +// diff header. We ask git for these prefixes explicitly (diff.noprefix=false), +// so they are always there. +func stripDiffPathPrefix(path string) string { + if strings.HasPrefix(path, "a/") || strings.HasPrefix(path, "b/") { + return path[2:] + } + return path +} + +// pathFromDiffGitLine extracts the new-file path from a "diff --git a/X b/X" +// line. A path containing " b/" would defeat this, but the +++/--- lines are +// unambiguous and we only get here when they are absent. +func pathFromDiffGitLine(line string) string { + rest := strings.TrimPrefix(line, diffFilePrefix) + if idx := strings.LastIndex(rest, " b/"); idx != -1 { + return rest[idx+len(" b/"):] + } + return "" +} diff --git a/pkg/gui/controllers/helpers/diff_line_parser_test.go b/pkg/gui/controllers/helpers/diff_line_parser_test.go new file mode 100644 index 000000000..8e8bec91a --- /dev/null +++ b/pkg/gui/controllers/helpers/diff_line_parser_test.go @@ -0,0 +1,266 @@ +package helpers + +import ( + "slices" + "strings" + "testing" + + "github.com/jesseduffield/lazygit/pkg/gui/types" + "github.com/stretchr/testify/assert" +) + +// A two-file commit diff as it appears (decolorized) in the main view. file1 has +// two consecutive deletions (grape, pear) that share a new-file line number; +// file2 has two consecutive additions. +const twoFileDiff = `diff --git a/file1.go b/file1.go +index 1111111..2222222 100644 +--- a/file1.go ++++ b/file1.go +@@ -1,4 +1,2 @@ + apple +-grape +-pear + lemon +diff --git a/dir/file2.go b/dir/file2.go +index 3333333..4444444 100644 +--- a/dir/file2.go ++++ b/dir/file2.go +@@ -10,2 +9,4 @@ func foo() { + ctx ++added1 ++added2 + ctx2` + +func TestParseDiffLineFromBuffer(t *testing.T) { + bufferLines := strings.Split(twoFileDiff, "\n") + + scenarios := []struct { + name string + targetIdx int + expected parsedDiffLine + expectOk bool + }{ + {"file header", 0, parsedDiffLine{RelPath: "file1.go", Type: types.DiffLineFileHeader, NewLine: 1}, true}, + {"hunk header", 4, parsedDiffLine{RelPath: "file1.go", Type: types.DiffLineHunkHeader, NewLine: 1}, true}, + {"context line", 5, parsedDiffLine{RelPath: "file1.go", Type: types.DiffLineContext, NewLine: 1}, true}, + // The two deletions share new-file line 2 but have distinct old-file lines. + {"first deletion", 6, parsedDiffLine{RelPath: "file1.go", Type: types.DiffLineDeleted, NewLine: 2, OldLine: 2}, true}, + {"second deletion", 7, parsedDiffLine{RelPath: "file1.go", Type: types.DiffLineDeleted, NewLine: 2, OldLine: 3}, true}, + // The second file: its path comes from the second "diff --git" section, + // and its additions get distinct new-file line numbers. + {"first addition", 15, parsedDiffLine{RelPath: "dir/file2.go", Type: types.DiffLineAdded, NewLine: 10}, true}, + {"second addition", 16, parsedDiffLine{RelPath: "dir/file2.go", Type: types.DiffLineAdded, NewLine: 11}, true}, + {"out of range", 999, parsedDiffLine{}, false}, + } + + for _, s := range scenarios { + t.Run(s.name, func(t *testing.T) { + result, ok := parseDiffLineFromBuffer(bufferLines, s.targetIdx) + assert.Equal(t, s.expectOk, ok) + if s.expectOk { + assert.Equal(t, s.expected, result) + } + }) + } +} + +func TestParseDiffLineFromBufferRename(t *testing.T) { + // A rename with no content change has no hunks and no +++/--- lines, so the + // path has to come from the "diff --git" line; a rename with a content + // change has them, and they carry the new path. + pureRename := strings.Split(`diff --git a/old.go b/new.go +similarity index 100% +rename from old.go +rename to new.go`, "\n") + + result, ok := parseDiffLineFromBuffer(pureRename, 2) + assert.True(t, ok) + assert.Equal(t, parsedDiffLine{RelPath: "new.go", Type: types.DiffLineFileHeader, NewLine: 1}, result) + + renameWithModification := strings.Split(`diff --git a/old.go b/new.go +similarity index 62% +rename from old.go +rename to new.go +index 1111111..2222222 100644 +--- a/old.go ++++ b/new.go +@@ -1,2 +1,2 @@ + apple +-grape ++kiwi`, "\n") + + result, ok = parseDiffLineFromBuffer(renameWithModification, 10) + assert.True(t, ok) + assert.Equal(t, parsedDiffLine{RelPath: "new.go", Type: types.DiffLineAdded, NewLine: 2}, result) +} + +func TestParseDiffLineFromBufferDeletedFile(t *testing.T) { + // The new path is /dev/null, so the identity comes from the old path. + deletedFile := strings.Split(`diff --git a/gone.go b/gone.go +deleted file mode 100644 +index 1111111..0000000 +--- a/gone.go ++++ /dev/null +@@ -1,2 +0,0 @@ +-apple +-grape`, "\n") + + result, ok := parseDiffLineFromBuffer(deletedFile, 7) + assert.True(t, ok) + assert.Equal(t, parsedDiffLine{RelPath: "gone.go", Type: types.DiffLineDeleted, NewLine: 0, OldLine: 2}, result) +} + +func TestParseDiffLineFromBufferSubmodule(t *testing.T) { + // A submodule has no "diff --git" header and no hunks: git opens its section with + // the commits it moved between and lists them below. So the section ends the one + // above it, and every row of it belongs to the submodule as a whole. + withSubmodule := strings.Split(`diff --git a/file.txt b/file.txt +index 1111111..2222222 100644 +--- a/file.txt ++++ b/file.txt +@@ -1,2 +1,3 @@ + hello + world ++world +Submodule modules/xyz a32f27c..2d9f921: + > bump the thing`, "\n") + + for _, targetIdx := range []int{8, 9} { + result, ok := parseDiffLineFromBuffer(withSubmodule, targetIdx) + assert.True(t, ok) + assert.Equal(t, + parsedDiffLine{RelPath: "modules/xyz", Type: types.DiffLineFileHeader, NewLine: 1}, + result, "line %d", targetIdx) + } + + result, ok := parseDiffLineFromBuffer(withSubmodule, 7) + assert.True(t, ok) + assert.Equal(t, parsedDiffLine{RelPath: "file.txt", Type: types.DiffLineAdded, NewLine: 3}, result) +} + +func TestParseDiffLineFromBufferSubmoduleInARendering(t *testing.T) { + // A diff renderer passes the lines git writes for a submodule through as they + // are, while printing nothing below them that a section could end at. The + // section has to end where what git writes for the submodule ends all the same: + // the rows below belong to the other files of the diff, and can only be placed + // by the records the renderer states for them. + rendered := strings.Split(`Submodule modules/xyz a32f27c..2d9f921: + > bump the thing + +products/a.txt + + one + two`, "\n") + + all := parseAllDiffLinesFromBuffer(rendered) + assert.Equal(t, "modules/xyz", all[0].parsed.RelPath) + assert.Equal(t, "modules/xyz", all[1].parsed.RelPath) + for i := 2; i < len(rendered); i++ { + assert.False(t, all[i].ok, "line %d: %q", i, rendered[i]) + } +} + +func TestSubmodulePath(t *testing.T) { + scenarios := []struct { + name string + line string + expected string + }{ + {"moved on", "Submodule modules/xyz a32f27c..2d9f921:", "modules/xyz"}, + {"moved back", "Submodule modules/xyz 2d9f921..a32f27c (rewind):", "modules/xyz"}, + {"moved sideways", "Submodule modules/xyz a32f27c...2d9f921:", "modules/xyz"}, + {"added", "Submodule modules/xyz 0000000...2d9f921 (new submodule)", "modules/xyz"}, + {"removed", "Submodule modules/xyz a32f27c...0000000 (submodule deleted)", "modules/xyz"}, + {"commits missing", "Submodule modules/xyz a32f27c...2d9f921 (commits not present)", "modules/xyz"}, + {"dirty", "Submodule modules/xyz contains modified content", "modules/xyz"}, + {"with something new in it", "Submodule modules/xyz contains untracked content", "modules/xyz"}, + // git writes the path unquoted, so one with a space in it, or one ending in + // something that reads like a range of commits, is told apart by matching the + // last range on the line. + {"path with a space", "Submodule my modules/xyz a32f27c..2d9f921:", "my modules/xyz"}, + {"path reading like a range", "Submodule a32f27c..2d9f921 deadbee..fa1afe1:", "a32f27c..2d9f921"}, + // A line of a file that reads like one of these is indented by the column the + // diff states the line's side in, so it cannot be mistaken for one. + {"a line of a file", " Submodule modules/xyz contains modified content", ""}, + {"something else entirely", "Submodule support was added", ""}, + } + + for _, s := range scenarios { + t.Run(s.name, func(t *testing.T) { + assert.Equal(t, s.expected, submodulePath(s.line)) + }) + } +} + +func TestParseDiffLineFromBufferNotADiff(t *testing.T) { + // A rendering with no "diff --git" line can't be parsed, so the caller falls + // back rather than acting on the line. + bufferLines := []string{"some", "lines", "that", "are not a diff"} + _, ok := parseDiffLineFromBuffer(bufferLines, 2) + assert.False(t, ok) +} + +func TestParseDiffLineFromBufferGutterMangled(t *testing.T) { + // A diff renderer that moves the line numbers into a gutter keeps the diff + // and hunk headers but pushes the +/- markers off the start of each body + // line, so every line reads as context. The body no longer matches the hunk + // header, so we refuse to parse rather than return a confident mis-parse. + mangled := strings.Split(`diff --git a/file1.txt b/file1.txt +index 1111111..2222222 100644 +--- a/file1.txt ++++ b/file1.txt +@@ -1,5 +1,3 @@ + 1 ⋮ 1 │ apple + 2 ⋮ │-grape + 3 ⋮ │-pear + 4 ⋮ 2 │ lemon + 5 ⋮ 3 │ mango`, "\n") + + _, ok := parseDiffLineFromBuffer(mangled, 6) + assert.False(t, ok) +} + +func TestParseDiffLineFromBufferReadInPart(t *testing.T) { + lines := strings.Split(twoFileDiff, "\n") + + // A long diff is read a screenful at a time, so the buffer breaks off part way + // through a hunk. The lines that did arrive are resolved all the same, since + // holding out for the whole hunk would leave the diff on screen with nothing to + // act on. + cutShort := lines[:len(lines)-1] + result, ok := parseDiffLineFromBuffer(cutShort, 15) + assert.True(t, ok) + assert.Equal(t, parsedDiffLine{RelPath: "dir/file2.go", Type: types.DiffLineAdded, NewLine: 10}, result) + + // Only the section the buffer breaks off in is read that way. One that another + // section follows is all there, so a hunk short of what its header declares means + // the rendering restructured the diff, and none of it is resolved. + shortFirstSection := append(slices.Clone(lines[:8]), lines[9:]...) + _, ok = parseDiffLineFromBuffer(shortFirstSection, 5) + assert.False(t, ok) +} + +func TestParseAllDiffLinesFromBuffer(t *testing.T) { + // Some decoration above the diff, which belongs to no file section: a commit + // message and a diffstat, as `git show` renders them. + bufferLines := append( + []string{"commit 1234567", "", " do a thing", "", " file1.go | 2 --", ""}, + strings.Split(twoFileDiff, "\n")..., + ) + + all := parseAllDiffLinesFromBuffer(bufferLines) + + // The batch parse resolves each file section once, and has to agree with + // resolving the lines one at a time. + assert.Len(t, all, len(bufferLines)) + for i := range bufferLines { + parsed, ok := parseDiffLineFromBuffer(bufferLines, i) + assert.Equal(t, bufferLineParse{parsed, ok}, all[i], "line %d: %q", i, bufferLines[i]) + } + + // The lines above the first file section are left unresolved. + for i := range 6 { + assert.False(t, all[i].ok) + } + assert.True(t, all[6].ok) +} diff --git a/pkg/gui/controllers/helpers/helpers.go b/pkg/gui/controllers/helpers/helpers.go index 4c9c79f3d..ea1214699 100644 --- a/pkg/gui/controllers/helpers/helpers.go +++ b/pkg/gui/controllers/helpers/helpers.go @@ -39,6 +39,7 @@ type Helpers struct { Snake *SnakeHelper // lives in context package because our contexts need it to render to main Diff *DiffHelper + DiffLine *DiffLineHelper Repos *ReposHelper RecordDirectory *RecordDirectoryHelper Update *UpdateHelper @@ -76,6 +77,7 @@ func NewStubHelpers() *Helpers { Commits: &CommitsHelper{}, Snake: &SnakeHelper{}, Diff: &DiffHelper{}, + DiffLine: &DiffLineHelper{}, Repos: &ReposHelper{}, RecordDirectory: &RecordDirectoryHelper{}, Update: &UpdateHelper{}, diff --git a/pkg/gui/types/diff_line_info.go b/pkg/gui/types/diff_line_info.go new file mode 100644 index 000000000..ae3d8eee9 --- /dev/null +++ b/pkg/gui/types/diff_line_info.go @@ -0,0 +1,42 @@ +package types + +// DiffLineType classifies a row of a rendered diff. +type DiffLineType int + +const ( + DiffLineFileHeader DiffLineType = iota + DiffLineHunkHeader + DiffLineContext + DiffLineAdded + DiffLineDeleted + // DiffLineOther is anything that isn't one of the above, e.g. the + // "\ No newline at end of file" marker. + DiffLineOther +) + +// DiffLineInfo is the identity of a row of a rendered diff in terms of the patch +// it was rendered from: which file the row belongs to, what kind of row it is, +// and where the line sits in the old and new versions of that file. This lets us +// act on the line the user is pointing at in a diff view: stage it, open it in an +// editor, keep the cursor on it across a re-render. The rendered text alone tells +// us none of that. +type DiffLineInfo struct { + // Path is the absolute path of the file the line belongs to. + Path string + Type DiffLineType + // NewLine is the line's position in the new version of the file. Set for all + // content lines (for a deletion it is the position the deletion sits at) and + // for hunk headers (the first line of the hunk they head). + NewLine int + // OldLine is the line's position in the old version of the file. Set only + // for deletions, which are the only rows that need it: two consecutive + // deletions share a new-file position and differ only here. + OldLine int +} + +// IsChange reports whether the row is an added or deleted line, as opposed to a +// context line or a header. It mirrors patch.PatchLine.IsChange: those are the +// rows a patch is built from, and the rows navigation moves between. +func (self DiffLineInfo) IsChange() bool { + return self.Type == DiffLineAdded || self.Type == DiffLineDeleted +} From 8cd0a0e9ad47ab1d40f3bf85d650d5ece3ef4067 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sun, 9 Aug 2026 13:51:40 +0200 Subject: [PATCH 12/12] Decode the path of a diff header the way git writes it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit git doesn't just print the path: it terminates it with a tab when the path contains a space, and C-quotes the whole field when the path contains anything it won't print raw — with core.quotePath, which is on by default, that means any non-ASCII byte, so a file called café shows up as "b/caf\303\251". Taking the field verbatim therefore gives a path that doesn't exist, for a whole class of perfectly ordinary file names. The quoting is Go's own string syntax, octal escapes and all, so decoding it is a call to strconv.Unquote. Co-authored-by: Claude Opus 5 (1M context) --- .../controllers/helpers/diff_line_parser.go | 42 ++++++++++++-- .../helpers/diff_line_parser_test.go | 56 +++++++++++++++++++ 2 files changed, 93 insertions(+), 5 deletions(-) diff --git a/pkg/gui/controllers/helpers/diff_line_parser.go b/pkg/gui/controllers/helpers/diff_line_parser.go index c9abcf5c2..d58633f7e 100644 --- a/pkg/gui/controllers/helpers/diff_line_parser.go +++ b/pkg/gui/controllers/helpers/diff_line_parser.go @@ -2,6 +2,7 @@ package helpers import ( "regexp" + "strconv" "strings" "github.com/jesseduffield/lazygit/pkg/commands/patch" @@ -249,9 +250,9 @@ func pathFromDiffHeader(fileLines []string) string { } switch { case strings.HasPrefix(line, "+++ "): - newPath = stripDiffPathPrefix(strings.TrimPrefix(line, "+++ ")) + newPath = pathFromDiffHeaderField(strings.TrimPrefix(line, "+++ ")) case strings.HasPrefix(line, "--- "): - oldPath = stripDiffPathPrefix(strings.TrimPrefix(line, "--- ")) + oldPath = pathFromDiffHeaderField(strings.TrimPrefix(line, "--- ")) } } @@ -264,6 +265,33 @@ func pathFromDiffHeader(fileLines []string) string { return pathFromDiffGitLine(fileLines[0]) } +// pathFromDiffHeaderField decodes one path field of a diff header — the part +// after "--- " or "+++ ", or one of the two paths on the "diff --git" line — +// into the repo-relative path it names. +// +// git spells such a field in three ways: plain; terminated by a tab, when the +// path contains a space; or C-quoted as a whole, when the path contains +// characters git won't print raw — which, with core.quotePath enabled (the +// default), includes every non-ASCII byte, so `café` arrives as +// `"b/caf\303\251"`. The quoting is Go's string syntax, octal escapes included, +// so strconv decodes it for us. +// +// Returns "" for a quoted field we can't decode: better to resolve nothing than +// to point a consumer at a path that doesn't exist. +func pathFromDiffHeaderField(field string) string { + field = strings.TrimSuffix(field, "\t") + + if strings.HasPrefix(field, `"`) { + unquoted, err := strconv.Unquote(field) + if err != nil { + return "" + } + field = unquoted + } + + return stripDiffPathPrefix(field) +} + // stripDiffPathPrefix removes the a/ or b/ prefix git puts on the paths in a // diff header. We ask git for these prefixes explicitly (diff.noprefix=false), // so they are always there. @@ -275,12 +303,16 @@ func stripDiffPathPrefix(path string) string { } // pathFromDiffGitLine extracts the new-file path from a "diff --git a/X b/X" -// line. A path containing " b/" would defeat this, but the +++/--- lines are -// unambiguous and we only get here when they are absent. +// line, where the two paths are separated by a space and either may be quoted. +// A path containing " b/" (or ` "b/`) would defeat this, but the +++/--- lines +// are unambiguous and we only get here when they are absent. func pathFromDiffGitLine(line string) string { rest := strings.TrimPrefix(line, diffFilePrefix) + if idx := strings.LastIndex(rest, ` "b/`); idx != -1 { + return pathFromDiffHeaderField(rest[idx+1:]) + } if idx := strings.LastIndex(rest, " b/"); idx != -1 { - return rest[idx+len(" b/"):] + return pathFromDiffHeaderField(rest[idx+1:]) } return "" } diff --git a/pkg/gui/controllers/helpers/diff_line_parser_test.go b/pkg/gui/controllers/helpers/diff_line_parser_test.go index 8e8bec91a..ae620d23c 100644 --- a/pkg/gui/controllers/helpers/diff_line_parser_test.go +++ b/pkg/gui/controllers/helpers/diff_line_parser_test.go @@ -240,6 +240,62 @@ func TestParseDiffLineFromBufferReadInPart(t *testing.T) { assert.False(t, ok) } +func TestPathFromDiffHeaderField(t *testing.T) { + scenarios := []struct { + name string + field string + expected string + }{ + {"new side", "b/file.go", "file.go"}, + {"old side", "a/file.go", "file.go"}, + {"a missing file", "/dev/null", "/dev/null"}, + // git terminates the field with a tab when the path has a space in it. + {"path with a space", "b/with space.go\t", "with space.go"}, + // With core.quotePath enabled (the default) every non-ASCII byte is + // escaped, and the field is quoted as a whole, prefix included. + {"non-ASCII path", `"b/caf\303\251.go"`, "café.go"}, + {"non-ASCII path with a space", "\"b/caf\\303\\251 x.go\"\t", "café x.go"}, + {"path with a double quote", `"b/we\"ird.go"`, `we"ird.go`}, + {"path with a backslash", `"b/back\\slash.go"`, `back\slash.go`}, + {"path with a tab", `"b/tab\there.go"`, "tab\there.go"}, + {"undecodable", `"b/unterminated`, ""}, + } + + for _, s := range scenarios { + t.Run(s.name, func(t *testing.T) { + assert.Equal(t, s.expected, pathFromDiffHeaderField(s.field)) + }) + } +} + +func TestParseDiffLineFromBufferQuotedPath(t *testing.T) { + // A rename of a file whose name needs quoting, with a content change: the + // path is quoted on the "diff --git" line and on both of the +++/--- lines. + renamed := []string{ + `diff --git "a/caf\303\251 old.go" "b/caf\303\251 new.go"`, + "similarity index 62%", + `rename from "caf\303\251 old.go"`, + `rename to "caf\303\251 new.go"`, + "index 1111111..2222222 100644", + "--- \"a/caf\\303\\251 old.go\"\t", + "+++ \"b/caf\\303\\251 new.go\"\t", + "@@ -1,2 +1,2 @@", + " apple", + "-grape", + "+kiwi", + } + + result, ok := parseDiffLineFromBuffer(renamed, 10) + assert.True(t, ok) + assert.Equal(t, parsedDiffLine{RelPath: "café new.go", Type: types.DiffLineAdded, NewLine: 2}, result) + + // The same rename without a content change has no +++/--- lines, so the path + // comes from the "diff --git" line, where both paths are quoted. + result, ok = parseDiffLineFromBuffer(renamed[:4], 2) + assert.True(t, ok) + assert.Equal(t, parsedDiffLine{RelPath: "café new.go", Type: types.DiffLineFileHeader, NewLine: 1}, result) +} + func TestParseAllDiffLinesFromBuffer(t *testing.T) { // Some decoration above the diff, which belongs to no file section: a commit // message and a diffstat, as `git show` renders them.