From 6e9a84cf30734f6527988930cd8a09c551b70518 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Thu, 20 Aug 2026 19:05:10 +0200 Subject: [PATCH] Preserve whole-file operations for added and deleted files A focused diff must replace the patch builder's whole-file toggle without losing file creation or deletion metadata. Ordinary modifications and renames must remain line-level selections even when every changed content line is selected. Ask the patch builder's canonical raw diff whether it consists of one hunk containing only additions or only deletions and no context. Use the whole-file operation only when the selection also covers every change. Co-Authored-By: GitHub Copilot --- pkg/commands/patch/patch_builder.go | 14 +++- pkg/commands/patch/patch_builder_test.go | 37 ++++++++- pkg/gui/controllers/commit_diff_actions.go | 77 +++++++++++++------ .../tests/main_view/move_patch_to_index.go | 3 - 4 files changed, 100 insertions(+), 31 deletions(-) diff --git a/pkg/commands/patch/patch_builder.go b/pkg/commands/patch/patch_builder.go index fee20c455..7b619e369 100644 --- a/pkg/commands/patch/patch_builder.go +++ b/pkg/commands/patch/patch_builder.go @@ -442,16 +442,22 @@ func ChangeLineIndicesForLines(parsed *Patch, lines []LineIdentity) []int { } // PatchLineIndicesForLines maps change lines of filename to their indices in that -// file's diff, which is what the patch is built in terms of. +// file's diff; the patch is built in terms of those indices. everyChange reports +// whether the given lines cover all of the file's changes; this distinguishes acting +// on some of a file's lines from acting on the file itself. func (p *PatchBuilder) PatchLineIndicesForLines( filename string, previousPath string, lines []LineIdentity, -) ([]int, error) { +) (indices []int, everyChange bool, err error) { info, err := p.getFileInfo(filename, previousPath) if err != nil { - return nil, err + return nil, false, err } - return ChangeLineIndicesForLines(Parse(info.diff), lines), nil + parsed := Parse(info.diff) + selected := set.NewFromSlice(lines) + everyChange = lo.EveryBy(maps.Keys(ChangeLineIndexByIdentity(parsed)), + func(identity LineIdentity) bool { return selected.Includes(identity) }) + return ChangeLineIndicesForLines(parsed, lines), everyChange, nil } // IncludedLineIdentities says which change lines of filename are in the patch, as the diff --git a/pkg/commands/patch/patch_builder_test.go b/pkg/commands/patch/patch_builder_test.go index cb6bb3684..883c6761a 100644 --- a/pkg/commands/patch/patch_builder_test.go +++ b/pkg/commands/patch/patch_builder_test.go @@ -26,13 +26,14 @@ func newTestPatchBuilder(diff string) *PatchBuilder { func TestPatchLineIndicesForLines(t *testing.T) { patchBuilder := newTestPatchBuilder(simpleDiff) - indices, err := patchBuilder.PatchLineIndicesForLines("filename", "", []LineIdentity{ + indices, everyChange, 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") + assert.True(t, everyChange, "the two changes are all the diff has") } // A renamed file's rename header makes its change lines sit further down the diff, and @@ -40,12 +41,44 @@ func TestPatchLineIndicesForLines(t *testing.T) { func TestPatchLineIndicesForLinesOfARenamedFile(t *testing.T) { patchBuilder := newTestPatchBuilder(renameWithModificationDiff) - indices, err := patchBuilder.PatchLineIndicesForLines("newname", "oldname", []LineIdentity{ + indices, everyChange, 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) + assert.True(t, everyChange) +} + +// everyChange is about the diff alone: whether anything the file changes was left out +// of the selection. What that then means for the patch is the caller's question. +func TestPatchLineIndicesForLinesEveryChange(t *testing.T) { + patchBuilder := newTestPatchBuilder(newFile) + + _, everyChange, err := patchBuilder.PatchLineIndicesForLines("newfile", "", []LineIdentity{ + {LineNumber: 1}, + {LineNumber: 2}, + }) + assert.NoError(t, err) + assert.False(t, everyChange, "the file's third added line is left out") + + _, everyChange, err = patchBuilder.PatchLineIndicesForLines("newfile", "", []LineIdentity{ + {LineNumber: 1}, + {LineNumber: 2}, + {LineNumber: 3}, + }) + assert.NoError(t, err) + assert.True(t, everyChange) + + // A line the diff doesn't have doesn't stand in for one it does. + patchBuilder = newTestPatchBuilder(deletedFile) + _, everyChange, err = patchBuilder.PatchLineIndicesForLines("newfile", "", []LineIdentity{ + {LineNumber: 1, IsDeletion: true}, + {LineNumber: 2, IsDeletion: true}, + {LineNumber: 4, IsDeletion: true}, + }) + assert.NoError(t, err) + assert.False(t, everyChange) } func TestIncludedLineIdentities(t *testing.T) { diff --git a/pkg/gui/controllers/commit_diff_actions.go b/pkg/gui/controllers/commit_diff_actions.go index b23c0961a..bf894fb43 100644 --- a/pkg/gui/controllers/commit_diff_actions.go +++ b/pkg/gui/controllers/commit_diff_actions.go @@ -169,7 +169,7 @@ func (self *CommitDiffActions) removePatchLines( } patchBuilder := self.c.Git().Patch.PatchBuilder - previousPaths := self.previousPaths() + files := self.filesInDiff() for path, ordinals := range self.c.Helpers().DiffLine.ChangeLineOrdinals(self.customPatchDiff(), lines) { filename := self.patchBuilderPath(path) if filename == "" { @@ -185,7 +185,7 @@ func (self *CommitDiffActions) removePatchLines( if len(indices) == 0 { continue } - if err := patchBuilder.RemoveFileLineRange(filename, previousPaths[filename], indices); err != nil { + if err := patchBuilder.RemoveFileLineRange(filename, files.previousPath(filename), indices); err != nil { return err } } @@ -346,41 +346,78 @@ func (self *CommitDiffActions) togglePatchLines(lines []types.DiffLineInfo) erro return nil } - previousPaths := self.previousPaths() + files := self.filesInDiff() indicesByPath := map[string][]int{} + wholeFileByPath := map[string]bool{} for _, path := range paths { - indices, err := patchBuilder.PatchLineIndicesForLines(path, previousPaths[path], linesByPath[path]) + indices, everyChange, err := patchBuilder.PatchLineIndicesForLines( + path, files.previousPath(path), linesByPath[path]) if err != nil { return err } indicesByPath[path] = indices + + // Selecting every change of a file the commit adds or deletes is selecting the + // file: what the commit did to it is not something its content lines can carry, + // so a patch of those alone would move the content and leave the file behind. + wholeFileByPath[path] = everyChange && files.isWholeFileOperation(path) } - included, err := patchBuilder.GetFileIncLineIndices(paths[0], previousPaths[paths[0]]) + included, err := patchBuilder.GetFileIncLineIndices(paths[0], files.previousPath(paths[0])) if err != nil { return err } - toggle := patchBuilder.AddFileLineRange - if first := indicesByPath[paths[0]]; len(first) > 0 && lo.Contains(included, first[0]) { - toggle = patchBuilder.RemoveFileLineRange - } + removing := len(indicesByPath[paths[0]]) > 0 && lo.Contains(included, indicesByPath[paths[0]][0]) for _, path := range paths { if len(indicesByPath[path]) == 0 { continue } - if err := toggle(path, previousPaths[path], indicesByPath[path]); err != nil { + previousPath := files.previousPath(path) + var err error + switch { + case wholeFileByPath[path] && removing: + err = patchBuilder.RemoveFile(path, previousPath) + case wholeFileByPath[path]: + err = patchBuilder.AddFileWhole(path, previousPath) + case removing: + err = patchBuilder.RemoveFileLineRange(path, previousPath, indicesByPath[path]) + default: + err = patchBuilder.AddFileLineRange(path, previousPath, indicesByPath[path]) + } + if err != nil { return err } } return nil } -// previousPaths says which files of the diff were renamed, and what they were called -// before. A renamed file's diff only comes out as a rename when git is asked about both -// of its paths, and its lines are numbered in the file under its old name, so the patch -// builder has to be told the old path along with them. -func (self *CommitDiffActions) previousPaths() map[string]string { +// commitDiffFiles records what the diff a patch is being built from says about each of +// the files it covers, by the path the diff shows them under. +type commitDiffFiles map[string]*models.CommitFile + +// previousPath says what a file of the diff was called before, and is empty for one +// that wasn't renamed. A renamed file's diff only comes out as a rename when git is +// asked about both of its paths, and its lines are numbered in the file under its old +// name, so the patch builder has to be told the old path along with them. +func (self commitDiffFiles) previousPath(path string) string { + if file, ok := self[path]; ok { + return file.PreviousPath + } + return "" +} + +// isWholeFileOperation reports whether what the commit did to this file is something +// its diff's content lines don't say: creating it or deleting it, which the file +// header carries and a patch built from lines alone would leave out. +func (self commitDiffFiles) isWholeFileOperation(path string) bool { + file, ok := self[path] + return ok && (file.Added() || file.Deleted()) +} + +// filesInDiff asks git which files the diff a patch is being built from covers, and +// what it does to each. +func (self *CommitDiffActions) filesInDiff() commitDiffFiles { target := self.target() if target == nil { return nil @@ -391,13 +428,9 @@ func (self *CommitDiffActions) previousPaths() map[string]string { return nil } - previousPaths := map[string]string{} - for _, file := range files { - if file.PreviousPath != "" { - previousPaths[file.Path] = file.PreviousPath - } - } - return previousPaths + return lo.SliceToMap(files, func(file *models.CommitFile) (string, *models.CommitFile) { + return file.Path, file + }) } // patchEndpoints gives the two ends of the diff a patch is built from. They are the ends diff --git a/pkg/integration/tests/main_view/move_patch_to_index.go b/pkg/integration/tests/main_view/move_patch_to_index.go index d6fae2f32..ed4886a96 100644 --- a/pkg/integration/tests/main_view/move_patch_to_index.go +++ b/pkg/integration/tests/main_view/move_patch_to_index.go @@ -43,10 +43,7 @@ var MovePatchToIndex = NewIntegrationTest(NewIntegrationTestArgs{ t.Common().SelectPatchOption(Contains("Move patch out into index")) t.Views().Files().Lines( - /* EXPECTED: Contains("A").Contains("file1"), - ACTUAL: */ - Contains("M").Contains("file1"), ) t.Views().Main(). IsFocused().