From 387d0eac3a7a61098961e2f39c995aece8a128c3 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sun, 9 Aug 2026 15:21:48 +0200 Subject: [PATCH] Take a diff renderer at its word about a diff line Where a renderer states which line of which file it is rendering, take that as the line's identity. The renderer knows, and for a rendering that no longer looks like a diff nothing else does. Reading the rendered text stays the way for the renderings that keep a diff's structure, and for the renderers that say nothing at all. Which of the two is used is settled for the rendering as a whole, not row by row. A rendering with records is not parsed at all, not even for the rows the renderer says nothing about. Its text is no unified diff, and a row of it can read like one. Under delta, a commit that adds a test whose input is a diff shows the test's "diff --git a/img.png b/img.png" line as a row of its own; parsed, that row opens a file section that runs to the end of the rendering, and every row delta puts between hunks and files comes out as a header of a file called img.png. So the untagged rows of such a rendering stay unresolved, as the protocol has it (spec section 6.4). Header records are accepted too. This lets a renderer point at a file with no content lines, such as a pure rename, a mode change or a binary file: for those files the header is the only row in the diff. Co-authored-by: Claude Opus 5 (1M context) Co-Authored-By: Claude Fable 5.1 --- .../controllers/helpers/diff_line_helper.go | 49 +++++++-- .../controllers/helpers/diff_line_parser.go | 83 +++++++++++++++ .../helpers/diff_line_parser_test.go | 100 ++++++++++++++++++ 3 files changed, 222 insertions(+), 10 deletions(-) diff --git a/pkg/gui/controllers/helpers/diff_line_helper.go b/pkg/gui/controllers/helpers/diff_line_helper.go index 77f70611e..be96e4912 100644 --- a/pkg/gui/controllers/helpers/diff_line_helper.go +++ b/pkg/gui/controllers/helpers/diff_line_helper.go @@ -18,10 +18,16 @@ func NewDiffLineHelper(c *HelperCommon) *DiffLineHelper { // 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. +// identity can change without them noticing. +// +// There are two ways, and which one is used is settled for the rendering as a +// whole (see renderingStatesDiffLines). A diff renderer that speaks the OSC 1717 +// protocol states the identity of each line it renders. That is the only way to +// recover it from a rendering that doesn't look like a diff any more (columns, or +// +/- markers replaced by colour), and a row such a renderer says nothing about +// has no identity. Otherwise we parse the view's contents as a unified diff; this +// works for the renderings that keep a diff's structure (no renderer, `git diff +// --color`, a renderer that only colorizes) and fails for the rest. // // 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. @@ -33,19 +39,42 @@ func (self *DiffLineHelper) GetDiffLineInfo(view *gocui.View, viewLineIdx int) ( return types.DiffLineInfo{}, false } - parsed, ok := parseDiffLineFromBuffer(diffLineTexts(view.DiffLineContents()), bufferLineIdx) + contents := view.DiffLineContents() + if bufferLineIdx >= len(contents) { + return types.DiffLineInfo{}, false + } + + if renderingStatesDiffLines(contents) { + // A row can carry more than one record, when the rendering puts two diff + // lines on it; the first one is the row's identity, and the leftmost record + // is the one a reader would call the row's own. + if metadata := contents[bufferLineIdx].Metadata; len(metadata) > 0 { + if parsed, ok := parseDiffLineMetadata(metadata[0]); ok { + return self.diffLineInfo(parsed), true + } + } + return types.DiffLineInfo{}, false + } + + parsed, ok := parseDiffLineFromBuffer(diffLineTexts(contents), bufferLineIdx) if !ok { return types.DiffLineInfo{}, false } - return self.diffLineInfoFromParsed(parsed), true + return self.diffLineInfo(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 { +// diffLineInfo turns a parser's result into the absolute-path identity consumers +// work with. The path arrives repo-relative from the diff header, but a renderer +// states it however it likes, absolute paths included. +func (self *DiffLineHelper) diffLineInfo(parsed parsedDiffLine) types.DiffLineInfo { + path := parsed.Path + if !filepath.IsAbs(path) { + path = filepath.Join(self.c.Git().RepoPaths.WorktreePath(), path) + } + return types.DiffLineInfo{ - Path: filepath.Join(self.c.Git().RepoPaths.WorktreePath(), parsed.Path), + Path: path, 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 index c9033960d..dd7270af2 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" + "slices" "strconv" "strings" @@ -103,6 +104,27 @@ func diffLineTexts(contents []gocui.DiffLineContent) []string { return texts } +// renderingStatesDiffLines reports whether the renderer stated, for at least one row +// of the rendering, which diff line it shows. The version-only record a renderer +// announces the protocol with names no line, and doesn't count. +// +// The answer settles how the whole rendering is read. A renderer that states its +// lines lays the diff out as it likes, so its text is no unified diff and must not be +// parsed as one, not even for the rows it says nothing about. Such a row can read +// like a diff header when the file being diffed is itself a diff; the parser would +// take that for the start of a file section and place every untagged row below it +// in a file the diff doesn't have. So the rows a renderer leaves untagged (dividers, +// padding) have no identity, as the protocol has it. A rendering without any record +// is a diff that describes itself, and is parsed as one. +func renderingStatesDiffLines(contents []gocui.DiffLineContent) bool { + return slices.ContainsFunc(contents, func(content gocui.DiffLineContent) bool { + return slices.ContainsFunc(content.Metadata, func(record string) bool { + _, ok := parseDiffLineMetadata(record) + return ok + }) + }) +} + // 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 @@ -313,6 +335,67 @@ func stripDiffPathPrefix(path string) string { return path } +// parseDiffLineMetadata parses the payload of an OSC 1717 record, in which a +// diff renderer states which line of which file it is rendering. The v1 payload +// is positional and ';'-delimited: +// +// version;type;new-line;old-line;file +// +// The file comes last so that it may itself contain a ';'. The old-file line is +// empty unless the line is a deletion, the only kind that needs it, and the +// new-file line is empty on a file header, the one kind that has no line. +// +// ok is false for a payload of an unknown version or shape, so that the caller +// can fall back to reading the rendered text. +func parseDiffLineMetadata(payload string) (parsedDiffLine, bool) { + fields := strings.SplitN(payload, ";", 5) + if len(fields) < 5 || fields[0] != "1" { + return parsedDiffLine{}, false + } + + lineType, ok := diffLineTypeFromMetadata(fields[1]) + if !ok { + return parsedDiffLine{}, false + } + + newLine := 0 + if fields[2] != "" { + var err error + if newLine, err = strconv.Atoi(fields[2]); err != nil { + return parsedDiffLine{}, false + } + } else if lineType != types.DiffLineFileHeader { + return parsedDiffLine{}, false + } + + oldLine := 0 + if fields[3] != "" { + var err error + if oldLine, err = strconv.Atoi(fields[3]); err != nil { + return parsedDiffLine{}, false + } + } + + return parsedDiffLine{Path: fields[4], Type: lineType, NewLine: newLine, OldLine: oldLine}, true +} + +func diffLineTypeFromMetadata(typeField string) (types.DiffLineType, bool) { + switch typeField { + case "c": + return types.DiffLineContext, true + case "a": + return types.DiffLineAdded, true + case "d": + return types.DiffLineDeleted, true + case "f": + return types.DiffLineFileHeader, true + case "h": + return types.DiffLineHunkHeader, true + default: + return types.DiffLineOther, false + } +} + // pathFromDiffGitLine extracts the new-file path from a "diff --git a/X b/X" // 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 diff --git a/pkg/gui/controllers/helpers/diff_line_parser_test.go b/pkg/gui/controllers/helpers/diff_line_parser_test.go index 11f078abd..f320a6ef5 100644 --- a/pkg/gui/controllers/helpers/diff_line_parser_test.go +++ b/pkg/gui/controllers/helpers/diff_line_parser_test.go @@ -5,6 +5,7 @@ import ( "strings" "testing" + "github.com/jesseduffield/lazygit/pkg/gocui" "github.com/jesseduffield/lazygit/pkg/gui/types" "github.com/stretchr/testify/assert" ) @@ -320,3 +321,102 @@ func TestParseAllDiffLinesFromBuffer(t *testing.T) { } assert.True(t, all[6].ok) } + +func TestParseDiffLineMetadata(t *testing.T) { + scenarios := []struct { + name string + payload string + expected parsedDiffLine + expectOk bool + }{ + {"context", "1;c;1;;foo.txt", parsedDiffLine{Path: "foo.txt", Type: types.DiffLineContext, NewLine: 1}, true}, + {"added", "1;a;3;;foo.txt", parsedDiffLine{Path: "foo.txt", Type: types.DiffLineAdded, NewLine: 3}, true}, + // A deletion carries both numbers; two consecutive deletions share the + // new-file line and differ only in the old-file one. + {"first deletion", "1;d;2;2;foo.txt", parsedDiffLine{Path: "foo.txt", Type: types.DiffLineDeleted, NewLine: 2, OldLine: 2}, true}, + {"second deletion", "1;d;2;3;foo.txt", parsedDiffLine{Path: "foo.txt", Type: types.DiffLineDeleted, NewLine: 2, OldLine: 3}, true}, + // A whole-file deletion has new-file position 0 and the old path. + {"deleted file", "1;d;0;1;gone.txt", parsedDiffLine{Path: "gone.txt", Type: types.DiffLineDeleted, NewLine: 0, OldLine: 1}, true}, + // The path is the last field, so a ';' within it survives. + {"path with semicolon", "1;c;5;;weird;name.txt", parsedDiffLine{Path: "weird;name.txt", Type: types.DiffLineContext, NewLine: 5}, true}, + // A renderer may state the path absolutely; the parser keeps it verbatim + // and leaves resolving it to the caller. + {"absolute path", "1;a;7;;/abs/foo.txt", parsedDiffLine{Path: "/abs/foo.txt", Type: types.DiffLineAdded, NewLine: 7}, true}, + // A file header has no line number; a hunk header carries the new-file + // line of the hunk's first line (0 for a whole-file deletion, mirroring + // `@@ -1,N +0,0 @@`). + {"file header", "1;f;;;foo.txt", parsedDiffLine{Path: "foo.txt", Type: types.DiffLineFileHeader}, true}, + {"hunk header", "1;h;10;;foo.txt", parsedDiffLine{Path: "foo.txt", Type: types.DiffLineHunkHeader, NewLine: 10}, true}, + {"hunk header of a deleted file", "1;h;0;;gone.txt", parsedDiffLine{Path: "gone.txt", Type: types.DiffLineHunkHeader, NewLine: 0}, true}, + // A file header's line number is always empty, but a renderer that fills + // it in anyway is taken at its word rather than rejected. + {"file header with a line number", "1;f;10;;foo.txt", parsedDiffLine{Path: "foo.txt", Type: types.DiffLineFileHeader, NewLine: 10}, true}, + + {"unknown version", "2;c;1;;foo.txt", parsedDiffLine{}, false}, + {"unknown type", "1;x;1;;foo.txt", parsedDiffLine{}, false}, + {"too few fields", "1;c;1", parsedDiffLine{}, false}, + {"non-numeric new-line", "1;c;x;;foo.txt", parsedDiffLine{}, false}, + {"non-numeric old-line", "1;d;2;y;foo.txt", parsedDiffLine{}, false}, + // Only a file header may omit the new-file line; on any other kind the + // record is malformed, and rejecting it falls the row back to the diff + // text rather than acting on a line number we don't have. + {"empty new-line on a content line", "1;c;;;foo.txt", parsedDiffLine{}, false}, + {"empty new-line on a hunk header", "1;h;;;foo.txt", parsedDiffLine{}, false}, + } + + for _, s := range scenarios { + t.Run(s.name, func(t *testing.T) { + result, ok := parseDiffLineMetadata(s.payload) + assert.Equal(t, s.expectOk, ok) + if s.expectOk { + assert.Equal(t, s.expected, result) + } + }) + } +} + +func TestRenderingStatesDiffLines(t *testing.T) { + row := func(text string, records ...string) gocui.DiffLineContent { + return gocui.DiffLineContent{Text: text, Metadata: records} + } + + scenarios := []struct { + name string + contents []gocui.DiffLineContent + expected bool + }{ + { + name: "a rendering without records is read as a diff", + contents: []gocui.DiffLineContent{row("diff --git a/foo.txt b/foo.txt"), row("+one")}, + expected: false, + }, + { + name: "a record on any row makes the records the source", + contents: []gocui.DiffLineContent{row("foo.txt"), row("one", "1;c;1;;foo.txt"), row("")}, + expected: true, + }, + { + // A renderer announces the protocol with a record that names no line; a + // rendering with nothing but that one says nothing about its rows. + name: "the version-only handshake record doesn't count", + contents: []gocui.DiffLineContent{row("foo.txt", "1"), row("one")}, + expected: false, + }, + { + name: "records of a version we don't understand don't count", + contents: []gocui.DiffLineContent{row("one", "2;c;1;;foo.txt")}, + expected: false, + }, + { + name: "an empty rendering states nothing", + contents: nil, + expected: false, + }, + } + + for _, s := range scenarios { + t.Run(s.name, func(t *testing.T) { + assert.Equal(t, s.expected, renderingStatesDiffLines(s.contents)) + }) + } +}