Support range select removing files from a commit (#3276)

- **PR Description**
Support adding range select for removing multiple files from a commit.
Closes #3260.

The approaches I saw were to either modify
`RebaseCommands.DiscardOldFileChanges` to accept multiple files or do
the file removals as part of the custom patch workflow. I ended up going
with the second way, but I'd be happy to re-implement with the first
approach if that's preferred.

I added a test that handles several different situations when removing
commit files, and updated the original test for removing a single file
from a commit.

I changed how the user gets informed when removing files from a commit.
The previous version had 2 prompts that I combined into 1 (because those
two situations are now possible to see at the same time), and I added
the total number of files that will be affected. This feature seems like
it could cause some real damage if used improperly, so I'm trying to let
the user know as much as possible.

There are 2 or 3 i18n strings that I removed because they're no longer
used. `RebaseCommands.DiscardOldFileChanges` isn't used anywhere any
more, but I left it in there anyway.

- **Please check if the PR fulfills these requirements**

* [x] Cheatsheets are up-to-date (run `go generate ./...`)
* [x] Code has been formatted (see
[here](https://github.com/jesseduffield/lazygit/blob/master/CONTRIBUTING.md#code-formatting))
* [x] Tests have been added/updated (see
[here](https://github.com/jesseduffield/lazygit/blob/master/pkg/integration/README.md)
for the integration test guide)
* [x] Text is internationalised (see
[here](https://github.com/jesseduffield/lazygit/blob/master/CONTRIBUTING.md#internationalisation))
* [ ] Docs (specifically `docs/Config.md`) have been updated if
necessary
* [x] You've read through your own file changes for silly mistakes etc

<!--
Be sure to name your PR with an imperative e.g. 'Add worktrees view'
see https://github.com/jesseduffield/lazygit/releases/tag/v0.40.0 for
examples
-->

pkg/commands/git_commands/rebase.go

@@ -423,23 +423,25 @@ func (self *RebaseCommands) runSkipEditorCommand(cmdObj oscommands.ICmdObj) erro
 }
 
 // DiscardOldFileChanges discards changes to a file from an old commit
-func (self *RebaseCommands) DiscardOldFileChanges(commits []*models.Commit, commitIndex int, fileName string) error {
+func (self *RebaseCommands) DiscardOldFileChanges(commits []*models.Commit, commitIndex int, filePaths []string) error {
 	if err := self.BeginInteractiveRebaseForCommit(commits, commitIndex, false); err != nil {
 		return err
 	}
 
-	// check if file exists in previous commit (this command returns an error if the file doesn't exist)
-	cmdArgs := NewGitCmd("cat-file").Arg("-e", "HEAD^:"+fileName).ToArgv()
+	for _, filePath := range filePaths {
+		// check if file exists in previous commit (this command returns an error if the file doesn't exist)
+		cmdArgs := NewGitCmd("cat-file").Arg("-e", "HEAD^:"+filePath).ToArgv()
 
-	if err := self.cmd.New(cmdArgs).Run(); err != nil {
-		if err := self.os.Remove(fileName); err != nil {
+		if err := self.cmd.New(cmdArgs).Run(); err != nil {
+			if err := self.os.Remove(filePath); err != nil {
+				return err
+			}
+			if err := self.workingTree.StageFile(filePath); err != nil {
+				return err
+			}
+		} else if err := self.workingTree.CheckoutFile("HEAD^", filePath); err != nil {
 			return err
 		}
-		if err := self.workingTree.StageFile(fileName); err != nil {
-			return err
-		}
-	} else if err := self.workingTree.CheckoutFile("HEAD^", fileName); err != nil {
-		return err
 	}
 
 	// amend the commit

pkg/commands/git_commands/rebase_test.go

@@ -111,7 +111,7 @@ func TestRebaseDiscardOldFileChanges(t *testing.T) {
 		gitConfigMockResponses map[string]string
 		commits                []*models.Commit
 		commitIndex            int
-		fileName               string
+		fileName               []string
 		runner                 *oscommands.FakeCmdObjRunner
 		test                   func(error)
 	}
@@ -122,7 +122,7 @@ func TestRebaseDiscardOldFileChanges(t *testing.T) {
 			gitConfigMockResponses: nil,
 			commits:                []*models.Commit{},
 			commitIndex:            0,
-			fileName:               "test999.txt",
+			fileName:               []string{"test999.txt"},
 			runner:                 oscommands.NewFakeRunner(t),
 			test: func(err error) {
 				assert.Error(t, err)
@@ -133,7 +133,7 @@ func TestRebaseDiscardOldFileChanges(t *testing.T) {
 			gitConfigMockResponses: map[string]string{"commit.gpgsign": "true"},
 			commits:                []*models.Commit{{Name: "commit", Sha: "123456"}},
 			commitIndex:            0,
-			fileName:               "test999.txt",
+			fileName:               []string{"test999.txt"},
 			runner:                 oscommands.NewFakeRunner(t),
 			test: func(err error) {
 				assert.Error(t, err)
@@ -147,7 +147,7 @@ func TestRebaseDiscardOldFileChanges(t *testing.T) {
 				{Name: "commit2", Sha: "abcdef"},
 			},
 			commitIndex: 0,
-			fileName:    "test999.txt",
+			fileName:    []string{"test999.txt"},
 			runner: oscommands.NewFakeRunner(t).
 				ExpectGitArgs([]string{"rebase", "--interactive", "--autostash", "--keep-empty", "--no-autosquash", "--rebase-merges", "abcdef"}, "", nil).
 				ExpectGitArgs([]string{"cat-file", "-e", "HEAD^:test999.txt"}, "", nil).

pkg/gui/controllers/commits_files_controller.go

@@ -50,8 +50,8 @@ func (self *CommitFilesController) GetKeybindings(opts types.KeybindingsOpts) []
 		},
 		{
 			Key:               opts.GetKey(opts.Config.Universal.Remove),
-			Handler:           self.withItem(self.discard),
-			GetDisabledReason: self.require(self.singleItemSelected()),
+			Handler:           self.withItems(self.discard),
+			GetDisabledReason: self.require(self.itemsSelected()),
 			Description:       self.c.Tr.Remove,
 			Tooltip:           self.c.Tr.DiscardOldFileChangeTooltip,
 			DisplayOnScreen:   true,
@@ -176,40 +176,51 @@ func (self *CommitFilesController) checkout(node *filetree.CommitFileNode) error
 	return self.c.Refresh(types.RefreshOptions{Mode: types.ASYNC})
 }
 
-func (self *CommitFilesController) discard(node *filetree.CommitFileNode) error {
+func (self *CommitFilesController) discard(selectedNodes []*filetree.CommitFileNode) error {
 	parentContext, ok := self.c.CurrentContext().GetParentContext()
 	if !ok || parentContext.GetKey() != context.LOCAL_COMMITS_CONTEXT_KEY {
 		return self.c.ErrorMsg(self.c.Tr.CanOnlyDiscardFromLocalCommits)
 	}
 
-	if node.File == nil {
-		return self.c.ErrorMsg(self.c.Tr.DiscardNotSupportedForDirectory)
-	}
-
 	if ok, err := self.c.Helpers().PatchBuilding.ValidateNormalWorkingTreeState(); !ok {
 		return err
 	}
 
-	prompt := self.c.Tr.DiscardFileChangesPrompt
-	if node.File.Added() {
-		prompt = self.c.Tr.DiscardAddedFileChangesPrompt
-	} else if node.File.Deleted() {
-		prompt = self.c.Tr.DiscardDeletedFileChangesPrompt
-	}
-
 	return self.c.Confirm(types.ConfirmOpts{
 		Title:  self.c.Tr.DiscardFileChangesTitle,
-		Prompt: prompt,
+		Prompt: self.c.Tr.DiscardFileChangesPrompt,
 		HandleConfirm: func() error {
 			return self.c.WithWaitingStatus(self.c.Tr.RebasingStatus, func(gocui.Task) error {
-				self.c.LogAction(self.c.Tr.Actions.DiscardOldFileChange)
-				if err := self.c.Git().Rebase.DiscardOldFileChanges(self.c.Model().Commits, self.c.Contexts().LocalCommits.GetSelectedLineIdx(), node.GetPath()); err != nil {
-					if err := self.c.Helpers().MergeAndRebase.CheckMergeOrRebase(err); err != nil {
+				var filePaths []string
+				selectedNodes = normalisedSelectedCommitFileNodes(selectedNodes)
+
+				// Reset the current patch if there is one.
+				if self.c.Git().Patch.PatchBuilder.Active() {
+					self.c.Git().Patch.PatchBuilder.Reset()
+					if err := self.c.Refresh(types.RefreshOptions{Mode: types.BLOCK_UI}); err != nil {
 						return err
 					}
 				}
 
-				return self.c.Refresh(types.RefreshOptions{Mode: types.BLOCK_UI})
+				for _, node := range selectedNodes {
+					err := node.ForEachFile(func(file *models.CommitFile) error {
+						filePaths = append(filePaths, file.GetPath())
+						return nil
+					})
+					if err != nil {
+						return self.c.Error(err)
+					}
+				}
+
+				err := self.c.Git().Rebase.DiscardOldFileChanges(self.c.Model().Commits, self.c.Contexts().LocalCommits.GetSelectedLineIdx(), filePaths)
+				if err := self.c.Helpers().MergeAndRebase.CheckMergeOrRebase(err); err != nil {
+					return err
+				}
+
+				if self.context().RangeSelectEnabled() {
+					self.context().GetList().CancelRangeSelect()
+				}
+				return self.c.Refresh(types.RefreshOptions{Mode: types.SYNC})
 			})
 		},
 	})

pkg/i18n/english.go

@@ -354,9 +354,6 @@ type TranslationSet struct {
 	DiscardOldFileChangeTooltip           string
 	DiscardFileChangesTitle               string
 	DiscardFileChangesPrompt              string
-	DiscardAddedFileChangesPrompt         string
-	DiscardDeletedFileChangesPrompt       string
-	DiscardNotSupportedForDirectory       string
 	DisabledForGPG                        string
 	CreateRepo                            string
 	BareRepo                              string
@@ -420,6 +417,7 @@ type TranslationSet struct {
 	ScrollRight                           string
 	DiscardPatch                          string
 	DiscardPatchConfirm                   string
+	DiscardPatchSameCommitConfirm         string
 	CantPatchWhileRebasingError           string
 	ToggleAddToPatch                      string
 	ToggleAddToPatchTooltip               string
@@ -1295,10 +1293,7 @@ func EnglishTranslationSet() TranslationSet {
 		Remove:                              "Remove",
 		DiscardOldFileChangeTooltip:         "Discard this commit's changes to this file. This runs an interactive rebase in the background, so you may get a merge conflict if a later commit also changes this file.",
 		DiscardFileChangesTitle:             "Discard file changes",
-		DiscardFileChangesPrompt:            "Are you sure you want to discard this commit's changes to this file?",
-		DiscardAddedFileChangesPrompt:       "Are you sure you want to discard this commit's changes to this file? The file was added in this commit, so it will be deleted again.",
-		DiscardDeletedFileChangesPrompt:     "Are you sure you want to discard this commit's changes to this file? The file was deleted in this commit, so it will reappear.",
-		DiscardNotSupportedForDirectory:     "Discarding changes is not supported for entire directories. Please use a custom patch for this.",
+		DiscardFileChangesPrompt:            "Are you sure you want to remove changes to the selected file(s) from this commit?\n\nThis action will start a rebase, reverting these file changes. Be aware that if subsequent commits depend on these changes, you may need to resolve conflicts.\nNote: This will also reset any active custom patches.",
 		DisabledForGPG:                      "Feature not available for users using GPG",
 		CreateRepo:                          "Not in a git repository. Create a new git repository? (y/n): ",
 		BareRepo:                            "You've attempted to open Lazygit in a bare repo but Lazygit does not yet support bare repos. Open most recent repo? (y/n) ",
@@ -1363,6 +1358,7 @@ func EnglishTranslationSet() TranslationSet {
 		ScrollRight:                         "Scroll right",
 		DiscardPatch:                        "Discard patch",
 		DiscardPatchConfirm:                 "You can only build a patch from one commit/stash-entry at a time. Discard current patch?",
+		DiscardPatchSameCommitConfirm:       "You currently have changes added to a patch for this commit. Discard current patch?",
 		CantPatchWhileRebasingError:         "You cannot build a patch or run patch commands while in a merging or rebasing state",
 		ToggleAddToPatch:                    "Toggle file included in patch",
 		ToggleAddToPatchTooltip:             "Toggle whether the file is included in the custom patch. See {{.doc}}.",

pkg/i18n/russian.go

@@ -289,10 +289,7 @@ func RussianTranslationSet() TranslationSet {
 		CanOnlyDiscardFromLocalCommits:      "Изменения можно отменить только из локальных коммитов.",
 		DiscardOldFileChangeTooltip:         "Отменить изменения коммита в этом файле",
 		DiscardFileChangesTitle:             "Отменить изменения файла",
-		DiscardFileChangesPrompt:            "Вы уверены, что хотите отменить изменения коммита в этом файле? Если файл был создан в этом коммите, он будет удалён",
-		DiscardAddedFileChangesPrompt:       "Вы уверены, что хотите отменить изменения, внесённые в этот файл коммитом? Файл был добавлен в этот коммит, поэтому он снова будет удален.",
-		DiscardDeletedFileChangesPrompt:     "Вы уверены, что хотите отменить изменения, внесённые в этот файл коммитом? Файл был удалён в этом коммите, поэтому он снова появится.",
-		DiscardNotSupportedForDirectory:     "Отмена изменений не поддерживается для всех каталогов. Используйте для этого специальный патч.",
+		DiscardFileChangesPrompt:            "Вы уверены, что хотите удалить изменения в выбранных файлах из этого коммита?\n\nЭто действие запустит перебазирование и отменит изменения в этих файлах. Обратите внимание, что если последующие коммиты зависят от этих изменений, вам, возможно, придется разрешить конфликты.\nПримечание: это также сбросит все активные пользовательские патчи.",
 		DisabledForGPG:                      "Функция недоступна для пользователей, использующих GPG",
 		CreateRepo:                          "Не в git репозитории. Создать новый git репозиторий? (y/n):",
 		BareRepo:                            "Вы пытались открыть Lazygit в пустом репозитории, но Lazygit ещё не поддерживает пустые репозитории. Открыть последний репозиторий? (y/n)",

pkg/integration/tests/commit/discard_old_file_change.go

@@ -1,57 +0,0 @@
-package commit
-
-import (
-	"github.com/jesseduffield/lazygit/pkg/config"
-	. "github.com/jesseduffield/lazygit/pkg/integration/components"
-)
-
-var DiscardOldFileChange = NewIntegrationTest(NewIntegrationTestArgs{
-	Description:  "Discarding a single file from an old commit (does rebase in background to remove the file but retain the other one)",
-	ExtraCmdArgs: []string{},
-	Skip:         false,
-	SetupConfig:  func(config *config.AppConfig) {},
-	SetupRepo: func(shell *Shell) {
-		shell.CreateFileAndAdd("file0", "file0")
-		shell.Commit("first commit")
-
-		shell.CreateFileAndAdd("file1", "file2")
-		shell.CreateFileAndAdd("fileToRemove", "fileToRemove")
-		shell.Commit("commit to change")
-
-		shell.CreateFileAndAdd("file3", "file3")
-		shell.Commit("third commit")
-	},
-	Run: func(t *TestDriver, keys config.KeybindingConfig) {
-		t.Views().Commits().
-			Focus().
-			Lines(
-				Contains("third commit").IsSelected(),
-				Contains("commit to change"),
-				Contains("first commit"),
-			).
-			SelectNextItem().
-			PressEnter()
-
-		t.Views().CommitFiles().
-			IsFocused().
-			Lines(
-				Contains("file1").IsSelected(),
-				Contains("fileToRemove"),
-			).
-			SelectNextItem().
-			Press(keys.Universal.Remove)
-
-		t.ExpectPopup().Confirmation().
-			Title(Equals("Discard file changes")).
-			Content(Equals("Are you sure you want to discard this commit's changes to this file? The file was added in this commit, so it will be deleted again.")).
-			Confirm()
-
-		t.Views().CommitFiles().
-			IsFocused().
-			Lines(
-				Contains("file1").IsSelected(),
-			)
-
-		t.FileSystem().PathNotPresent("fileToRemove")
-	},
-})

pkg/integration/tests/commit/discard_old_file_changes.go

@@ -0,0 +1,170 @@
+package commit
+
+import (
+	"github.com/jesseduffield/lazygit/pkg/config"
+	. "github.com/jesseduffield/lazygit/pkg/integration/components"
+)
+
+var DiscardOldFileChanges = NewIntegrationTest(NewIntegrationTestArgs{
+	Description:  "Discarding a range of files from an old commit.",
+	ExtraCmdArgs: []string{},
+	Skip:         false,
+	SetupConfig:  func(config *config.AppConfig) {},
+	SetupRepo: func(shell *Shell) {
+		shell.CreateFileAndAdd("dir1/d1_file0", "file0\n")
+		shell.CreateFileAndAdd("dir1/subd1/subfile0", "file1\n")
+		shell.CreateFileAndAdd("dir2/d2_file1", "d2f1 content\n")
+		shell.CreateFileAndAdd("dir2/d2_file2", "d2f4 content\n")
+		shell.Commit("remove one file from this commit")
+
+		shell.UpdateFileAndAdd("dir2/d2_file1", "d2f1 content\nsecond line\n")
+		shell.DeleteFileAndAdd("dir2/d2_file2")
+		shell.CreateFileAndAdd("dir2/d2_file3", "d2f3 content\n")
+		shell.CreateFileAndAdd("dir2/d2_file4", "d2f2 content\n")
+		shell.Commit("remove four files from this commit")
+
+		shell.CreateFileAndAdd("dir1/fileToRemove", "file to remove content\n")
+		shell.CreateFileAndAdd("dir1/multiLineFile", "this file has\ncontent on\nthree lines\n")
+		shell.CreateFileAndAdd("dir1/subd1/file2ToRemove", "file2 to remove content\n")
+		shell.Commit("remove changes in multiple dirs from this commit")
+	},
+	Run: func(t *TestDriver, keys config.KeybindingConfig) {
+		t.Views().Commits().
+			Focus().
+			Lines(
+				Contains("remove changes in multiple dirs from this commit").IsSelected(),
+				Contains("remove four files from this commit"),
+				Contains("remove one file from this commit"),
+			).
+			NavigateToLine(Contains("remove one file from this commit")).
+			PressEnter()
+
+		// Check removing a single file from an old commit
+		t.Views().CommitFiles().
+			IsFocused().
+			Lines(
+				Contains("dir1").IsSelected(),
+				Contains("subd1"),
+				Contains("subfile0"),
+				Contains("d1_file0"),
+				Contains("dir2"),
+				Contains("d2_file1"),
+				Contains("d2_file2"),
+			).
+			NavigateToLine(Contains("d1_file0")).
+			Press(keys.Universal.Remove)
+
+		t.ExpectPopup().Confirmation().
+			Title(Equals("Discard file changes")).
+			Content(Equals("Are you sure you want to remove changes to the selected file(s) from this commit?\n\nThis action will start a rebase, reverting these file changes. Be aware that if subsequent commits depend on these changes, you may need to resolve conflicts.\nNote: This will also reset any active custom patches.")).
+			Confirm()
+
+		t.Views().CommitFiles().
+			IsFocused().
+			Lines(
+				Contains("dir1/subd1"),
+				Contains("subfile0"),
+				Contains("dir2"),
+				Contains("d2_file1").IsSelected(),
+				Contains("d2_file2"),
+			).
+			PressEscape()
+
+		// Check removing 4 files in the same directory
+		t.Views().Commits().
+			Focus().
+			Lines(
+				Contains("remove changes in multiple dirs from this commit"),
+				Contains("remove four files from this commit"),
+				Contains("remove one file from this commit").IsSelected(),
+			).
+			NavigateToLine(Contains("remove four files from this commit")).
+			PressEnter()
+
+		t.Views().CommitFiles().
+			IsFocused().
+			Lines(
+				Contains("dir2").IsSelected(),
+				Contains("d2_file1"),
+				Contains("d2_file2"),
+				Contains("d2_file3"),
+				Contains("d2_file4"),
+			).
+			NavigateToLine(Contains("d2_file1")).
+			Press(keys.Universal.ToggleRangeSelect).
+			NavigateToLine(Contains("d2_file4")).
+			Press(keys.Universal.Remove)
+
+		t.ExpectPopup().Confirmation().
+			Title(Equals("Discard file changes")).
+			Content(Equals("Are you sure you want to remove changes to the selected file(s) from this commit?\n\nThis action will start a rebase, reverting these file changes. Be aware that if subsequent commits depend on these changes, you may need to resolve conflicts.\nNote: This will also reset any active custom patches.")).
+			Confirm()
+
+		t.Views().CommitFiles().
+			IsFocused().
+			Lines(
+				Contains("(none)"),
+			).
+			PressEscape()
+
+		// Check removing multiple files from 2 directories w/ a custom patch.
+		// This checks node selection logic & if the custom patch is getting reset.
+		t.Views().Commits().
+			IsFocused().
+			Lines(
+				Contains("remove changes in multiple dirs from this commit"),
+				Contains("remove four files from this commit").IsSelected(),
+				Contains("remove one file from this commit"),
+			).
+			NavigateToLine(Contains("remove changes in multiple dirs from this commit")).
+			PressEnter()
+
+		t.Views().CommitFiles().
+			IsFocused().
+			Lines(
+				Contains("dir1").IsSelected(),
+				Contains("subd1"),
+				Contains("file2ToRemove"),
+				Contains("fileToRemove"),
+				Contains("multiLineFile"),
+			).
+			NavigateToLine(Contains("multiLineFile")).
+			PressEnter()
+
+		t.Views().PatchBuilding().
+			IsFocused().
+			SelectedLine(
+				Contains("+this file has"),
+			).
+			PressPrimaryAction().
+			PressEscape()
+
+		t.Views().CommitFiles().
+			IsFocused().
+			Lines(
+				Contains("dir1"),
+				Contains("subd1"),
+				Contains("file2ToRemove"),
+				Contains("fileToRemove"),
+				Contains("multiLineFile").IsSelected(),
+			).
+			NavigateToLine(Contains("dir1")).
+			Press(keys.Universal.ToggleRangeSelect).
+			NavigateToLine(Contains("subd1")).
+			Press(keys.Universal.Remove)
+
+		t.ExpectPopup().Confirmation().
+			Title(Equals("Discard file changes")).
+			Content(Equals("Are you sure you want to remove changes to the selected file(s) from this commit?\n\nThis action will start a rebase, reverting these file changes. Be aware that if subsequent commits depend on these changes, you may need to resolve conflicts.\nNote: This will also reset any active custom patches.")).
+			Confirm()
+
+		// "Building patch" will still be in this view if the patch isn't reset properly
+		t.Views().Information().Content(DoesNotContain("Building patch"))
+
+		t.Views().CommitFiles().
+			IsFocused().
+			Lines(
+				Contains("(none)"),
+			)
+	},
+})

pkg/integration/tests/test_list.go

@@ -70,7 +70,7 @@ var tests = []*components.IntegrationTest{
 	commit.CommitWipWithPrefix,
 	commit.CommitWithPrefix,
 	commit.CreateTag,
-	commit.DiscardOldFileChange,
+	commit.DiscardOldFileChanges,
 	commit.FindBaseCommitForFixup,
 	commit.FindBaseCommitForFixupWarningForAddedLines,
 	commit.Highlight,