Allow reverting a range of commits (#4444)

- **PR Description**

This is part four of a four part series of PRs that improve the
cherry-pick and revert experience.

With this PR we support reverting a range selection of commits.

Fixes #3272

Author: stefanhaller

Diff for: pkg/commands/git_commands/commit.go

@@ -285,15 +285,14 @@ func (self *CommitCommands) ShowFileContentCmdObj(hash string, filePath string)
 	return self.cmd.New(cmdArgs).DontLog()
 }
 
-// Revert reverts the selected commit by hash
-func (self *CommitCommands) Revert(hash string) error {
-	cmdArgs := NewGitCmd("revert").Arg(hash).ToArgv()
-
-	return self.cmd.New(cmdArgs).Run()
-}
-
-func (self *CommitCommands) RevertMerge(hash string, parentNumber int) error {
-	cmdArgs := NewGitCmd("revert").Arg(hash, "-m", fmt.Sprintf("%d", parentNumber)).
+// Revert reverts the selected commits by hash. If isMerge is true, we'll pass -m 1
+// to say we want to revert the first parent of the merge commit, which is the one
+// people want in 99.9% of cases. In current git versions we could unconditionally
+// pass -m 1 even for non-merge commits, but older versions of git choke on it.
+func (self *CommitCommands) Revert(hashes []string, isMerge bool) error {
+	cmdArgs := NewGitCmd("revert").
+		ArgIf(isMerge, "-m", "1").
+		Arg(hashes...).
 		ToArgv()
 
 	return self.cmd.New(cmdArgs).Run()

Diff for: pkg/gui/controllers/local_commits_controller.go

@@ -1,7 +1,6 @@
 package controllers
 
 import (
-	"fmt"
 	"strings"
 
 	"github.com/go-errors/errors"
@@ -249,8 +248,8 @@ func (self *LocalCommitsController) GetKeybindings(opts types.KeybindingsOpts) [
 		},
 		{
 			Key:               opts.GetKey(opts.Config.Commits.RevertCommit),
-			Handler:           self.withItem(self.revert),
-			GetDisabledReason: self.require(self.singleItemSelected()),
+			Handler:           self.withItemsRange(self.revert),
+			GetDisabledReason: self.require(self.itemRangeSelected()),
 			Description:       self.c.Tr.Revert,
 			Tooltip:           self.c.Tr.RevertCommitTooltip,
 		},
@@ -858,66 +857,41 @@ func (self *LocalCommitsController) addCoAuthor(start, end int) error {
 	return nil
 }
 
-func (self *LocalCommitsController) revert(commit *models.Commit) error {
-	if commit.IsMerge() {
-		return self.createRevertMergeCommitMenu(commit)
+func (self *LocalCommitsController) revert(commits []*models.Commit, start, end int) error {
+	var promptText string
+	if len(commits) == 1 {
+		promptText = utils.ResolvePlaceholderString(
+			self.c.Tr.ConfirmRevertCommit,
+			map[string]string{
+				"selectedCommit": commits[0].ShortHash(),
+			})
+	} else {
+		promptText = self.c.Tr.ConfirmRevertCommitRange
 	}
+	hashes := lo.Map(commits, func(c *models.Commit, _ int) string { return c.Hash })
+	isMerge := lo.SomeBy(commits, func(c *models.Commit) bool { return c.IsMerge() })
 
 	self.c.Confirm(types.ConfirmOpts{
-		Title: self.c.Tr.Actions.RevertCommit,
-		Prompt: utils.ResolvePlaceholderString(
-			self.c.Tr.ConfirmRevertCommit,
-			map[string]string{
-				"selectedCommit": commit.ShortHash(),
-			}),
+		Title:  self.c.Tr.Actions.RevertCommit,
+		Prompt: promptText,
 		HandleConfirm: func() error {
 			self.c.LogAction(self.c.Tr.Actions.RevertCommit)
 			return self.c.WithWaitingStatusSync(self.c.Tr.RevertingStatus, func() error {
-				result := self.c.Git().Commit.Revert(commit.Hash)
+				result := self.c.Git().Commit.Revert(hashes, isMerge)
 				if err := self.c.Helpers().MergeAndRebase.CheckMergeOrRebase(result); err != nil {
 					return err
 				}
-				return self.afterRevertCommit()
+				self.context().MoveSelection(len(commits))
+				return self.c.Refresh(types.RefreshOptions{
+					Mode: types.SYNC, Scope: []types.RefreshableView{types.COMMITS, types.BRANCHES},
+				})
 			})
 		},
 	})
 
 	return nil
 }
 
-func (self *LocalCommitsController) createRevertMergeCommitMenu(commit *models.Commit) error {
-	menuItems := make([]*types.MenuItem, len(commit.Parents))
-	for i, parentHash := range commit.Parents {
-		message, err := self.c.Git().Commit.GetCommitMessageFirstLine(parentHash)
-		if err != nil {
-			return err
-		}
-
-		menuItems[i] = &types.MenuItem{
-			Label: fmt.Sprintf("%s: %s", utils.SafeTruncate(parentHash, 8), message),
-			OnPress: func() error {
-				parentNumber := i + 1
-				self.c.LogAction(self.c.Tr.Actions.RevertCommit)
-				return self.c.WithWaitingStatusSync(self.c.Tr.RevertingStatus, func() error {
-					if err := self.c.Git().Commit.RevertMerge(commit.Hash, parentNumber); err != nil {
-						return err
-					}
-					return self.afterRevertCommit()
-				})
-			},
-		}
-	}
-
-	return self.c.Menu(types.CreateMenuOptions{Title: self.c.Tr.SelectParentCommitForMerge, Items: menuItems})
-}
-
-func (self *LocalCommitsController) afterRevertCommit() error {
-	self.context().MoveSelection(1)
-	return self.c.Refresh(types.RefreshOptions{
-		Mode: types.SYNC, Scope: []types.RefreshableView{types.COMMITS, types.BRANCHES},
-	})
-}
-
 func (self *LocalCommitsController) createFixupCommit(commit *models.Commit) error {
 	var disabledReasonWhenFilesAreNeeded *types.DisabledReason
 	if len(self.c.Model().Files) == 0 {

Diff for: pkg/i18n/english.go

@@ -727,7 +727,6 @@ type TranslationSet struct {
 	FocusCommandLog                          string
 	CommandLogHeader                         string
 	RandomTip                                string
-	SelectParentCommitForMerge               string
 	ToggleWhitespaceInDiffView               string
 	ToggleWhitespaceInDiffViewTooltip        string
 	IgnoreWhitespaceDiffViewSubTitle         string
@@ -772,6 +771,7 @@ type TranslationSet struct {
 	OpenCommitInBrowser                      string
 	ViewBisectOptions                        string
 	ConfirmRevertCommit                      string
+	ConfirmRevertCommitRange                 string
 	RewordInEditorTitle                      string
 	RewordInEditorPrompt                     string
 	CheckoutAutostashPrompt                  string
@@ -1795,7 +1795,6 @@ func EnglishTranslationSet() *TranslationSet {
 		FocusCommandLog:                          "Focus command log",
 		CommandLogHeader:                         "You can hide/focus this panel by pressing '%s'\n",
 		RandomTip:                                "Random tip",
-		SelectParentCommitForMerge:               "Select parent commit for merge",
 		ToggleWhitespaceInDiffView:               "Toggle whitespace",
 		ToggleWhitespaceInDiffViewTooltip:        "Toggle whether or not whitespace changes are shown in the diff view.",
 		IgnoreWhitespaceDiffViewSubTitle:         "(ignoring whitespace)",
@@ -1839,6 +1838,7 @@ func EnglishTranslationSet() *TranslationSet {
 		OpenCommitInBrowser:                      "Open commit in browser",
 		ViewBisectOptions:                        "View bisect options",
 		ConfirmRevertCommit:                      "Are you sure you want to revert {{.selectedCommit}}?",
+		ConfirmRevertCommitRange:                 "Are you sure you want to revert the selected commits?",
 		RewordInEditorTitle:                      "Reword in editor",
 		RewordInEditorPrompt:                     "Are you sure you want to reword this commit in your editor?",
 		HardResetAutostashPrompt:                 "Are you sure you want to hard reset to '%s'? An auto-stash will be performed if necessary.",

Diff for: pkg/integration/tests/commit/revert_merge.go

@@ -21,14 +21,9 @@ var RevertMerge = NewIntegrationTest(NewIntegrationTestArgs{
 			).
 			Press(keys.Commits.RevertCommit)
 
-		t.ExpectPopup().Menu().
-			Title(Equals("Select parent commit for merge")).
-			Lines(
-				Contains("first change"),
-				Contains("second-change-branch unrelated change"),
-				Contains("Cancel"),
-			).
-			Select(Contains("first change")).
+		t.ExpectPopup().Confirmation().
+			Title(Equals("Revert commit")).
+			Content(MatchesRegexp(`Are you sure you want to revert \w+?`)).
 			Confirm()
 
 		t.Views().Commits().IsFocused().

Diff for: pkg/integration/tests/commit/revert_with_conflict_multiple_commits.go

@@ -9,16 +9,7 @@ var RevertWithConflictMultipleCommits = NewIntegrationTest(NewIntegrationTestArg
 	Description:  "Reverts a range of commits, the first of which conflicts",
 	ExtraCmdArgs: []string{},
 	Skip:         false,
-	SetupConfig: func(cfg *config.AppConfig) {
-		// TODO: use our revert UI once we support range-select for reverts
-		cfg.GetUserConfig().CustomCommands = []config.CustomCommand{
-			{
-				Key:     "X",
-				Context: "commits",
-				Command: "git -c core.editor=: revert HEAD^ HEAD^^",
-			},
-		}
-	},
+	SetupConfig:  func(cfg *config.AppConfig) {},
 	SetupRepo: func(shell *Shell) {
 		shell.CreateFileAndAdd("myfile", "")
 		shell.Commit("add empty file")
@@ -38,13 +29,18 @@ var RevertWithConflictMultipleCommits = NewIntegrationTest(NewIntegrationTestArg
 				Contains("CI ◯ unrelated change"),
 				Contains("CI ◯ add empty file"),
 			).
-			Press("X").
+			SelectNextItem().
+			Press(keys.Universal.RangeSelectDown).
+			Press(keys.Commits.RevertCommit).
 			Tap(func() {
-				t.ExpectPopup().Alert().
-					Title(Equals("Error")).
-					// The exact error message is different on different git versions,
-					// but they all contain the word 'conflict' somewhere.
-					Content(Contains("conflict")).
+				t.ExpectPopup().Confirmation().
+					Title(Equals("Revert commit")).
+					Content(Equals("Are you sure you want to revert the selected commits?")).
+					Confirm()
+
+				t.ExpectPopup().Menu().
+					Title(Equals("Conflicts!")).
+					Select(Contains("View conflicts")).
 					Confirm()
 			}).
 			Lines(
@@ -59,7 +55,7 @@ var RevertWithConflictMultipleCommits = NewIntegrationTest(NewIntegrationTestArg
 		t.Views().Options().Content(Contains("View revert options: m"))
 		t.Views().Information().Content(Contains("Reverting (Reset)"))
 
-		t.Views().Files().Focus().
+		t.Views().Files().IsFocused().
 			Lines(
 				Contains("UU myfile").IsSelected(),
 			).

Diff for: pkg/integration/tests/interactive_rebase/revert_during_rebase_when_stopped_on_edit.go

@@ -9,18 +9,10 @@ var RevertDuringRebaseWhenStoppedOnEdit = NewIntegrationTest(NewIntegrationTestA
 	Description:  "Revert a series of commits while stopped in a rebase",
 	ExtraCmdArgs: []string{},
 	Skip:         false,
-	SetupConfig: func(cfg *config.AppConfig) {
-		// TODO: use our revert UI once we support range-select for reverts
-		cfg.GetUserConfig().CustomCommands = []config.CustomCommand{
-			{
-				Key:     "X",
-				Context: "commits",
-				Command: "git -c core.editor=: revert HEAD^ HEAD^^",
-			},
-		}
-	},
+	SetupConfig:  func(cfg *config.AppConfig) {},
 	SetupRepo: func(shell *Shell) {
-		shell.EmptyCommit("master commit")
+		shell.EmptyCommit("master commit 1")
+		shell.EmptyCommit("master commit 2")
 		shell.NewBranch("branch")
 		shell.CreateNCommits(4)
 	},
@@ -32,7 +24,8 @@ var RevertDuringRebaseWhenStoppedOnEdit = NewIntegrationTest(NewIntegrationTestA
 				Contains("commit 03"),
 				Contains("commit 02"),
 				Contains("commit 01"),
-				Contains("master commit"),
+				Contains("master commit 2"),
+				Contains("master commit 1"),
 			).
 			NavigateToLine(Contains("commit 03")).
 			Press(keys.Universal.Edit).
@@ -41,17 +34,27 @@ var RevertDuringRebaseWhenStoppedOnEdit = NewIntegrationTest(NewIntegrationTestA
 				Contains("<-- YOU ARE HERE --- commit 03").IsSelected(),
 				Contains("commit 02"),
 				Contains("commit 01"),
-				Contains("master commit"),
+				Contains("master commit 2"),
+				Contains("master commit 1"),
 			).
-			Press("X").
+			SelectNextItem().
+			Press(keys.Universal.RangeSelectDown).
+			Press(keys.Commits.RevertCommit).
+			Tap(func() {
+				t.ExpectPopup().Confirmation().
+					Title(Equals("Revert commit")).
+					Content(MatchesRegexp(`Are you sure you want to revert \w+?`)).
+					Confirm()
+			}).
 			Lines(
 				Contains("pick").Contains("commit 04"),
-				Contains(`<-- YOU ARE HERE --- Revert "commit 01"`).IsSelected(),
+				Contains(`<-- YOU ARE HERE --- Revert "commit 01"`),
 				Contains(`Revert "commit 02"`),
 				Contains("commit 03"),
-				Contains("commit 02"),
-				Contains("commit 01"),
-				Contains("master commit"),
+				Contains("commit 02").IsSelected(),
+				Contains("commit 01").IsSelected(),
+				Contains("master commit 2"),
+				Contains("master commit 1"),
 			)
 	},
 })

Diff for: pkg/integration/tests/interactive_rebase/revert_multiple_commits_in_interactive_rebase.go

@@ -9,16 +9,7 @@ var RevertMultipleCommitsInInteractiveRebase = NewIntegrationTest(NewIntegration
 	Description:  "Reverts a range of commits, the first of which conflicts, in the middle of an interactive rebase",
 	ExtraCmdArgs: []string{},
 	Skip:         false,
-	SetupConfig: func(cfg *config.AppConfig) {
-		// TODO: use our revert UI once we support range-select for reverts
-		cfg.GetUserConfig().CustomCommands = []config.CustomCommand{
-			{
-				Key:     "X",
-				Context: "commits",
-				Command: "git -c core.editor=: revert HEAD^ HEAD^^",
-			},
-		}
-	},
+	SetupConfig:  func(cfg *config.AppConfig) {},
 	SetupRepo: func(shell *Shell) {
 		shell.CreateFileAndAdd("myfile", "")
 		shell.Commit("add empty file")
@@ -44,13 +35,18 @@ var RevertMultipleCommitsInInteractiveRebase = NewIntegrationTest(NewIntegration
 			).
 			NavigateToLine(Contains("add second line")).
 			Press(keys.Universal.Edit).
-			Press("X").
+			SelectNextItem().
+			Press(keys.Universal.RangeSelectDown).
+			Press(keys.Commits.RevertCommit).
 			Tap(func() {
-				t.ExpectPopup().Alert().
-					Title(Equals("Error")).
-					// The exact error message is different on different git versions,
-					// but they all contain the word 'conflict' somewhere.
-					Content(Contains("conflict")).
+				t.ExpectPopup().Confirmation().
+					Title(Equals("Revert commit")).
+					Content(Equals("Are you sure you want to revert the selected commits?")).
+					Confirm()
+
+				t.ExpectPopup().Menu().
+					Title(Equals("Conflicts!")).
+					Select(Contains("View conflicts")).
 					Confirm()
 			}).
 			Lines(
@@ -67,7 +63,7 @@ var RevertMultipleCommitsInInteractiveRebase = NewIntegrationTest(NewIntegration
 		t.Views().Options().Content(Contains("View revert options: m"))
 		t.Views().Information().Content(Contains("Reverting (Reset)"))
 
-		t.Views().Files().Focus().
+		t.Views().Files().IsFocused().
 			Lines(
 				Contains("UU myfile").IsSelected(),
 			).