Repository: jesseduffield/lazygit (language: go)

Issue:
Support range select for reverting commits
**Is your feature request related to a problem? Please describe.**
Sometimes it's useful to revert a range of commits.

**Describe the solution you'd like**
In git you can revert commits A-B via `git revert ^A..B`. Now that we support range select, we should use it for handling reverts.

**Additional context**
Currently if you try to revert an individual merge commit, you're asked to select a parent commit for the revert. When we're dealing with a range, the range could include multiple merge commits it's not clear to me what we should do in that situation. As such, for now let's just not support reverting a range of commits which includes merge commits (via GetDisabledReason).


Discussion / hints:
Hey i would love to work on this. it would be great if you assign this to me! thanks
@ankit-pn done :)
Hi @jesseduffield ,

As nothing was happening here for a while and I was looking for something to contribute, I took a crack at it.

See over at https://github.com/uberrice/lazygit/tree/3272---range-revert-commit

I haven't gone over the internationalization yet - I have put a TODO wherever that would be a thing to do; if my approach is seen as sane, I'd be happy to put that in as well.

I am still unsure about a few things:

- I tried working with `startIdx` and `endIdx` rather than just the first and last list element - I ran into problems there, where for example for a list of 2 things, if I addressed with `startIdx` and `endIdx-1`, I got the same commit twice, but `startIdx` was already 0, and `endIdx` 2. So I went with just the first and last element of the selected commits - is that fine?
- Visibility: To not clutter up the menu, it makes sense that 'revert' is not in the bottom bar. However, if we select a range, the tips at the bottom change. Is it possible to use some variation of `DisplayOnScreen` to only display it if a range is selected? -> Currently, it's set to be visible
- I implemented the range support by basically just checking whether one or more commits are selected, and running the old revert code if that is the case. Would it be nicer to somehow integrate this all into a 'one-in-all' handler?
- Is my `canBeReverted` correct? Not sure of all the edge cases
- Git command: I'm not a full on git pro that reverts ranges often - to get an 'inclusive selection' I select the parent commit as well. Should that maybe be solved in another way?
@uberrice Thanks for working on this!

One general remark first: I think we shouldn't add this feature before we did #1807. For a range of reverts it's easy to run into the situation where one of them conflicts, but lazygit currently has no way of detecting that state, and it doesn't let you abort or continue the revert operation. We need to fix this first. #1807 is pretty high up on my list of things to work on next, but I don't have a huge amount of time right now, so it might take a bit. This shouldn't stop you from continuing to work on this, we just wouldn't merge it yet.

On to your questions:
- I wouldn't bother trying to use git's `..` feature for specifying a range. You can pass a list of commits to `git revert`, so that's what I would do. Simply pass all the commits that get passed into the handler as `selectedCommits` (in reverse order, they are passed in top to bottom). This way the handler works for a single commit or for a range of commits; also, should we ever support non-contiguous selections in the future, it will just work for them too.
- I see no reason why we need to disallow reverting merge commits. I would put up the same prompt as we now do for a single commit when it is a merge, asking for which parent you want to use. If there are several merge commits contained in the range, we'd use the same parent choice for all of them, which I think is fine. (Command-line git doesn't let you specify it more fine-grained either.)
Sorry for the late reply: I agree with @stefanhaller .

With regards to visibility, I'm fine for the keybinding to not be on the bottom bar regardless of whether multiple commits are selected: single-commit revert is a common use case, but not common enough to be included in the already over-crowded keybinding suggestions, and multi-commit revert is not common enough to warrant inclusion.

In general your branch isn't doing anything crazy so if you implement @stefanhaller 's feedback it'll be in good shape.
Thanks for the replies. I'll apply your suggestions and get everything cleaned up when I get the time and then submit a pull request (to be blocked til #1807 is dealt with, of course - makes sense)
Updated my branch with the following:
- Removed visibility of 'revert' when range selected
- Changed implementation to pass all individual SHAs rather than use git CLI's .. operator

