From cf8e5fd27ef28dbb729b3973476004b02f0ab5f8 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 6 Jun 2026 15:38:43 +0200 Subject: [PATCH] Recover diff-line identity by parsing the buffer, behind a swappable seam MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The focused main view's click/enter/e/G handlers all need the same thing: given a rendered diff row, the patch-space line it corresponds to. Until now that came solely from delta's lazygit-edit:// hyperlinks, which only carry a path and a single line number — no side. That's lossy: for a deletion the number is the old line, but the consumers fed it into new-file lookups, and two consecutive deletions (which share a new-file line number) couldn't be told apart at all. Replace GetFileAndLineForClickedDiffLine with GetDiffLineInfo, returning the fuller (file, type, new-line, old-line) record from diff-line-metadata-notes.md. This is mechanism #1: parse the decolorized view buffer — walk up to the file's "diff --git" section, reuse patch.Parse on it (splitting multi-file commit diffs on the "diff --git" boundaries), and read the type and line numbers off the patch arithmetic. It serves the structure-preserving renderings — no pager, git diff --color, and delta --color-only without line numbers — with no external dependency. To avoid trusting a mis-parse, the parser bails when a hunk's body no longer matches its header (Patch.IsWellFormed). That's what happens when a pager keeps the diff/hunk headers but restructures the body: delta's line-number gutters push the +/- marker off the start of each line, so every body line reads as context. Such renderings fall through to the next backend rather than yielding a confident wrong answer. (diff-so-fancy goes further and rewrites the headers too, so it fails even earlier, on the missing "diff --git".) GetDiffLineInfo is a seam with swappable backends: the buffer parser first, then the old hyperlink reader as a fallback for renderings the parser can't handle (delta's default mode, or delta with line-number gutters). The future #2 OSC per-cell metadata reader plugs in ahead of both, behind the same record shape. Wire the consumers to the record per that doc's field mapping: - dive into staging/patch building lands on the exact patch line, looking a deletion up by its old-file line number (PatchLineForOldLineNumber) so the two-deletions case resolves correctly; - `e` edits at the new-file line; - `G` anchors the PR link on the left (old) side for a deletion, the right (new) side otherwise. The hyperlink fallback can't convey the side, so it reports DiffLineOther, which the consumers treat as a non-deletion — i.e. exactly today's behavior. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/commands/patch/hunk.go | 5 + pkg/commands/patch/parse.go | 37 +++-- pkg/commands/patch/patch.go | 17 ++ pkg/commands/patch/patch_test.go | 24 +++ .../controllers/commits_files_controller.go | 12 +- pkg/gui/controllers/files_controller.go | 12 +- .../controllers/helpers/diff_line_parser.go | 154 ++++++++++++++++++ .../helpers/diff_line_parser_test.go | 92 +++++++++++ .../helpers/patch_building_helper.go | 2 +- pkg/gui/controllers/helpers/staging_helper.go | 78 +++++++-- pkg/gui/controllers/main_view_controller.go | 24 +-- .../controllers/patch_explorer_controller.go | 14 +- .../switch_to_diff_files_controller.go | 7 +- pkg/gui/patch_exploring/state.go | 18 +- pkg/gui/types/context.go | 10 +- pkg/gui/types/diff_line_info.go | 56 +++++++ 16 files changed, 502 insertions(+), 60 deletions(-) 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/commands/patch/hunk.go b/pkg/commands/patch/hunk.go index 6d0177d05..955d6b89f 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 hunk header ('2' and '3' in the above example), + // kept so we can verify the parsed body actually matches the header (see + // Patch.IsWellFormed). Only populated 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..75dca8ed4 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: old start, old length (omitted when 1), new start, new +// length (omitted when 1), 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 hunk header length capture, 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 d656bc543..40d7991c0 100644 --- a/pkg/commands/patch/patch.go +++ b/pkg/commands/patch/patch.go @@ -79,6 +79,23 @@ 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 — e.g. delta with line-number gutters +// 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. This lets a +// parser tell a real unified diff from a mangled one and fall back rather than +// trust a mis-parse. Only meaningful for patches produced by Parse (the lengths +// are read from the header there). +func (self *Patch) IsWellFormed() bool { + for _, hunk := range self.hunks { + 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 9b7683123..dc2237fad 100644 --- a/pkg/commands/patch/patch_test.go +++ b/pkg/commands/patch/patch_test.go @@ -761,6 +761,30 @@ func TestPatchLineForOldLineNumber(t *testing.T) { } } +func TestIsWellFormed(t *testing.T) { + assert.True(t, Parse(simpleDiff).IsWellFormed()) + assert.True(t, Parse(twoHunks).IsWellFormed()) + assert.True(t, Parse(consecutiveDeletions).IsWellFormed()) + assert.True(t, Parse(newFile).IsWellFormed()) + assert.True(t, Parse(deletedFile).IsWellFormed()) + assert.True(t, Parse(addNewlineToEndOfFile).IsWellFormed()) + + // A diff whose body has been mangled so the +/- markers no longer start each + // line (as delta's line-number gutters do) reads as all-context, so the body + // lengths no longer match the header. + gutterMangled := `diff --git a/filename b/filename +index 9320895..6d79956 100644 +--- a/filename ++++ b/filename +@@ -1,4 +1,2 @@ + apple + grape + pear + lemon +` + assert.False(t, Parse(gutterMangled).IsWellFormed()) +} + func TestGetNextStageableLineIndex(t *testing.T) { type scenario struct { testName string diff --git a/pkg/gui/controllers/commits_files_controller.go b/pkg/gui/controllers/commits_files_controller.go index e42aa6e21..4d29aa171 100644 --- a/pkg/gui/controllers/commits_files_controller.go +++ b/pkg/gui/controllers/commits_files_controller.go @@ -551,9 +551,11 @@ func (self *CommitFilesController) GetOnClickFocusedMainView() func(mainViewName // Capture before any mutation below that might re-render the main view. snapshot := focusedMainViewSnapshot(self.c, mainViewName, self.context(), clickedLineIdx) - clickedFile, line, ok := self.c.Helpers().Staging.GetFileAndLineForClickedDiffLine(mainViewName, clickedLineIdx) - if !ok { - line = -1 + info, ok := self.c.Helpers().Staging.GetDiffLineInfo(mainViewName, clickedLineIdx) + line := -1 + isDeletion := false + if ok { + line, isDeletion = info.PatchSelectLine() } node := self.getSelectedItem() @@ -562,7 +564,7 @@ func (self *CommitFilesController) GetOnClickFocusedMainView() func(mainViewName } if !node.IsFile() && ok { - relativePath, err := filepath.Rel(self.c.Git().RepoPaths.WorktreePath(), clickedFile) + relativePath, err := filepath.Rel(self.c.Git().RepoPaths.WorktreePath(), info.Path) if err != nil { return err } @@ -580,7 +582,7 @@ func (self *CommitFilesController) GetOnClickFocusedMainView() func(mainViewName } // Entered from the focused main view, so escaping returns there. - return self.c.Helpers().CommitFiles.EnterCommitFile(node, snapshot, types.OnFocusOpts{ClickedWindowName: "main", ClickedViewLineIdx: line, ClickedViewRealLineIdx: line, SelectLineInDefaultMode: true}) + return self.c.Helpers().CommitFiles.EnterCommitFile(node, snapshot, types.OnFocusOpts{ClickedWindowName: "main", ClickedViewLineIdx: line, ClickedViewRealLineIdx: line, ClickedViewRealLineIsDeletion: isDeletion, SelectLineInDefaultMode: true}) } } diff --git a/pkg/gui/controllers/files_controller.go b/pkg/gui/controllers/files_controller.go index d3dc4a304..fc378eebd 100644 --- a/pkg/gui/controllers/files_controller.go +++ b/pkg/gui/controllers/files_controller.go @@ -412,9 +412,11 @@ func (self *FilesController) GetOnClickFocusedMainView() func(mainViewName strin // Capture before any mutation below that might re-render the main view. snapshot := focusedMainViewSnapshot(self.c, mainViewName, self.context(), clickedLineIdx) - clickedFile, line, ok := self.c.Helpers().Staging.GetFileAndLineForClickedDiffLine(mainViewName, clickedLineIdx) - if !ok { - line = -1 + info, ok := self.c.Helpers().Staging.GetDiffLineInfo(mainViewName, clickedLineIdx) + line := -1 + isDeletion := false + if ok { + line, isDeletion = info.PatchSelectLine() } node := self.context().GetSelected() @@ -423,7 +425,7 @@ func (self *FilesController) GetOnClickFocusedMainView() func(mainViewName strin } if !node.IsFile() && ok { - relativePath, err := filepath.Rel(self.c.Git().RepoPaths.WorktreePath(), clickedFile) + relativePath, err := filepath.Rel(self.c.Git().RepoPaths.WorktreePath(), info.Path) if err != nil { return err } @@ -439,7 +441,7 @@ func (self *FilesController) GetOnClickFocusedMainView() func(mainViewName strin } } - return self.EnterFile(snapshot, types.OnFocusOpts{ClickedWindowName: mainViewName, ClickedViewLineIdx: line, ClickedViewRealLineIdx: line, SelectLineInDefaultMode: true}) + return self.EnterFile(snapshot, types.OnFocusOpts{ClickedWindowName: mainViewName, ClickedViewLineIdx: line, ClickedViewRealLineIdx: line, ClickedViewRealLineIsDeletion: isDeletion, SelectLineInDefaultMode: true}) } } 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..c2af555c9 --- /dev/null +++ b/pkg/gui/controllers/helpers/diff_line_parser.go @@ -0,0 +1,154 @@ +package helpers + +import ( + "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 " + +// parsedDiffLine is what parseDiffLineFromBuffer recovers about a rendered diff +// row. RelPath is repo-relative (as the diff header spells it); the caller turns +// it into the absolute path of types.DiffLineInfo. +type parsedDiffLine struct { + RelPath string + Type types.DiffLineType + NewLine int + OldLine int +} + +// parseDiffLineFromBuffer recovers a rendered diff row's patch-space identity by +// parsing the decolorized diff buffer (mechanism #1 in diff-line-metadata-notes.md). +// +// bufferLines is the full unwrapped view buffer; targetIdx is the buffer line to +// resolve. A commit diff spans multiple files, so we first split on the +// "diff --git" boundaries to isolate the file section containing targetIdx, then +// reuse patch.Parse on that single-file section: its patch line indices line up +// 1:1 with the section's buffer lines, so the type and old/new line numbers fall +// straight out of the patch arithmetic. +// +// ok is false when the buffer isn't a parseable unified diff at targetIdx (e.g. +// a pager that restructures the diff, like delta's default mode), so the caller +// can fall back to another backend. +func parseDiffLineFromBuffer(bufferLines []string, targetIdx int) (parsedDiffLine, bool) { + if targetIdx < 0 || targetIdx >= len(bufferLines) { + return parsedDiffLine{}, false + } + + // Find the file section containing the target: the nearest "diff --git" at or + // above it, up to the next one (or the end of the buffer). + fileStart := -1 + for i := targetIdx; i >= 0; i-- { + if strings.HasPrefix(bufferLines[i], diffFilePrefix) { + fileStart = i + break + } + } + if fileStart == -1 { + return parsedDiffLine{}, false + } + fileEnd := len(bufferLines) + for i := fileStart + 1; i < len(bufferLines); i++ { + if strings.HasPrefix(bufferLines[i], diffFilePrefix) { + fileEnd = i + break + } + } + + fileLines := bufferLines[fileStart:fileEnd] + relPath := pathFromDiffHeader(fileLines) + if relPath == "" { + return parsedDiffLine{}, false + } + + p := patch.Parse(strings.Join(fileLines, "\n")) + // Bail if the body doesn't match the hunk headers: the rendering restructured + // the diff (e.g. delta's line-number gutters push the +/- marker off the + // start of the line, so every body line reads as context), and trusting the + // mis-parse would land us on the wrong line. Better to fall back. + if !p.IsWellFormed() { + return parsedDiffLine{}, false + } + patchLines := p.Lines() + patchLineIdx := targetIdx - fileStart + if patchLineIdx < 0 || patchLineIdx >= len(patchLines) { + return parsedDiffLine{}, false + } + + result := parsedDiffLine{ + RelPath: relPath, + Type: diffLineTypeForKind(patchLines[patchLineIdx].Kind), + NewLine: p.LineNumberOfLine(patchLineIdx), + } + if result.Type == types.DiffLineDeleted { + result.OldLine = p.OldLineNumberOfLine(patchLineIdx) + } + return result, true +} + +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 file's diff section. +// It prefers the "+++ b/" line (falling back to "--- a/" when the +// new path is /dev/null, i.e. a deleted file), and as a last resort the +// "diff --git" line. Paths with characters git C-quotes are not handled (this is +// a prototype); the common unquoted case is. +func pathFromDiffHeader(fileLines []string) string { + 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 git's default a/ or b/ diff path prefix if present. +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, used only when the +++/--- lines 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..b59b3ead7 --- /dev/null +++ b/pkg/gui/controllers/helpers/diff_line_parser_test.go @@ -0,0 +1,92 @@ +package helpers + +import ( + "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 TestParseDiffLineFromBufferNotADiff(t *testing.T) { + // A rendering with no "diff --git" line (e.g. delta's default mode) can't be + // parsed, so the caller falls back to another backend. + bufferLines := []string{"some", "lines", "that", "are not a diff"} + _, ok := parseDiffLineFromBuffer(bufferLines, 2) + assert.False(t, ok) +} + +func TestParseDiffLineFromBufferGutterMangled(t *testing.T) { + // delta with line-number gutters keeps the diff/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 + // (and the caller falls back) 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) +} diff --git a/pkg/gui/controllers/helpers/patch_building_helper.go b/pkg/gui/controllers/helpers/patch_building_helper.go index 55b6dbe1b..25335e2cc 100644 --- a/pkg/gui/controllers/helpers/patch_building_helper.go +++ b/pkg/gui/controllers/helpers/patch_building_helper.go @@ -159,7 +159,7 @@ func (self *PatchBuildingHelper) RefreshPatchBuildingPanel(opts types.OnFocusOpt oldState := context.GetState() - state := patch_exploring.NewState(diff, selectedLineIdx, selectedRealLineIdx, context.GetView(), oldState, self.c.UserConfig().Gui.UseHunkModeInStagingView, opts.SelectLineInDefaultMode) + state := patch_exploring.NewState(diff, selectedLineIdx, selectedRealLineIdx, opts.ClickedViewRealLineIsDeletion, context.GetView(), oldState, self.c.UserConfig().Gui.UseHunkModeInStagingView, opts.SelectLineInDefaultMode) context.SetState(state) if state == nil { self.Escape() diff --git a/pkg/gui/controllers/helpers/staging_helper.go b/pkg/gui/controllers/helpers/staging_helper.go index 16b21e777..9be563056 100644 --- a/pkg/gui/controllers/helpers/staging_helper.go +++ b/pkg/gui/controllers/helpers/staging_helper.go @@ -1,14 +1,18 @@ package helpers import ( + "path/filepath" "regexp" "github.com/jesseduffield/lazygit/pkg/commands/models" + "github.com/jesseduffield/lazygit/pkg/gocui" "github.com/jesseduffield/lazygit/pkg/gui/patch_exploring" "github.com/jesseduffield/lazygit/pkg/gui/types" "github.com/jesseduffield/lazygit/pkg/utils" ) +var lazygitEditURLRegexp = regexp.MustCompile(`^lazygit-edit://(.+?):(\d+)$`) + type StagingHelper struct { c *HelperCommon windowHelper *WindowHelper @@ -74,11 +78,11 @@ func (self *StagingHelper) RefreshStagingPanel(focusOpts types.OnFocusOpts) { hunkMode := self.c.UserConfig().Gui.UseHunkModeInStagingView mainContext.SetState( - patch_exploring.NewState(mainDiff, mainSelectedLineIdx, mainSelectedRealLineIdx, mainContext.GetView(), mainContext.GetState(), hunkMode, focusOpts.SelectLineInDefaultMode), + patch_exploring.NewState(mainDiff, mainSelectedLineIdx, mainSelectedRealLineIdx, focusOpts.ClickedViewRealLineIsDeletion, mainContext.GetView(), mainContext.GetState(), hunkMode, focusOpts.SelectLineInDefaultMode), ) secondaryContext.SetState( - patch_exploring.NewState(secondaryDiff, secondarySelectedLineIdx, secondarySelectedRealLineIdx, secondaryContext.GetView(), secondaryContext.GetState(), hunkMode, focusOpts.SelectLineInDefaultMode), + patch_exploring.NewState(secondaryDiff, secondarySelectedLineIdx, secondarySelectedRealLineIdx, focusOpts.ClickedViewRealLineIsDeletion, secondaryContext.GetView(), secondaryContext.GetState(), hunkMode, focusOpts.SelectLineInDefaultMode), ) mainState := mainContext.GetState() @@ -136,19 +140,67 @@ func (self *StagingHelper) mainStagingFocused() bool { return self.c.Context().CurrentStatic().GetKey() == self.c.Contexts().Staging.GetKey() } -func (self *StagingHelper) GetFileAndLineForClickedDiffLine(windowName string, lineIdx int) (string, int, bool) { +// GetDiffLineInfo recovers the patch-space identity — (file, type, new-line, +// old-line) — of a rendered diff row, given the window showing the diff and the +// (wrapped) view line index. It is the single seam the focused main view and +// patch explorer consumers go through to act on the line the user is pointing +// at, and the strategy behind it is swappable (see diff-line-metadata-notes.md). +// +// Today it first parses the decolorized view buffer (mechanism #1), which serves +// the structure-preserving renderings (no pager, git diff --color, +// delta --color-only, diff-so-fancy --patch). When that fails — e.g. a pager +// restructured the diff, as delta's default mode does — it falls back to delta's +// lazygit-edit:// hyperlinks. The hyperlink can't convey the side, so its result +// is reported as a non-deletion content line. A future backend reading #2's +// per-cell OSC metadata would slot in ahead of these, behind the same shape. +func (self *StagingHelper) GetDiffLineInfo(windowName string, viewLineIdx int) (types.DiffLineInfo, bool) { v, _ := self.c.GocuiGui().View(self.windowHelper.GetViewNameForWindow(windowName)) - hyperlink, ok := v.HyperLinkInLine(lineIdx, "lazygit-edit:") - if !ok { - return "", 0, false + if v == nil { + return types.DiffLineInfo{}, false } - re := regexp.MustCompile(`^lazygit-edit://(.+?):(\d+)$`) - matches := re.FindStringSubmatch(hyperlink) - if matches == nil { - return "", 0, false + if info, ok := self.diffLineInfoFromBuffer(v, viewLineIdx); ok { + return info, true } - filepath := matches[1] - lineNumber := utils.MustConvertToInt(matches[2]) - return filepath, lineNumber, true + return self.diffLineInfoFromHyperlink(v, viewLineIdx) +} + +func (self *StagingHelper) diffLineInfoFromBuffer(v *gocui.View, viewLineIdx int) (types.DiffLineInfo, bool) { + bufferLineIdx, ok := v.BufferLineForViewLine(viewLineIdx) + if !ok { + return types.DiffLineInfo{}, false + } + + parsed, ok := parseDiffLineFromBuffer(v.BufferLines(), bufferLineIdx) + if !ok { + return types.DiffLineInfo{}, false + } + + return types.DiffLineInfo{ + Path: filepath.Join(self.c.Git().RepoPaths.WorktreePath(), parsed.RelPath), + Type: parsed.Type, + NewLine: parsed.NewLine, + OldLine: parsed.OldLine, + }, true +} + +func (self *StagingHelper) diffLineInfoFromHyperlink(v *gocui.View, viewLineIdx int) (types.DiffLineInfo, bool) { + hyperlink, ok := v.HyperLinkInLine(viewLineIdx, "lazygit-edit:") + if !ok { + return types.DiffLineInfo{}, false + } + + matches := lazygitEditURLRegexp.FindStringSubmatch(hyperlink) + if matches == nil { + return types.DiffLineInfo{}, false + } + + return types.DiffLineInfo{ + // delta emits an absolute path here, which is what the consumers want. + Path: matches[1], + // The hyperlink carries no side, so it can't distinguish a deletion from + // an addition or context line; report it as a plain content line. + Type: types.DiffLineOther, + NewLine: utils.MustConvertToInt(matches[2]), + }, true } diff --git a/pkg/gui/controllers/main_view_controller.go b/pkg/gui/controllers/main_view_controller.go index baf107309..a4782203c 100644 --- a/pkg/gui/controllers/main_view_controller.go +++ b/pkg/gui/controllers/main_view_controller.go @@ -228,13 +228,13 @@ func (self *MainViewController) editLine() error { return nil } // Figure out the clicked file and line the same way entering staging does. - path, lineNumber, ok := self.c.Helpers().Staging.GetFileAndLineForClickedDiffLine( + info, ok := self.c.Helpers().Staging.GetDiffLineInfo( self.context.GetViewName(), self.context.GetView().SelectedLineIdx()) if !ok { return nil } - lineNumber = self.c.Helpers().Diff.AdjustLineNumber(path, lineNumber, self.context.GetViewName()) - return self.c.Helpers().Files.EditFileAtLine(path, lineNumber) + lineNumber := self.c.Helpers().Diff.AdjustLineNumber(info.Path, info.NewLine, self.context.GetViewName()) + return self.c.Helpers().Files.EditFileAtLine(info.Path, lineNumber) } func (self *MainViewController) openPullRequestForSelectedLine() error { @@ -273,20 +273,24 @@ func (self *MainViewController) openPullRequestForSelectedLine() error { } // Figure out the clicked file and line the same way entering staging does. - path, lineNumber, ok := self.c.Helpers().Staging.GetFileAndLineForClickedDiffLine( + info, ok := self.c.Helpers().Staging.GetDiffLineInfo( self.context.GetViewName(), self.context.GetView().SelectedLineIdx()) if !ok { return nil } - relativePath, err := filepath.Rel(self.c.Git().RepoPaths.WorktreePath(), path) + relativePath, err := filepath.Rel(self.c.Git().RepoPaths.WorktreePath(), info.Path) if err != nil { return err } + // A deletion isn't on the right (new) side of the diff, so anchor it on the + // left (old) side; everything else on the right. + side, lineNumber := info.PullRequestAnchor() + self.c.LogAction(self.c.Tr.Actions.OpenPullRequest) return self.c.OS().OpenLink( - githubPullRequestLineURL(pr.Url, commitSha, filepath.ToSlash(relativePath), lineNumber)) + githubPullRequestLineURL(pr.Url, commitSha, filepath.ToSlash(relativePath), side, lineNumber)) } // branchForPullRequest returns the local branch whose pull request applies to @@ -318,12 +322,12 @@ func (self *MainViewController) branchForPullRequest(sidePanelContext types.Cont // githubPullRequestLineURL builds a URL that opens the given line of a file in // the diff of a specific commit within a GitHub pull request. The file is -// identified by the SHA-256 of its repo-relative path, and R targets the -// right (new) side of the diff. See +// identified by the SHA-256 of its repo-relative path, and side ("R"/"L") +// selects the right (new) or left (old) side of the diff. See // https://github.com/orgs/community/discussions/55764. -func githubPullRequestLineURL(prURL string, commitSha string, relativePath string, lineNumber int) string { +func githubPullRequestLineURL(prURL string, commitSha string, relativePath string, side string, lineNumber int) string { pathHash := sha256.Sum256([]byte(relativePath)) - anchor := fmt.Sprintf("diff-%sR%d", hex.EncodeToString(pathHash[:]), lineNumber) + anchor := fmt.Sprintf("diff-%s%s%d", hex.EncodeToString(pathHash[:]), side, lineNumber) return fmt.Sprintf("%s/changes/%s#%s", prURL, commitSha, anchor) } diff --git a/pkg/gui/controllers/patch_explorer_controller.go b/pkg/gui/controllers/patch_explorer_controller.go index 1492853cd..2f106d6ce 100644 --- a/pkg/gui/controllers/patch_explorer_controller.go +++ b/pkg/gui/controllers/patch_explorer_controller.go @@ -151,15 +151,17 @@ func (self *PatchExplorerController) GetMouseKeybindings(opts types.KeybindingsO return self.withRenderAndFocus(self.HandleMouseDown)() } - _, line, ok := self.c.Helpers().Staging.GetFileAndLineForClickedDiffLine(self.context.GetWindowName(), opts.Y) - if !ok { - line = -1 + line := -1 + isDeletion := false + if info, ok := self.c.Helpers().Staging.GetDiffLineInfo(self.context.GetWindowName(), opts.Y); ok { + line, isDeletion = info.PatchSelectLine() } self.c.Context().Push(self.context, types.OnFocusOpts{ - ClickedWindowName: self.context.GetWindowName(), - ClickedViewLineIdx: opts.Y, - ClickedViewRealLineIdx: line, + ClickedWindowName: self.context.GetWindowName(), + ClickedViewLineIdx: opts.Y, + ClickedViewRealLineIdx: line, + ClickedViewRealLineIsDeletion: isDeletion, }) return nil diff --git a/pkg/gui/controllers/switch_to_diff_files_controller.go b/pkg/gui/controllers/switch_to_diff_files_controller.go index 3528f2613..f3bee3c2d 100644 --- a/pkg/gui/controllers/switch_to_diff_files_controller.go +++ b/pkg/gui/controllers/switch_to_diff_files_controller.go @@ -54,10 +54,11 @@ func (self *SwitchToDiffFilesController) GetKeybindings(opts types.KeybindingsOp func (self *SwitchToDiffFilesController) GetOnClickFocusedMainView() func(mainViewName string, clickedLineIdx int) error { return func(mainViewName string, clickedLineIdx int) error { - clickedFile, line, ok := self.c.Helpers().Staging.GetFileAndLineForClickedDiffLine(mainViewName, clickedLineIdx) + info, ok := self.c.Helpers().Staging.GetDiffLineInfo(mainViewName, clickedLineIdx) if !ok { return nil } + line, isDeletion := info.PatchSelectLine() // Capture before self.enter() pushes the commit files panel, which // re-renders the main view. We escape "all the way out" to this side @@ -71,7 +72,7 @@ func (self *SwitchToDiffFilesController) GetOnClickFocusedMainView() func(mainVi context := self.c.Contexts().CommitFiles var node *filetree.CommitFileNode - relativePath, err := filepath.Rel(self.c.Git().RepoPaths.WorktreePath(), clickedFile) + relativePath, err := filepath.Rel(self.c.Git().RepoPaths.WorktreePath(), info.Path) if err != nil { return err } @@ -88,7 +89,7 @@ func (self *SwitchToDiffFilesController) GetOnClickFocusedMainView() func(mainVi context.GetViewTrait().FocusPoint( context.ModelIndexToViewIndex(idx), false) node = context.GetSelected() - return self.c.Helpers().CommitFiles.EnterCommitFile(node, snapshot, types.OnFocusOpts{ClickedWindowName: "main", ClickedViewLineIdx: line, ClickedViewRealLineIdx: line, SelectLineInDefaultMode: true}) + return self.c.Helpers().CommitFiles.EnterCommitFile(node, snapshot, types.OnFocusOpts{ClickedWindowName: "main", ClickedViewLineIdx: line, ClickedViewRealLineIdx: line, ClickedViewRealLineIsDeletion: isDeletion, SelectLineInDefaultMode: true}) } } diff --git a/pkg/gui/patch_exploring/state.go b/pkg/gui/patch_exploring/state.go index 3d1be981c..aa64a2284 100644 --- a/pkg/gui/patch_exploring/state.go +++ b/pkg/gui/patch_exploring/state.go @@ -45,7 +45,7 @@ const ( HUNK ) -func NewState(diff string, selectedLineIdx int, selectedRealLineIdx int, view *gocui.View, oldState *State, useHunkModeByDefault bool, selectLineInDefaultMode bool) *State { +func NewState(diff string, selectedLineIdx int, selectedRealLineIdx int, selectedRealLineIsDeletion bool, view *gocui.View, oldState *State, useHunkModeByDefault bool, selectLineInDefaultMode bool) *State { if oldState != nil && diff == oldState.diff && selectedLineIdx == -1 { // if we're here then we can return the old state. If selectedLineIdx was not -1 // then that would mean we were trying to click and potentially drag a range, which @@ -62,10 +62,18 @@ func NewState(diff string, selectedLineIdx int, selectedRealLineIdx int, view *g viewLineIndices, patchLineIndices := wrapPatchLines(diff, view) if selectedRealLineIdx != -1 { - // PatchLineForLineNumber returns a patch line index, but selectedLineIdx - // is in view-line (wrapped) space, so convert it. Without this the - // landing line is off by the number of wrapped lines above it. - patchLineIdx := patch.PatchLineForLineNumber(selectedRealLineIdx) + // Look the source line number up in the freshly parsed patch. A deletion + // is identified by its old-file line number (two consecutive deletions + // share a new-file line number), everything else by its new-file one. + // The result is a patch line index, but selectedLineIdx is in view-line + // (wrapped) space, so convert it; without this the landing line is off by + // the number of wrapped lines above it. + var patchLineIdx int + if selectedRealLineIsDeletion { + patchLineIdx = patch.PatchLineForOldLineNumber(selectedRealLineIdx) + } else { + patchLineIdx = patch.PatchLineForLineNumber(selectedRealLineIdx) + } selectedLineIdx = viewLineIndices[lo.Clamp(patchLineIdx, 0, len(viewLineIndices)-1)] } diff --git a/pkg/gui/types/context.go b/pkg/gui/types/context.go index 0e2d551c2..49eebf4fc 100644 --- a/pkg/gui/types/context.go +++ b/pkg/gui/types/context.go @@ -258,9 +258,17 @@ type OnFocusOpts struct { ClickedWindowName string ClickedViewLineIdx int - // If not -1, takes precedence over ClickedViewLineIdx. + // A source line number identifying the line to land on in the patch + // explorer. If not -1, takes precedence over ClickedViewLineIdx. It is a + // new-file line number, unless ClickedViewRealLineIsDeletion is set, in which + // case it is an old-file line number (two consecutive deletions share a + // new-file line number, so only the old-file number identifies a deletion). ClickedViewRealLineIdx int + // Whether ClickedViewRealLineIdx is an old-file line number for a deletion; + // see above. + ClickedViewRealLineIsDeletion bool + // When entering a patch explorer (staging or patch building) by clicking or // pressing enter on a line in a focused main view, we select that line using // the default select mode (hunk or line, per the UseHunkModeInStagingView diff --git a/pkg/gui/types/diff_line_info.go b/pkg/gui/types/diff_line_info.go new file mode 100644 index 000000000..c49297e8f --- /dev/null +++ b/pkg/gui/types/diff_line_info.go @@ -0,0 +1,56 @@ +package types + +// DiffLineType classifies a rendered diff row. It mirrors the per-line "type" +// of the metadata model described in diff-line-metadata-notes.md, so that the +// host-side buffer parser (mechanism #1) and the future pager-emitted OSC +// metadata (#2) produce the same shape. +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). It is also what a backend that + // can't determine the side reports — delta's lazygit-edit hyperlinks carry + // no side — so consumers treat it like a non-deletion content line. + DiffLineOther +) + +// DiffLineInfo is the patch-space identity of a rendered diff row, as recovered +// by StagingHelper.GetDiffLineInfo. It is the single shape the focused main view +// and patch explorer consumers act on, regardless of which backend produced it. +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 file. Set for all content lines + // (for a deletion it is the new-file position the deletion sits at). + NewLine int + // OldLine is the line's position in the old file. Set only for deletions. + OldLine int +} + +// PatchSelectLine returns the source line to land on when diving into the patch +// explorer for this row, in source-line-number space so it survives the patch +// being regenerated. For a deletion it is the old-file line number — two +// consecutive deletions share a new-file line number, so only the old-file +// number tells them apart — and for everything else the new-file line number. +func (self DiffLineInfo) PatchSelectLine() (lineNumber int, isDeletion bool) { + if self.Type == DiffLineDeleted { + return self.OldLine, true + } + return self.NewLine, false +} + +// PullRequestAnchor returns the side ("L"/"R") and line number to anchor a +// GitHub PR deep-link at: the left/old side for a deletion, the right/new side +// otherwise. +func (self DiffLineInfo) PullRequestAnchor() (side string, lineNumber int) { + if self.Type == DiffLineDeleted { + return "L", self.OldLine + } + return "R", self.NewLine +}