Disallow any changes to commits or todos when cherry-picking or reverting

We treat the .git/sequencer/todo file as read-only. Technically it seems it
would be possible to treat it as modifiable in the same way as
.git/rebase-merge/git-rebase-todo, effectively turning a cherry-pick or revert
that stops at a conflict into an interactive rebase; however, git itself doesn't
allow this (there is no "git cherry-pick --edit-todo"), so it seems safer not to
rely on it.

Theoretically it would be possible to allow modifying the rebase todos when a
cherry-pick or revert conflicts in the middle of a rebase. However, it would
introduce a bit of complexity to support this, as we would have to be able to
distinguish between rebasing todos and cherry-picking/reverting todos, which we
currently can't; it could also be a bit error-prone as far as edge cases are
concerned. And it's really a pretty uncommon situation, so it doesn't seem worth
it, and we just forbid all modifications to todos whenever we are cherry-picking
or reverting.

Author: stefanhaller

pkg/gui/controllers/local_commits_controller.go

@@ -684,6 +684,11 @@ func (self *LocalCommitsController) isRebasing() bool {
 	return self.c.Model().WorkingTreeStateAtLastCommitRefresh.IsRebasing()
 }
 
+func (self *LocalCommitsController) isCherryPickingOrReverting() bool {
+	return self.c.Model().WorkingTreeStateAtLastCommitRefresh != enums.WORKING_TREE_STATE_NONE &&
+		self.c.Model().WorkingTreeStateAtLastCommitRefresh.Effective() != enums.WORKING_TREE_STATE_REBASING
+}
+
 func (self *LocalCommitsController) moveDown(selectedCommits []*models.Commit, startIdx int, endIdx int) error {
 	if self.isRebasing() {
 		if err := self.c.Git().Rebase.MoveTodosDown(selectedCommits); err != nil {
@@ -1415,6 +1420,10 @@ func (self *LocalCommitsController) canMoveUp(selectedCommits []*models.Commit,
 
 // Ensures that if we are mid-rebase, we're only selecting valid commits (non-conflict TODO commits)
 func (self *LocalCommitsController) midRebaseCommandEnabled(selectedCommits []*models.Commit, startIdx int, endIdx int) *types.DisabledReason {
+	if self.isCherryPickingOrReverting() {
+		return &types.DisabledReason{Text: self.c.Tr.NotAllowedMidCherryPickOrRevert}
+	}
+
 	if !self.isRebasing() {
 		return nil
 	}
@@ -1434,6 +1443,10 @@ func (self *LocalCommitsController) midRebaseCommandEnabled(selectedCommits []*m
 
 // Ensures that if we are mid-rebase, we're only selecting commits that can be moved
 func (self *LocalCommitsController) midRebaseMoveCommandEnabled(selectedCommits []*models.Commit, startIdx int, endIdx int) *types.DisabledReason {
+	if self.isCherryPickingOrReverting() {
+		return &types.DisabledReason{Text: self.c.Tr.NotAllowedMidCherryPickOrRevert}
+	}
+
 	if !self.isRebasing() {
 		if lo.SomeBy(selectedCommits, func(c *models.Commit) bool { return c.IsMerge() }) {
 			return &types.DisabledReason{Text: self.c.Tr.CannotMoveMergeCommit}
@@ -1458,6 +1471,10 @@ func (self *LocalCommitsController) midRebaseMoveCommandEnabled(selectedCommits
 }
 
 func (self *LocalCommitsController) canDropCommits(selectedCommits []*models.Commit, startIdx int, endIdx int) *types.DisabledReason {
+	if self.isCherryPickingOrReverting() {
+		return &types.DisabledReason{Text: self.c.Tr.NotAllowedMidCherryPickOrRevert}
+	}
+
 	if !self.isRebasing() {
 		if len(selectedCommits) > 1 && lo.SomeBy(selectedCommits, func(c *models.Commit) bool { return c.IsMerge() }) {
 			return &types.DisabledReason{Text: self.c.Tr.DroppingMergeRequiresSingleSelection}
@@ -1502,6 +1519,10 @@ func isChangeOfRebaseTodoAllowed(oldAction todo.TodoCommand) bool {
 }
 
 func (self *LocalCommitsController) pickEnabled(selectedCommits []*models.Commit, startIdx int, endIdx int) *types.DisabledReason {
+	if self.isCherryPickingOrReverting() {
+		return &types.DisabledReason{Text: self.c.Tr.NotAllowedMidCherryPickOrRevert}
+	}
+
 	if !self.isRebasing() {
 		// if not rebasing, we're going to do a pull so we don't care about the selection
 		return nil

pkg/i18n/english.go

@@ -342,6 +342,7 @@ type TranslationSet struct {
 	YouDied                               string
 	RewordNotSupported                    string
 	ChangingThisActionIsNotAllowed        string
+	NotAllowedMidCherryPickOrRevert       string
 	DroppingMergeRequiresSingleSelection  string
 	CherryPickCopy                        string
 	CherryPickCopyTooltip                 string
@@ -1396,6 +1397,7 @@ func EnglishTranslationSet() *TranslationSet {
 		YouDied:                              "YOU DIED!",
 		RewordNotSupported:                   "Rewording commits while interactively rebasing is not currently supported",
 		ChangingThisActionIsNotAllowed:       "Changing this kind of rebase todo entry is not allowed",
+		NotAllowedMidCherryPickOrRevert:      "This action is not allowed while cherry-picking or reverting",
 		DroppingMergeRequiresSingleSelection: "Dropping a merge commit requires a single selected item",
 		CherryPickCopy:                       "Copy (cherry-pick)",
 		CherryPickCopyTooltip:                "Mark commit as copied. Then, within the local commits view, you can press `{{.paste}}` to paste (cherry-pick) the copied commit(s) into your checked out branch. At any time you can press `{{.escape}}` to cancel the selection.",

pkg/integration/tests/interactive_rebase/revert_single_commit_in_interactive_rebase.go

@@ -42,7 +42,7 @@ var RevertSingleCommitInInteractiveRebase = NewIntegrationTest(NewIntegrationTes
 				t.ExpectPopup().Menu().
 					Title(Equals("Conflicts!")).
 					Select(Contains("View conflicts")).
-					Confirm()
+					Cancel() // stay in commits panel
 			}).
 			Lines(
 				Contains("CI unrelated change 2"),
@@ -51,12 +51,20 @@ var RevertSingleCommitInInteractiveRebase = NewIntegrationTest(NewIntegrationTes
 				Contains("CI ◯ add second line"),
 				Contains("CI ◯ add first line").IsSelected(),
 				Contains("CI ◯ add empty file"),
-			)
+			).
+			Press(keys.Commits.MoveDownCommit).
+			Tap(func() {
+				t.ExpectToast(Equals("Disabled: This action is not allowed while cherry-picking or reverting"))
+			}).
+			Press(keys.Universal.Remove).
+			Tap(func() {
+				t.ExpectToast(Equals("Disabled: This action is not allowed while cherry-picking or reverting"))
+			})
 
 		t.Views().Options().Content(Contains("View revert options: m"))
 		t.Views().Information().Content(Contains("Reverting (Reset)"))
 
-		t.Views().Files().IsFocused().
+		t.Views().Files().Focus().
 			Lines(
 				Contains("UU myfile").IsSelected(),
 			).