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().