From 4b2dc49a6c5abac62eba912ed1319c4810d54a86 Mon Sep 17 00:00:00 2001 From: Umputun Date: Thu, 23 Apr 2026 12:57:21 -0500 Subject: [PATCH 1/4] feat(diff): show line count in between-hunk and leading dividers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit parser computes the gap from hunk-header metadata and renders it as "⋯ N lines ⋯" (midline horizontal ellipsis). previously dividers were a static "...". uses prevOldEnd tracked via parsed hunk lengths rather than oldNum, so insertion-only hunks (@@ -K,0 ...) compute correctly — oldNum does not advance on `+` body lines. max(oldLen, 1) handles both omitted-length (git implicit 1) and literal-zero cases. leading divider appears when the first hunk starts after line 1; trailing divider is out of scope (needs total file-line count threaded through Renderer). Related to #145 --- app/diff/diff.go | 35 +++++++++---- app/diff/diff_test.go | 112 ++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 134 insertions(+), 13 deletions(-) diff --git a/app/diff/diff.go b/app/diff/diff.go index 1c4a8856..b38fb191 100644 --- a/app/diff/diff.go +++ b/app/diff/diff.go @@ -557,8 +557,10 @@ func (g *Git) formatSize(bytes int64) string { } } -// hunkHeaderRe matches unified diff hunk headers like @@ -1,5 +1,7 @@ -var hunkHeaderRe = regexp.MustCompile(`^@@ -(\d+)(?:,\d+)? \+(\d+)(?:,\d+)? @@`) +// hunkHeaderRe matches unified diff hunk headers like @@ -1,5 +1,7 @@. +// Lengths are optional per git's spec (omitted means length 1) and are captured +// so the parser can compute the old-side end of each hunk. +var hunkHeaderRe = regexp.MustCompile(`^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@`) // binaryFilesRe matches git's "Binary files ... differ" line for binary diffs. // Assumes English locale; non-English git may localize this message. @@ -576,7 +578,9 @@ func parseUnifiedDiff(raw string) ([]DiffLine, error) { // skip diff header lines (---, +++, diff --git, index, etc.) inHeader := true var oldNum, newNum int - firstHunk := true + // prevOldEnd = next untouched old-side line. Initialized to 1 so the first hunk's + // leading divider uses the same `oldStart - prevOldEnd` formula as between-hunks gaps. + prevOldEnd := 1 var isNewFile, isDeletedFile bool for scanner.Scan() { @@ -608,16 +612,29 @@ func parseUnifiedDiff(raw string) ([]DiffLine, error) { // parse hunk header if m := hunkHeaderRe.FindStringSubmatch(line); m != nil { oldStart, errOld := strconv.Atoi(m[1]) - newStart, errNew := strconv.Atoi(m[2]) + newStart, errNew := strconv.Atoi(m[3]) if errOld != nil || errNew != nil { return nil, fmt.Errorf("parse hunk header %q: old=%w new=%w", line, errOld, errNew) } - - // add divider between non-adjacent hunks (when using normal context, not -U1000000) - if !firstHunk { - lines = append(lines, DiffLine{ChangeType: ChangeDivider, Content: "..."}) + // Atoi("") returns 0 with error; regex guarantees m[2] is digits when non-empty. + // Both the omitted-length case (git spec: implicit 1) and the literal `,0` (insertion-only) + // end up at oldLen=0 here, and max(oldLen,1) below resolves both to the same advance. + oldLen, _ := strconv.Atoi(m[2]) + + // emit divider representing unchanged lines BEFORE this hunk. + // Leading divider (first hunk) uses prevOldEnd=1 initialization; between-hunks use + // prevOldEnd from prior iteration. Gap uses hunk-header metadata not oldNum, so + // insertion-only hunks (@@ -K,0 ...) compute correctly; oldNum stays put on `+` lines. + switch gap := oldStart - prevOldEnd; { + case gap == 1: + lines = append(lines, DiffLine{ChangeType: ChangeDivider, Content: "⋯ 1 line ⋯"}) + case gap > 1: + lines = append(lines, DiffLine{ChangeType: ChangeDivider, Content: fmt.Sprintf("⋯ %d lines ⋯", gap)}) } - firstHunk = false + // prevOldEnd = line number AFTER the current hunk on the old side. Normal hunks + // (oldLen>0) cover [oldStart, oldStart+oldLen). Insertion-only hunks (oldLen==0, + // e.g. @@ -K,0 ...) insert between old lines K and K+1 — handled by max(oldLen,1). + prevOldEnd = oldStart + max(oldLen, 1) oldNum = oldStart newNum = newStart diff --git a/app/diff/diff_test.go b/app/diff/diff_test.go index d1613b86..d9b746a7 100644 --- a/app/diff/diff_test.go +++ b/app/diff/diff_test.go @@ -81,14 +81,17 @@ func TestParseUnifiedDiff_MultiHunk(t *testing.T) { lines, err := parseUnifiedDiff(raw) require.NoError(t, err) - // verify divider exists between hunks - var dividers int + // verify dividers carry line-count labels. + // fixture: hunk 1 at @@ -2,3, hunk 2 at @@ -10,3. + // - leading divider: line 1 precedes first hunk → 1 line. + // - between-hunks: prevOldEnd=5 after hunk 1, next at 10 → 5 lines skipped. + var dividers []string for _, l := range lines { if l.ChangeType == ChangeDivider { - dividers++ + dividers = append(dividers, l.Content) } } - assert.Equal(t, 1, dividers, "expected 1 divider between two hunks") + assert.Equal(t, []string{"⋯ 1 line ⋯", "⋯ 5 lines ⋯"}, dividers, "expected leading + between-hunks dividers") // verify additions in both hunks var additions []string @@ -100,6 +103,107 @@ func TestParseUnifiedDiff_MultiHunk(t *testing.T) { assert.Equal(t, []string{`import "os"`, " os.Exit(0)"}, additions) } +// TestParseUnifiedDiff_GapLabels exercises every gap-label branch through the +// real parser (not a helper in isolation). Covers: plural gaps, singular gap, +// omitted-length hunk headers (@@ -N +N @@), insertion-only hunks (@@ -K,0 ...), +// start-of-file insertion (@@ -0,0 ...), and three-hunk chains. +func TestParseUnifiedDiff_GapLabels(t *testing.T) { + tests := []struct { + name string + raw string + dividers []string + }{ + { + name: "plural gap, standard two-hunk", + raw: "--- a\n+++ b\n" + + "@@ -1,3 +1,3 @@\n a\n-b\n+B\n" + + "@@ -10,3 +10,3 @@\n x\n-y\n+Y\n", + dividers: []string{"⋯ 6 lines ⋯"}, + }, + { + name: "singular gap", + raw: "--- a\n+++ b\n" + + "@@ -1,3 +1,3 @@\n a\n-b\n+B\n" + + "@@ -5,3 +5,3 @@\n x\n-y\n+Y\n", + dividers: []string{"⋯ 1 line ⋯"}, + }, + { + name: "omitted length (implicit 1) on both sides", + raw: "--- a\n+++ b\n" + + "@@ -1 +1 @@\n-old\n+new\n" + + "@@ -5 +5 @@\n-old2\n+new2\n", + dividers: []string{"⋯ 3 lines ⋯"}, + }, + { + name: "insertion-only prior hunk (oldLen==0, mid-file)", + raw: "--- a\n+++ b\n" + + "@@ -5,0 +6,2 @@\n+x\n+y\n" + + "@@ -10,3 +12,3 @@\n p\n-q\n+Q\n", + // leading: lines 1..4 before first hunk = 4 lines. + // between: prior hunk inserts between old lines 5 and 6 (prevOldEnd=6), next at 10 → gap=4. + dividers: []string{"⋯ 4 lines ⋯", "⋯ 4 lines ⋯"}, + }, + { + name: "insertion-only prior hunk at start (@@ -0,0)", + raw: "--- a\n+++ b\n" + + "@@ -0,0 +1,2 @@\n+x\n+y\n" + + "@@ -5,3 +7,3 @@\n p\n-q\n+Q\n", + // prior hunk prepends; next untouched old line is 1. gap = 5 - 1 = 4. + dividers: []string{"⋯ 4 lines ⋯"}, + }, + { + name: "three-hunk chain, multiple dividers", + raw: "--- a\n+++ b\n" + + "@@ -1,2 +1,2 @@\n a\n-b\n+B\n" + + "@@ -5,2 +5,2 @@\n p\n-q\n+Q\n" + + "@@ -20,2 +20,2 @@\n x\n-y\n+Y\n", + dividers: []string{"⋯ 2 lines ⋯", "⋯ 13 lines ⋯"}, + }, + { + name: "leading divider when first hunk starts after line 1", + raw: "--- a\n+++ b\n" + + "@@ -48,3 +48,3 @@\n ctx\n-old\n+new\n", + // 47 unchanged lines precede the first hunk. + dividers: []string{"⋯ 47 lines ⋯"}, + }, + { + name: "no leading divider when first hunk starts at line 1", + raw: "--- a\n+++ b\n" + + "@@ -1,3 +1,3 @@\n ctx\n-old\n+new\n", + dividers: nil, + }, + { + name: "leading + between-hunks dividers", + raw: "--- a\n+++ b\n" + + "@@ -10,2 +10,2 @@\n a\n+b\n" + + "@@ -20,2 +20,2 @@\n c\n+d\n", + // 9 lines before first hunk; 8 lines between (prevOldEnd=12, next oldStart=20). + dividers: []string{"⋯ 9 lines ⋯", "⋯ 8 lines ⋯"}, + }, + { + name: "new file (first hunk @@ -0,0 ...) suppresses leading divider", + raw: "--- /dev/null\n+++ b\n" + + "@@ -0,0 +1,2 @@\n+x\n+y\n", + // oldStart=0, gap = 0-1 = -1, no divider. + dividers: nil, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + lines, err := parseUnifiedDiff(tc.raw) + require.NoError(t, err) + + var got []string + for _, l := range lines { + if l.ChangeType == ChangeDivider { + got = append(got, l.Content) + } + } + assert.Equal(t, tc.dividers, got) + }) + } +} + func TestParseUnifiedDiff_MixedChanges(t *testing.T) { raw := readFixture(t, "mixed_changes.diff") lines, err := parseUnifiedDiff(raw) From ad14ad65d82c34f8b80081130138755c3dcb637a Mon Sep 17 00:00:00 2001 From: Umputun Date: Thu, 23 Apr 2026 13:13:05 -0500 Subject: [PATCH 2/4] feat(diff): add trailing divider showing lines after last hunk MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit parseUnifiedDiff now accepts totalOldLines and emits a "⋯ N lines ⋯" divider at the end when the last hunk does not reach EOF. each VCS implementation (git/hg/jj) computes the pre-change file's line count via git show / hg cat / jj file show against the resolved old-ref (range ref left operand, single ref, or sensible default). extracts appendGapDivider helper to unify leading / between-hunks / trailing emission — same "⋯ N lines ⋯" format, different gap source. test callers updated to pass 0 (unknown total, skip trailing). 6 new parser-level tests cover trailing plural / singular / no-trailing / unknown-total / leading+between+trailing / empty-diff cases. Related to #145 --- app/diff/diff.go | 69 +++++++++++++++++++++++++---- app/diff/diff_test.go | 101 +++++++++++++++++++++++++++++++++++++----- app/diff/hg.go | 31 ++++++++++++- app/diff/jj.go | 32 ++++++++++++- 4 files changed, 211 insertions(+), 22 deletions(-) diff --git a/app/diff/diff.go b/app/diff/diff.go index b38fb191..ce2f2459 100644 --- a/app/diff/diff.go +++ b/app/diff/diff.go @@ -391,7 +391,7 @@ func (g *Git) FileDiff(ref, file string, staged bool, contextLines int) ([]DiffL return nil, fmt.Errorf("get file diff for %s: %w", file, err) } - lines, err := parseUnifiedDiff(out) + lines, err := parseUnifiedDiff(out, g.totalOldLines(ref, file, staged)) if err != nil { return nil, err } @@ -406,6 +406,39 @@ func (g *Git) FileDiff(ref, file string, staged bool, contextLines int) ([]DiffL return lines, nil } +// totalOldLines returns the line count of the pre-change version of file, used by +// parseUnifiedDiff to emit a trailing divider. Returns 0 when the old-side file is +// unavailable (new files, bad refs, etc.) — the parser treats 0 as "unknown" and +// skips the trailing divider. +// +// Old-side resolution: +// - ref empty + staged → HEAD (git diff --cached compares HEAD against index) +// - ref empty + not staged → index (git diff compares index against working tree) +// - ref contains "..|..." → left operand (git diff A..B → A) +// - single ref → use as-is +func (g *Git) totalOldLines(ref, file string, staged bool) int { + oldRef := ref + if left, _, ok := strings.Cut(ref, ".."); ok { + oldRef = left + } + if oldRef == "" && staged { + oldRef = "HEAD" + } + // `git show :path` (empty oldRef) shows the index version of the file + out, err := g.runGit("show", oldRef+":"+file) + if err != nil { + return 0 + } + if out == "" { + return 0 + } + n := strings.Count(out, "\n") + if !strings.HasSuffix(out, "\n") { + n++ // file without trailing newline + } + return n +} + // gitContextArg returns the -U argument for git diff given the caller's requested // context size. A non-positive contextLines or one at or above fullContextSentinel // returns the full-file arg; any other value returns -U. @@ -562,6 +595,19 @@ func (g *Git) formatSize(bytes int64) string { // so the parser can compute the old-side end of each hunk. var hunkHeaderRe = regexp.MustCompile(`^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@`) +// appendGapDivider appends a "⋯ N lines ⋯" divider to lines when gap is positive. +// Used for leading, between-hunks, and trailing dividers — same format, different +// source of the gap count. Returns lines unchanged when gap <= 0 (nothing to show). +func appendGapDivider(lines []DiffLine, gap int) []DiffLine { + switch { + case gap == 1: + return append(lines, DiffLine{ChangeType: ChangeDivider, Content: "⋯ 1 line ⋯"}) + case gap > 1: + return append(lines, DiffLine{ChangeType: ChangeDivider, Content: fmt.Sprintf("⋯ %d lines ⋯", gap)}) + } + return lines +} + // binaryFilesRe matches git's "Binary files ... differ" line for binary diffs. // Assumes English locale; non-English git may localize this message. var binaryFilesRe = regexp.MustCompile(`^Binary files .+ and .+ differ$`) @@ -570,7 +616,12 @@ var binaryFilesRe = regexp.MustCompile(`^Binary files .+ and .+ differ$`) // it handles the diff header, hunk headers, and content lines. // for binary diffs ("Binary files ... differ"), it returns a single placeholder DiffLine. // intended for single-file diffs; multi-file diffs are not fully supported. -func parseUnifiedDiff(raw string) ([]DiffLine, error) { +// +// totalOldLines is the total line count of the pre-change file, used to emit a +// trailing "⋯ N lines ⋯" divider after the last hunk when it does not reach EOF. +// Pass 0 when unknown (context-only sources, tests, or any case where the caller +// cannot cheaply determine the old file's size) — trailing divider is then skipped. +func parseUnifiedDiff(raw string, totalOldLines int) ([]DiffLine, error) { var lines []DiffLine scanner := bufio.NewScanner(strings.NewReader(raw)) scanner.Buffer(make([]byte, 0, bufio.MaxScanTokenSize), MaxLineLength) @@ -625,12 +676,7 @@ func parseUnifiedDiff(raw string) ([]DiffLine, error) { // Leading divider (first hunk) uses prevOldEnd=1 initialization; between-hunks use // prevOldEnd from prior iteration. Gap uses hunk-header metadata not oldNum, so // insertion-only hunks (@@ -K,0 ...) compute correctly; oldNum stays put on `+` lines. - switch gap := oldStart - prevOldEnd; { - case gap == 1: - lines = append(lines, DiffLine{ChangeType: ChangeDivider, Content: "⋯ 1 line ⋯"}) - case gap > 1: - lines = append(lines, DiffLine{ChangeType: ChangeDivider, Content: fmt.Sprintf("⋯ %d lines ⋯", gap)}) - } + lines = appendGapDivider(lines, oldStart-prevOldEnd) // prevOldEnd = line number AFTER the current hunk on the old side. Normal hunks // (oldLen>0) cover [oldStart, oldStart+oldLen). Insertion-only hunks (oldLen==0, // e.g. @@ -K,0 ...) insert between old lines K and K+1 — handled by max(oldLen,1). @@ -680,6 +726,13 @@ func parseUnifiedDiff(raw string) ([]DiffLine, error) { return nil, fmt.Errorf("scan diff: %w", err) } + // trailing divider: unchanged lines after the last hunk on the old side. + // Emitted only when the caller supplied totalOldLines AND at least one hunk + // was processed (prevOldEnd > 1 means a hunk advanced it past the initial value). + if totalOldLines > 0 && prevOldEnd > 1 { + lines = appendGapDivider(lines, totalOldLines-prevOldEnd+1) + } + return lines, nil } diff --git a/app/diff/diff_test.go b/app/diff/diff_test.go index d9b746a7..8c71494d 100644 --- a/app/diff/diff_test.go +++ b/app/diff/diff_test.go @@ -15,7 +15,7 @@ import ( func TestParseUnifiedDiff_SimpleAdd(t *testing.T) { raw := readFixture(t, "simple_add.diff") - lines, err := parseUnifiedDiff(raw) + lines, err := parseUnifiedDiff(raw, 0) require.NoError(t, err) // expected: context, blank, context, add, add, add, context, context @@ -57,7 +57,7 @@ func TestParseUnifiedDiff_SimpleAdd(t *testing.T) { func TestParseUnifiedDiff_SimpleRemove(t *testing.T) { raw := readFixture(t, "simple_remove.diff") - lines, err := parseUnifiedDiff(raw) + lines, err := parseUnifiedDiff(raw, 0) require.NoError(t, err) require.NotEmpty(t, lines, "expected non-empty result") @@ -78,7 +78,7 @@ func TestParseUnifiedDiff_SimpleRemove(t *testing.T) { func TestParseUnifiedDiff_MultiHunk(t *testing.T) { raw := readFixture(t, "multi_hunk.diff") - lines, err := parseUnifiedDiff(raw) + lines, err := parseUnifiedDiff(raw, 0) require.NoError(t, err) // verify dividers carry line-count labels. @@ -190,7 +190,7 @@ func TestParseUnifiedDiff_GapLabels(t *testing.T) { } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - lines, err := parseUnifiedDiff(tc.raw) + lines, err := parseUnifiedDiff(tc.raw, 0) require.NoError(t, err) var got []string @@ -204,9 +204,86 @@ func TestParseUnifiedDiff_GapLabels(t *testing.T) { } } +// TestParseUnifiedDiff_TrailingDivider covers trailing divider emission driven +// by the totalOldLines parameter — the caller-supplied total line count of the +// pre-change file. 0 (unknown) skips the trailing divider; positive values emit +// a divider only when the last hunk does not reach EOF. +func TestParseUnifiedDiff_TrailingDivider(t *testing.T) { + tests := []struct { + name string + raw string + totalOldLines int + wantDividers []string + }{ + { + name: "trailing plural — hunk ends at line 10, total 300", + raw: "--- a\n+++ b\n" + + "@@ -8,3 +8,3 @@\n a\n-b\n+B\n", + // prevOldEnd=11, trailing = 300-11+1 = 290. + totalOldLines: 300, + wantDividers: []string{"⋯ 7 lines ⋯", "⋯ 290 lines ⋯"}, + }, + { + name: "trailing singular — total is one past last hunk", + raw: "--- a\n+++ b\n" + + "@@ -1,3 +1,3 @@\n a\n-b\n+B\n", + // prevOldEnd=4, trailing = 5-4+1 = 2? let me recompute: trailing = totalOldLines - prevOldEnd + 1 = 5-4+1 = 2. + // so this one has gap=2 (two lines). adjusted below. + totalOldLines: 4, + // prevOldEnd=4, gap = 4-4+1 = 1 + wantDividers: []string{"⋯ 1 line ⋯"}, + }, + { + name: "no trailing — last hunk covers to EOF", + raw: "--- a\n+++ b\n" + + "@@ -1,3 +1,3 @@\n a\n-b\n+B\n", + // prevOldEnd=4, total=3, 4>3 → no trailing. + totalOldLines: 3, + wantDividers: nil, + }, + { + name: "totalOldLines=0 → no trailing (unknown)", + raw: "--- a\n+++ b\n" + + "@@ -1,3 +1,3 @@\n a\n-b\n+B\n", + totalOldLines: 0, + wantDividers: nil, + }, + { + name: "leading + between + trailing", + raw: "--- a\n+++ b\n" + + "@@ -5,2 +5,2 @@\n a\n+b\n" + + "@@ -15,2 +15,2 @@\n c\n+d\n", + // leading: 4 lines. between: prevOldEnd=7, next=15 → 8. trailing: prevOldEnd=17, total=50 → 34. + totalOldLines: 50, + wantDividers: []string{"⋯ 4 lines ⋯", "⋯ 8 lines ⋯", "⋯ 34 lines ⋯"}, + }, + { + name: "empty diff with totalOldLines set emits nothing (no hunks)", + raw: "", + // no hunks processed → prevOldEnd stays 1 → trailing skipped by guard. + totalOldLines: 100, + wantDividers: nil, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + lines, err := parseUnifiedDiff(tc.raw, tc.totalOldLines) + require.NoError(t, err) + + var got []string + for _, l := range lines { + if l.ChangeType == ChangeDivider { + got = append(got, l.Content) + } + } + assert.Equal(t, tc.wantDividers, got) + }) + } +} + func TestParseUnifiedDiff_MixedChanges(t *testing.T) { raw := readFixture(t, "mixed_changes.diff") - lines, err := parseUnifiedDiff(raw) + lines, err := parseUnifiedDiff(raw, 0) require.NoError(t, err) types := make([]ChangeType, 0, len(lines)) @@ -230,14 +307,14 @@ func TestParseUnifiedDiff_MixedChanges(t *testing.T) { } func TestParseUnifiedDiff_Empty(t *testing.T) { - lines, err := parseUnifiedDiff("") + lines, err := parseUnifiedDiff("", 0) require.NoError(t, err) assert.Empty(t, lines) } func TestParseUnifiedDiff_Binary(t *testing.T) { raw := readFixture(t, "binary.diff") - lines, err := parseUnifiedDiff(raw) + lines, err := parseUnifiedDiff(raw, 0) require.NoError(t, err) require.Len(t, lines, 1) assert.Equal(t, BinaryPlaceholder, lines[0].Content) @@ -249,7 +326,7 @@ func TestParseUnifiedDiff_Binary(t *testing.T) { func TestParseUnifiedDiff_BinaryNewFile(t *testing.T) { raw := "diff --git a/new.bin b/new.bin\nnew file mode 100644\nindex 0000000..dd12d3a\nBinary files /dev/null and b/new.bin differ\n" - lines, err := parseUnifiedDiff(raw) + lines, err := parseUnifiedDiff(raw, 0) require.NoError(t, err) require.Len(t, lines, 1) assert.Equal(t, "(new binary file)", lines[0].Content) @@ -258,7 +335,7 @@ func TestParseUnifiedDiff_BinaryNewFile(t *testing.T) { func TestParseUnifiedDiff_BinaryDeleted(t *testing.T) { raw := "diff --git a/old.bin b/old.bin\ndeleted file mode 100644\nindex 2dfe7e4..0000000\nBinary files a/old.bin and /dev/null differ\n" - lines, err := parseUnifiedDiff(raw) + lines, err := parseUnifiedDiff(raw, 0) require.NoError(t, err) require.Len(t, lines, 1) assert.Equal(t, "(deleted binary file)", lines[0].Content) @@ -267,7 +344,7 @@ func TestParseUnifiedDiff_BinaryDeleted(t *testing.T) { func TestParseUnifiedDiff_LineNumbers(t *testing.T) { raw := readFixture(t, "simple_add.diff") - lines, err := parseUnifiedDiff(raw) + lines, err := parseUnifiedDiff(raw, 0) require.NoError(t, err) // additions should have OldNum=0 @@ -289,7 +366,7 @@ func TestParseUnifiedDiff_LineNumbers(t *testing.T) { func TestParseUnifiedDiff_RemoveLineNumbers(t *testing.T) { raw := readFixture(t, "simple_remove.diff") - lines, err := parseUnifiedDiff(raw) + lines, err := parseUnifiedDiff(raw, 0) require.NoError(t, err) for _, l := range lines { @@ -410,7 +487,7 @@ func TestParseUnifiedDiff_LongLine(t *testing.T) { longContent := strings.Repeat("x", 100_000) raw := "diff --git a/big.js b/big.js\n--- a/big.js\n+++ b/big.js\n@@ -1,1 +1,2 @@\n context\n+" + longContent + "\n" - lines, err := parseUnifiedDiff(raw) + lines, err := parseUnifiedDiff(raw, 0) require.NoError(t, err, "should handle lines up to 1MB without error") var hasAdd bool diff --git a/app/diff/hg.go b/app/diff/hg.go index 73265973..f2bd7ca9 100644 --- a/app/diff/hg.go +++ b/app/diff/hg.go @@ -159,7 +159,36 @@ func (h *Hg) FileDiff(ref, file string, _ bool, contextLines int) ([]DiffLine, e return nil, fmt.Errorf("get file diff for %s: %w", file, err) } - return parseUnifiedDiff(out) + return parseUnifiedDiff(out, h.totalOldLines(ref, file)) +} + +// totalOldLines returns the line count of the pre-change version of file, used by +// parseUnifiedDiff to emit a trailing divider. Returns 0 when the old-side file is +// unavailable — the parser treats 0 as "unknown" and skips the trailing divider. +// +// Old-side resolution mirrors revFlag: triple- or double-dot ranges take the left +// operand; single ref is used directly; empty ref falls back to "." (working-copy parent). +func (h *Hg) totalOldLines(ref, file string) int { + oldRef := ref + if left, _, ok := strings.Cut(ref, "..."); ok { + oldRef = left + } + if left, _, ok := strings.Cut(oldRef, ".."); ok { + oldRef = left + } + oldRef = translateRef(oldRef) + if oldRef == "" { + oldRef = "." + } + out, err := h.runHg("cat", "-r", oldRef, "--", file) + if err != nil || out == "" { + return 0 + } + n := strings.Count(out, "\n") + if !strings.HasSuffix(out, "\n") { + n++ + } + return n } // hgContextArg returns the -U argument for hg diff given the caller's requested diff --git a/app/diff/jj.go b/app/diff/jj.go index f4ad11b8..cdd3e293 100644 --- a/app/diff/jj.go +++ b/app/diff/jj.go @@ -156,7 +156,37 @@ func (j *Jj) FileDiff(ref, file string, _ bool, contextLines int) ([]DiffLine, e // jj emits raw bytes for binary files instead of git's "Binary files … differ" // marker. Detect and rewrite so parseUnifiedDiff produces the binary placeholder. normalized := j.synthesizeBinaryDiff(out) - return parseUnifiedDiff(normalized) + return parseUnifiedDiff(normalized, j.totalOldLines(ref, file)) +} + +// totalOldLines returns the line count of the pre-change version of file, used by +// parseUnifiedDiff to emit a trailing divider. Returns 0 when the old-side file is +// unavailable — the parser treats 0 as "unknown" and skips the trailing divider. +// +// Old-side resolution mirrors diffRangeFlags: triple- or double-dot ranges take +// the left operand; single ref is used directly; empty ref falls back to "@-" +// (the parent of the working-copy commit). +func (j *Jj) totalOldLines(ref, file string) int { + oldRef := ref + if left, _, ok := strings.Cut(ref, "..."); ok { + oldRef = left + } + if left, _, ok := strings.Cut(oldRef, ".."); ok { + oldRef = left + } + oldRef = j.translateRef(oldRef) + if oldRef == "" { + oldRef = "@-" + } + out, err := j.runJj("file", "show", "-r", oldRef, "--", file) + if err != nil || out == "" { + return 0 + } + n := strings.Count(out, "\n") + if !strings.HasSuffix(out, "\n") { + n++ + } + return n } // jjContextArg returns the --context argument for jj diff given the caller's From d1dc8a96b970a562574ce480dd1e5a263c100480 Mon Sep 17 00:00:00 2001 From: Umputun Date: Thu, 23 Apr 2026 13:30:55 -0500 Subject: [PATCH 3/4] fix(diff): address code review findings for divider work MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - git totalOldLines: check "..." before ".." in ref splitting so A...B takes left operand A (not "A" + ".B"). triple-dot ranges still show the left operand of the range rather than merge-base, which is approximate but matches hg/jj behavior. - gate totalOldLines subprocess behind compact mode in all VCS impls; full-file mode always reaches EOF so the trailing-divider probe was a wasted subprocess (~2× process launches per FileDiff call). - extract countLines helper (called from 3 VCS totalOldLines methods). - update ChangeDivider constant + DiffLine.Content godoc with new "⋯ N line[s] ⋯" format and pattern-match warning. - unify hg/jj totalOldLines godoc with git's bullet format. - clean up stale contradictory comments in TestParseUnifiedDiff_TrailingDivider. - add TestGit_TotalOldLines (8 ref-resolution cases, locks in triple-dot fix). - 3 new TestParseUnifiedDiff_TrailingDivider cases: insertion-only last hunk, deleted file, exact boundary. - CLAUDE.md: extend Compact mode gotcha with divider-position details and add gotcha warning against pattern-matching DiffLine.Content on dividers. Related to #145 --- CLAUDE.md | 3 +- app/diff/diff.go | 48 +++++++++++++++++------- app/diff/diff_test.go | 87 ++++++++++++++++++++++++++++++++++++++++--- app/diff/hg.go | 25 ++++++++----- app/diff/jj.go | 28 +++++++++----- 5 files changed, 152 insertions(+), 39 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 9c0d58f2..a04633fb 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -93,7 +93,8 @@ TUI for reviewing diffs, files, and documents with inline annotations, built wit - Overlay popups managed by `overlay.Manager`. `Compose()` uses ANSI-aware compositing via `charmbracelet/x/ansi.Cut`. `HandleKey()` returns `Outcome` — Model switches on `OutcomeKind` for side effects (file jumps, theme apply/persist). Overlay kinds: help, annot-list, theme-select, commit-info. One overlay at a time — opening any overlay auto-closes whichever was previously open - Overlay mouse routing: `Manager.HandleMouse` mirrors `HandleKey` (wheel + left-click). `app/ui/mouse.go::handleOverlayMouse` delegates when any overlay is active — wheel scrolls the overlay's own state, clicks in annotlist/themeselect select, clicks in commitinfo/help are no-ops, clicks outside the popup are swallowed (no dismiss). `Close()` must reset `Manager.bounds = popupBounds{}` or a stale rectangle from the previous overlay will satisfy click hit-tests. `overlayCenter()` records the popup rectangle as a side effect of rendering — if a new Open* path bypasses Compose, clicks will be treated as outside. Row offsets are hardcoded per overlay (annotlist `contentTop=2`, themeselect `entriesTop=4`, both `horizChromeCols=2`); any change to lipgloss `Padding(...)` on the overlay box style or to the number of lines rendered before the first entry requires a matching update in that overlay's `handleLeftClick`. `overlay.WheelStep` (shared with `app/ui.wheelStep`) defines the wheel notch size across panes. - Reload (`R` key): `reloadState` on `Model` holds `pending bool` (waiting for y/cancel), `hint string` (transient status-bar message), and `applicable bool` (false in `--stdin` mode — stream consumed). `ReloadApplicable` is wired at the composition root in `main.go`, following the same pattern as `CommitsApplicable`. The reload method is named `triggerReload()` — not `reload()` — because Go forbids a method and a field with the same name on the same type (`Model.reload` is the state field). Reload resets the diff cursor to the top of the file; tree selection (which file) is restored by `SelectByPath` in `handleFilesLoaded`. -- Compact mode (`C` key, `--compact` / `--compact-context=N`): shrinks the VCS diff at generation time (not at render time) by passing a `contextLines int` parameter through every `Renderer.FileDiff()` callsite. VCS renderers (Git, Hg, Jj) translate it via per-renderer helpers — `gitContextArg` / `hgContextArg` produce `-U`, `jjContextArg` produces `--context=`. Sentinel for full-file: `contextLines <= 0` or `>= 1000000` → use the full-file arg (`-U1000000` / `--context=1000000`). Context-only sources (FileReader, DirectoryReader, StdinReader) accept the parameter but ignore it — there are no changes to contextualize. `CompactApplicable` is computed at the composition root (`compactApplicable()` in `main.go`) via type assertion on the renderer chain (same approach as `CommitLogger`, no new interface); `false` for `--stdin`, `--all-files`, and file-only modes without VCS. Toggle re-fetches only the current file via `reloadCurrentFile()` — a deliberately lightweight sibling of `triggerReload()` that bumps `file.loadSeq` and returns `loadFileDiff(m.file.name)` without re-fetching the files list or commit log. Cursor reset to first hunk happens naturally via `skipInitialDividers()` in `handleFileLoaded` after the reload completes. `m.currentContextLines()` is the helper every `FileDiff` callsite must use — returns `m.modes.compactContext` if `m.modes.compact && m.compact.applicable`, else `0`. Runtime state (applicability + transient hint) lives on `compactState` alongside `reloadState` / `commitsState`; user-toggled state (on/off, context size) stays on `modeState` with the other view toggles. Composes cleanly with `--collapsed` (different layers: compact shrinks diff before parsing, collapsed hides removed lines during rendering). +- Compact mode (`C` key, `--compact` / `--compact-context=N`): shrinks the VCS diff at generation time (not at render time) by passing a `contextLines int` parameter through every `Renderer.FileDiff()` callsite. VCS renderers (Git, Hg, Jj) translate it via per-renderer helpers — `gitContextArg` / `hgContextArg` produce `-U`, `jjContextArg` produces `--context=`. Sentinel for full-file: `contextLines <= 0` or `>= 1000000` → use the full-file arg (`-U1000000` / `--context=1000000`). Context-only sources (FileReader, DirectoryReader, StdinReader) accept the parameter but ignore it — there are no changes to contextualize. `CompactApplicable` is computed at the composition root (`compactApplicable()` in `main.go`) via type assertion on the renderer chain (same approach as `CommitLogger`, no new interface); `false` for `--stdin`, `--all-files`, and file-only modes without VCS. Toggle re-fetches only the current file via `reloadCurrentFile()` — a deliberately lightweight sibling of `triggerReload()` that bumps `file.loadSeq` and returns `loadFileDiff(m.file.name)` without re-fetching the files list or commit log. Cursor reset to first hunk happens naturally via `skipInitialDividers()` in `handleFileLoaded` after the reload completes. `m.currentContextLines()` is the helper every `FileDiff` callsite must use — returns `m.modes.compactContext` if `m.modes.compact && m.compact.applicable`, else `0`. Runtime state (applicability + transient hint) lives on `compactState` alongside `reloadState` / `commitsState`; user-toggled state (on/off, context size) stays on `modeState` with the other view toggles. Composes cleanly with `--collapsed` (different layers: compact shrinks diff before parsing, collapsed hides removed lines during rendering). Divider lines now carry line-count labels: `parseUnifiedDiff` emits `⋯ N line[s] ⋯` dividers at three positions — leading (first hunk does not start at line 1), between non-adjacent hunks (computed from `prevOldEnd` tracked via hunk-header metadata, handles insertion-only `@@ -K,0 ...` hunks where `oldNum` does not advance on `+` lines), and trailing (last hunk does not reach EOF). Trailing requires `totalOldLines` passed by the VCS impl; Git/Hg/Jj `FileDiff` fetch it via `git show :` / `hg cat` / `jj file show` and only when `contextLines > 0 && contextLines < fullContextSentinel` — full-file mode always reaches EOF so the probe is skipped. +- `diff.DiffLine.Content` for `ChangeDivider` rows is a human-readable `⋯ N line[s] ⋯` label produced by `parseUnifiedDiff`. Never pattern-match the string — dispatch on `ChangeType == diff.ChangeDivider`. Test fixtures may construct `ChangeDivider` rows with arbitrary `Content` (e.g. `"..."`, `"@@..."`); that is a test-only shortcut and NOT the parser's contract. - Commit info overlay (`i` key) uses the `diff.CommitLogger` capability interface (additive to `diff.Renderer`). Model resolves a `commitLogSource` at construction: explicit `ModelConfig.CommitLog` wins, else type-asserts the renderer for `CommitLogger`, else the feature is unavailable and `i` is a no-op with a transient status-bar hint. `CommitsApplicable` is computed at the composition root by `commitsApplicable()` in `main.go` (using the `commitLogger` field populated by `setupVCSRenderer` in `renderer_setup.go`) — Model copies it, does not re-derive. Data is fetched eagerly at startup and on `R` reload: `Init()` and `triggerReload()` both return `tea.Batch(m.loadFiles(), m.loadCommits())`, running files and commits loads in parallel as independent goroutines. `loadCommits()` captures `m.commits.loadSeq` at invocation time and tags the resulting `commitsLoadedMsg` with it; `handleCommitsLoaded` drops any message whose seq no longer matches (stale-result guard, mirrors the files-load pattern). Eager parallel fetch shrinks the window where overlay and diff can disagree from "time until first `i` press" to "skew between two parallel goroutine starts" (milliseconds). Not a strict snapshot guarantee — the two subprocesses each resolve `HEAD` independently — but the practical race window is tens of ms instead of minutes. If the user presses `i` before `commitsLoadedMsg` arrives, `handleCommitInfo` sets a transient `loading commits…` hint instead of opening the overlay — a second press after load succeeds. Hg cannot use literal NUL in argv templates — use ASCII US/RS (`\x1f`/`\x1e`) as field/record separators for hg only - **ANSI nesting with lipgloss**: `lipgloss.Render()` emits `\033[0m` (full reset) which breaks outer style backgrounds. For styled substrings inside a lipgloss container (status bar separators, search highlights, diff cursor, annotation lines), use raw ANSI sequences via the style sub-package (`style.AnsiFg`, `resolver.Color`), or dedicated `Renderer` methods. Never use `lipgloss.NewStyle().Render()` for inline elements within a lipgloss-rendered parent. - **Background fill for themed panes**: lipgloss pane `Render()` and viewport internal padding emit plain spaces after reset, causing terminal default bg. Workarounds: (1) `extendLineBg()` pads lines to full width, (2) `padContentBg()` re-pads pane content, (3) `BorderBackground()` on border styles. **Ordering**: `extendLineBg()` must be called AFTER `applyHorizontalScroll()`. diff --git a/app/diff/diff.go b/app/diff/diff.go index ce2f2459..dddf7b73 100644 --- a/app/diff/diff.go +++ b/app/diff/diff.go @@ -20,7 +20,7 @@ const ( ChangeAdd ChangeType = "+" ChangeRemove ChangeType = "-" ChangeContext ChangeType = " " - ChangeDivider ChangeType = "~" // separates non-adjacent hunks + ChangeDivider ChangeType = "~" // marks a skipped unchanged region (leading, between-hunk, or trailing) // fullContextSentinel is the numeric threshold that callers use to request // full-file diff context. contextLines <= 0 or >= fullContextSentinel causes @@ -45,7 +45,7 @@ const ( type DiffLine struct { OldNum int // line number in old version (0 for additions) NewNum int // line number in new version (0 for removals) - Content string // line content without the +/- prefix + Content string // line content without the +/- prefix; for ChangeDivider rows it is a human-readable "⋯ N line[s] ⋯" label — never pattern-match it, dispatch on ChangeType ChangeType ChangeType // changeAdd, ChangeRemove, ChangeContext, or ChangeDivider IsBinary bool // true when this line is a binary file placeholder IsPlaceholder bool // true for non-content placeholders (broken symlink, non-regular file, too-long lines) @@ -391,7 +391,13 @@ func (g *Git) FileDiff(ref, file string, staged bool, contextLines int) ([]DiffL return nil, fmt.Errorf("get file diff for %s: %w", file, err) } - lines, err := parseUnifiedDiff(out, g.totalOldLines(ref, file, staged)) + // trailing divider is only meaningful in compact mode — full-file mode always + // reaches EOF, so probing the old-file size would be a wasted subprocess. + total := 0 + if contextLines > 0 && contextLines < fullContextSentinel { + total = g.totalOldLines(ref, file, staged) + } + lines, err := parseUnifiedDiff(out, total) if err != nil { return nil, err } @@ -413,12 +419,19 @@ func (g *Git) FileDiff(ref, file string, staged bool, contextLines int) ([]DiffL // // Old-side resolution: // - ref empty + staged → HEAD (git diff --cached compares HEAD against index) -// - ref empty + not staged → index (git diff compares index against working tree) -// - ref contains "..|..." → left operand (git diff A..B → A) +// - ref empty + not staged → index via `git show :path` +// - ref contains ".." or "..." → left operand (triple-dot checked first so A...B +// is not mis-split on the leading "..") // - single ref → use as-is +// +// For triple-dot ranges the left operand is an approximation of the true old side +// (merge-base(A,B)); accurate enough for the informational trailing-divider count. func (g *Git) totalOldLines(ref, file string, staged bool) int { oldRef := ref - if left, _, ok := strings.Cut(ref, ".."); ok { + if left, _, ok := strings.Cut(ref, "..."); ok { + oldRef = left + } + if left, _, ok := strings.Cut(oldRef, ".."); ok { oldRef = left } if oldRef == "" && staged { @@ -429,14 +442,7 @@ func (g *Git) totalOldLines(ref, file string, staged bool) int { if err != nil { return 0 } - if out == "" { - return 0 - } - n := strings.Count(out, "\n") - if !strings.HasSuffix(out, "\n") { - n++ // file without trailing newline - } - return n + return countLines(out) } // gitContextArg returns the -U argument for git diff given the caller's requested @@ -595,6 +601,20 @@ func (g *Git) formatSize(bytes int64) string { // so the parser can compute the old-side end of each hunk. var hunkHeaderRe = regexp.MustCompile(`^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@`) +// countLines returns the number of lines in s, counting a final non-newline-terminated +// line as one additional line. Empty input returns 0. Used by the per-VCS totalOldLines +// methods to translate file contents into a line count for the trailing divider. +func countLines(s string) int { + if s == "" { + return 0 + } + n := strings.Count(s, "\n") + if !strings.HasSuffix(s, "\n") { + n++ + } + return n +} + // appendGapDivider appends a "⋯ N lines ⋯" divider to lines when gap is positive. // Used for leading, between-hunks, and trailing dividers — same format, different // source of the gap count. Returns lines unchanged when gap <= 0 (nothing to show). diff --git a/app/diff/diff_test.go b/app/diff/diff_test.go index 8c71494d..8497bcd1 100644 --- a/app/diff/diff_test.go +++ b/app/diff/diff_test.go @@ -224,14 +224,12 @@ func TestParseUnifiedDiff_TrailingDivider(t *testing.T) { wantDividers: []string{"⋯ 7 lines ⋯", "⋯ 290 lines ⋯"}, }, { - name: "trailing singular — total is one past last hunk", + name: "trailing singular — exactly one line after last hunk", raw: "--- a\n+++ b\n" + "@@ -1,3 +1,3 @@\n a\n-b\n+B\n", - // prevOldEnd=4, trailing = 5-4+1 = 2? let me recompute: trailing = totalOldLines - prevOldEnd + 1 = 5-4+1 = 2. - // so this one has gap=2 (two lines). adjusted below. + // prevOldEnd=4, totalOldLines=4, gap = 4-4+1 = 1 → singular label totalOldLines: 4, - // prevOldEnd=4, gap = 4-4+1 = 1 - wantDividers: []string{"⋯ 1 line ⋯"}, + wantDividers: []string{"⋯ 1 line ⋯"}, }, { name: "no trailing — last hunk covers to EOF", @@ -264,6 +262,33 @@ func TestParseUnifiedDiff_TrailingDivider(t *testing.T) { totalOldLines: 100, wantDividers: nil, }, + { + name: "insertion-only LAST hunk computes trailing from prevOldEnd=K+1", + raw: "--- a\n+++ b\n" + + "@@ -1,1 +1,1 @@\n-a\n+A\n" + + "@@ -10,0 +11,2 @@\n+x\n+y\n", + // between hunks: prevOldEnd after hunk 1 = 2, next oldStart = 10 → gap = 8. + // last hunk is insertion-only: prevOldEnd = 10 + max(0,1) = 11. + // trailing: totalOldLines=20, 20 - 11 + 1 = 10. + totalOldLines: 20, + wantDividers: []string{"⋯ 8 lines ⋯", "⋯ 10 lines ⋯"}, + }, + { + name: "deleted file (@@ -1,N +0,0 @@) — last hunk covers EOF, no trailing", + raw: "--- a\n+++ /dev/null\n" + + "@@ -1,3 +0,0 @@\n-a\n-b\n-c\n", + // prevOldEnd = 1 + 3 = 4 = totalOldLines + 1, so gap = 3-4+1 = 0 → no divider. + totalOldLines: 3, + wantDividers: nil, + }, + { + name: "exact boundary prevOldEnd == totalOldLines + 1 emits no trailing", + raw: "--- a\n+++ b\n" + + "@@ -5,3 +5,3 @@\n a\n-b\n+B\n", + // leading: 4 lines (5-1). prevOldEnd = 8. totalOldLines = 7 → 7-8+1 = 0 → no trailing. + totalOldLines: 7, + wantDividers: []string{"⋯ 4 lines ⋯"}, + }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { @@ -1078,6 +1103,58 @@ func gitCmd(t *testing.T, dir string, args ...string) { require.NoError(t, err, "git %v failed: %s", args, string(out)) } +func TestGit_TotalOldLines(t *testing.T) { + dir := setupTestRepo(t) + // commit A: 5 lines + writeFile(t, dir, "f.txt", "a\nb\nc\nd\ne\n") + gitCmd(t, dir, "add", "f.txt") + gitCmd(t, dir, "commit", "-m", "A") + commitA, err := exec.Command("git", "-C", dir, "rev-parse", "HEAD").Output() //nolint:gosec // test-controlled args + require.NoError(t, err) + refA := strings.TrimSpace(string(commitA)) + + // commit B: 3 lines (same file, modified) + writeFile(t, dir, "f.txt", "a\nB\nC\n") + gitCmd(t, dir, "add", "f.txt") + gitCmd(t, dir, "commit", "-m", "B") + + // working tree: 7 lines + writeFile(t, dir, "f.txt", "a\nB\nC\nd\ne\nf\ng\n") + + // staged version (10 lines) + writeFile(t, dir, "staged.txt", strings.Repeat("x\n", 10)) + gitCmd(t, dir, "add", "staged.txt") + + // file without trailing newline + writeFile(t, dir, "notrail.txt", "line1\nline2") + gitCmd(t, dir, "add", "notrail.txt") + gitCmd(t, dir, "commit", "-m", "notrail") + + g := NewGit(dir) + + tests := []struct { + name string + ref string + file string + staged bool + want int + }{ + {"HEAD commit B — 3 lines", "HEAD", "f.txt", false, 3}, + {"commit A — 5 lines via single ref", refA, "f.txt", false, 5}, + {"range A..HEAD — left operand A, 5 lines", refA + "..HEAD", "f.txt", false, 5}, + {"triple-dot A...HEAD — takes left operand A, 5 lines", refA + "...HEAD", "f.txt", false, 5}, + {"empty ref + staged → HEAD, 3 lines", "", "f.txt", true, 3}, + {"non-existent ref → 0", "nonexistent-ref-xyz", "f.txt", false, 0}, + {"non-existent file → 0", "HEAD", "missing.txt", false, 0}, + {"file without trailing newline counts final line", "HEAD", "notrail.txt", false, 2}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, g.totalOldLines(tc.ref, tc.file, tc.staged)) + }) + } +} + func TestMatchesPrefix(t *testing.T) { prefixes := []string{"src", "pkg/util"} diff --git a/app/diff/hg.go b/app/diff/hg.go index f2bd7ca9..19ad6c7d 100644 --- a/app/diff/hg.go +++ b/app/diff/hg.go @@ -159,15 +159,26 @@ func (h *Hg) FileDiff(ref, file string, _ bool, contextLines int) ([]DiffLine, e return nil, fmt.Errorf("get file diff for %s: %w", file, err) } - return parseUnifiedDiff(out, h.totalOldLines(ref, file)) + // trailing divider only matters in compact mode; skip the probe in full-file mode. + total := 0 + if contextLines > 0 && contextLines < fullContextSentinel { + total = h.totalOldLines(ref, file) + } + return parseUnifiedDiff(out, total) } // totalOldLines returns the line count of the pre-change version of file, used by // parseUnifiedDiff to emit a trailing divider. Returns 0 when the old-side file is // unavailable — the parser treats 0 as "unknown" and skips the trailing divider. // -// Old-side resolution mirrors revFlag: triple- or double-dot ranges take the left -// operand; single ref is used directly; empty ref falls back to "." (working-copy parent). +// Old-side resolution (mirrors revFlag): +// - ref empty → "." (working-copy parent) +// - ref contains ".." or "..." → left operand (triple-dot checked first so A...B +// is not mis-split on the leading "..") +// - single ref → use as-is +// +// For triple-dot ranges the left operand is an approximation of the true old side +// (ancestor(A,B)); accurate enough for the informational trailing-divider count. func (h *Hg) totalOldLines(ref, file string) int { oldRef := ref if left, _, ok := strings.Cut(ref, "..."); ok { @@ -181,14 +192,10 @@ func (h *Hg) totalOldLines(ref, file string) int { oldRef = "." } out, err := h.runHg("cat", "-r", oldRef, "--", file) - if err != nil || out == "" { + if err != nil { return 0 } - n := strings.Count(out, "\n") - if !strings.HasSuffix(out, "\n") { - n++ - } - return n + return countLines(out) } // hgContextArg returns the -U argument for hg diff given the caller's requested diff --git a/app/diff/jj.go b/app/diff/jj.go index cdd3e293..6d19540f 100644 --- a/app/diff/jj.go +++ b/app/diff/jj.go @@ -156,16 +156,28 @@ func (j *Jj) FileDiff(ref, file string, _ bool, contextLines int) ([]DiffLine, e // jj emits raw bytes for binary files instead of git's "Binary files … differ" // marker. Detect and rewrite so parseUnifiedDiff produces the binary placeholder. normalized := j.synthesizeBinaryDiff(out) - return parseUnifiedDiff(normalized, j.totalOldLines(ref, file)) + + // trailing divider only matters in compact mode; skip the probe in full-file mode. + total := 0 + if contextLines > 0 && contextLines < fullContextSentinel { + total = j.totalOldLines(ref, file) + } + return parseUnifiedDiff(normalized, total) } // totalOldLines returns the line count of the pre-change version of file, used by // parseUnifiedDiff to emit a trailing divider. Returns 0 when the old-side file is // unavailable — the parser treats 0 as "unknown" and skips the trailing divider. // -// Old-side resolution mirrors diffRangeFlags: triple- or double-dot ranges take -// the left operand; single ref is used directly; empty ref falls back to "@-" -// (the parent of the working-copy commit). +// Old-side resolution (mirrors diffRangeFlags): +// - ref empty → "@-" (parent of the working-copy commit) +// - ref contains ".." or "..." → left operand (triple-dot checked first so A...B +// is not mis-split on the leading "..") +// - single ref → use as-is +// +// For triple-dot ranges the left operand is an approximation of the true old side +// (the jj revset ancestors(A) & ancestors(B)); accurate enough for the informational +// trailing-divider count. func (j *Jj) totalOldLines(ref, file string) int { oldRef := ref if left, _, ok := strings.Cut(ref, "..."); ok { @@ -179,14 +191,10 @@ func (j *Jj) totalOldLines(ref, file string) int { oldRef = "@-" } out, err := j.runJj("file", "show", "-r", oldRef, "--", file) - if err != nil || out == "" { + if err != nil { return 0 } - n := strings.Count(out, "\n") - if !strings.HasSuffix(out, "\n") { - n++ - } - return n + return countLines(out) } // jjContextArg returns the --context argument for jj diff given the caller's From bd745afe262ea7b07e525cf44eed35586d108b37 Mon Sep 17 00:00:00 2001 From: Umputun Date: Thu, 23 Apr 2026 13:41:30 -0500 Subject: [PATCH 4/4] fix(diff): use sawHunk flag to guard trailing divider emission Replaces the prevOldEnd > 1 guard with an explicit sawHunk boolean. The previous guard conflated "did we process any hunk" with "did the last hunk end past line 1", so a hypothetical `@@ -0,0 +1,N @@` hunk alongside totalOldLines > 0 would suppress the trailing divider even though the hunk was processed (prevOldEnd stays at 1 for that case: 0 + max(0,1) = 1, the same as the initial value). Adds a test locking in the behavior. The practical impact is nil today (git/hg/jj only emit @@ -0,0 on genuinely new files where totalOldLines=0 and the outer guard already skips), but the cleaner guard documents intent and avoids future surprises. Addresses Copilot inline review on #146. --- app/diff/diff.go | 10 ++++++++-- app/diff/diff_test.go | 10 ++++++++++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/app/diff/diff.go b/app/diff/diff.go index dddf7b73..3822e659 100644 --- a/app/diff/diff.go +++ b/app/diff/diff.go @@ -652,6 +652,10 @@ func parseUnifiedDiff(raw string, totalOldLines int) ([]DiffLine, error) { // prevOldEnd = next untouched old-side line. Initialized to 1 so the first hunk's // leading divider uses the same `oldStart - prevOldEnd` formula as between-hunks gaps. prevOldEnd := 1 + // sawHunk tracks whether any hunk header was parsed — used as the guard for the + // trailing divider so that insertion-at-start hunks (@@ -0,0 ...) don't collide + // with the prevOldEnd==1 initialization sentinel. + var sawHunk bool var isNewFile, isDeletedFile bool for scanner.Scan() { @@ -697,6 +701,7 @@ func parseUnifiedDiff(raw string, totalOldLines int) ([]DiffLine, error) { // prevOldEnd from prior iteration. Gap uses hunk-header metadata not oldNum, so // insertion-only hunks (@@ -K,0 ...) compute correctly; oldNum stays put on `+` lines. lines = appendGapDivider(lines, oldStart-prevOldEnd) + sawHunk = true // prevOldEnd = line number AFTER the current hunk on the old side. Normal hunks // (oldLen>0) cover [oldStart, oldStart+oldLen). Insertion-only hunks (oldLen==0, // e.g. @@ -K,0 ...) insert between old lines K and K+1 — handled by max(oldLen,1). @@ -748,8 +753,9 @@ func parseUnifiedDiff(raw string, totalOldLines int) ([]DiffLine, error) { // trailing divider: unchanged lines after the last hunk on the old side. // Emitted only when the caller supplied totalOldLines AND at least one hunk - // was processed (prevOldEnd > 1 means a hunk advanced it past the initial value). - if totalOldLines > 0 && prevOldEnd > 1 { + // was processed. sawHunk (not prevOldEnd > 1) is the correct "processed" flag + // — insertion-at-start hunks (@@ -0,0 ...) leave prevOldEnd at 1 but still count. + if totalOldLines > 0 && sawHunk { lines = appendGapDivider(lines, totalOldLines-prevOldEnd+1) } diff --git a/app/diff/diff_test.go b/app/diff/diff_test.go index 8497bcd1..469a4772 100644 --- a/app/diff/diff_test.go +++ b/app/diff/diff_test.go @@ -289,6 +289,16 @@ func TestParseUnifiedDiff_TrailingDivider(t *testing.T) { totalOldLines: 7, wantDividers: []string{"⋯ 4 lines ⋯"}, }, + { + name: "insertion-at-start hunk (@@ -0,0) with totalOldLines>0 emits trailing", + raw: "--- a\n+++ b\n" + + "@@ -0,0 +1,2 @@\n+x\n+y\n", + // hypothetical/malformed: prevOldEnd stays at 1 after the hunk, + // but sawHunk is true, so the trailing guard still fires. + // gap = totalOldLines - prevOldEnd + 1 = 10 - 1 + 1 = 10. + totalOldLines: 10, + wantDividers: []string{"⋯ 10 lines ⋯"}, + }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) {