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.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

pkg/commands/git_cmd_obj_builder.go

@@ -1,6 +1,7 @@
 package commands
 
 import (
+	"github.com/jesseduffield/lazygit/pkg/commands/git_commands"
 	"github.com/jesseduffield/lazygit/pkg/commands/oscommands"
 	"github.com/sirupsen/logrus"
 )
@@ -14,6 +15,12 @@ type gitCmdObjBuilder struct {
 
 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 {
@@ -29,11 +36,11 @@ func NewGitCmdObjBuilder(log *logrus.Entry, innerBuilder *oscommands.CmdObjBuild
 }
 
 func (self *gitCmdObjBuilder) New(args []string) *oscommands.CmdObj {
-	return self.innerBuilder.New(args)
+	return self.innerBuilder.New(args).AddEnvVars(defaultEnvVar)
 }
 
 func (self *gitCmdObjBuilder) NewShell(cmdStr string, shellFunctionsFile string) *oscommands.CmdObj {
-	return self.innerBuilder.NewShell(cmdStr, shellFunctionsFile)
+	return self.innerBuilder.NewShell(cmdStr, shellFunctionsFile).AddEnvVars(defaultEnvVar)
 }
 
 func (self *gitCmdObjBuilder) Quote(str string) string {

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 {

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
 }