diff --git a/pkg/gui/controllers/helpers/diff_line_navigation.go b/pkg/gui/controllers/helpers/diff_line_navigation.go index ef7b4324a..1feb20dcb 100644 --- a/pkg/gui/controllers/helpers/diff_line_navigation.go +++ b/pkg/gui/controllers/helpers/diff_line_navigation.go @@ -67,9 +67,9 @@ type DiffFile struct { // FilesInDiff lists the files shown in view's diff, in display order, each paired with // the view line its section starts at. It is the jump-to-file menu's source: jumping to -// a file goes to its FirstViewLine, computed the same way (backUpOverHeader) that -// AdjacentFile lands on a file, so the menu and n/N agree on where each file begins. A -// file whose start row isn't currently mapped to a view line (not loaded yet) is skipped. +// a file goes to its FirstViewLine — the file's first located row, the same row that +// AdjacentFile lands on, so the menu and n/N agree on where each file begins. A file +// whose start row isn't currently mapped to a view line (not loaded yet) is skipped. func (self *StagingHelper) FilesInDiff(view *gocui.View) []DiffFile { resolved := self.resolveDiffLines(view.DiffLineContents()) paths := make([]string, len(resolved)) @@ -86,7 +86,7 @@ func (self *StagingHelper) FilesInDiff(view *gocui.View) []DiffFile { continue } seen[path] = true - if viewLine, ok := view.ViewLineForBufferLine(backUpOverHeader(paths, i)); ok { + if viewLine, ok := view.ViewLineForBufferLine(i); ok { files = append(files, DiffFile{Path: path, FirstViewLine: viewLine}) } } @@ -228,13 +228,17 @@ func changeBlockStart(isChange []bool, from int, forward bool) (int, bool) { } // fileStart finds, in a diff whose lines carry the file path they belong to (empty -// for a row no backend could place, e.g. a restructuring pager's file headers), the -// top row of the file adjacent to `from` in the given direction. It is the pure -// index arithmetic behind AdjacentFile. A file is identified by its path, so we find -// where the path changes and then back up over the neighbouring file's unplaced -// header rows, landing on its first row — the `diff --git`/`@@` header when the -// buffer is parseable, or whatever the pager renders above the file's first tagged -// line otherwise. +// for a row no backend could place), the first located row of the file adjacent to +// `from` in the given direction — the row file navigation lands on. It is the pure +// index arithmetic behind AdjacentFile. A file is identified by its path, so we +// find where the path changes, skipping unlocated rows: those are blank separator +// rows between files, or the header rows of a pager that doesn't emit `f`/`h` +// records. So the landing row is the file's header for any conforming source (a +// parseable buffer, or a pager tagging its headers), and the first content line +// under a pager that leaves its headers untagged — an accepted degradation. (An +// earlier version instead backed up over the untagged rows above the first located +// one, to reach the file's top under such pagers; but that overshoots onto the +// blank line above the header whenever the headers themselves are tagged.) func fileStart(paths []string, from int, forward bool) (int, bool) { anchorPath, ok := anchorFilePath(paths, from) if !ok { @@ -244,14 +248,15 @@ func fileStart(paths []string, from int, forward bool) (int, bool) { if forward { for i := from; i < len(paths); i++ { if paths[i] != "" && paths[i] != anchorPath { - return backUpOverHeader(paths, i), true + return i, true } } return 0, false } - // Walk back past the current file (its rows and any unplaced rows) to the - // previous file's last located row, then back over that whole file to its top. + // Walk back past the current file (its rows and any unlocated rows) to the + // previous file's last located row, then back over that whole file, landing + // on its first located row. i := from for i >= 0 && (paths[i] == "" || paths[i] == anchorPath) { i-- @@ -263,18 +268,10 @@ func fileStart(paths []string, from int, forward bool) (int, bool) { for i > 0 && (paths[i-1] == "" || paths[i-1] == prevPath) { i-- } - return i, true -} - -// backUpOverHeader moves from a file's first located row up over the unplaced header -// rows directly above it, to the file's top. It stops at the previous file's last -// located row, so it never crosses into it. -func backUpOverHeader(paths []string, firstLocated int) int { - i := firstLocated - for i > 0 && paths[i-1] == "" { - i-- + for paths[i] != prevPath { + i++ } - return i + return i, true } // anchorFilePath returns the path of the file the anchor sits in: the first row at or diff --git a/pkg/gui/controllers/helpers/diff_line_navigation_test.go b/pkg/gui/controllers/helpers/diff_line_navigation_test.go index e5d1e6545..241f475eb 100644 --- a/pkg/gui/controllers/helpers/diff_line_navigation_test.go +++ b/pkg/gui/controllers/helpers/diff_line_navigation_test.go @@ -51,8 +51,9 @@ func TestFileStart(t *testing.T) { // included), as the buffer parser reports. parseable := []string{"a", "a", "a", "a", "b", "b", "b", "b"} - // The same diff as a restructuring pager emits it: only content lines carry the - // path; the file/hunk header rows above each file are untagged (empty). + // The same diff as a pager without `f`/`h` header records emits it: only content + // lines carry the path; the file/hunk header rows above each file are untagged + // (empty), so navigation can only land on each file's first content line. tagged := []string{"", "", "a", "a", "", "", "b", "b"} // Three such files, to exercise navigating from one file's untagged header to the @@ -61,6 +62,15 @@ func TestFileStart(t *testing.T) { // into b and a second `n` couldn't advance. taggedThree := []string{"", "", "a", "a", "", "", "b", "b", "", "", "c", "c"} + // A pager that tags its header rows with `f`/`h` records (delta): the two-row + // file header and the hunk-header box carry the file's path, but the blank + // separator rows — above each file header, and between it and the first hunk + // header — carry nothing. Navigation must land on the header's first row, not + // the blank line above it. + // 0 blank 1 file hdr 2 file hdr 3 blank 4-6 hunk hdr box 7 content + // 8 blank 9 file hdr 10 file hdr 11 blank 12-13 hunk hdr box 14 content + headerTagged := []string{"", "a", "a", "", "a", "a", "a", "a", "", "b", "b", "", "b", "b", "b"} + scenarios := []struct { name string paths []string @@ -74,17 +84,24 @@ func TestFileStart(t *testing.T) { {"parseable: previous file lands on its header", parseable, 5, false, 0, true}, {"parseable: previous from the first file finds nothing", parseable, 1, false, 0, false}, - // With only content tagged, both directions still land on the file's top - // (the untagged header rows), so navigation feels the same. - {"tagged: next file lands on its header, not its first content", tagged, 2, true, 4, true}, - {"tagged: next from an untagged header still advances", tagged, 0, true, 4, true}, - {"tagged: previous file lands on its header", tagged, 7, false, 0, true}, + // With only content tagged, both directions land on the file's first content + // line — the untagged header rows above it can't be told apart from the + // blank separator rows, so they are never a landing spot. + {"tagged: next file lands on its first content line", tagged, 2, true, 6, true}, + {"tagged: next from an untagged header still advances", tagged, 0, true, 6, true}, + {"tagged: previous file lands on its first content line", tagged, 7, false, 2, true}, {"tagged: previous from the first file finds nothing", tagged, 2, false, 0, false}, // From b's untagged header (row 4), the anchor file is b (below), so next goes // to c and previous goes to a — neither sticks on b. - {"tagged: next from a middle file's header advances past it", taggedThree, 4, true, 8, true}, - {"tagged: previous from a middle file's header lands on the prior file", taggedThree, 4, false, 0, true}, + {"tagged: next from a middle file's header advances past it", taggedThree, 4, true, 10, true}, + {"tagged: previous from a middle file's header lands on the prior file", taggedThree, 4, false, 2, true}, + + // With headers tagged, both directions land on the header's first row — + // crucially not on the blank separator row above it. + {"header-tagged: next file lands on the header, not the blank above it", headerTagged, 7, true, 9, true}, + {"header-tagged: next from inside a file's header advances", headerTagged, 2, true, 9, true}, + {"header-tagged: previous file lands on the header, not the blank above it", headerTagged, 14, false, 1, true}, } for _, s := range scenarios { diff --git a/pkg/gui/controllers/helpers/diff_line_parser.go b/pkg/gui/controllers/helpers/diff_line_parser.go index aef2b6c9c..e16ec1a32 100644 --- a/pkg/gui/controllers/helpers/diff_line_parser.go +++ b/pkg/gui/controllers/helpers/diff_line_parser.go @@ -205,9 +205,10 @@ func pathFromDiffGitLine(line string) string { // parseDiffLineMetadata parses mechanism #2's OSC 1717 payload (v1): // version;type;new-line;old-line;file — positional and ';'-delimited, with the -// file last (so it may itself contain ';') and old-line empty unless the line is -// a deletion. See diff-line-metadata-notes.md §9.2. ok is false for a payload of -// an unknown version or shape, so the caller can fall back to another backend. +// file last (so it may itself contain ';'), old-line empty unless the line is a +// deletion, and new-line empty on a file header (the one type that carries no +// line number). See diff-line-metadata-notes.md §9.2. ok is false for a payload +// of an unknown version or shape, so the caller can fall back to another backend. func parseDiffLineMetadata(payload string) (parsedDiffLine, bool) { fields := strings.SplitN(payload, ";", 5) if len(fields) < 5 || fields[0] != "1" { @@ -219,13 +220,19 @@ func parseDiffLineMetadata(payload string) (parsedDiffLine, bool) { return parsedDiffLine{}, false } - newLine, err := strconv.Atoi(fields[2]) - if err != nil { + 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 } @@ -242,6 +249,10 @@ func diffLineTypeFromMetadata(typeField string) (types.DiffLineType, bool) { 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 } diff --git a/pkg/gui/controllers/helpers/diff_line_parser_test.go b/pkg/gui/controllers/helpers/diff_line_parser_test.go index d749a94fd..b8c7d9bc3 100644 --- a/pkg/gui/controllers/helpers/diff_line_parser_test.go +++ b/pkg/gui/controllers/helpers/diff_line_parser_test.go @@ -113,12 +113,26 @@ func TestParseDiffLineMetadata(t *testing.T) { // A pager may emit an absolute path; the parser keeps it verbatim (the // caller decides whether to join the worktree path). {"absolute path", "1;a;7;;/abs/foo.txt", parsedDiffLine{RelPath: "/abs/foo.txt", Type: types.DiffLineAdded, NewLine: 7}, true}, + // A file header carries no line numbers; 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{RelPath: "foo.txt", Type: types.DiffLineFileHeader}, true}, + {"hunk header", "1;h;10;;foo.txt", parsedDiffLine{RelPath: "foo.txt", Type: types.DiffLineHunkHeader, NewLine: 10}, true}, + {"hunk header of a deleted file", "1;h;0;;gone.txt", parsedDiffLine{RelPath: "gone.txt", Type: types.DiffLineHunkHeader, NewLine: 0}, true}, + // The spec leaves a file header's new-line always empty, but the parser + // doesn't police a number being there — it just carries it along. + {"file header with a line number (tolerated)", "1;f;10;;foo.txt", parsedDiffLine{RelPath: "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-line; on any other type an empty + // new-line is a malformed record, and rejecting it makes the row fall + // back to the other backends rather than acting on wrong data. + {"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 { diff --git a/pkg/gui/controllers/main_view_controller.go b/pkg/gui/controllers/main_view_controller.go index d4ff16f85..d046d0926 100644 --- a/pkg/gui/controllers/main_view_controller.go +++ b/pkg/gui/controllers/main_view_controller.go @@ -1082,11 +1082,17 @@ func (self *MainViewController) editLine() error { // editDiffLine opens the file the given diff line belongs to in the editor, at // that line. The file and line are resolved the same way entering staging does. +// A file-header row points at the file as a whole, not at a line in it, so it +// opens the file without jumping anywhere — the same behavior as pressing edit +// on a file in a side panel. func (self *MainViewController) editDiffLine(viewLineIdx int) error { info, ok := self.c.Helpers().Staging.GetDiffLineInfo(self.context.GetViewName(), viewLineIdx) if !ok { return nil } + if info.Type == types.DiffLineFileHeader { + return self.c.Helpers().Files.EditFiles([]string{info.Path}) + } lineNumber := self.c.Helpers().Diff.AdjustLineNumber(info.Path, info.NewLine, self.context.GetViewName()) return self.c.Helpers().Files.EditFileAtLine(info.Path, lineNumber) } diff --git a/pkg/gui/types/diff_line_info.go b/pkg/gui/types/diff_line_info.go index e86da410a..d768563fa 100644 --- a/pkg/gui/types/diff_line_info.go +++ b/pkg/gui/types/diff_line_info.go @@ -27,7 +27,10 @@ type DiffLineInfo struct { 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). + // (for a deletion it is the new-file position the deletion sits at) and for + // hunk headers (the first line of the hunk they head). Not meaningful for + // file headers: a pager's `f` record carries no line number (so the OSC + // backend reports 0), and the buffer parser reports 1. NewLine int // OldLine is the line's position in the old file. Set only for deletions. OldLine int @@ -64,15 +67,30 @@ func (self DiffLineInfo) PatchSelectLine() (lineNumber int, isDeletion bool) { // DiffLineOther) yields a non-deletion identity, so a deletion captured from a // full-fidelity backend won't match such a row — the restore then just doesn't // find its line, which is the acceptable degradation for that pager config. +// +// A header row shares its line number with a content row — a hunk header carries +// the hunk's first line — so headers only match headers of the same kind: +// otherwise a restore aiming at a hunk's first content line would land one row up +// on the header above it (or vice versa). func (self DiffLineInfo) SamePatchLine(other DiffLineInfo) bool { if self.Path != other.Path { return false } + if self.isHeader() != other.isHeader() { + return false + } + if self.isHeader() && self.Type != other.Type { + return false + } selfLine, selfIsDeletion := self.PatchSelectLine() otherLine, otherIsDeletion := other.PatchSelectLine() return selfLine == otherLine && selfIsDeletion == otherIsDeletion } +func (self DiffLineInfo) isHeader() bool { + return self.Type == DiffLineFileHeader || self.Type == DiffLineHunkHeader +} + // 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. diff --git a/pkg/gui/types/diff_line_info_test.go b/pkg/gui/types/diff_line_info_test.go new file mode 100644 index 000000000..df48a8893 --- /dev/null +++ b/pkg/gui/types/diff_line_info_test.go @@ -0,0 +1,81 @@ +package types + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestSamePatchLine(t *testing.T) { + scenarios := []struct { + name string + a, b DiffLineInfo + expected bool + }{ + { + "content lines at the same new-file line match", + DiffLineInfo{Path: "foo", Type: DiffLineContext, NewLine: 10}, + DiffLineInfo{Path: "foo", Type: DiffLineAdded, NewLine: 10}, + true, + }, + { + "different files don't match", + DiffLineInfo{Path: "foo", Type: DiffLineContext, NewLine: 10}, + DiffLineInfo{Path: "bar", Type: DiffLineContext, NewLine: 10}, + false, + }, + { + "a deletion doesn't match a non-deletion at the same position", + DiffLineInfo{Path: "foo", Type: DiffLineDeleted, NewLine: 10, OldLine: 10}, + DiffLineInfo{Path: "foo", Type: DiffLineContext, NewLine: 10}, + false, + }, + // A hunk header carries the new-file line of the hunk's first line, so + // it shares its number with that content line; the header/content guard + // is what keeps a restore aiming at one from landing on the other. + { + "a hunk header doesn't match the hunk's first content line", + DiffLineInfo{Path: "foo", Type: DiffLineHunkHeader, NewLine: 10}, + DiffLineInfo{Path: "foo", Type: DiffLineContext, NewLine: 10}, + false, + }, + { + "a content line doesn't match a hunk header at its line", + DiffLineInfo{Path: "foo", Type: DiffLineContext, NewLine: 10}, + DiffLineInfo{Path: "foo", Type: DiffLineHunkHeader, NewLine: 10}, + false, + }, + { + "hunk headers of the same hunk match", + DiffLineInfo{Path: "foo", Type: DiffLineHunkHeader, NewLine: 10}, + DiffLineInfo{Path: "foo", Type: DiffLineHunkHeader, NewLine: 10}, + true, + }, + { + "hunk headers of different hunks don't match", + DiffLineInfo{Path: "foo", Type: DiffLineHunkHeader, NewLine: 10}, + DiffLineInfo{Path: "foo", Type: DiffLineHunkHeader, NewLine: 25}, + false, + }, + { + "file headers of the same file match", + DiffLineInfo{Path: "foo", Type: DiffLineFileHeader}, + DiffLineInfo{Path: "foo", Type: DiffLineFileHeader}, + true, + }, + // A whole-file deletion's hunk header carries new-line 0, the same + // number a file header reports; only the type tells them apart. + { + "a file header doesn't match a deleted file's hunk header", + DiffLineInfo{Path: "foo", Type: DiffLineFileHeader}, + DiffLineInfo{Path: "foo", Type: DiffLineHunkHeader, NewLine: 0}, + false, + }, + } + + for _, s := range scenarios { + t.Run(s.name, func(t *testing.T) { + assert.Equal(t, s.expected, s.a.SamePatchLine(s.b)) + }) + } +}