From b350876f847cbd1c048388bb028d05678b1fbcfa Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Tue, 18 Jun 2024 18:01:14 +0200 Subject: [PATCH] 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. --- .../controllers/local_commits_controller.go | 21 +++++++++++++++++++ pkg/i18n/english.go | 2 ++ ...ert_single_commit_in_interactive_rebase.go | 14 ++++++++++--- 3 files changed, 34 insertions(+), 3 deletions(-) diff --git a/pkg/gui/controllers/local_commits_controller.go b/pkg/gui/controllers/local_commits_controller.go index 12def1c9b..956482720 100644 --- a/pkg/gui/controllers/local_commits_controller.go +++ b/pkg/gui/controllers/local_commits_controller.go @@ -684,6 +684,11 @@ func (self *LocalCommitsController) isRebasing() bool { return self.c.Model().WorkingTreeStateAtLastCommitRefresh.Any() } +func (self *LocalCommitsController) isCherryPickingOrReverting() bool { + return self.c.Model().WorkingTreeStateAtLastCommitRefresh.CherryPicking || + self.c.Model().WorkingTreeStateAtLastCommitRefresh.Reverting +} + 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 diff --git a/pkg/i18n/english.go b/pkg/i18n/english.go index b316f3b89..5642e696f 100644 --- a/pkg/i18n/english.go +++ b/pkg/i18n/english.go @@ -353,6 +353,7 @@ type TranslationSet struct { YouDied string RewordNotSupported string ChangingThisActionIsNotAllowed string + NotAllowedMidCherryPickOrRevert string DroppingMergeRequiresSingleSelection string CherryPickCopy string CherryPickCopyTooltip string @@ -1421,6 +1422,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.", diff --git a/pkg/integration/tests/interactive_rebase/revert_single_commit_in_interactive_rebase.go b/pkg/integration/tests/interactive_rebase/revert_single_commit_in_interactive_rebase.go index f6f104dc0..05849f21c 100644 --- a/pkg/integration/tests/interactive_rebase/revert_single_commit_in_interactive_rebase.go +++ b/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(), ).