From e90daaf81287b4ab5d53166d69651d9aba75ec9e Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Fri, 10 Jul 2026 08:56:06 +0200 Subject: [PATCH] Unify the git command lock-retry loops RunWithOutput and RunWithOutputs each carried their own near-identical copy of the index.lock retry loop. Extract the loop into a single retryOnLockError helper so the retry policy lives in one place, ahead of changing that policy. Behavior is unchanged; the added tests characterize it (success and non-lock errors run once, a lock error in the output is retried). Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/commands/git_cmd_obj_runner.go | 42 +++++------ pkg/commands/git_cmd_obj_runner_test.go | 92 +++++++++++++++++++++++++ 2 files changed, 113 insertions(+), 21 deletions(-) create mode 100644 pkg/commands/git_cmd_obj_runner_test.go diff --git a/pkg/commands/git_cmd_obj_runner.go b/pkg/commands/git_cmd_obj_runner.go index 668feef93..a72565b5c 100644 --- a/pkg/commands/git_cmd_obj_runner.go +++ b/pkg/commands/git_cmd_obj_runner.go @@ -33,11 +33,30 @@ 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.innerRunner.RunWithOutput(cmdObj.Clone()) + }) +} + +func (self *gitCmdObjRunner) RunWithOutputs(cmdObj *oscommands.CmdObj) (string, string, error) { + var stdout, stderr string + _, err := self.retryOnLockError(func() (string, error) { + var runErr error + stdout, stderr, runErr = self.innerRunner.RunWithOutputs(cmdObj.Clone()) + return stdout + stderr, runErr + }) + 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 +// 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) { var output string var err error for range RetryCount { - newCmdObj := cmdObj.Clone() - output, err = self.innerRunner.RunWithOutput(newCmdObj) + output, err = run() if err == nil || !isRetryableError(output) { return output, err @@ -51,25 +70,6 @@ func (self *gitCmdObjRunner) RunWithOutput(cmdObj *oscommands.CmdObj) (string, e return output, err } -func (self *gitCmdObjRunner) RunWithOutputs(cmdObj *oscommands.CmdObj) (string, string, error) { - var stdout, stderr string - var err error - for range RetryCount { - newCmdObj := cmdObj.Clone() - stdout, stderr, err = self.innerRunner.RunWithOutputs(newCmdObj) - - if err == nil || !isRetryableError(stdout+stderr) { - return stdout, stderr, err - } - - // if we have an error based on a lock, we should wait a bit and then retry - self.log.Warn("lock error prevented command from running. Retrying command after a small wait") - time.Sleep(WaitTime) - } - - return stdout, stderr, err -} - // Retry logic not implemented here, but these commands typically don't need to obtain a lock. func (self *gitCmdObjRunner) RunAndProcessLines(cmdObj *oscommands.CmdObj, onLine func(line string) (bool, error)) error { return self.innerRunner.RunAndProcessLines(cmdObj, onLine) diff --git a/pkg/commands/git_cmd_obj_runner_test.go b/pkg/commands/git_cmd_obj_runner_test.go new file mode 100644 index 000000000..e8c73727d --- /dev/null +++ b/pkg/commands/git_cmd_obj_runner_test.go @@ -0,0 +1,92 @@ +package commands + +import ( + "errors" + "testing" + + "github.com/jesseduffield/lazygit/pkg/commands/oscommands" + "github.com/jesseduffield/lazygit/pkg/utils" + "github.com/stretchr/testify/assert" +) + +type runnerResult struct { + output string + err error +} + +// 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. +type scriptedRunner struct { + results []runnerResult + calls int +} + +func (self *scriptedRunner) next() (string, error) { + result := self.results[self.calls] + self.calls++ + return result.output, result.err +} + +func (self *scriptedRunner) Run(*oscommands.CmdObj) error { + _, err := self.next() + return err +} + +func (self *scriptedRunner) RunWithOutput(*oscommands.CmdObj) (string, error) { + return self.next() +} + +func (self *scriptedRunner) RunWithOutputs(*oscommands.CmdObj) (string, string, error) { + output, err := self.next() + return output, "", err +} + +func (self *scriptedRunner) RunAndProcessLines(*oscommands.CmdObj, func(string) (bool, error)) error { + panic("not implemented") +} + +func newTestRunner(inner *scriptedRunner) *gitCmdObjRunner { + return &gitCmdObjRunner{ + log: utils.NewDummyLog(), + innerRunner: inner, + } +} + +// 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"}) +} + +func TestRunWithOutputReturnsSuccessWithoutRetrying(t *testing.T) { + inner := &scriptedRunner{results: []runnerResult{{output: "done", err: nil}}} + + output, err := newTestRunner(inner).RunWithOutput(dummyCmdObj()) + + assert.NoError(t, err) + assert.Equal(t, "done", output) + assert.Equal(t, 1, inner.calls) +} + +func TestRunWithOutputDoesNotRetryNonLockError(t *testing.T) { + inner := &scriptedRunner{results: []runnerResult{{output: "boom", err: errors.New("boom")}}} + + _, err := newTestRunner(inner).RunWithOutput(dummyCmdObj()) + + assert.Error(t, err) + assert.Equal(t, 1, inner.calls) +} + +func TestRunWithOutputRetriesWhenLockErrorIsInOutput(t *testing.T) { + inner := &scriptedRunner{results: []runnerResult{ + {output: "fatal: Unable to create '/repo/.git/index.lock': File exists.", err: errors.New("exit status 128")}, + {output: "done", err: nil}, + }} + + output, err := newTestRunner(inner).RunWithOutput(dummyCmdObj()) + + assert.NoError(t, err) + assert.Equal(t, "done", output) + assert.Equal(t, 2, inner.calls) +}