Fix submodule index lock contention (#5755)

Suppress optional locks by default again, except foreground refresh.

Commit d94f2f05 dropped the `GIT_OPTIONAL_LOCKS=0` env var that we used
to set on every git command, and re-added lock suppression only as a
`--no-optional-locks` flag on the background files refresh. The intent
was sound — a foreground `git status` should persist git's refreshed
stat-cache — but the change was too broad: it stopped suppressing
optional locks for every other command too.

The one that bites is the main-view diff. When a folder containing
submodules is selected, we render `git diff --submodule -- <dir>`, and
`--submodule` makes git run `git status` inside each submodule to
describe its "modified" state. That status now grabs the submodule's
index.lock. It runs as a PTY task on its own goroutine, so it races any
submodule-mutating action the user triggers — e.g. resetting a submodule
runs `git -C <submodule> stash`, which then fails with "index.lock: File
exists". This is what made submodule/reset_folder flaky. `git status` is
in fact the only command that takes the optional lock, but the env var
also covered its use inside `git diff --submodule`, inside PTY-run
commands, and inside git's own submodule child processes — none of which
a per-command flag reaches cleanly.

Invert the polarity to match how it worked before d94f2f05: the git
command builder disables optional locks on every command by default, and
the single command that benefits from taking the lock — the foreground
files refresh — opts back in. This restores the original contention
avoidance (including against the user's terminal git) while keeping
d94f2f05's stat-cache-persistence win for the foreground refresh.

This fixes a regression introduced by #5712; labelling this as
"ignore-for-release" because that PR hasn't been released yet.

pkg/commands/git_cmd_obj_builder.go

@@ -1,20 +1,48 @@
 package commands
 
 import (
+	"github.com/jesseduffield/lazygit/pkg/commands/git_commands"
 	"github.com/jesseduffield/lazygit/pkg/commands/oscommands"
 	"github.com/sirupsen/logrus"
 )
 
-// NewGitCmdObjBuilder returns a command object builder whose runner is wrapped
-// with our git-specific runner (logging, credential handling, etc.).
-func NewGitCmdObjBuilder(log *logrus.Entry, innerBuilder *oscommands.CmdObjBuilder) *oscommands.CmdObjBuilder {
-	// We decorate the runner rather than exposing the builder's runner field:
-	// that field stays unexported so there's a single API for running commands
-	// across the codebase.
-	return innerBuilder.CloneWithNewRunner(func(runner oscommands.ICmdObjRunner) oscommands.ICmdObjRunner {
+// all we're doing here is wrapping the default command object builder with
+// some git-specific stuff: e.g. adding a git-specific env var
+
+type gitCmdObjBuilder struct {
+	innerBuilder *oscommands.CmdObjBuilder
+}
+
+var _ oscommands.ICmdObjBuilder = &gitCmdObjBuilder{}
+
+// We disable git's optional locks on every command by default so that our git
+// invocations never contend for index.lock. See git_commands.OptionalLocksEnvVar
+// for the full rationale. Individual commands that do want the lock (currently
+// only the foreground files refresh) opt back in via CmdObj.RemoveEnvVar.
+var defaultEnvVar = git_commands.OptionalLocksEnvVar + "=0"
+
+func NewGitCmdObjBuilder(log *logrus.Entry, innerBuilder *oscommands.CmdObjBuilder) *gitCmdObjBuilder {
+	// the price of having a convenient interface where we can say .New(...).Run() is that our builder now depends on our runner, so when we want to wrap the default builder/runner in new functionality we need to jump through some hoops. We could avoid the use of a decorator function here by just exporting the runner field on the default builder but that would be misleading because we don't want anybody using that to run commands (i.e. we want there to be a single API used across the codebase)
+	updatedBuilder := innerBuilder.CloneWithNewRunner(func(runner oscommands.ICmdObjRunner) oscommands.ICmdObjRunner {
 		return &gitCmdObjRunner{
 			log:         log,
 			innerRunner: runner,
 		}
 	})
+
+	return &gitCmdObjBuilder{
+		innerBuilder: updatedBuilder,
+	}
+}
+
+func (self *gitCmdObjBuilder) New(args []string) *oscommands.CmdObj {
+	return self.innerBuilder.New(args).AddEnvVars(defaultEnvVar)
+}
+
+func (self *gitCmdObjBuilder) NewShell(cmdStr string, shellFunctionsFile string) *oscommands.CmdObj {
+	return self.innerBuilder.NewShell(cmdStr, shellFunctionsFile).AddEnvVars(defaultEnvVar)
+}
+
+func (self *gitCmdObjBuilder) Quote(str string) string {
+	return self.innerBuilder.Quote(str)
 }

pkg/commands/git_cmd_obj_builder_test.go

@@ -0,0 +1,24 @@
+package commands
+
+import (
+	"testing"
+
+	"github.com/jesseduffield/lazygit/pkg/commands/git_commands"
+	"github.com/jesseduffield/lazygit/pkg/commands/oscommands"
+	"github.com/jesseduffield/lazygit/pkg/utils"
+	"github.com/stretchr/testify/assert"
+)
+
+// Every git command we build disables optional locks by default, so that our
+// invocations never contend for index.lock (see git_commands.OptionalLocksEnvVar
+// for the rationale). Commands that want the lock opt back in with
+// CmdObj.RemoveEnvVar.
+func TestGitCmdObjBuilderDisablesOptionalLocksByDefault(t *testing.T) {
+	builder := NewGitCmdObjBuilder(
+		utils.NewDummyLog(),
+		oscommands.NewDummyCmdObjBuilder(oscommands.NewFakeRunner(t)),
+	)
+
+	assert.Contains(t, builder.New([]string{"git", "status"}).GetEnvVars(), git_commands.OptionalLocksEnvVar+"=0")
+	assert.Contains(t, builder.NewShell("git status", "").GetEnvVars(), git_commands.OptionalLocksEnvVar+"=0")
+}

pkg/commands/git_commands/file_loader.go

@@ -36,10 +36,11 @@ type GetStatusFileOptions struct {
 	// This is useful for users with bare repos for dotfiles who default to hiding untracked files,
 	// but want to occasionally see them to `git add` a new file.
 	ForceShowUntracked bool
-	// When true, this status is part of an unattended background refresh, so we
-	// pass --no-optional-locks to avoid index.lock contention with git commands
-	// the user runs in a terminal (at the cost of not persisting git's refreshed
-	// stat-cache).
+	// When true, this status is part of an unattended background refresh, so it
+	// keeps the default suppression of optional locks (avoiding index.lock
+	// contention with git commands the user runs in a terminal, at the cost of
+	// not persisting git's refreshed stat-cache). A foreground status opts back
+	// in; see gitStatus.
 	Background bool
 }
 
@@ -175,7 +176,6 @@ func (self *FileLoader) gitDiffNumStat() (string, error) {
 
 func (self *FileLoader) gitStatus(opts GitStatusOptions) ([]FileStatus, error) {
 	cmdArgs := NewGitCmd("status").
-		GlobalArgIf(opts.Background, "--no-optional-locks").
 		Arg(opts.UntrackedFilesArg).
 		Arg("--porcelain").
 		Arg("-z").
@@ -186,7 +186,17 @@ func (self *FileLoader) gitStatus(opts GitStatusOptions) ([]FileStatus, error) {
 		).
 		ToArgv()
 
-	statusLines, _, err := self.cmd.New(cmdArgs).DontLog().RunWithOutputs()
+	cmdObj := self.cmd.New(cmdArgs).DontLog()
+	if !opts.Background {
+		// Every git command suppresses optional locks by default (see
+		// OptionalLocksEnvVar). A foreground refresh is the one exception: we let
+		// it take the lock so it persists git's refreshed stat-cache, which keeps
+		// subsequent status calls fast. Background refreshes leave it suppressed so
+		// they can't contend for index.lock.
+		cmdObj.RemoveEnvVar(OptionalLocksEnvVar)
+	}
+
+	statusLines, _, err := cmdObj.RunWithOutputs()
 	if err != nil {
 		return []FileStatus{}, err
 	}

pkg/commands/git_commands/file_loader_test.go

@@ -13,7 +13,6 @@ func TestFileGetStatusFiles(t *testing.T) {
 	type scenario struct {
 		testName               string
 		similarityThreshold    int
-		background             bool
 		runner                 oscommands.ICmdObjRunner
 		showNumstatInFilesView bool
 		expectedFiles          []*models.File
@@ -27,14 +26,6 @@ func TestFileGetStatusFiles(t *testing.T) {
 				ExpectGitArgs([]string{"status", "--untracked-files=yes", "--porcelain", "-z", "--find-renames=50%"}, "", nil),
 			expectedFiles: []*models.File{},
 		},
-		{
-			testName:            "Background refresh passes --no-optional-locks",
-			similarityThreshold: 50,
-			background:          true,
-			runner: oscommands.NewFakeRunner(t).
-				ExpectGitArgs([]string{"--no-optional-locks", "status", "--untracked-files=yes", "--porcelain", "-z", "--find-renames=50%"}, "", nil),
-			expectedFiles: []*models.File{},
-		},
 		{
 			testName:            "Several files found",
 			similarityThreshold: 50,
@@ -255,7 +246,7 @@ func TestFileGetStatusFiles(t *testing.T) {
 				getFileType: func(string) string { return "file" },
 			}
 
-			assert.EqualValues(t, s.expectedFiles, loader.GetStatusFiles(GetStatusFileOptions{Background: s.background}))
+			assert.EqualValues(t, s.expectedFiles, loader.GetStatusFiles(GetStatusFileOptions{}))
 		})
 	}
 }

pkg/commands/git_commands/git_command_builder.go

@@ -6,6 +6,16 @@ import (
 	"github.com/jesseduffield/lazygit/pkg/commands/oscommands"
 )
 
+// OptionalLocksEnvVar is the name of the environment variable that tells git
+// whether it may take "optional" locks — chiefly the index.lock that `git
+// status` grabs to write back a refreshed stat-cache. We set it to 0 on every
+// git command by default (see NewGitCmdObjBuilder) so our invocations never
+// contend for index.lock, neither with each other (e.g. a main-view `git diff
+// --submodule`, which runs `git status` inside submodules, racing a submodule
+// action) nor with git commands the user runs in a terminal. The one command
+// that opts back in is the foreground files refresh; see FileLoader.gitStatus.
+const OptionalLocksEnvVar = "GIT_OPTIONAL_LOCKS"
+
 // convenience struct for building git commands. Especially useful when
 // including conditional args
 type GitCommandBuilder struct {
@@ -38,22 +48,6 @@ func (self *GitCommandBuilder) ArgIfElse(condition bool, ifTrue string, ifFalse
 	return self.Arg(ifFalse)
 }
 
-// GlobalArg adds top-level options for git itself (e.g. --no-optional-locks).
-// Unlike Arg, these are prepended before the command, where git expects them.
-func (self *GitCommandBuilder) GlobalArg(args ...string) *GitCommandBuilder {
-	self.args = append(append([]string{}, args...), self.args...)
-
-	return self
-}
-
-func (self *GitCommandBuilder) GlobalArgIf(condition bool, args ...string) *GitCommandBuilder {
-	if condition {
-		self.GlobalArg(args...)
-	}
-
-	return self
-}
-
 func (self *GitCommandBuilder) Config(value string) *GitCommandBuilder {
 	// config settings come before the command
 	self.args = append([]string{"-c", value}, self.args...)

pkg/commands/oscommands/cmd_obj.go

@@ -91,6 +91,19 @@ func (self *CmdObj) AddEnvVars(vars ...string) *CmdObj {
 	return self
 }
 
+// RemoveEnvVar removes every occurrence of the named environment variable from
+// the command's environment. It's the counterpart to AddEnvVars, used to opt a
+// single command out of a variable that the builder sets on every command by
+// default.
+func (self *CmdObj) RemoveEnvVar(name string) *CmdObj {
+	prefix := name + "="
+	self.cmd.Env = lo.Filter(self.cmd.Env, func(envVar string, _ int) bool {
+		return !strings.HasPrefix(envVar, prefix)
+	})
+
+	return self
+}
+
 func (self *CmdObj) GetEnvVars() []string {
 	return self.cmd.Env
 }

pkg/commands/oscommands/cmd_obj_test.go

@@ -5,8 +5,29 @@ import (
 	"testing"
 
 	"github.com/jesseduffield/lazygit/pkg/gocui"
+	"github.com/stretchr/testify/assert"
 )
 
+func TestRemoveEnvVar(t *testing.T) {
+	cmd := exec.Command("git", "status")
+	cmd.Env = []string{
+		"PATH=/usr/bin",
+		"GIT_OPTIONAL_LOCKS=0",
+		"GIT_OPTIONAL_LOCKS_OTHER=1", // name is a prefix of ours but not the same var
+		"GIT_OPTIONAL_LOCKS=0",       // duplicates must all be removed
+		"HOME=/home/me",
+	}
+	cmdObj := &CmdObj{cmd: cmd}
+
+	cmdObj.RemoveEnvVar("GIT_OPTIONAL_LOCKS")
+
+	assert.Equal(t, []string{
+		"PATH=/usr/bin",
+		"GIT_OPTIONAL_LOCKS_OTHER=1",
+		"HOME=/home/me",
+	}, cmdObj.GetEnvVars())
+}
+
 func TestCmdObjToString(t *testing.T) {
 	quote := func(s string) string {
 		return "\"" + s + "\""

pkg/gui/types/refresh.go

@@ -72,10 +72,11 @@ type RefreshOptions struct {
 	CommitSelection CommitSelectionBehavior
 
 	// When true, this refresh was initiated by a background routine rather than
-	// by a user action. We use it to keep background `git status` calls from
-	// taking optional git locks, so they don't contend for index.lock with git
-	// commands the user runs in a terminal. The cost is that such a status won't
-	// persist git's refreshed stat-cache, which is the right trade-off for
-	// unattended work; foreground refreshes leave this false so they do persist.
+	// by a user action. Every git command suppresses optional locks by default
+	// so it can't contend for index.lock (see git_commands.OptionalLocksEnvVar);
+	// a foreground files refresh (this false) is the one command that opts back
+	// in, so it persists git's refreshed stat-cache and keeps later status calls
+	// fast. Background refreshes leave the suppression in place: not persisting
+	// the stat-cache is the right trade-off for unattended work.
 	Background bool
 }