From aeacc20d99de98958fcf89007c8fa1c9bf62f4ef Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Wed, 19 Aug 2026 16:54:06 +0200 Subject: [PATCH] Let the patch builder be told which lines by their identity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A patch is built in terms of where a line sits in the file's diff, which is what the patch explorer has to hand. The main view doesn't: a row of it resolves to a line of a file, and what index that line has depends on how much of the diff has been read and how a diff renderer chose to lay it out. So let the two meet at the patch builder's edge, in the identity of a change line — its number on the side it belongs to — leaving the main view to speak only of lines it can see. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/commands/patch/patch_builder.go | 76 ++++++++++++++++++++++++ pkg/commands/patch/patch_builder_test.go | 75 +++++++++++++++++++++++ 2 files changed, 151 insertions(+) create mode 100644 pkg/commands/patch/patch_builder_test.go diff --git a/pkg/commands/patch/patch_builder.go b/pkg/commands/patch/patch_builder.go index 78c539e86..55256a99c 100644 --- a/pkg/commands/patch/patch_builder.go +++ b/pkg/commands/patch/patch_builder.go @@ -5,6 +5,7 @@ import ( "strings" "github.com/jesseduffield/generics/maps" + "github.com/jesseduffield/generics/set" "github.com/samber/lo" "github.com/sasha-s/go-deadlock" "github.com/sirupsen/logrus" @@ -291,6 +292,81 @@ func (p *PatchBuilder) GetFileStatus(filename string, parent string) PatchStatus return info.mode } +// LineIdentity says which change line of a file is meant — the line number it has on +// the side it belongs to, and whether it is a deletion — without reference to where +// that line sits in the file's parsed diff. +// +// It is how a diff shown in the main view speaks about its lines: what a rendered row +// resolves to is a line of a file, while the index of that line in the diff depends on +// how much of the diff is being shown and in what order a renderer laid it out. +type LineIdentity struct { + LineNumber int + IsDeletion bool +} + +// ChangeLineIndexByIdentity indexes a parsed diff's change lines by their identity. An +// addition is numbered in the new file and a deletion in the old one. Two consecutive +// deletions share the one new-file position between them, and numbering them in the +// old file keeps them apart. +func ChangeLineIndexByIdentity(parsed *Patch) map[LineIdentity]int { + byIdentity := map[LineIdentity]int{} + for idx, line := range parsed.Lines() { + switch { + case line.IsAddition(): + byIdentity[LineIdentity{parsed.LineNumberOfLine(idx), false}] = idx + case line.IsDeletion(): + byIdentity[LineIdentity{parsed.OldLineNumberOfLine(idx), true}] = idx + } + } + return byIdentity +} + +// ChangeLineIndicesForLines maps the given change lines of a parsed diff to their +// indices in it. A line that names no change line of the diff — a context line, or a +// line that isn't in the diff at all — contributes nothing. +func ChangeLineIndicesForLines(parsed *Patch, lines []LineIdentity) []int { + byIdentity := ChangeLineIndexByIdentity(parsed) + indices := make([]int, 0, len(lines)) + for _, line := range lines { + if idx, ok := byIdentity[line]; ok { + indices = append(indices, idx) + } + } + return indices +} + +// PatchLineIndicesForLines maps change lines of filename to their indices in that +// file's diff, which is what the patch is built in terms of. +func (p *PatchBuilder) PatchLineIndicesForLines( + filename string, previousPath string, lines []LineIdentity, +) ([]int, error) { + info, err := p.getFileInfo(filename, previousPath) + if err != nil { + return nil, err + } + + return ChangeLineIndicesForLines(Parse(info.diff), lines), nil +} + +// IncludedLineIdentities says which change lines of filename are in the patch, as the +// identities a diff of that file shown anywhere can be compared against. Empty for a +// file that is no part of the patch. +func (p *PatchBuilder) IncludedLineIdentities(filename string) []LineIdentity { + info, ok := p.snapshotFileInfoMap()[filename] + if !ok || info.mode == UNSELECTED { + return nil + } + + included := set.NewFromSlice(info.includedLineIndices) + identities := []LineIdentity{} + for identity, idx := range ChangeLineIndexByIdentity(Parse(info.diff)) { + if included.Includes(idx) { + identities = append(identities, identity) + } + } + return identities +} + func (p *PatchBuilder) GetFileIncLineIndices(filename string, previousPath string) ([]int, error) { info, err := p.getFileInfo(filename, previousPath) if err != nil { diff --git a/pkg/commands/patch/patch_builder_test.go b/pkg/commands/patch/patch_builder_test.go new file mode 100644 index 000000000..9fd8caf80 --- /dev/null +++ b/pkg/commands/patch/patch_builder_test.go @@ -0,0 +1,75 @@ +package patch + +import ( + "testing" + + "github.com/sirupsen/logrus" + "github.com/stretchr/testify/assert" +) + +// newTestPatchBuilder returns a patch builder started for a dummy commit, in which +// every file's diff is the given one. +func newTestPatchBuilder(diff string) *PatchBuilder { + patchBuilder := NewPatchBuilder(logrus.New().WithField("test", "test"), + func(from string, to string, reverse bool, filename string, previousPath string) (string, error) { + return diff, nil + }) + patchBuilder.Start("from", "to", false, true) + return patchBuilder +} + +// In simpleDiff the deletion "-orange" is line index 6 of the parsed diff (line 2 of +// the old file) and the addition "+grape" is index 7 (line 2 of the new file). +func TestPatchLineIndicesForLines(t *testing.T) { + patchBuilder := newTestPatchBuilder(simpleDiff) + + indices, err := patchBuilder.PatchLineIndicesForLines("filename", "", []LineIdentity{ + {LineNumber: 2, IsDeletion: true}, // -orange + {LineNumber: 2, IsDeletion: false}, // +grape + {LineNumber: 1, IsDeletion: false}, // " apple", a context line + }) + assert.NoError(t, err) + assert.Equal(t, []int{6, 7}, indices, "the context line names no change line") +} + +// A renamed file's rename header makes its change lines sit further down the diff, and +// its old-file line numbers are of the file under its previous name. +func TestPatchLineIndicesForLinesOfARenamedFile(t *testing.T) { + patchBuilder := newTestPatchBuilder(renameWithModificationDiff) + + indices, err := patchBuilder.PatchLineIndicesForLines("newname", "oldname", []LineIdentity{ + {LineNumber: 2, IsDeletion: true}, // -orange + {LineNumber: 2, IsDeletion: false}, // +grape + }) + assert.NoError(t, err) + assert.Equal(t, []int{9, 10}, indices) +} + +func TestIncludedLineIdentities(t *testing.T) { + patchBuilder := newTestPatchBuilder(simpleDiff) + + // A file no part of the patch has nothing included. + assert.Empty(t, patchBuilder.IncludedLineIdentities("filename")) + + // With only the deletion in, only its identity comes back. + assert.NoError(t, patchBuilder.AddFileLineRange("filename", "", []int{6})) + assert.Equal(t, + []LineIdentity{{LineNumber: 2, IsDeletion: true}}, + patchBuilder.IncludedLineIdentities("filename")) + + // With the addition in as well, both do. + assert.NoError(t, patchBuilder.AddFileLineRange("filename", "", []int{7})) + assert.ElementsMatch(t, + []LineIdentity{{LineNumber: 2, IsDeletion: true}, {LineNumber: 2, IsDeletion: false}}, + patchBuilder.IncludedLineIdentities("filename")) +} + +// A file taken into the patch whole has every one of its change lines in it. +func TestIncludedLineIdentitiesOfAWholeFile(t *testing.T) { + patchBuilder := newTestPatchBuilder(simpleDiff) + + assert.NoError(t, patchBuilder.AddFileWhole("filename", "")) + assert.ElementsMatch(t, + []LineIdentity{{LineNumber: 2, IsDeletion: true}, {LineNumber: 2, IsDeletion: false}}, + patchBuilder.IncludedLineIdentities("filename")) +}