I'm currently working on the merge commit menu for range reverts with a merge commit - I'll have to rework `createRevertMergeCommit` a bit for that (or overload the function to accept a range of commits?), and that wasn't immediately obvious to me. When that's done, the feature should be good to go.

---

-> I could also change my implementation that it would call git revert for every single reverted commit - this would remove complexity (no RevertRange function to call) at the cost of potentially doing a lot of git operations. Would that maybe be preferred?

Other option: when reverting a range with multiple merge commits, revert 'range' up to merge commit, revert that one individually with the menu, and so on. That would add granularity, and you could choose which parent to use on every merge commit included in the range.
> I'll have to rework `createRevertMergeCommit` a bit for that

Ah, this is a bit trickier than I thought. I didn't realize that the menu actually shows the commit subjects of the merge commits's parents to choose from. My suggestion would be to keep this behavior only if a single merge commit is selected; if a range of commits is selected, and it contains one or more merge commits, put up a similar menu but with generic entries like "first parent" and "second parent". (Technically it's possible to have merge commits with more than two parents, but I've never seen them in real life, so I wouldn't worry about that case.)

> -> I could also change my implementation that it would call git revert for every single reverted commit

No, definitely not. It would be way too slow, and it would also make it very difficult to handle the case where one commit in the middle has conflicts.

> Other option: when reverting a range with multiple merge commits, revert 'range' up to merge commit, revert that one individually with the menu, and so on.

This would have the same problem as the one above (what if there are conflicts), but it's also unnecessary complexity. As I said earlier, command-line git doesn't let you specify the merge parents separately for each merge commit either. If someone really has the need to pick a different parent for two merge commits, they can just do the revert in multiple steps, starting with the newest one.

Fix patch:
diff --git a/pkg/commands/git_commands/commit.go b/pkg/commands/git_commands/commit.go
index cd6033b3915..01bf190cd7f 100644
--- a/pkg/commands/git_commands/commit.go
+++ b/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 --git a/pkg/gui/controllers/local_commits_controller.go b/pkg/gui/controllers/local_commits_controller.go
index 9564827204f..16cdab3ecc8 100644
--- a/pkg/gui/controllers/local_commits_controller.go
+++ b/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,26 +857,34 @@ 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},
+				})
 			})
 		},
 	})
@@ -885,39 +892,6 @@ func (self *LocalCommitsController) revert(commit *models.Commit) error {
 	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 --git a/pkg/i18n/english.go b/pkg/i18n/english.go
index 5642e696f42..ced96cc8a01 100644
--- a/pkg/i18n/english.go
+++ b/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.",


Test patch:
diff --git a/pkg/integration/tests/commit/revert_merge.go b/pkg/integration/tests/commit/revert_merge.go
index 5d27970d0b6..1d45180869b 100644
--- a/pkg/integration/tests/commit/revert_merge.go
+++ b/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 --git a/pkg/integration/tests/commit/revert_with_conflict_multiple_commits.go b/pkg/integration/tests/commit/revert_with_conflict_multiple_commits.go
index b4186d55a20..ad3761ba238 100644
--- a/pkg/integration/tests/commit/revert_with_conflict_multiple_commits.go
+++ b/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 --git a/pkg/integration/tests/interactive_rebase/revert_during_rebase_when_stopped_on_edit.go b/pkg/integration/tests/interactive_rebase/revert_during_rebase_when_stopped_on_edit.go
index 1db0f0370d9..7d49f06db5f 100644
--- a/pkg/integration/tests/interactive_rebase/revert_during_rebase_when_stopped_on_edit.go
+++ b/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 --git a/pkg/integration/tests/interactive_rebase/revert_multiple_commits_in_interactive_rebase.go b/pkg/integration/tests/interactive_rebase/revert_multiple_commits_in_interactive_rebase.go
index 2953e3c3a82..3d3aa42623c 100644
--- a/pkg/integration/tests/interactive_rebase/revert_multiple_commits_in_interactive_rebase.go
+++ b/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(),
 			).