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 <copilot@github.com>

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

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) {

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

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