From d4b11b8cfdb46d23d20b6f46a24339dacc9b18fc Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 7 Nov 2026 09:52:18 +0100 Subject: [PATCH] Abort and redo a cherry-pick or revert that a lock error stopped If a lock stops a cherry-pick or revert of several commits partway through, the git command runner ran the same command again. This failed with "cherry-pick is already in progress", and the user was left with an operation that stopped halfway. Continuing it would skip the commit that failed, because git doesn't reschedule it the way a rebase does. Abort the operation instead, and then run the command again. Also abort when the runner gives up, so that the user doesn't end up with the stopped operation. Only do this for several commits. For a single commit, git doesn't use .git/sequencer, and a cherry-pick of one commit may even run while a cherry-pick of several commits is in progress. If .git/sequencer exists after a single cherry-pick failed, it belongs to that other operation, and we must not abort it. Co-Authored-By: Claude Opus 5.5 --- pkg/commands/git_commands/commit.go | 5 +- .../git_commands/lock_error_recovery.go | 39 +++++++++++ .../git_commands/lock_error_recovery_test.go | 69 +++++++++++++++++++ pkg/commands/git_commands/rebase.go | 5 +- .../lock_error_partway_through_cherry_pick.go | 15 ---- 5 files changed, 116 insertions(+), 17 deletions(-) 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"), - ) }, })