diff --git a/pkg/commands/git_commands/lock_error_recovery.go b/pkg/commands/git_commands/lock_error_recovery.go index c0f1243dc..7a0834f95 100644 --- a/pkg/commands/git_commands/lock_error_recovery.go +++ b/pkg/commands/git_commands/lock_error_recovery.go @@ -1,6 +1,7 @@ package git_commands import ( + "bytes" "os" "path/filepath" "strings" @@ -88,6 +89,30 @@ func abortSequenceAfterLockError( }) } +// retryContinueOrSkipAfterLockError sets the lock-error recovery (see +// CmdObj.RecoverFromLockErrorWith) of cmdObj, a command that continues or skips +// a step of an operation in progress, such as a cherry-pick or revert. +// progressFile is the file in which git records how far the operation has got. +// Call this function right before running the command, since it records the +// contents of progressFile at that time. +// +// If a lock stops the command before it changes progressFile, we run the +// command again. Otherwise it has got past the step that it continued or +// skipped, and the lock stopped it at the next one. Unlike a rebase, git doesn't +// reschedule that step (see abortSequenceAfterLockError), and running the +// command again could skip it. So we give up and leave the operation to the +// user. +func retryContinueOrSkipAfterLockError(cmdObj *oscommands.CmdObj, progressFile string) *oscommands.CmdObj { + progressBefore, _ := os.ReadFile(progressFile) + return cmdObj.RecoverFromLockErrorWith(func() *oscommands.CmdObj { + progress, _ := os.ReadFile(progressFile) + if bytes.Equal(progress, progressBefore) { + return cmdObj + } + return nil + }) +} + // 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 e48d55a26..3b3eff1ba 100644 --- a/pkg/commands/git_commands/lock_error_recovery_test.go +++ b/pkg/commands/git_commands/lock_error_recovery_test.go @@ -175,3 +175,55 @@ func TestAbortSequenceAfterLockError(t *testing.T) { }) } } + +func TestRetryContinueOrSkipAfterLockError(t *testing.T) { + newCmdObj := func(args ...string) *oscommands.CmdObj { + return oscommands.NewDummyCmdObjBuilder(nil).New(args) + } + + writeTodos := func(gitDir string, todos string) { + dir := filepath.Join(gitDir, "sequencer") + assert.NoError(t, os.MkdirAll(dir, 0o755)) + assert.NoError(t, os.WriteFile(filepath.Join(dir, "todo"), []byte(todos), 0o644)) + } + + t.Run("runs the command again if it didn't change the todos", func(t *testing.T) { + gitDir := t.TempDir() + writeTodos(gitDir, "pick 1234567 one\npick 2345678 two\n") + cmdObj := newCmdObj("git", "cherry-pick", "--continue") + + retryContinueOrSkipAfterLockError(cmdObj, filepath.Join(gitDir, "sequencer", "todo")) + + assert.Same(t, cmdObj, cmdObj.GetLockErrorRecovery()()) + }) + + t.Run("runs the command again for a cherry-pick of a single commit", func(t *testing.T) { + cmdObj := newCmdObj("git", "cherry-pick", "--continue") + + retryContinueOrSkipAfterLockError(cmdObj, filepath.Join(t.TempDir(), "sequencer", "todo")) + + assert.Same(t, cmdObj, cmdObj.GetLockErrorRecovery()()) + }) + + t.Run("gives up if it changed the todos", func(t *testing.T) { + gitDir := t.TempDir() + writeTodos(gitDir, "pick 1234567 one\npick 2345678 two\n") + cmdObj := newCmdObj("git", "cherry-pick", "--continue") + + retryContinueOrSkipAfterLockError(cmdObj, filepath.Join(gitDir, "sequencer", "todo")) + writeTodos(gitDir, "pick 2345678 two\n") + + assert.Nil(t, cmdObj.GetLockErrorRecovery()()) + }) + + t.Run("gives up if the cherry-pick is gone", func(t *testing.T) { + gitDir := t.TempDir() + writeTodos(gitDir, "pick 1234567 one\n") + cmdObj := newCmdObj("git", "cherry-pick", "--continue") + + retryContinueOrSkipAfterLockError(cmdObj, filepath.Join(gitDir, "sequencer", "todo")) + assert.NoError(t, os.RemoveAll(filepath.Join(gitDir, "sequencer"))) + + assert.Nil(t, cmdObj.GetLockErrorRecovery()()) + }) +} diff --git a/pkg/commands/git_commands/rebase.go b/pkg/commands/git_commands/rebase.go index b60d30a94..e166687a9 100644 --- a/pkg/commands/git_commands/rebase.go +++ b/pkg/commands/git_commands/rebase.go @@ -501,7 +501,18 @@ func (self *RebaseCommands) GenericMergeOrRebaseAction(commandType string, comma } } - err := skipEditor(self.GenericMergeOrRebaseActionCmdObj(commandType, command)).Run() + cmdObj := skipEditor(self.GenericMergeOrRebaseActionCmdObj(commandType, command)) + if command == "continue" || command == "skip" { + switch commandType { + case "rebase": + cmdObj = self.resumeAfterLockError(cmdObj) + case "cherry-pick", "revert": + cmdObj = retryContinueOrSkipAfterLockError(cmdObj, + filepath.Join(self.repoPaths.WorktreeGitDirPath(), "sequencer", "todo")) + } + } + + err := cmdObj.Run() if err != nil { undoRepeatingTodo() if !strings.Contains(err.Error(), "no rebase in progress") { @@ -524,8 +535,8 @@ func (self *RebaseCommands) GenericMergeOrRebaseAction(commandType string, comma return nil } -// resumeAfterLockError makes cmdObj, a command that starts a rebase, resume -// the rebase if a lock error stops it partway through; see +// resumeAfterLockError makes cmdObj, a command that starts or continues a +// rebase, resume the rebase if a lock error stops it partway through; see // resumeRebaseAfterLockError. func (self *RebaseCommands) resumeAfterLockError(cmdObj *oscommands.CmdObj) *oscommands.CmdObj { return resumeRebaseAfterLockError(cmdObj, self.repoPaths.WorktreeGitDirPath(), self.config.GetCoreCommentChar(), diff --git a/pkg/integration/tests/interactive_rebase/lock_error_partway_through_skip.go b/pkg/integration/tests/interactive_rebase/lock_error_partway_through_skip.go index c2865aff1..992225336 100644 --- a/pkg/integration/tests/interactive_rebase/lock_error_partway_through_skip.go +++ b/pkg/integration/tests/interactive_rebase/lock_error_partway_through_skip.go @@ -55,7 +55,6 @@ var LockErrorPartwayThroughSkip = NewIntegrationTest(NewIntegrationTestArgs{ Select(Contains("skip")). Confirm() - /* EXPECTED: t.ExpectPopup().Alert(). Title(Equals("Error")). Content(Contains("index.lock': File exists.")). @@ -65,12 +64,5 @@ var LockErrorPartwayThroughSkip = NewIntegrationTest(NewIntegrationTestArgs{ Lines( Contains("A").Contains("file-y"), ) - ACTUAL: */ - t.Views().Commits(). - Lines( - Contains("add file-z"), - Contains("edited commit"), - Contains("base"), - ) }, })