diff --git a/pkg/commands/git_cmd_obj_runner.go b/pkg/commands/git_cmd_obj_runner.go index 8112b0f30..4f6bd37ec 100644 --- a/pkg/commands/git_cmd_obj_runner.go +++ b/pkg/commands/git_cmd_obj_runner.go @@ -55,14 +55,14 @@ func (self *gitCmdObjRunner) Run(cmdObj *oscommands.CmdObj) error { } func (self *gitCmdObjRunner) RunWithOutput(cmdObj *oscommands.CmdObj) (string, error) { - return self.retryOnLockError(func() (string, error) { + return self.retryOnLockError(cmdObj, func(cmdObj *oscommands.CmdObj) (string, error) { return self.innerRunner.RunWithOutput(cmdObj.Clone()) }) } func (self *gitCmdObjRunner) RunWithOutputs(cmdObj *oscommands.CmdObj) (string, string, error) { var stdout, stderr string - _, err := self.retryOnLockError(func() (string, error) { + _, err := self.retryOnLockError(cmdObj, func(cmdObj *oscommands.CmdObj) (string, error) { var runErr error stdout, stderr, runErr = self.innerRunner.RunWithOutputs(cmdObj.Clone()) return stdout + stderr, runErr @@ -70,23 +70,33 @@ func (self *gitCmdObjRunner) RunWithOutputs(cmdObj *oscommands.CmdObj) (string, return stdout, stderr, err } -// retryOnLockError runs the given function, retrying if it fails with a -// transient lock error (see isRetryableError). The string returned by run is +// retryOnLockError runs the given command with the given function, trying +// again if it fails with a transient lock error (see isRetryableError). If the +// command has a lock-error recovery (see CmdObj.RecoverFromLockErrorWith), the +// recovery decides which command to run next. The string returned by run is // the command output we inspect to classify the failure. We clone the command // for each attempt (inside run) because an *exec.Cmd can only be run once. -func (self *gitCmdObjRunner) retryOnLockError(run func() (string, error)) (string, error) { +func (self *gitCmdObjRunner) retryOnLockError(cmdObj *oscommands.CmdObj, run func(*oscommands.CmdObj) (string, error)) (string, error) { delay := self.initialRetryDelay var output string var err error for attempt := range maxRetries { - output, err = run() + output, err = run(cmdObj) if err == nil || !isRetryableError(output, err) { break } + if recovery := cmdObj.GetLockErrorRecovery(); recovery != nil { + cmdObj = recovery() + if cmdObj == nil { + self.log.Warn("lock error stopped command partway through; giving up") + break + } + } + if attempt < maxRetries-1 { - self.log.Warnf("lock error prevented command from running; retrying in %s", delay) + self.log.Warnf("lock error prevented command from completing; running %q in %s", cmdObj.ToString(), delay) time.Sleep(delay) delay *= 2 } diff --git a/pkg/commands/git_cmd_obj_runner_test.go b/pkg/commands/git_cmd_obj_runner_test.go index bf938da54..cc6496bde 100644 --- a/pkg/commands/git_cmd_obj_runner_test.go +++ b/pkg/commands/git_cmd_obj_runner_test.go @@ -16,29 +16,32 @@ type runnerResult struct { // scriptedRunner is an ICmdObjRunner stub that returns a preconfigured result // for each successive call, letting us drive the retry loop deterministically. -// It counts calls so tests can assert whether a command was retried. +// It counts calls and records the commands it ran, so tests can assert whether +// and how a command was retried. type scriptedRunner struct { results []runnerResult calls int + ran []string } -func (self *scriptedRunner) next() (string, error) { +func (self *scriptedRunner) next(cmdObj *oscommands.CmdObj) (string, error) { result := self.results[self.calls] self.calls++ + self.ran = append(self.ran, cmdObj.ToString()) return result.output, result.err } -func (self *scriptedRunner) Run(*oscommands.CmdObj) error { - _, err := self.next() +func (self *scriptedRunner) Run(cmdObj *oscommands.CmdObj) error { + _, err := self.next(cmdObj) return err } -func (self *scriptedRunner) RunWithOutput(*oscommands.CmdObj) (string, error) { - return self.next() +func (self *scriptedRunner) RunWithOutput(cmdObj *oscommands.CmdObj) (string, error) { + return self.next(cmdObj) } -func (self *scriptedRunner) RunWithOutputs(*oscommands.CmdObj) (string, string, error) { - output, err := self.next() +func (self *scriptedRunner) RunWithOutputs(cmdObj *oscommands.CmdObj) (string, string, error) { + output, err := self.next(cmdObj) return output, "", err } @@ -58,9 +61,15 @@ func newTestRunner(inner *scriptedRunner) *gitCmdObjRunner { // dummyCmdObj returns a throwaway command; only its clonability matters, since // the scriptedRunner ignores it and returns preconfigured results. func dummyCmdObj() *oscommands.CmdObj { - return oscommands.NewDummyCmdObjBuilder(nil).New([]string{"git", "status"}) + return newCmdObj("git", "status") } +func newCmdObj(args ...string) *oscommands.CmdObj { + return oscommands.NewDummyCmdObjBuilder(nil).New(args) +} + +var errLock = errors.New("fatal: Unable to create '/repo/.git/index.lock': File exists.") + func TestRunWithOutputReturnsSuccessWithoutRetrying(t *testing.T) { inner := &scriptedRunner{results: []runnerResult{{output: "done", err: nil}}} @@ -135,3 +144,73 @@ func TestRunWithOutputRetriesLockErrorInLinkedWorktree(t *testing.T) { assert.NoError(t, err) assert.Equal(t, 2, inner.calls) } + +func TestRunWithOutputRunsTheCommandReturnedByTheLockErrorRecovery(t *testing.T) { + inner := &scriptedRunner{results: []runnerResult{ + {err: errLock}, + {err: errLock}, + {output: "done", err: nil}, + }} + continueCmdObj := newCmdObj("git", "rebase", "--continue") + cmdObj := newCmdObj("git", "rebase", "--interactive", "main") + cmdObj.RecoverFromLockErrorWith(func() *oscommands.CmdObj { return continueCmdObj }) + + output, err := newTestRunner(inner).RunWithOutput(cmdObj) + + assert.NoError(t, err) + assert.Equal(t, "done", output) + // After switching to the continue command, the runner uses that command's + // recovery; it has none, so the runner runs it again. + assert.Equal(t, []string{ + "git rebase --interactive main", + "git rebase --continue", + "git rebase --continue", + }, inner.ran) +} + +func TestRunWithOutputGivesUpWhenTheLockErrorRecoveryReturnsNil(t *testing.T) { + inner := &scriptedRunner{results: []runnerResult{{err: errLock}}} + cmdObj := dummyCmdObj() + cmdObj.RecoverFromLockErrorWith(func() *oscommands.CmdObj { return nil }) + + _, err := newTestRunner(inner).RunWithOutput(cmdObj) + + assert.Equal(t, errLock, err) + assert.Equal(t, 1, inner.calls) +} + +func TestRunWithOutputCallsTheLockErrorRecoveryAfterTheLastAttempt(t *testing.T) { + results := make([]runnerResult, maxRetries) + for i := range results { + results[i] = runnerResult{err: errLock} + } + inner := &scriptedRunner{results: results} + recoveryCalls := 0 + cmdObj := dummyCmdObj() + cmdObj.RecoverFromLockErrorWith(func() *oscommands.CmdObj { + recoveryCalls++ + return cmdObj + }) + + _, err := newTestRunner(inner).RunWithOutput(cmdObj) + + assert.Error(t, err) + assert.Equal(t, maxRetries, inner.calls) + assert.Equal(t, maxRetries, recoveryCalls) +} + +func TestRunWithOutputDoesNotCallTheLockErrorRecoveryForOtherErrors(t *testing.T) { + inner := &scriptedRunner{results: []runnerResult{{output: "boom", err: errors.New("boom")}}} + recoveryCalls := 0 + cmdObj := dummyCmdObj() + cmdObj.RecoverFromLockErrorWith(func() *oscommands.CmdObj { + recoveryCalls++ + return cmdObj + }) + + _, err := newTestRunner(inner).RunWithOutput(cmdObj) + + assert.Error(t, err) + assert.Equal(t, 1, inner.calls) + assert.Equal(t, 0, recoveryCalls) +} 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/commit_loader.go b/pkg/commands/git_commands/commit_loader.go index 987f2394e..c0b6cf580 100644 --- a/pkg/commands/git_commands/commit_loader.go +++ b/pkg/commands/git_commands/commit_loader.go @@ -259,8 +259,15 @@ func (self *CommitLoader) getHydratedSequencerCommits(hashPool *utils.StringPool commits := self.getSequencerCommits(hashPool) if len(commits) > 0 { // If we have any commits in .git/sequencer/todo, then the last one of - // those is the conflicting one. - commits[len(commits)-1].Status = models.StatusConflicted + // those is the one that git stopped at. + switch getCurrentSequencerTodoState(self.cmd, self.repoPaths.WorktreeGitDirPath()) { + case sequencerTodoStarted: + commits[len(commits)-1].Status = models.StatusConflicted + case sequencerTodoNotStarted: + // It is still pending like the other ones + case sequencerTodoCommitted: + commits = commits[:len(commits)-1] + } } else { // For single-commit cherry-picks and reverts, git apparently doesn't // use the sequencer; in that case, CHERRY_PICK_HEAD or REVERT_HEAD is @@ -430,38 +437,7 @@ func (self *CommitLoader) getConflictedCommitImpl(hashPool *utils.StringPool, to return nil } - // In certain cases, git reschedules commands that failed. One example is if - // a patch would overwrite an untracked file (another one is an "exec" that - // failed, but we don't care about that here because we dealt with exec - // already above). To detect this, compare the last command of the "done" - // file against the first command of "git-rebase-todo"; if they are the - // same, the command was rescheduled. - if len(doneTodos) > 0 && len(todos) > 0 && doneTodos[len(doneTodos)-1] == todos[0] { - // Command was rescheduled, no need to display it - return nil - } - - // Older versions of git have a bug whereby, if a command is rescheduled, - // the last successful command is appended to the "done" file again. To - // detect this, we need to compare the second-to-last done entry against the - // first todo entry, and also compare the last done entry against the - // last-but-two done entry; this latter check is needed for the following - // case: - // pick A - // exec make test - // pick B - // exec make test - // If pick B fails with conflicts, then the "done" file contains - // pick A - // exec make test - // pick B - // and git-rebase-todo contains - // exec make test - // Without the last condition we would erroneously treat this as the exec - // command being rescheduled, so we wouldn't display our fake entry for - // "pick B". - if len(doneTodos) >= 3 && len(todos) > 0 && doneTodos[len(doneTodos)-2] == todos[0] && - doneTodos[len(doneTodos)-1] == doneTodos[len(doneTodos)-3] { + if utils.LastDoneTodoWasRescheduled(doneTodos, todos) { // Command was rescheduled, no need to display it return nil } diff --git a/pkg/commands/git_commands/lock_error_recovery.go b/pkg/commands/git_commands/lock_error_recovery.go new file mode 100644 index 000000000..7a0834f95 --- /dev/null +++ b/pkg/commands/git_commands/lock_error_recovery.go @@ -0,0 +1,156 @@ +package git_commands + +import ( + "bytes" + "os" + "path/filepath" + "strings" + + "github.com/jesseduffield/lazygit/pkg/commands/oscommands" + "github.com/jesseduffield/lazygit/pkg/utils" +) + +// resumeRebaseAfterLockError sets the lock-error recovery (see +// CmdObj.RecoverFromLockErrorWith) of cmdObj, a command that starts or +// continues a rebase in the worktree whose git dir is gitDir. Call it right +// before running the command, since it records the state of the rebase at +// that time. +// +// If a lock stops the command before it changes the state of the rebase, we +// run the command again. This includes a command that hasn't started the +// rebase yet, since git cleans up after itself in that case. If a lock stops +// the rebase partway through, git normally reschedules the todo that failed, +// and the command made by newContinueCmdObj ("git rebase --continue") runs it +// again. In some cases git doesn't reschedule it, for example if the lock +// stops a pick after it has applied the changes but before it has committed +// them. Continuing could then lose the commit, so we give up and leave the +// rebase to the user. +func resumeRebaseAfterLockError( + cmdObj *oscommands.CmdObj, + gitDir string, + commentChar byte, + newContinueCmdObj func() *oscommands.CmdObj, +) *oscommands.CmdObj { + var recoverFrom func(cmdObj *oscommands.CmdObj, stateBefore string) *oscommands.CmdObj + recoverFrom = func(cmdObj *oscommands.CmdObj, stateBefore string) *oscommands.CmdObj { + return cmdObj.RecoverFromLockErrorWith(func() *oscommands.CmdObj { + state := rebaseState(gitDir) + if state == stateBefore { + return cmdObj + } + + if !wasLastRebaseTodoRescheduled(gitDir, commentChar) { + return nil + } + + return recoverFrom(newContinueCmdObj(), state) + }) + } + + 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 + }) +} + +// 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. +func rebaseState(gitDir string) string { + var state strings.Builder + for _, dir := range []string{"rebase-merge", "rebase-apply"} { + if dirExists(filepath.Join(gitDir, dir)) { + state.WriteString(dir + "\n") + } + } + for _, file := range []string{"rebase-merge/done", "rebase-merge/git-rebase-todo", "rebase-apply/next"} { + if content, err := os.ReadFile(filepath.Join(gitDir, file)); err == nil { + state.WriteString(file + "\n") + state.Write(content) + } + } + return state.String() +} + +// wasLastRebaseTodoRescheduled tells whether git put the todo that it executed +// last back into the todo list of the rebase that is in progress in gitDir. +// It's false if no rebase is in progress, or if the rebase doesn't use the +// rebase-merge directory; that kind of rebase never reschedules anything. +func wasLastRebaseTodoRescheduled(gitDir string, commentChar byte) bool { + doneTodos, err := utils.ReadRebaseTodoFile(filepath.Join(gitDir, "rebase-merge", "done"), commentChar) + if err != nil { + return false + } + + todos, err := utils.ReadRebaseTodoFile(filepath.Join(gitDir, "rebase-merge", "git-rebase-todo"), commentChar) + if err != nil { + return false + } + + return utils.LastDoneTodoWasRescheduled(doneTodos, todos) +} + +func dirExists(path string) bool { + info, err := os.Stat(path) + return err == nil && info.IsDir() +} diff --git a/pkg/commands/git_commands/lock_error_recovery_test.go b/pkg/commands/git_commands/lock_error_recovery_test.go new file mode 100644 index 000000000..3b3eff1ba --- /dev/null +++ b/pkg/commands/git_commands/lock_error_recovery_test.go @@ -0,0 +1,229 @@ +package git_commands + +import ( + "errors" + "os" + "path/filepath" + "testing" + + "github.com/jesseduffield/lazygit/pkg/commands/oscommands" + "github.com/stretchr/testify/assert" +) + +func TestResumeRebaseAfterLockError(t *testing.T) { + newCmdObj := func(args ...string) *oscommands.CmdObj { + return oscommands.NewDummyCmdObjBuilder(nil).New(args) + } + + writeRebaseMergeFiles := func(gitDir string, done string, todo string) { + dir := filepath.Join(gitDir, "rebase-merge") + assert.NoError(t, os.MkdirAll(dir, 0o755)) + assert.NoError(t, os.WriteFile(filepath.Join(dir, "done"), []byte(done), 0o644)) + assert.NoError(t, os.WriteFile(filepath.Join(dir, "git-rebase-todo"), []byte(todo), 0o644)) + } + + // Sets up the recovery for cmdObj, then calls runUntilLockError to do what + // the command did before the lock stopped it, and returns the command that + // the recovery wants to run next. + recoverFromLockError := func(gitDir string, cmdObj *oscommands.CmdObj, runUntilLockError func()) *oscommands.CmdObj { + resumeRebaseAfterLockError(cmdObj, gitDir, '#', func() *oscommands.CmdObj { + return newCmdObj("git", "rebase", "--continue") + }) + runUntilLockError() + return cmdObj.GetLockErrorRecovery()() + } + + t.Run("runs the command again if it didn't start the rebase", func(t *testing.T) { + cmdObj := newCmdObj("git", "rebase", "--interactive", "main") + + assert.Same(t, cmdObj, recoverFromLockError(t.TempDir(), cmdObj, func() {})) + }) + + t.Run("runs the command again if it didn't change the rebase", func(t *testing.T) { + gitDir := t.TempDir() + writeRebaseMergeFiles(gitDir, + "pick 1234567 one\n", + "pick 2345678 two\n") + cmdObj := newCmdObj("git", "rebase", "--continue") + + assert.Same(t, cmdObj, recoverFromLockError(gitDir, cmdObj, func() {})) + }) + + t.Run("continues the rebase if git rescheduled the todo that failed", func(t *testing.T) { + gitDir := t.TempDir() + + next := recoverFromLockError(gitDir, newCmdObj("git", "rebase", "--interactive", "main"), func() { + writeRebaseMergeFiles(gitDir, + "pick 1234567 one\npick 2345678 two\n", + "pick 2345678 two\npick 3456789 three\n") + }) + + assert.Equal(t, "git rebase --continue", next.ToString()) + + // If continuing fails before it changes the rebase, we run it again + assert.Same(t, next, next.GetLockErrorRecovery()()) + + // If continuing fails after it has changed the rebase, and git + // rescheduled the todo that failed, we continue again + writeRebaseMergeFiles(gitDir, + "pick 1234567 one\npick 2345678 two\npick 2345678 two\npick 3456789 three\n", + "pick 3456789 three\n") + nextAfterContinuing := next.GetLockErrorRecovery()() + assert.Equal(t, "git rebase --continue", nextAfterContinuing.ToString()) + assert.NotSame(t, next, nextAfterContinuing) + + // If the rebase is gone after continuing failed, we give up + assert.NoError(t, os.RemoveAll(filepath.Join(gitDir, "rebase-merge"))) + assert.Nil(t, nextAfterContinuing.GetLockErrorRecovery()()) + }) + + t.Run("continues the rebase if an older git version rescheduled the todo that failed", func(t *testing.T) { + gitDir := t.TempDir() + + next := recoverFromLockError(gitDir, newCmdObj("git", "rebase", "--interactive", "main"), func() { + writeRebaseMergeFiles(gitDir, + "pick 1234567 one\npick 2345678 two\npick 1234567 one\n", + "pick 2345678 two\npick 3456789 three\n") + }) + + assert.Equal(t, "git rebase --continue", next.ToString()) + }) + + t.Run("gives up if git didn't reschedule the todo that failed", func(t *testing.T) { + gitDir := t.TempDir() + + assert.Nil(t, recoverFromLockError(gitDir, newCmdObj("git", "rebase", "--interactive", "main"), func() { + writeRebaseMergeFiles(gitDir, + "pick 1234567 one\npick 2345678 two\n", + "pick 3456789 three\n") + })) + }) + + t.Run("gives up if a rebase without a rebase-merge directory is in progress", func(t *testing.T) { + gitDir := t.TempDir() + + assert.Nil(t, recoverFromLockError(gitDir, newCmdObj("git", "pull"), func() { + assert.NoError(t, os.MkdirAll(filepath.Join(gitDir, "rebase-apply"), 0o755)) + })) + }) +} + +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() + }) + } +} + +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 c79105c1f..c5e289e49 100644 --- a/pkg/commands/git_commands/rebase.go +++ b/pkg/commands/git_commands/rebase.go @@ -256,7 +256,7 @@ func (self *RebaseCommands) PrepareInteractiveRebaseCommand(opts PrepareInteract cmdObj.AddEnvVars("GIT_EDITOR=" + ex) } - return cmdObj + return self.resumeAfterLockError(cmdObj) } // GitRebaseEditTodo runs "git rebase --edit-todo", saving the given todosFileContent to the file @@ -408,7 +408,7 @@ func (self *RebaseCommands) SquashAllAboveFixupCommits(commit *models.Commit) er Arg("--interactive", "--rebase-merges", "--autostash", "--autosquash", hashOrRoot). ToArgv() - return self.runSkipEditorCommand(self.cmd.New(cmdArgs)) + return self.resumeAfterLockError(skipEditor(self.cmd.New(cmdArgs))).Run() } // BeginInteractiveRebaseForCommit starts an interactive rebase to edit the current @@ -491,8 +491,36 @@ func (self *RebaseCommands) AbortRebase() error { // GenericMerge takes a commandType of "merge" or "rebase" and a command of "abort", "skip" or "continue" // By default we skip the editor in the case where a commit will be made func (self *RebaseCommands) GenericMergeOrRebaseAction(commandType string, command string) error { - err := self.runSkipEditorCommand(self.GenericMergeOrRebaseActionCmdObj(commandType, command)) + undoRepeatingTodo := func() {} + if command == "continue" && (commandType == "cherry-pick" || commandType == "revert") { + var err error + undoRepeatingTodo, err = repeatCurrentSequencerTodo( + self.cmd, self.repoPaths.WorktreeGitDirPath(), self.config.GetCoreCommentChar()) + if err != nil { + return err + } + } + + if commandType == "rebase" && command == "continue" { + if err := self.commitPickThatRebaseStoppedAt(); err != nil { + return err + } + } + + 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") { return err } @@ -513,7 +541,32 @@ func (self *RebaseCommands) GenericMergeOrRebaseAction(commandType string, comma return nil } -func (self *RebaseCommands) runSkipEditorCommand(cmdObj *oscommands.CmdObj) error { +// commitPickThatRebaseStoppedAt commits the changes of a pick if the rebase +// stopped after it applied them, but before it committed them (see +// rebaseStoppedBeforeCommittingPick). "git rebase --continue" refuses to commit +// them in that case. "git cherry-pick --continue" commits them with the author +// and message of the pick, like the rebase would have. +func (self *RebaseCommands) commitPickThatRebaseStoppedAt() error { + if !rebaseStoppedBeforeCommittingPick(self.repoPaths.WorktreeGitDirPath(), self.config.GetCoreCommentChar()) { + return nil + } + + return skipEditor(self.GenericMergeOrRebaseActionCmdObj("cherry-pick", "continue")).Run() +} + +// 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(), + func() *oscommands.CmdObj { + return skipEditor(self.GenericMergeOrRebaseActionCmdObj("rebase", "continue")) + }) +} + +// skipEditor makes git use lazygit as the editor for the given command; lazygit +// then exits right away, so git uses the file as it is. +func skipEditor(cmdObj *oscommands.CmdObj) *oscommands.CmdObj { instruction := daemon.NewExitImmediatelyInstruction() lazyGitPath := oscommands.GetLazygitPath() return cmdObj. @@ -523,8 +576,7 @@ func (self *RebaseCommands) runSkipEditorCommand(cmdObj *oscommands.CmdObj) erro "EDITOR="+lazyGitPath, "VISUAL="+lazyGitPath, ). - AddEnvVars(daemon.ToEnvVars(instruction)...). - Run() + AddEnvVars(daemon.ToEnvVars(instruction)...) } // DiscardOldFileChanges discards changes to a file from an old commit @@ -576,7 +628,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/commands/git_commands/rebase_test.go b/pkg/commands/git_commands/rebase_test.go index 46f1fcc1c..fa47e668e 100644 --- a/pkg/commands/git_commands/rebase_test.go +++ b/pkg/commands/git_commands/rebase_test.go @@ -89,7 +89,7 @@ func TestRebaseSkipEditorCommand(t *testing.T) { return true }, "", nil) instance := buildRebaseCommands(commonDeps{runner: runner}) - err := instance.runSkipEditorCommand(instance.cmd.New(cmdArgs)) + err := skipEditor(instance.cmd.New(cmdArgs)).Run() assert.NoError(t, err) runner.CheckForMissingCalls() } diff --git a/pkg/commands/git_commands/sequencer.go b/pkg/commands/git_commands/sequencer.go new file mode 100644 index 000000000..b216cc21e --- /dev/null +++ b/pkg/commands/git_commands/sequencer.go @@ -0,0 +1,101 @@ +package git_commands + +import ( + "bytes" + "os" + "path/filepath" + "strings" + + "github.com/jesseduffield/lazygit/pkg/commands/oscommands" + "github.com/samber/lo" +) + +// sequencerTodoState tells what has happened so far with the todo that a +// cherry-pick or revert of several commits stopped at. This is the first todo +// in .git/sequencer/todo. +type sequencerTodoState int + +const ( + // Git started to apply the todo, and stopped because of conflicts (or + // because it couldn't commit the result). CHERRY_PICK_HEAD or REVERT_HEAD + // exists, and continuing commits the changes. + sequencerTodoStarted sequencerTodoState = iota + // Git stopped before it started to apply the todo, for example because the + // commit would overwrite an untracked file. + sequencerTodoNotStarted + // The user has committed the result of the todo themselves, for example + // after resolving its conflicts. Continuing skips it. + sequencerTodoCommitted +) + +// getCurrentSequencerTodoState returns the state of the todo that the +// cherry-pick or revert of several commits that is in progress in gitDir +// stopped at. +// +// Without CHERRY_PICK_HEAD or REVERT_HEAD, we can tell whether the user has +// committed the todo by whether HEAD has moved since git last tried to apply a +// commit, which git records in .git/sequencer/abort-safety. Git does the same +// when it decides whether "--skip" is possible (see rollback_is_safe in git's +// sequencer.c). +func getCurrentSequencerTodoState(cmd oscommands.ICmdObjBuilder, gitDir string) sequencerTodoState { + for _, file := range []string{"CHERRY_PICK_HEAD", "REVERT_HEAD"} { + if _, err := os.Stat(filepath.Join(gitDir, file)); err == nil { + return sequencerTodoStarted + } + } + + // If the file doesn't exist, git compares HEAD to the null hash; an empty + // string does the same for us + abortSafety, _ := os.ReadFile(filepath.Join(gitDir, "sequencer", "abort-safety")) + + // If HEAD is unborn, this fails and returns an empty string + head, _ := cmd.New(NewGitCmd("rev-parse").Arg("--verify", "--quiet", "HEAD").ToArgv()).DontLog().RunWithOutput() + + if strings.TrimSpace(head) == strings.TrimSpace(string(abortSafety)) { + return sequencerTodoNotStarted + } + return sequencerTodoCommitted +} + +// repeatCurrentSequencerTodo prepares the cherry-pick or revert of several +// commits that is in progress in gitDir for "--continue". If git stopped it +// before it started to apply the current todo, "--continue" would skip that +// todo, because git can't tell it apart from a todo that the user has +// committed themselves. So we put a copy of the todo in front of it, and git +// skips the copy instead. +// +// The returned function removes the copy again if git hasn't got as far as +// skipping it, for example because it refused to continue. +func repeatCurrentSequencerTodo(cmd oscommands.ICmdObjBuilder, gitDir string, commentChar byte) (func(), error) { + noop := func() {} + + todoPath := filepath.Join(gitDir, "sequencer", "todo") + content, err := os.ReadFile(todoPath) + if err != nil { + // No cherry-pick or revert of several commits is in progress + return noop, nil + } + + if getCurrentSequencerTodoState(cmd, gitDir) != sequencerTodoNotStarted { + return noop, nil + } + + currentTodo, found := lo.Find(strings.Split(string(content), "\n"), func(line string) bool { + line = strings.TrimSpace(line) + return line != "" && line[0] != commentChar + }) + if !found { + return noop, nil + } + + newContent := append([]byte(currentTodo+"\n"), content...) + if err := os.WriteFile(todoPath, newContent, 0o644); err != nil { + return nil, err + } + + return func() { + if currentContent, err := os.ReadFile(todoPath); err == nil && bytes.Equal(currentContent, newContent) { + _ = os.WriteFile(todoPath, content, 0o644) + } + }, nil +} diff --git a/pkg/commands/git_commands/sequencer_test.go b/pkg/commands/git_commands/sequencer_test.go new file mode 100644 index 000000000..0ee54e3bb --- /dev/null +++ b/pkg/commands/git_commands/sequencer_test.go @@ -0,0 +1,145 @@ +package git_commands + +import ( + "errors" + "os" + "path/filepath" + "testing" + + "github.com/jesseduffield/lazygit/pkg/commands/oscommands" + "github.com/stretchr/testify/assert" +) + +func TestGetCurrentSequencerTodoState(t *testing.T) { + revParseArgs := []string{"rev-parse", "--verify", "--quiet", "HEAD"} + + scenarios := []struct { + testName string + files map[string]string + runner *oscommands.FakeCmdObjRunner + expectedState sequencerTodoState + }{ + { + testName: "CHERRY_PICK_HEAD exists", + files: map[string]string{"CHERRY_PICK_HEAD": "1234567\n"}, + runner: oscommands.NewFakeRunner(t), + expectedState: sequencerTodoStarted, + }, + { + testName: "REVERT_HEAD exists", + files: map[string]string{"REVERT_HEAD": "1234567\n"}, + runner: oscommands.NewFakeRunner(t), + expectedState: sequencerTodoStarted, + }, + { + testName: "HEAD hasn't moved since git tried to apply a commit", + files: map[string]string{"sequencer/abort-safety": "1234567\n"}, + runner: oscommands.NewFakeRunner(t).ExpectGitArgs(revParseArgs, "1234567\n", nil), + expectedState: sequencerTodoNotStarted, + }, + { + testName: "HEAD has moved since git tried to apply a commit", + files: map[string]string{"sequencer/abort-safety": "1234567\n"}, + runner: oscommands.NewFakeRunner(t).ExpectGitArgs(revParseArgs, "2345678\n", nil), + expectedState: sequencerTodoCommitted, + }, + { + testName: "HEAD is unborn, and there is no abort-safety file", + files: map[string]string{}, + runner: oscommands.NewFakeRunner(t).ExpectGitArgs(revParseArgs, "", errors.New("error")), + expectedState: sequencerTodoNotStarted, + }, + } + + for _, s := range scenarios { + t.Run(s.testName, func(t *testing.T) { + gitDir := t.TempDir() + for path, content := range s.files { + fullPath := filepath.Join(gitDir, path) + assert.NoError(t, os.MkdirAll(filepath.Dir(fullPath), 0o755)) + assert.NoError(t, os.WriteFile(fullPath, []byte(content), 0o644)) + } + + state := getCurrentSequencerTodoState(oscommands.NewDummyCmdObjBuilder(s.runner), gitDir) + + assert.Equal(t, s.expectedState, state) + s.runner.CheckForMissingCalls() + }) + } +} + +func TestRepeatCurrentSequencerTodo(t *testing.T) { + revParseArgs := []string{"rev-parse", "--verify", "--quiet", "HEAD"} + todos := "pick 1234567 one\npick 2345678 two\n" + repeatedTodos := "pick 1234567 one\npick 1234567 one\npick 2345678 two\n" + + setUp := func(t *testing.T, files map[string]string) string { + t.Helper() + gitDir := t.TempDir() + for path, content := range files { + fullPath := filepath.Join(gitDir, path) + assert.NoError(t, os.MkdirAll(filepath.Dir(fullPath), 0o755)) + assert.NoError(t, os.WriteFile(fullPath, []byte(content), 0o644)) + } + return gitDir + } + + readTodos := func(t *testing.T, gitDir string) string { + t.Helper() + content, err := os.ReadFile(filepath.Join(gitDir, "sequencer", "todo")) + assert.NoError(t, err) + return string(content) + } + + t.Run("no cherry-pick or revert of several commits in progress", func(t *testing.T) { + gitDir := setUp(t, map[string]string{}) + runner := oscommands.NewFakeRunner(t) + + undo, err := repeatCurrentSequencerTodo(oscommands.NewDummyCmdObjBuilder(runner), gitDir, '#') + + assert.NoError(t, err) + undo() + assert.NoFileExists(t, filepath.Join(gitDir, "sequencer", "todo")) + runner.CheckForMissingCalls() + }) + + t.Run("git started to apply the current todo", func(t *testing.T) { + gitDir := setUp(t, map[string]string{"sequencer/todo": todos, "CHERRY_PICK_HEAD": "1234567\n"}) + runner := oscommands.NewFakeRunner(t) + + _, err := repeatCurrentSequencerTodo(oscommands.NewDummyCmdObjBuilder(runner), gitDir, '#') + + assert.NoError(t, err) + assert.Equal(t, todos, readTodos(t, gitDir)) + runner.CheckForMissingCalls() + }) + + t.Run("git stopped before it started to apply the current todo", func(t *testing.T) { + gitDir := setUp(t, map[string]string{"sequencer/todo": todos, "sequencer/abort-safety": "abcdef0\n"}) + runner := oscommands.NewFakeRunner(t).ExpectGitArgs(revParseArgs, "abcdef0\n", nil) + + undo, err := repeatCurrentSequencerTodo(oscommands.NewDummyCmdObjBuilder(runner), gitDir, '#') + + assert.NoError(t, err) + assert.Equal(t, repeatedTodos, readTodos(t, gitDir)) + + // If git didn't touch the todos, undo restores them + undo() + assert.Equal(t, todos, readTodos(t, gitDir)) + runner.CheckForMissingCalls() + }) + + t.Run("undo leaves the todos alone if git has changed them", func(t *testing.T) { + gitDir := setUp(t, map[string]string{"sequencer/todo": todos, "sequencer/abort-safety": "abcdef0\n"}) + runner := oscommands.NewFakeRunner(t).ExpectGitArgs(revParseArgs, "abcdef0\n", nil) + + undo, err := repeatCurrentSequencerTodo(oscommands.NewDummyCmdObjBuilder(runner), gitDir, '#') + assert.NoError(t, err) + gitsTodos := "pick 2345678 two\n" + assert.NoError(t, os.WriteFile(filepath.Join(gitDir, "sequencer", "todo"), []byte(gitsTodos), 0o644)) + + undo() + assert.Equal(t, gitsTodos, readTodos(t, gitDir)) + runner.CheckForMissingCalls() + }) +} diff --git a/pkg/commands/git_commands/status.go b/pkg/commands/git_commands/status.go index d9120d87d..f6b1fd460 100644 --- a/pkg/commands/git_commands/status.go +++ b/pkg/commands/git_commands/status.go @@ -7,7 +7,9 @@ import ( "time" "github.com/jesseduffield/lazygit/pkg/commands/models" + "github.com/jesseduffield/lazygit/pkg/utils" "github.com/spf13/afero" + "github.com/stefanhaller/git-todo-parser/todo" ) type StatusCommands struct { @@ -49,39 +51,87 @@ func (self *StatusCommands) IsInMergeState() (bool, error) { } func (self *StatusCommands) IsInCherryPick() (bool, error) { - exists, err := self.os.FileExists(filepath.Join(self.repoPaths.WorktreeGitDirPath(), "CHERRY_PICK_HEAD")) + hasCherryPickHead, err := self.hasCherryPickHead() + if err != nil || hasCherryPickHead { + return hasCherryPickHead, err + } + return self.sequencerCommand() == todo.Pick, nil +} + +func (self *StatusCommands) hasCherryPickHead() (bool, error) { + gitDir := self.repoPaths.WorktreeGitDirPath() + exists, err := self.os.FileExists(filepath.Join(gitDir, "CHERRY_PICK_HEAD")) if err != nil || !exists { return exists, err } - // Sometimes, CHERRY_PICK_HEAD is present during rebases even if no - // cherry-pick is in progress. I suppose this is because rebase used to be - // implemented as a series of cherry-picks, so this could be remnants of - // code that is shared between cherry-pick and rebase, or something. The way - // to tell if this is the case is to check for the presence of the - // stopped-sha file, which records the sha of the last pick that was - // executed before the rebase stopped, and seeing if the sha in that file is - // the same as the one in CHERRY_PICK_HEAD. - cherryPickHead, err := os.ReadFile(filepath.Join(self.repoPaths.WorktreeGitDirPath(), "CHERRY_PICK_HEAD")) + cherryPickHead, err := os.ReadFile(filepath.Join(gitDir, "CHERRY_PICK_HEAD")) if err != nil { return false, err } - stoppedSha, err := os.ReadFile(filepath.Join(self.repoPaths.WorktreeGitDirPath(), "rebase-merge", "stopped-sha")) + return !cherryPickHeadBelongsToRebase(gitDir, strings.TrimSpace(string(cherryPickHead)), self.config.GetCoreCommentChar()), nil +} + +// cherryPickHeadBelongsToRebase tells whether CHERRY_PICK_HEAD, whose value is +// cherryPickHead, belongs to a pick of the rebase that is in progress in +// gitDir, rather than to a cherry-pick. A rebase writes CHERRY_PICK_HEAD when it +// has applied the changes of a pick, and removes it when it has committed them. +// So it is still there if the rebase stopped because the pick conflicted; git +// records the pick in rebase-merge/stopped-sha then. It is also still there if +// git couldn't commit the changes, for example because it couldn't take the +// index lock. Git exits without writing stopped-sha in that case, but the pick +// is the last line of rebase-merge/done. +func cherryPickHeadBelongsToRebase(gitDir string, cherryPickHead string, commentChar byte) bool { + // The hashes in these files may be abbreviated + isPrefixOfCherryPickHead := func(hash string) bool { + return hash != "" && strings.HasPrefix(cherryPickHead, hash) + } + + stoppedSha, err := os.ReadFile(filepath.Join(gitDir, "rebase-merge", "stopped-sha")) + if err == nil && isPrefixOfCherryPickHead(strings.TrimSpace(string(stoppedSha))) { + return true + } + + doneTodos, err := utils.ReadRebaseTodoFile(filepath.Join(gitDir, "rebase-merge", "done"), commentChar) + return err == nil && len(doneTodos) > 0 && isPrefixOfCherryPickHead(doneTodos[len(doneTodos)-1].Commit) +} + +// rebaseStoppedBeforeCommittingPick tells whether the rebase that is in +// progress in gitDir has applied the changes of a pick, but exited before it +// could commit them (see cherryPickHeadBelongsToRebase). "git rebase +// --continue" refuses to commit the changes in that case, because git didn't +// write the message file that it writes when a pick conflicts. +func rebaseStoppedBeforeCommittingPick(gitDir string, commentChar byte) bool { + cherryPickHead, err := os.ReadFile(filepath.Join(gitDir, "CHERRY_PICK_HEAD")) if err != nil { - // If we get an error we assume the file doesn't exist - return true, nil + return false } - cherryPickHeadStr := strings.TrimSpace(string(cherryPickHead)) - stoppedShaStr := strings.TrimSpace(string(stoppedSha)) - // Need to use HasPrefix here because the cherry-pick HEAD is a full sha1, - // but stopped-sha is an abbreviated sha1 - if strings.HasPrefix(cherryPickHeadStr, stoppedShaStr) { - return false, nil + if _, err := os.Stat(filepath.Join(gitDir, "rebase-merge", "message")); err == nil { + return false } - return true, nil + return cherryPickHeadBelongsToRebase(gitDir, strings.TrimSpace(string(cherryPickHead)), commentChar) } func (self *StatusCommands) IsInRevert() (bool, error) { - return self.os.FileExists(filepath.Join(self.repoPaths.WorktreeGitDirPath(), "REVERT_HEAD")) + hasRevertHead, err := self.os.FileExists(filepath.Join(self.repoPaths.WorktreeGitDirPath(), "REVERT_HEAD")) + if err != nil || hasRevertHead { + return hasRevertHead, err + } + return self.sequencerCommand() == todo.Revert, nil +} + +// sequencerCommand returns the command of the first todo of a cherry-pick or +// revert of several commits that is in progress, or 0 if there is none. Git +// keeps the todos of such an operation in .git/sequencer. If it stops before it +// starts to apply a commit (for example because the commit would overwrite an +// untracked file), it doesn't write CHERRY_PICK_HEAD or REVERT_HEAD. Like git +// status, we still consider the cherry-pick or revert to be in progress then. +func (self *StatusCommands) sequencerCommand() todo.TodoCommand { + todos, err := utils.ReadRebaseTodoFile( + filepath.Join(self.repoPaths.WorktreeGitDirPath(), "sequencer", "todo"), self.config.GetCoreCommentChar()) + if err != nil || len(todos) == 0 { + return 0 + } + return todos[0].Command } // RefsSnapshot returns a string fingerprint of the current state of local diff --git a/pkg/commands/git_commands/status_test.go b/pkg/commands/git_commands/status_test.go index dc6b2d558..45987f912 100644 --- a/pkg/commands/git_commands/status_test.go +++ b/pkg/commands/git_commands/status_test.go @@ -1,6 +1,8 @@ package git_commands import ( + "os" + "path/filepath" "testing" "github.com/go-errors/errors" @@ -89,3 +91,134 @@ func TestStatusRefsSnapshot(t *testing.T) { }) } } + +func TestStatusWorkingTreeStateOfCherryPickOrRevert(t *testing.T) { + scenarios := []struct { + testName string + files map[string]string + expectedCherryPicking bool + expectedReverting bool + }{ + { + testName: "nothing in progress", + files: map[string]string{}, + }, + { + testName: "CHERRY_PICK_HEAD", + files: map[string]string{"CHERRY_PICK_HEAD": "1234567\n"}, + expectedCherryPicking: true, + }, + { + testName: "REVERT_HEAD", + files: map[string]string{"REVERT_HEAD": "1234567\n"}, + expectedReverting: true, + }, + { + testName: "CHERRY_PICK_HEAD of a rebase that stopped because of conflicts", + files: map[string]string{ + "CHERRY_PICK_HEAD": "2345678abcdef\n", + "rebase-merge/stopped-sha": "2345678\n", + "rebase-merge/done": "pick 1234567abcdef one\npick 2345678abcdef two\n", + }, + expectedCherryPicking: false, + }, + { + testName: "CHERRY_PICK_HEAD of a rebase that couldn't commit a pick", + files: map[string]string{ + "CHERRY_PICK_HEAD": "2345678abcdef\n", + "rebase-merge/done": "pick 1234567abcdef one\npick 2345678abcdef two\n", + }, + expectedCherryPicking: false, + }, + { + testName: "cherry-pick while a rebase is stopped", + files: map[string]string{ + "CHERRY_PICK_HEAD": "3456789abcdef\n", + "rebase-merge/done": "pick 1234567abcdef one\nedit 2345678abcdef two\n", + }, + expectedCherryPicking: true, + }, + { + testName: "cherry-pick of several commits that stopped before applying one", + files: map[string]string{"sequencer/todo": "pick 1234567 one\npick 2345678 two\n"}, + expectedCherryPicking: true, + }, + { + testName: "revert of several commits that stopped before applying one", + files: map[string]string{"sequencer/todo": "revert 1234567 one\nrevert 2345678 two\n"}, + expectedReverting: true, + }, + } + + for _, s := range scenarios { + t.Run(s.testName, func(t *testing.T) { + repoDir := t.TempDir() + for path, content := range s.files { + fullPath := filepath.Join(repoDir, ".git", path) + assert.NoError(t, os.MkdirAll(filepath.Dir(fullPath), 0o755)) + assert.NoError(t, os.WriteFile(fullPath, []byte(content), 0o644)) + } + instance := buildStatusCommands(commonDeps{repoPaths: MockRepoPaths(repoDir)}) + + state := instance.WorkingTreeState() + + assert.Equal(t, s.expectedCherryPicking, state.CherryPicking) + assert.Equal(t, s.expectedReverting, state.Reverting) + }) + } +} + +func TestRebaseStoppedBeforeCommittingPick(t *testing.T) { + scenarios := []struct { + testName string + files map[string]string + expected bool + }{ + { + testName: "the rebase couldn't commit a pick", + files: map[string]string{ + "CHERRY_PICK_HEAD": "2345678abcdef\n", + "rebase-merge/done": "pick 1234567abcdef one\npick 2345678abcdef two\n", + }, + expected: true, + }, + { + testName: "the rebase stopped because a pick conflicted", + files: map[string]string{ + "CHERRY_PICK_HEAD": "2345678abcdef\n", + "rebase-merge/stopped-sha": "2345678\n", + "rebase-merge/message": "two\n", + "rebase-merge/done": "pick 1234567abcdef one\npick 2345678abcdef two\n", + }, + expected: false, + }, + { + testName: "no CHERRY_PICK_HEAD", + files: map[string]string{ + "rebase-merge/done": "pick 1234567abcdef one\nedit 2345678abcdef two\n", + }, + expected: false, + }, + { + testName: "CHERRY_PICK_HEAD of a cherry-pick while a rebase is stopped", + files: map[string]string{ + "CHERRY_PICK_HEAD": "3456789abcdef\n", + "rebase-merge/done": "pick 1234567abcdef one\nedit 2345678abcdef two\n", + }, + expected: false, + }, + } + + for _, s := range scenarios { + t.Run(s.testName, func(t *testing.T) { + gitDir := t.TempDir() + for path, content := range s.files { + fullPath := filepath.Join(gitDir, path) + assert.NoError(t, os.MkdirAll(filepath.Dir(fullPath), 0o755)) + assert.NoError(t, os.WriteFile(fullPath, []byte(content), 0o644)) + } + + assert.Equal(t, s.expected, rebaseStoppedBeforeCommittingPick(gitDir, '#')) + }) + } +} diff --git a/pkg/commands/git_commands/sync.go b/pkg/commands/git_commands/sync.go index 603e5b8eb..59f052db8 100644 --- a/pkg/commands/git_commands/sync.go +++ b/pkg/commands/git_commands/sync.go @@ -111,7 +111,19 @@ func (self *SyncCommands) Pull(task gocui.Task, opts PullOptions) error { // setting GIT_SEQUENCE_EDITOR to ':' as a way of skipping it, in case the user // has 'pull.rebase = interactive' configured. - return self.cmd.New(cmdArgs).AddEnvVars("GIT_SEQUENCE_EDITOR=:").PromptOnCredentialRequest(task).Run() + cmdObj := self.cmd.New(cmdArgs).AddEnvVars("GIT_SEQUENCE_EDITOR=:").PromptOnCredentialRequest(task) + + // With pull.rebase set, a lock error can stop the pull partway through its + // rebase. + gitDir := lo.Ternary(opts.WorktreeGitDir != "", opts.WorktreeGitDir, self.repoPaths.WorktreeGitDirPath()) + return resumeRebaseAfterLockError(cmdObj, gitDir, self.config.GetCoreCommentChar(), func() *oscommands.CmdObj { + continueArgs := NewGitCmd("rebase"). + Arg("--continue"). + GitDirIf(opts.WorktreeGitDir != "", opts.WorktreeGitDir). + WorktreePathIf(opts.WorktreePath != "", opts.WorktreePath). + ToArgv() + return skipEditor(self.cmd.New(continueArgs)) + }).Run() } // Fetches the given branches of the given remote, updating their diff --git a/pkg/commands/oscommands/cmd_obj.go b/pkg/commands/oscommands/cmd_obj.go index 2f7e66f7f..dd38fd384 100644 --- a/pkg/commands/oscommands/cmd_obj.go +++ b/pkg/commands/oscommands/cmd_obj.go @@ -37,6 +37,9 @@ type CmdObj struct { // can be set so that we don't run certain commands simultaneously mutex *deadlock.Mutex + + // see RecoverFromLockErrorWith() + lockErrorRecovery func() *CmdObj } type CredentialStrategy int @@ -201,6 +204,27 @@ func (self *CmdObj) WithMutex(mutex *deadlock.Mutex) *CmdObj { return self } +// RecoverFromLockErrorWith sets what the git command runner does when this +// command fails because another git process holds a lock (see +// gitCmdObjRunner.retryOnLockError). Without it, the runner runs the command +// again. That is wrong for a command that starts an operation of several +// steps, such as a rebase, if the lock only stopped it partway through. +// +// The runner calls recovery after each failed attempt, including the last +// one, so that recovery can also clean up after the command. It returns the +// command to run next: this one to run it again, a different one (for +// example, one that resumes the operation), or nil to give up and return the +// error. +func (self *CmdObj) RecoverFromLockErrorWith(recovery func() *CmdObj) *CmdObj { + self.lockErrorRecovery = recovery + + return self +} + +func (self *CmdObj) GetLockErrorRecovery() func() *CmdObj { + return self.lockErrorRecovery +} + // runs the command and returns an error if any func (self *CmdObj) Run() error { return self.runner.Run(self) diff --git a/pkg/integration/tests/cherry_pick/cherry_pick_conflicts_committed_by_user.go b/pkg/integration/tests/cherry_pick/cherry_pick_conflicts_committed_by_user.go new file mode 100644 index 000000000..be986de05 --- /dev/null +++ b/pkg/integration/tests/cherry_pick/cherry_pick_conflicts_committed_by_user.go @@ -0,0 +1,84 @@ +package cherry_pick + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" + "github.com/jesseduffield/lazygit/pkg/integration/tests/shared" +) + +var CherryPickConflictsCommittedByUser = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Cherry pick two commits, the first of which conflicts; the user commits the resolution themselves before continuing", + ExtraCmdArgs: []string{}, + Skip: false, + SetupConfig: func(config *config.AppConfig) { + config.GetUserConfig().Git.LocalBranchSortOrder = "recency" + }, + SetupRepo: func(shell *Shell) { + shared.MergeConflictsSetup(shell) + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Branches(). + Focus(). + Lines( + Contains("first-change-branch"), + Contains("second-change-branch"), + Contains("original-branch"), + ). + SelectNextItem(). + PressEnter() + + t.Views().SubCommits(). + IsFocused(). + TopLines( + Contains("second-change-branch unrelated change"), + Contains("second change"), + ). + Press(keys.Commits.CherryPickCopy). + SelectNextItem(). + Press(keys.Commits.CherryPickCopy) + + t.Views().Information().Content(Contains("2 commits copied")) + + t.Views().Commits(). + Focus(). + TopLines( + Contains("first change").IsSelected(), + ). + Press(keys.Commits.PasteCommits) + + t.ExpectPopup().Alert(). + Title(Equals("Cherry-pick")). + Content(Contains("Are you sure you want to cherry-pick the 2 copied commit(s) onto this branch?")). + Confirm() + + t.Common().AcknowledgeConflicts() + + t.Shell(). + UpdateFileAndAdd("file", shared.SecondChangeFileContent). + RunCommand([]string{"git", "commit", "--no-edit"}) + + t.GlobalPress(keys.Universal.Refresh) + + t.Views().Status().Content(Equals("(cherry-picking) repo → first-change-branch")) + + t.Views().Commits(). + TopLines( + Contains("─── Pending cherry-picks"), + Contains("pick").DoesNotContain("CONFLICT").Contains("second-change-branch unrelated change"), + Contains("─── Commits"), + Contains("second change"), + Contains("first change"), + ) + + t.Common().ContinueOnConflictsResolved("cherry-pick") + + t.Views().Status().Content(Equals("repo → first-change-branch")) + + t.Views().Commits(). + TopLines( + Contains("second-change-branch unrelated change"), + Contains("second change"), + Contains("first change"), + ) + }, +}) diff --git a/pkg/integration/tests/cherry_pick/cherry_pick_stopped_by_untracked_file.go b/pkg/integration/tests/cherry_pick/cherry_pick_stopped_by_untracked_file.go new file mode 100644 index 000000000..40c148cf6 --- /dev/null +++ b/pkg/integration/tests/cherry_pick/cherry_pick_stopped_by_untracked_file.go @@ -0,0 +1,96 @@ +package cherry_pick + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var CherryPickStoppedByUntrackedFile = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Cherry pick two commits; an untracked file stops the second one before git starts to apply it; continue after removing the file", + ExtraCmdArgs: []string{}, + Skip: false, + SetupConfig: func(config *config.AppConfig) { + config.GetUserConfig().Git.LocalBranchSortOrder = "recency" + }, + SetupRepo: func(shell *Shell) { + shell. + CreateFileAndAdd("file", "original\n"). + Commit("base"). + NewBranch("source"). + CreateFileAndAdd("file1", "one\n"). + Commit("one"). + CreateFileAndAdd("file2", "two\n"). + Commit("two"). + Checkout("master"). + CreateFile("file2", "untracked\n") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Branches(). + Focus(). + Lines( + Contains("master"), + Contains("source"), + ). + SelectNextItem(). + PressEnter() + + t.Views().SubCommits(). + IsFocused(). + Lines( + Contains("two").IsSelected(), + Contains("one"), + Contains("base"), + ). + Press(keys.Commits.CherryPickCopy). + SelectNextItem(). + Press(keys.Commits.CherryPickCopy) + + t.Views().Information().Content(Contains("2 commits copied")) + + t.Views().Commits(). + Focus(). + Lines( + Contains("base").IsSelected(), + ). + Press(keys.Commits.PasteCommits). + Tap(func() { + t.ExpectPopup().Alert(). + Title(Equals("Cherry-pick")). + Content(Contains("Are you sure you want to cherry-pick the 2 copied commit(s) onto this branch?")). + Confirm() + }). + Tap(func() { + t.ExpectPopup().Alert(). + Title(Equals("Error")). + Content(Contains("The following untracked working tree files would be overwritten by merge")). + Confirm() + }). + Lines( + Contains("─── Pending cherry-picks"), + Contains("pick").DoesNotContain("CONFLICT").Contains("two"), + Contains("─── Commits"), + Contains("one"), + Contains("base"), + ) + + t.Views().Status(). + Content(Equals("(cherry-picking) repo → master")) + + t.Shell().DeleteFile("file2") + + t.GlobalPress(keys.Universal.CreateRebaseOptionsMenu) + t.ExpectPopup().Menu(). + Title(Equals("Cherry-pick options")). + Select(Contains("continue")). + Confirm() + + t.Views().Status().Content(Equals("repo → master")) + + t.Views().Commits(). + Lines( + Contains("two"), + Contains("one"), + Contains("base"), + ) + }, +}) 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 new file mode 100644 index 000000000..d1ae75715 --- /dev/null +++ b/pkg/integration/tests/cherry_pick/lock_error_partway_through_cherry_pick.go @@ -0,0 +1,72 @@ +package cherry_pick + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" + "github.com/jesseduffield/lazygit/pkg/integration/tests/shared" +) + +var LockErrorPartwayThroughCherryPick = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Cherry pick two commits while another git process takes index.lock partway through", + ExtraCmdArgs: []string{}, + Skip: false, + SetupConfig: func(config *config.AppConfig) { + config.GetUserConfig().Git.LocalBranchSortOrder = "recency" + }, + SetupRepo: func(shell *Shell) { + shell. + CreateFileAndAdd("file", "original\n"). + Commit("base"). + NewBranch("source"). + CreateFileAndAdd("file1", "one\n"). + Commit("one"). + CreateFileAndAdd("file2", "two\n"). + Commit("two"). + Checkout("master") + + // The cherry-pick commits "one"; the lock then stops it when it tries + // to pick "two". + shared.HoldIndexLockAfterNextCommit(shell) + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Branches(). + Focus(). + Lines( + Contains("master"), + Contains("source"), + ). + SelectNextItem(). + PressEnter() + + t.Views().SubCommits(). + IsFocused(). + Lines( + Contains("two").IsSelected(), + Contains("one"), + Contains("base"), + ). + Press(keys.Commits.CherryPickCopy). + SelectNextItem(). + Press(keys.Commits.CherryPickCopy) + + t.Views().Information().Content(Contains("2 commits copied")) + + t.Views().Commits(). + Focus(). + Lines( + Contains("base").IsSelected(), + ). + Press(keys.Commits.PasteCommits). + Tap(func() { + t.ExpectPopup().Alert(). + Title(Equals("Cherry-pick")). + Content(Contains("Are you sure you want to cherry-pick the 2 copied commit(s) onto this branch?")). + Confirm() + }). + Lines( + Contains("two"), + Contains("one"), + Contains("base"), + ) + }, +}) diff --git a/pkg/integration/tests/interactive_rebase/lock_error_partway_through_rebase.go b/pkg/integration/tests/interactive_rebase/lock_error_partway_through_rebase.go new file mode 100644 index 000000000..d5d697dfc --- /dev/null +++ b/pkg/integration/tests/interactive_rebase/lock_error_partway_through_rebase.go @@ -0,0 +1,44 @@ +package interactive_rebase + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" + "github.com/jesseduffield/lazygit/pkg/integration/tests/shared" +) + +var LockErrorPartwayThroughRebase = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Drops a commit while another git process takes index.lock partway through the rebase", + ExtraCmdArgs: []string{}, + Skip: false, + SetupConfig: func(config *config.AppConfig) {}, + SetupRepo: func(shell *Shell) { + shell.CreateNCommits(4) + // The rebase makes a commit when it picks commit-03; the lock then + // stops it when it tries to pick commit-04. + shared.HoldIndexLockAfterNextCommit(shell) + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Commits(). + Focus(). + Lines( + Contains("commit-04").IsSelected(), + Contains("commit-03"), + Contains("commit-02"), + Contains("commit-01"), + ). + NavigateToLine(Contains("commit-02")). + Press(keys.Universal.Remove). + Tap(func() { + t.ExpectPopup().Confirmation(). + Title(Equals("Drop commit")). + Content(Contains("Are you sure you want to drop the selected commit(s)?")). + Confirm() + }). + IsFocused(). + Lines( + Contains("commit-04"), + Contains("commit-03"), + Contains("commit-01"), + ) + }, +}) 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 new file mode 100644 index 000000000..f78bd9300 --- /dev/null +++ b/pkg/integration/tests/interactive_rebase/lock_error_partway_through_skip.go @@ -0,0 +1,84 @@ +package interactive_rebase + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" + "github.com/jesseduffield/lazygit/pkg/integration/tests/shared" +) + +var LockErrorPartwayThroughSkip = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Skip in a rebase while another git process takes index.lock before the next commit is committed", + ExtraCmdArgs: []string{}, + Skip: false, + SetupConfig: func(config *config.AppConfig) {}, + SetupRepo: func(shell *Shell) { + shell. + CreateFileAndAdd("file", "original\n"). + Commit("base"). + UpdateFileAndAdd("file", "changed\n"). + Commit("commit to edit"). + CreateFileAndAdd("file-y", "y\n"). + Commit("add file-y"). + CreateFileAndAdd("file-z", "z\n"). + Commit("add file-z") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Commits(). + Focus(). + Lines( + Contains("add file-z").IsSelected(), + Contains("add file-y"), + Contains("commit to edit"), + Contains("base"), + ). + NavigateToLine(Contains("commit to edit")). + Press(keys.Universal.Edit). + Lines( + Contains("─── Pending rebase todos"), + Contains("pick").Contains("add file-z"), + Contains("pick").Contains("add file-y"), + Contains("─── Commits"), + Contains("commit to edit").IsSelected(), + Contains("base"), + ) + + // After amending the commit we stopped at, git has to make a new commit + // when it picks "add file-y", rather than fast-forwarding to it. The + // lock then stops it after it has applied the changes, but before it + // has committed them. + t.Shell().RunCommand([]string{"git", "commit", "--amend", "-m", "edited commit"}) + shared.HoldIndexLockBeforeNextPickIsCommitted(t.Shell()) + + t.GlobalPress(keys.Universal.CreateRebaseOptionsMenu) + t.ExpectPopup().Menu(). + Title(Equals("Rebase options")). + Select(Contains("skip")). + Confirm() + + t.ExpectPopup().Alert(). + Title(Equals("Error")). + Content(Contains("index.lock': File exists.")). + Confirm() + + t.Views().Files(). + Lines( + Contains("A").Contains("file-y"), + ) + + t.Views().Status().Content(Contains("(rebasing)")) + + t.GlobalPress(keys.Universal.CreateRebaseOptionsMenu) + t.ExpectPopup().Menu(). + Title(Equals("Rebase options")). + Select(Contains("continue")). + Confirm() + + t.Views().Commits(). + Lines( + Contains("add file-z"), + Contains("add file-y"), + Contains("edited commit"), + Contains("base"), + ) + }, +}) diff --git a/pkg/integration/tests/shared/index_lock.go b/pkg/integration/tests/shared/index_lock.go new file mode 100644 index 000000000..c8414813f --- /dev/null +++ b/pkg/integration/tests/shared/index_lock.go @@ -0,0 +1,44 @@ +package shared + +import ( + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +// holdIndexLockScript is the part of a git hook that takes index.lock the +// first time the hook runs, and holds it until the git command that ran the +// hook has exited. This simulates another git process (for example an IDE +// running "git status" in the background) that takes the lock while a command +// of several steps, such as a rebase, is partway through. +const holdIndexLockScript = `if [ ! -e .git/index-lock-taken ]; then + touch .git/index-lock-taken .git/index.lock + ( + # If lazygit runs the command in a pty (as it does for a pull), we + # get SIGHUP when the command exits + trap '' HUP + while kill -0 $PPID 2>/dev/null; do sleep 0.01; done + rm -f .git/index.lock + ) >/dev/null 2>&1 0 && len(todos) > 0 && doneTodos[len(doneTodos)-1] == todos[0] { + return true + } + + // Older versions of git have a bug whereby, if a command is rescheduled, + // the last successful command is appended to the "done" file again. To + // detect this, we need to compare the second-to-last done entry against the + // first todo entry, and also compare the last done entry against the + // last-but-two done entry; this latter check is needed for the following + // case: + // pick A + // exec make test + // pick B + // exec make test + // If pick B fails with conflicts, then the "done" file contains + // pick A + // exec make test + // pick B + // and git-rebase-todo contains + // exec make test + // Without the last condition we would erroneously treat this as the exec + // command being rescheduled. + return len(doneTodos) >= 3 && len(todos) > 0 && doneTodos[len(doneTodos)-2] == todos[0] && + doneTodos[len(doneTodos)-1] == doneTodos[len(doneTodos)-3] +} + func ReadRebaseTodoFile(fileName string, commentChar byte) ([]todo.Todo, error) { f, err := os.Open(fileName) if err != nil {