Repository: jesseduffield/lazygit (language: go)

Issue:
Support range select for removing multiple files from a commit with 'd'
**Is your feature request related to a problem? Please describe.**
You can currently remove a file from a commit with 'd' without needing to explicitly make a custom patch. It would be good if this can support a selection range.

**Describe the solution you'd like**
When you select a range of files in the commit files view and press 'd', you can remove all the selected files from the commit in one go. We'll need to update the wording on the confirmation popup to say 'the selected file(s)' rather than 'this file'.

This should wait for https://github.com/jesseduffield/lazygit/pull/3259 to be merged because it will be making use of some of that PR's code.




Discussion / hints:
let me know if you wanna take this one @afhoffman
> let me know if you wanna take this one @afhoffman

Sure! I'd be happy to.

Fix patch:
diff --git a/pkg/commands/git_commands/rebase.go b/pkg/commands/git_commands/rebase.go
index 03d4030acf2..0bcfa5f67dd 100644
--- a/pkg/commands/git_commands/rebase.go
+++ b/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()
-
-	if err := self.cmd.New(cmdArgs).Run(); err != nil {
-		if err := self.os.Remove(fileName); err != nil {
-			return err
-		}
-		if err := self.workingTree.StageFile(fileName); err != nil {
+	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(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
 		}
-	} else if err := self.workingTree.CheckoutFile("HEAD^", fileName); err != nil {
-		return err
 	}
 
 	// amend the commit
diff --git a/pkg/gui/controllers/commits_files_controller.go b/pkg/gui/controllers/commits_files_controller.go
index 647d6594bb7..59e0a7f4ecb 100644
--- a/pkg/gui/controllers/commits_files_controller.go
+++ b/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})
 			})
 		},
 	})
diff --git a/pkg/i18n/english.go b/pkg/i18n/english.go
index 048f0e6c44a..da8dbc809e7 100644
--- a/pkg/i18n/english.go
+++ b/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}}.",
diff --git a/pkg/i18n/russian.go b/pkg/i18n/russian.go
index af3fdd6bd1a..e27ebabff1b 100644
--- a/pkg/i18n/russian.go
+++ b/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)",


Test patch:
diff --git a/pkg/commands/git_commands/rebase_test.go b/pkg/commands/git_commands/rebase_test.go
index d10746220eb..b84621497f6 100644
--- a/pkg/commands/git_commands/rebase_test.go
+++ b/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).
diff --git a/pkg/integration/tests/commit/discard_old_file_change.go b/pkg/integration/tests/commit/discard_old_file_change.go
deleted file mode 100644
index 5d5fcc0c8ea..00000000000
--- a/pkg/integration/tests/commit/discard_old_file_change.go
+++ /dev/null
@@ -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")
-	},
-})
diff --git a/pkg/integration/tests/commit/discard_old_file_changes.go b/pkg/integration/tests/commit/discard_old_file_changes.go
new file mode 100644
index 00000000000..1ab189264d7
--- /dev/null
+++ b/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)"),
+			)
+	},
+})
diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go
index cfadde3b133..6a448fff436 100644
--- a/pkg/integration/tests/test_list.go
+++ b/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,