diff --git a/pkg/commands/git_commands/commit.go b/pkg/commands/git_commands/commit.go index e46fecd84..3d61a3e11 100644 --- a/pkg/commands/git_commands/commit.go +++ b/pkg/commands/git_commands/commit.go @@ -275,7 +275,10 @@ func (self *CommitCommands) Revert(hashes []string, isMerge bool) error { Arg(hashes...). ToArgv() - return self.cmd.New(cmdArgs).Run() + return abortSequenceAfterLockError(self.cmd.New(cmdArgs), self.repoPaths.WorktreeGitDirPath(), len(hashes), + func() *oscommands.CmdObj { + return self.cmd.New(NewGitCmd("revert").Arg("--abort").ToArgv()) + }).Run() } // CreateFixupCommit creates a commit that fixes up a previous commit diff --git a/pkg/commands/git_commands/lock_error_recovery.go b/pkg/commands/git_commands/lock_error_recovery.go index 8925a65a4..c0f1243dc 100644 --- a/pkg/commands/git_commands/lock_error_recovery.go +++ b/pkg/commands/git_commands/lock_error_recovery.go @@ -49,6 +49,45 @@ func resumeRebaseAfterLockError( return recoverFrom(cmdObj, rebaseState(gitDir)) } +// abortSequenceAfterLockError sets the lock-error recovery (see +// CmdObj.RecoverFromLockErrorWith) of cmdObj, a cherry-pick or revert of +// numCommits commits in the worktree whose git dir is gitDir. +// +// If a lock stops a cherry-pick or revert of several commits after it has +// applied some of them, git leaves the operation in progress in +// .git/sequencer. Unlike a rebase, it doesn't reschedule the commit that +// failed, and continuing the operation would skip that commit. So we abort the +// operation with the command made by newAbortCmdObj, and run the command +// again. The runner calls the recovery after its last attempt too, so we +// also abort when it gives up, and the user isn't left with an operation that +// stopped halfway. +func abortSequenceAfterLockError( + cmdObj *oscommands.CmdObj, + gitDir string, + numCommits int, + newAbortCmdObj func() *oscommands.CmdObj, +) *oscommands.CmdObj { + // For a single commit, git doesn't use .git/sequencer. If a lock stops it, + // it has either not changed anything yet, or it has staged the changes, and + // running it again fails with an error. Git also doesn't check whether + // another operation is in progress in .git/sequencer, so we must not abort + // that one. + if numCommits < 2 { + return cmdObj + } + + return cmdObj.RecoverFromLockErrorWith(func() *oscommands.CmdObj { + // Git refuses to start a cherry-pick or revert of several commits while + // one is in progress, so if there is one now, our command started it. + if dirExists(filepath.Join(gitDir, "sequencer")) { + if err := newAbortCmdObj().Run(); err != nil { + return nil + } + } + return cmdObj + }) +} + // rebaseState returns a snapshot of the rebase that is in progress in gitDir, // made of the files in which git records how far it has got. It's empty if no // rebase is in progress. diff --git a/pkg/commands/git_commands/lock_error_recovery_test.go b/pkg/commands/git_commands/lock_error_recovery_test.go index 1fe967983..e48d55a26 100644 --- a/pkg/commands/git_commands/lock_error_recovery_test.go +++ b/pkg/commands/git_commands/lock_error_recovery_test.go @@ -1,6 +1,7 @@ package git_commands import ( + "errors" "os" "path/filepath" "testing" @@ -106,3 +107,71 @@ func TestResumeRebaseAfterLockError(t *testing.T) { })) }) } + +func TestAbortSequenceAfterLockError(t *testing.T) { + scenarios := []struct { + testName string + numCommits int + sequencerExists bool + runner *oscommands.FakeCmdObjRunner + expectRecovery bool + expectRunningAgain bool + }{ + { + testName: "single commit", + numCommits: 1, + runner: oscommands.NewFakeRunner(t), + expectRecovery: false, + }, + { + testName: "the lock stopped the command before it started", + numCommits: 2, + runner: oscommands.NewFakeRunner(t), + expectRecovery: true, + expectRunningAgain: true, + }, + { + testName: "the lock stopped the command partway through", + numCommits: 2, + sequencerExists: true, + runner: oscommands.NewFakeRunner(t). + ExpectGitArgs([]string{"cherry-pick", "--abort"}, "", nil), + expectRecovery: true, + expectRunningAgain: true, + }, + { + testName: "aborting fails", + numCommits: 2, + sequencerExists: true, + runner: oscommands.NewFakeRunner(t). + ExpectGitArgs([]string{"cherry-pick", "--abort"}, "", errors.New("error")), + expectRecovery: true, + expectRunningAgain: false, + }, + } + + for _, s := range scenarios { + t.Run(s.testName, func(t *testing.T) { + gitDir := t.TempDir() + if s.sequencerExists { + assert.NoError(t, os.MkdirAll(filepath.Join(gitDir, "sequencer"), 0o755)) + } + builder := oscommands.NewDummyCmdObjBuilder(s.runner) + cmdObj := builder.New([]string{"git", "cherry-pick", "1234567", "2345678"}) + + abortSequenceAfterLockError(cmdObj, gitDir, s.numCommits, func() *oscommands.CmdObj { + return builder.New([]string{"git", "cherry-pick", "--abort"}) + }) + + recovery := cmdObj.GetLockErrorRecovery() + if !s.expectRecovery { + assert.Nil(t, recovery) + } else if s.expectRunningAgain { + assert.Same(t, cmdObj, recovery()) + } else { + assert.Nil(t, recovery()) + } + s.runner.CheckForMissingCalls() + }) + } +} diff --git a/pkg/commands/git_commands/rebase.go b/pkg/commands/git_commands/rebase.go index 6098f9493..b60d30a94 100644 --- a/pkg/commands/git_commands/rebase.go +++ b/pkg/commands/git_commands/rebase.go @@ -598,7 +598,10 @@ func (self *RebaseCommands) CherryPickCommits(commits []*models.Commit) error { Arg(lo.Reverse(lo.Map(commits, func(c *models.Commit, _ int) string { return c.Hash() }))...). ToArgv() - return self.cmd.New(cmdArgs).Run() + return abortSequenceAfterLockError(self.cmd.New(cmdArgs), self.repoPaths.WorktreeGitDirPath(), len(commits), + func() *oscommands.CmdObj { + return self.cmd.New(NewGitCmd("cherry-pick").Arg("--abort").ToArgv()) + }).Run() } func (self *RebaseCommands) DropMergeCommit(commits []*models.Commit, commitIndex int) error { diff --git a/pkg/integration/tests/cherry_pick/lock_error_partway_through_cherry_pick.go b/pkg/integration/tests/cherry_pick/lock_error_partway_through_cherry_pick.go index b3329ab9a..d1ae75715 100644 --- a/pkg/integration/tests/cherry_pick/lock_error_partway_through_cherry_pick.go +++ b/pkg/integration/tests/cherry_pick/lock_error_partway_through_cherry_pick.go @@ -63,25 +63,10 @@ var LockErrorPartwayThroughCherryPick = NewIntegrationTest(NewIntegrationTestArg Content(Contains("Are you sure you want to cherry-pick the 2 copied commit(s) onto this branch?")). Confirm() }). - /* EXPECTED: Lines( Contains("two"), Contains("one"), Contains("base"), ) - ACTUAL: */ - Tap(func() { - t.ExpectPopup().Alert(). - Title(Equals("Error")). - Content(Contains("cherry-pick is already in progress")). - Confirm() - }). - Lines( - Contains("─── Pending cherry-picks"), - Contains("pick").Contains("two"), - Contains("─── Commits"), - Contains("one"), - Contains("base"), - ) }, })