diff --git a/pkg/gocui/escape.go b/pkg/gocui/escape.go index 7f3de9e6e..16b8f1cc7 100644 --- a/pkg/gocui/escape.go +++ b/pkg/gocui/escape.go @@ -20,6 +20,25 @@ type escapeInterpreter struct { instruction instruction hyperlink strings.Builder + // the digits of the OSC number seen so far, while we don't yet know which + // OSC this is + oscNumber strings.Builder + // the payload of an OSC 1717 sequence, in which a diff renderer states + // which line of which file it is about to render; accumulated like + // hyperlink, and attached to the cells that follow it + metadata strings.Builder + // whether the payload currently in metadata has reached a cell, so that one + // that never does can be recognized and kept as an orphan + metadataConsumed bool + // OSC 1717 payloads that no cell took, because the next record followed + // with nothing rendered in between. A renderer emits records back to back + // wherever two diff lines share a rendered line — the deletion and the + // addition of a modification collapsed into one column, or a banner + // announcing a file and its first hunk at once. The write loop gives these + // cells of their own, so that a line keeps every record it was given rather + // than only the last. + orphanedMetadata []string + // ConPTY emits cursor-positioning escapes (CUP) to skip over blank // rows rather than emitting LFs for them. To convert those into row // advances the view can act on, we track where in the pseudo-terminal @@ -82,9 +101,9 @@ const ( stateParams stateCSIDiscard stateOSC - stateOSCWaitForParams stateOSCParams stateOSCHyperlink + stateOSCMetadata stateOSCEndEscape stateOSCSkipUnknown @@ -427,27 +446,42 @@ func (ei *escapeInterpreter) parseOne(ch []byte) (isEscape bool, err error) { } return true, nil case stateOSC: - if characterEquals(ch, '8') { - ei.state = stateOSCWaitForParams - ei.hyperlink.Reset() + // Accumulate the OSC number until the ';' that terminates it, then + // dispatch on the whole number rather than on a single digit. + switch { + case len(ch) == 1 && ch[0] >= '0' && ch[0] <= '9': + ei.oscNumber.WriteByte(ch[0]) + return true, nil + case characterEquals(ch, ';'): + switch ei.oscNumber.String() { + case "8": + ei.hyperlink.Reset() + ei.state = stateOSCParams + case "1717": + ei.orphanUnconsumedMetadata() + ei.state = stateOSCMetadata + default: + ei.state = stateOSCSkipUnknown + } + ei.oscNumber.Reset() + return true, nil + default: + // Not an OSC we understand — it has no number, or a character + // follows the number where the ';' should be. Rather than + // erroring, which would reset state mid-OSC and leak the rest of + // the sequence into the view as literal text, skip to its + // terminator, which this character may already be. + ei.oscNumber.Reset() + switch { + case characterEquals(ch, 0x07): + ei.state = stateNone + case characterEquals(ch, 0x1b): + ei.state = stateOSCEndEscape + default: + ei.state = stateOSCSkipUnknown + } return true, nil } - - ei.state = stateOSCSkipUnknown - return true, nil - case stateOSCWaitForParams: - if !characterEquals(ch, ';') { - // Malformed OSC 8 (expected ';' after '8'). Rather than - // erroring — which would reset state mid-OSC and cause the - // rest of the sequence to leak as literal text — treat the - // whole OSC as one we don't understand and skip to its - // terminator. - ei.state = stateOSCSkipUnknown - return true, nil - } - - ei.state = stateOSCParams - return true, nil case stateOSCParams: if characterEquals(ch, ';') { ei.state = stateOSCHyperlink @@ -463,6 +497,18 @@ func (ei *escapeInterpreter) parseOne(ch []byte) (isEscape bool, err error) { ei.hyperlink.Write(ch) } return true, nil + case stateOSCMetadata: + switch { + case characterEquals(ch, 0x07): + ei.dropMetadataIfHandshake() + ei.state = stateNone + case characterEquals(ch, 0x1b): + ei.dropMetadataIfHandshake() + ei.state = stateOSCEndEscape + default: + ei.metadata.Write(ch) + } + return true, nil case stateOSCEndEscape: ei.state = stateNone return true, nil @@ -478,6 +524,37 @@ func (ei *escapeInterpreter) parseOne(ch []byte) (isEscape bool, err error) { return false, nil } +// orphanUnconsumedMetadata clears the metadata accumulator for a new OSC 1717 +// record, keeping the payload it held as an orphan if no cell took it (see +// orphanedMetadata). +func (ei *escapeInterpreter) orphanUnconsumedMetadata() { + if ei.metadata.Len() > 0 && !ei.metadataConsumed { + ei.orphanedMetadata = append(ei.orphanedMetadata, ei.metadata.String()) + } + ei.metadata.Reset() + ei.metadataConsumed = false +} + +// takeOrphanedMetadata hands the accumulated orphaned payloads to the caller and +// clears the list. +func (ei *escapeInterpreter) takeOrphanedMetadata() []string { + result := ei.orphanedMetadata + ei.orphanedMetadata = nil + return result +} + +// dropMetadataIfHandshake discards a just-completed OSC 1717 payload that +// carries nothing beyond the version. A diff renderer emits such a record ahead +// of everything else to announce that it speaks the protocol, so that a host can +// find that out by asking rather than by inspecting a rendering. It says nothing +// about any line, so it must not attach to the line that follows it; a per-line +// record always has fields, and is kept. +func (ei *escapeInterpreter) dropMetadataIfHandshake() { + if !strings.Contains(ei.metadata.String(), ";") { + ei.metadata.Reset() + } +} + func (ei *escapeInterpreter) outputCSI() error { n := len(ei.csiParam) for i := 0; i < n; { diff --git a/pkg/gocui/escape_test.go b/pkg/gocui/escape_test.go index 39ccbe908..0efef2a77 100644 --- a/pkg/gocui/escape_test.go +++ b/pkg/gocui/escape_test.go @@ -167,6 +167,7 @@ func TestParseOneIgnoresUnknownSequences(t *testing.T) { "\x1b[0 q", // intermediate byte after a param "\x1b[1;;m", // malformed SGR: empty middle param "\x1b]8bogus\x07", // OSC 8 missing ';' + "\x1b]1337;File=inline=1\x07", // OSC with a number we don't implement "\x1b[" + strings.Repeat("0", 300) + "m", // single param overflows length cap "\x1b[" + strings.Repeat("1;", 25) + "1m", // too many params } diff --git a/pkg/gocui/view.go b/pkg/gocui/view.go index 2961447f6..42a087dce 100644 --- a/pkg/gocui/view.go +++ b/pkg/gocui/view.go @@ -727,6 +727,9 @@ type cell struct { width int // number of terminal cells occupied by chr (always 1 or 2) bgColor, fgColor Attribute hyperlink string + // the OSC 1717 payload in effect when the cell was written, i.e. what the + // diff renderer said about the diff line this cell is part of + metadata string } type cells []cell @@ -1124,6 +1127,19 @@ func (b *viewBuffer) write(v *View, p []byte) { finishLine := func() { b.autoRenderHyperlinksInCurrentLine(v) + // A record that reached the line's end without covering a cell still + // belongs to the line: an orphan (see escapeInterpreter.orphanedMetadata), + // or the record of a changed line that is empty, which a renderer emits + // with nothing but the newline after it. Give each a cell of its own, so + // that the line is still recognizable as the diff line it renders rather + // than as nothing at all. + for _, payload := range b.ei.takeOrphanedMetadata() { + b.writeCells([]cell{{metadata: payload}}) + } + if b.ei.metadata.Len() > 0 && !b.ei.metadataConsumed { + b.writeCells([]cell{{metadata: b.ei.metadata.String()}}) + b.ei.metadataConsumed = true + } } advanceToNextLine := func() { @@ -1131,6 +1147,10 @@ func (b *viewBuffer) write(v *View, p []byte) { if b.wy >= len(b.lines) { b.lines = append(b.lines, lineType{}) } + // An OSC 1717 record describes the line it precedes and is never + // closed, so it stops applying at the line's end; a renderer emits a + // fresh one for each line it has something to say about. + b.ei.metadata.Reset() } if b.pendingNewline { @@ -1279,6 +1299,15 @@ func (b *viewBuffer) parseInput(v *View, ch []byte, width int, x int, _ int) (bo truncateLine := false isEscape, err := b.ei.parseOne(ch) + + // A record that the next one superseded before any cell took it still + // belongs to this line (see escapeInterpreter.orphanedMetadata); give each + // a cell of its own, in the order they were emitted, ahead of whatever this + // character produces. + for _, payload := range b.ei.takeOrphanedMetadata() { + cells = append(cells, cell{metadata: payload}) + } + if err != nil { characters := b.ei.characters() for _, chr := range characters { @@ -1307,7 +1336,7 @@ func (b *viewBuffer) parseInput(v *View, ch []byte, width int, x int, _ int) (bo fg: b.ei.curFgColor, bg: b.ei.curBgColor, } - return truncateLine, []cell{} + return truncateLine, cells } else if cf, ok := b.ei.instruction.(cursorForward); ok { // emit `n` space cells under the parser-tracked SGR — used // to materialize ConPTY's compressed runs of spaces (which @@ -1318,8 +1347,12 @@ func (b *viewBuffer) parseInput(v *View, ch []byte, width int, x int, _ int) (bo width = 1 b.noteAsWritten(bytes.Repeat(ch, repeatCount)) } else if isEscape { - // do not output anything - return truncateLine, nil + // the escape itself outputs nothing, but any cells carrying an + // orphaned record still need writing + if len(cells) == 0 { + return truncateLine, nil + } + return truncateLine, cells } else if characterEquals(ch, '\t') { // The cells hold a tab as the spaces it fills; the text as written // keeps the tab itself. @@ -1340,9 +1373,13 @@ func (b *viewBuffer) parseInput(v *View, ch []byte, width int, x int, _ int) (bo fgColor: b.ei.curFgColor, bgColor: b.ei.curBgColor, hyperlink: b.ei.hyperlink.String(), + metadata: b.ei.metadata.String(), chr: string(ch), width: width, } + if c.metadata != "" { + b.ei.metadataConsumed = true + } for range repeatCount { cells = append(cells, c) } @@ -1918,22 +1955,46 @@ 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 { +// DiffLineContent holds what one line of a rendered diff offers to a reader trying +// to recover which line of which file it came from: the line's text, which can be +// parsed as a unified diff when the rendering preserves one, and the OSC 1717 +// records a diff renderer attached to it, which state the answer outright. +type DiffLineContent struct { + // The line's text as its writer wrote it, escape sequences left out, where + // BufferLines gives the text as the cells spell it. 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. + // git terminates a path containing a space with a tab in a diff header, and a + // parser of the diff has to see the tab. + Text string + // The distinct OSC 1717 payloads carried by the line's cells, in + // left-to-right order. A single-column rendering tags every cell of a line + // with the same payload, so there is one; a side-by-side rendering tags + // each side separately, so a line showing a deletion beside the addition + // that replaces it carries both. + Metadata []string +} + +// DiffLineContents returns the per-line material a diff-line reader works from +// (see DiffLineContent), indexed by unwrapped buffer line. Text and records are +// snapshotted in a single locked pass, so they stay consistent with each other +// and with the buffer they came from even while a re-render rebuilds it. +func (v *View) DiffLineContents() []DiffLineContent { v.writeMutex.Lock() defer v.writeMutex.Unlock() - lines := make([]string, len(v.buf.lines)) + contents := make([]DiffLineContent, len(v.buf.lines)) for i := range v.buf.lines { - lines[i] = v.buf.lines[i].textAsWritten() + line := &v.buf.lines[i] + var metadata []string + for _, c := range line.cells { + if c.metadata != "" && !slices.Contains(metadata, c.metadata) { + metadata = append(metadata, c.metadata) + } + } + contents[i] = DiffLineContent{Text: line.textAsWritten(), Metadata: metadata} } - return lines + return contents } // BufferLineForViewLine maps a view line index (which counts wrapped lines) to diff --git a/pkg/gocui/view_test.go b/pkg/gocui/view_test.go index 57d18abde..e3ac6c49e 100644 --- a/pkg/gocui/view_test.go +++ b/pkg/gocui/view_test.go @@ -124,7 +124,12 @@ func TestOverwriteLinesAfterContentEndingInANewline(t *testing.T) { assert.Equal(t, []string{"x", "b"}, v.BufferLines()) } -func TestLinesAsWritten(t *testing.T) { +// diffLineTexts returns the Text of each of the given contents. +func diffLineTexts(contents []DiffLineContent) []string { + return lo.Map(contents, func(c DiffLineContent, _ int) string { return c.Text }) +} + +func TestDiffLineContentsTextIsTheTextAsWritten(t *testing.T) { tests := []struct { name string stringsToWrite []string @@ -184,12 +189,12 @@ func TestLinesAsWritten(t *testing.T) { v.writeString(s) } assert.Equal(t, test.expectedShown, v.BufferLines()) - assert.Equal(t, test.expectedAsWritten, v.LinesAsWritten()) + assert.Equal(t, test.expectedAsWritten, diffLineTexts(v.DiffLineContents())) }) } } -func TestLinesAsWrittenOfAnOverwrittenLine(t *testing.T) { +func TestDiffLineContentsTextOfAnOverwrittenLine(t *testing.T) { v := NewView("name", 0, 0, 20, 10, OutputNormal) v.writeString("a\tb\nc\td") @@ -198,7 +203,7 @@ func TestLinesAsWrittenOfAnOverwrittenLine(t *testing.T) { v.OverwriteLines(0, "xy") assert.Equal(t, []string{"xy", "c d"}, v.BufferLines()) - assert.Equal(t, []string{"xy", "c\td"}, v.LinesAsWritten()) + assert.Equal(t, []string{"xy", "c\td"}, diffLineTexts(v.DiffLineContents())) } func TestUpdatedCursorAndOrigin(t *testing.T) { @@ -246,6 +251,107 @@ func TestAutoRenderingHyperlinks(t *testing.T) { assert.Equal(t, "https://example.com", v.buf.lines[0].cells[0].hyperlink) } +// osc1717 wraps an OSC 1717 payload in the sequence a diff renderer emits it in: +// the ESC ] introducer with the OSC number, and ESC \ as the terminator. +func osc1717(payload string) string { + return "\x1b]1717;" + payload + "\x1b\\" +} + +func TestDiffLineContents(t *testing.T) { + v := NewView("name", 0, 0, 80, 10, OutputNormal) + + // A diff renderer prefixes each line it renders with a record naming the + // file and the line's position in it: version;type;new-line;old-line;file. + v.writeString(strings.Join([]string{ + osc1717("1;c;1;;foo.txt") + "line1", + osc1717("1;d;2;2;foo.txt") + "old2", + osc1717("1;a;2;;foo.txt") + "new2", + "@@ a hunk header, which carries no record @@", + }, "\n")) + + assert.Equal(t, []DiffLineContent{ + {Text: "line1", Metadata: []string{"1;c;1;;foo.txt"}}, + {Text: "old2", Metadata: []string{"1;d;2;2;foo.txt"}}, + {Text: "new2", Metadata: []string{"1;a;2;;foo.txt"}}, + // The record of the line before doesn't bleed onto this one. + {Text: "@@ a hunk header, which carries no record @@"}, + }, v.DiffLineContents()) +} + +func TestDiffLineContentsWithSideBySideRecords(t *testing.T) { + v := NewView("name", 0, 0, 80, 10, OutputNormal) + + // A side-by-side renderer puts two diff lines on one rendered line, and so + // emits a record before each half. + v.writeString(strings.Join([]string{ + osc1717("1;c;1;;foo.txt") + "context " + osc1717("1;c;1;;foo.txt") + "context", + osc1717("1;d;2;2;foo.txt") + "old2 " + osc1717("1;a;2;;foo.txt") + "new2", + }, "\n")) + + assert.Equal(t, []DiffLineContent{ + // The two halves of a context line are the same diff line, stated twice. + {Text: "context context", Metadata: []string{"1;c;1;;foo.txt"}}, + {Text: "old2 new2", Metadata: []string{"1;d;2;2;foo.txt", "1;a;2;;foo.txt"}}, + }, v.DiffLineContents()) +} + +func TestDiffLineContentsOfWrappedLine(t *testing.T) { + v := NewView("name", 0, 0, 10, 10, OutputNormal) // InnerWidth is 9 + v.Wrap = true + + // A line that gocui wraps is still one buffer line, so its record covers + // every view line it is displayed on. + v.writeString(osc1717("1;a;1;;foo.txt") + "a line too long to fit") + + assert.Equal(t, []DiffLineContent{ + {Text: "a line too long to fit", Metadata: []string{"1;a;1;;foo.txt"}}, + }, v.DiffLineContents()) + assert.Equal(t, 3, v.ViewLinesHeight()) + for viewLine := range 3 { + bufferLine, ok := v.BufferLineForViewLine(viewLine) + assert.True(t, ok) + assert.Equal(t, 0, bufferLine) + } +} + +func TestDiffLineContentsWithRecordsCoveringNoCell(t *testing.T) { + v := NewView("name", 0, 0, 80, 10, OutputNormal) + + v.writeString(strings.Join([]string{ + // A banner announcing a file and its first hunk at once carries both + // records back to back. + osc1717("1;f;;;foo.txt") + osc1717("1;h;5;;foo.txt") + "foo.txt --- Go", + // So does a modification whose deletion and addition are collapsed into + // a single rendered line. + osc1717("1;d;5;5;foo.txt") + osc1717("1;a;5;;foo.txt") + "595 new content", + // A changed line that is empty is rendered as its record and nothing else. + osc1717("1;a;6;;foo.txt"), + }, "\n") + "\n") + + assert.Equal(t, []DiffLineContent{ + {Text: "foo.txt --- Go", Metadata: []string{"1;f;;;foo.txt", "1;h;5;;foo.txt"}}, + {Text: "595 new content", Metadata: []string{"1;d;5;5;foo.txt", "1;a;5;;foo.txt"}}, + {Text: "", Metadata: []string{"1;a;6;;foo.txt"}}, + }, v.DiffLineContents()) +} + +func TestDiffLineContentsSwallowsHandshake(t *testing.T) { + v := NewView("name", 0, 0, 80, 10, OutputNormal) + + // A diff renderer announces itself with a version-only record before the + // diff. It must leave no trace: no visible bytes, no line of its own, and + // above all no record on the line that follows it. + v.writeString(osc1717("1") + strings.Join([]string{ + "diff --git a/foo.txt b/foo.txt", + osc1717("1;a;1;;foo.txt") + "added", + }, "\n")) + + assert.Equal(t, []DiffLineContent{ + {Text: "diff --git a/foo.txt b/foo.txt"}, + {Text: "added", Metadata: []string{"1;a;1;;foo.txt"}}, + }, v.DiffLineContents()) +} + // An async re-render builds into an off-screen buffer and swaps it in once it // has enough to paint, so readers keep seeing the previous render — coherent and // consistent — until the new content appears in one step. See View.offscreen. diff --git a/pkg/gui/controllers/helpers/diff_line_helper.go b/pkg/gui/controllers/helpers/diff_line_helper.go index de4a60960..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,21 +39,42 @@ func (self *DiffLineHelper) GetDiffLineInfo(view *gocui.View, viewLineIdx int) ( 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) + 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.RelPath), + 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 d58633f7e..dd7270af2 100644 --- a/pkg/gui/controllers/helpers/diff_line_parser.go +++ b/pkg/gui/controllers/helpers/diff_line_parser.go @@ -2,10 +2,12 @@ package helpers import ( "regexp" + "slices" "strconv" "strings" "github.com/jesseduffield/lazygit/pkg/commands/patch" + "github.com/jesseduffield/lazygit/pkg/gocui" "github.com/jesseduffield/lazygit/pkg/gui/types" ) @@ -31,10 +33,10 @@ 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. +// Path 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 + Path string Type types.DiffLineType NewLine int OldLine int @@ -92,6 +94,37 @@ func parseAllDiffLinesFromBuffer(bufferLines []string) []bufferLineParse { return result } +// diffLineTexts extracts the text of each rendered row — the material the buffer +// parser works on. +func diffLineTexts(contents []gocui.DiffLineContent) []string { + texts := make([]string, len(contents)) + for i, content := range contents { + texts[i] = content.Text + } + 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 @@ -204,7 +237,7 @@ func parseFileSection(fileLines []string, endsTheBuffer bool) []bufferLineParse break } parsed := parsedDiffLine{ - RelPath: relPath, + Path: relPath, Type: diffLineTypeForKind(patchLines[i].Kind), NewLine: p.LineNumberOfLine(i), } @@ -302,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 ae620d23c..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" ) @@ -40,16 +41,16 @@ func TestParseDiffLineFromBuffer(t *testing.T) { 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}, + {"file header", 0, parsedDiffLine{Path: "file1.go", Type: types.DiffLineFileHeader, NewLine: 1}, true}, + {"hunk header", 4, parsedDiffLine{Path: "file1.go", Type: types.DiffLineHunkHeader, NewLine: 1}, true}, + {"context line", 5, parsedDiffLine{Path: "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}, + {"first deletion", 6, parsedDiffLine{Path: "file1.go", Type: types.DiffLineDeleted, NewLine: 2, OldLine: 2}, true}, + {"second deletion", 7, parsedDiffLine{Path: "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}, + {"first addition", 15, parsedDiffLine{Path: "dir/file2.go", Type: types.DiffLineAdded, NewLine: 10}, true}, + {"second addition", 16, parsedDiffLine{Path: "dir/file2.go", Type: types.DiffLineAdded, NewLine: 11}, true}, {"out of range", 999, parsedDiffLine{}, false}, } @@ -75,7 +76,7 @@ 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) + assert.Equal(t, parsedDiffLine{Path: "new.go", Type: types.DiffLineFileHeader, NewLine: 1}, result) renameWithModification := strings.Split(`diff --git a/old.go b/new.go similarity index 62% @@ -91,7 +92,7 @@ index 1111111..2222222 100644 result, ok = parseDiffLineFromBuffer(renameWithModification, 10) assert.True(t, ok) - assert.Equal(t, parsedDiffLine{RelPath: "new.go", Type: types.DiffLineAdded, NewLine: 2}, result) + assert.Equal(t, parsedDiffLine{Path: "new.go", Type: types.DiffLineAdded, NewLine: 2}, result) } func TestParseDiffLineFromBufferDeletedFile(t *testing.T) { @@ -107,7 +108,7 @@ index 1111111..0000000 result, ok := parseDiffLineFromBuffer(deletedFile, 7) assert.True(t, ok) - assert.Equal(t, parsedDiffLine{RelPath: "gone.go", Type: types.DiffLineDeleted, NewLine: 0, OldLine: 2}, result) + assert.Equal(t, parsedDiffLine{Path: "gone.go", Type: types.DiffLineDeleted, NewLine: 0, OldLine: 2}, result) } func TestParseDiffLineFromBufferSubmodule(t *testing.T) { @@ -129,13 +130,13 @@ Submodule modules/xyz a32f27c..2d9f921: result, ok := parseDiffLineFromBuffer(withSubmodule, targetIdx) assert.True(t, ok) assert.Equal(t, - parsedDiffLine{RelPath: "modules/xyz", Type: types.DiffLineFileHeader, NewLine: 1}, + parsedDiffLine{Path: "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) + assert.Equal(t, parsedDiffLine{Path: "file.txt", Type: types.DiffLineAdded, NewLine: 3}, result) } func TestParseDiffLineFromBufferSubmoduleInARendering(t *testing.T) { @@ -153,8 +154,8 @@ products/a.txt two`, "\n") all := parseAllDiffLinesFromBuffer(rendered) - assert.Equal(t, "modules/xyz", all[0].parsed.RelPath) - assert.Equal(t, "modules/xyz", all[1].parsed.RelPath) + assert.Equal(t, "modules/xyz", all[0].parsed.Path) + assert.Equal(t, "modules/xyz", all[1].parsed.Path) for i := 2; i < len(rendered); i++ { assert.False(t, all[i].ok, "line %d: %q", i, rendered[i]) } @@ -230,7 +231,7 @@ func TestParseDiffLineFromBufferReadInPart(t *testing.T) { 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) + assert.Equal(t, parsedDiffLine{Path: "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 @@ -287,13 +288,13 @@ func TestParseDiffLineFromBufferQuotedPath(t *testing.T) { result, ok := parseDiffLineFromBuffer(renamed, 10) assert.True(t, ok) - assert.Equal(t, parsedDiffLine{RelPath: "café new.go", Type: types.DiffLineAdded, NewLine: 2}, result) + assert.Equal(t, parsedDiffLine{Path: "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) + assert.Equal(t, parsedDiffLine{Path: "café new.go", Type: types.DiffLineFileHeader, NewLine: 1}, result) } func TestParseAllDiffLinesFromBuffer(t *testing.T) { @@ -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)) + }) + } +} diff --git a/pkg/gui/main_view_render.go b/pkg/gui/main_view_render.go index fdaf73798..7b9b75c23 100644 --- a/pkg/gui/main_view_render.go +++ b/pkg/gui/main_view_render.go @@ -36,6 +36,16 @@ type renderSpec struct { // user has configured. The renderer lays its rendering out to the width of the // view, which only the layout settles, so the task is created after it. func (gui *Gui) newRenderTask(view *gocui.View, cmd *exec.Cmd, prefix string) error { + // Ask whatever renders the diff to state, in an OSC 1717 record per line, + // which line of which file it is rendering. This lets us act on the line the + // user is pointing at even when the rendering no longer looks like a diff. + // The variable names the protocol versions we understand, and a renderer + // that doesn't understand it ignores it, so we can set it always. It has to + // be set before the plain path below, since on that path git renders the + // diff itself, and git speaks the protocol too, for its word-diff formats, + // whose markup we could not otherwise resolve. + cmd.Env = append(cmd.Env, "OSC1717=V1") + if gui.stateAccessor.GetDiffRendererConfigManager().GetDiffRendererType() == config.DiffRendererType_RawGit { // If we're not using a custom diff renderer, then we don't need to use a pty return gui.newCmdTask(view, cmd, prefix) diff --git a/pkg/integration/tests/diff/diff_renderer_metadata.go b/pkg/integration/tests/diff/diff_renderer_metadata.go new file mode 100644 index 000000000..ff211717d --- /dev/null +++ b/pkg/integration/tests/diff/diff_renderer_metadata.go @@ -0,0 +1,50 @@ +package diff + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var DiffRendererMetadata = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "A diff renderer is told that we understand the OSC 1717 metadata protocol, and the records it emits don't show up in the rendered diff", + ExtraCmdArgs: []string{}, + Skip: false, + SetupConfig: func(cfg *config.AppConfig) { + // A fake conforming renderer: it announces the protocol with a + // version-only record, reports the protocol versions it was offered, and + // then passes the diff through with a per-line record in front of every + // line. + cfg.GetUserConfig().Git.DiffRenderers = []config.DiffRendererConfig{ + {Command: `printf '\033]1717;1\007'; ` + + `printf 'OFFERED:%s\n' "$OSC1717"; ` + + `while IFS= read -r line; do printf '\033]1717;1;c;1;;file1\007%s\n' "$line"; done`}, + } + }, + SetupRepo: func(shell *Shell) { + shell.CreateFileAndAdd("file1", "one\ntwo\n") + shell.Commit("one") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Commits(). + Focus(). + Lines( + Contains("one").IsSelected(), + ) + + t.Views().Main(). + // The renderer was offered the protocol version we understand. + Content(Contains("OFFERED:V1")). + // Its records are escape sequences, so none of them reaches the + // screen; the diff reads exactly as the renderer wrote it. + ContainsLines( + Equals("diff --git a/file1 b/file1"), + Contains("new file mode"), + Contains("index "), + Equals("--- /dev/null"), + Equals("+++ b/file1"), + Equals("@@ -0,0 +1,2 @@"), + Equals("+one"), + Equals("+two"), + ) + }, +}) diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go index 67caf3bbb..d699d8cb6 100644 --- a/pkg/integration/tests/test_list.go +++ b/pkg/integration/tests/test_list.go @@ -228,6 +228,7 @@ var tests = []*components.IntegrationTest{ diff.DiffAndApplyPatch, diff.DiffCommits, diff.DiffNonStickyRange, + diff.DiffRendererMetadata, diff.IgnoreWhitespace, diff.RenameSimilarityThresholdChange, diff.RenderThroughAPipe,