Prevent staging from becoming slower over time (#5712)

In a large repo, when touching (editing) more and more files, staging
hunks in lazygit could become slower and slower over time. Specifically,
this happened when you edited a lot of files and then discarded their
changes again. I have seen cases where staging a hunk began to take
seconds; the fix then was to type `git status` on the command line once,
this made it fast again.

The reason was that lazygit was trying too hard to be a good git
citizen, and used the `GIT_OPTIONAL_LOCKS=0` env var on every git
command it made. The consequence was that it never updated the mod date
cache in git's index file, which caused git to rehash every file whose
mod date doesn't match what it recorded in the index, on every refresh.
Typing `git status` updates that cache, which is why this was a
workaround.

Fix this by using the `GIT_OPTIONAL_LOCKS=0` flag only for refreshes
that are running unattended in the background, i.e. the periodic
autoRefresh and the newly external change detection. For those it is
important because it avoids "cannot lock index" errors for commands that
the user might issue at the same time. All other refreshes are user
initiated and no longer use the flag, which is in line with what `git
status` does, so this keeps performance from deteriorating over time.

pkg/commands/git_cmd_obj_builder.go

@@ -5,39 +5,16 @@ import (
 	"github.com/sirupsen/logrus"
 )
 
-// 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{}
-
-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 {
+// 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 {
 		return &gitCmdObjRunner{
 			log:         log,
 			innerRunner: runner,
 		}
 	})
-
-	return &gitCmdObjBuilder{
-		innerBuilder: updatedBuilder,
-	}
-}
-
-var defaultEnvVar = "GIT_OPTIONAL_LOCKS=0"
-
-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_commands/file_loader.go

@@ -36,6 +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).
+	Background bool
 }
 
 func (self *FileLoader) GetStatusFiles(opts GetStatusFileOptions) []*models.File {
@@ -47,7 +52,7 @@ func (self *FileLoader) GetStatusFiles(opts GetStatusFileOptions) []*models.File
 	}
 	untrackedFilesArg := fmt.Sprintf("--untracked-files=%s", untrackedFilesSetting)
 
-	statuses, err := self.gitStatus(GitStatusOptions{NoRenames: opts.NoRenames, UntrackedFilesArg: untrackedFilesArg})
+	statuses, err := self.gitStatus(GitStatusOptions{NoRenames: opts.NoRenames, UntrackedFilesArg: untrackedFilesArg, Background: opts.Background})
 	if err != nil {
 		self.Log.Error(err)
 	}
@@ -148,6 +153,7 @@ func (self *FileLoader) getFileDiffs() (map[string]FileDiff, error) {
 type GitStatusOptions struct {
 	NoRenames         bool
 	UntrackedFilesArg string
+	Background        bool
 }
 
 type FileStatus struct {
@@ -169,6 +175,7 @@ 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").

pkg/commands/git_commands/file_loader_test.go

@@ -13,6 +13,7 @@ func TestFileGetStatusFiles(t *testing.T) {
 	type scenario struct {
 		testName               string
 		similarityThreshold    int
+		background             bool
 		runner                 oscommands.ICmdObjRunner
 		showNumstatInFilesView bool
 		expectedFiles          []*models.File
@@ -26,6 +27,14 @@ 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,
@@ -246,7 +255,7 @@ func TestFileGetStatusFiles(t *testing.T) {
 				getFileType: func(string) string { return "file" },
 			}
 
-			assert.EqualValues(t, s.expectedFiles, loader.GetStatusFiles(GetStatusFileOptions{}))
+			assert.EqualValues(t, s.expectedFiles, loader.GetStatusFiles(GetStatusFileOptions{Background: s.background}))
 		})
 	}
 }

pkg/commands/git_commands/git_command_builder.go

@@ -38,6 +38,22 @@ 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/gui/background.go

@@ -133,7 +133,7 @@ func (self *BackgroundRoutineMgr) startBackgroundFilesRefresh() {
 
 	userConfig := self.gui.UserConfig()
 	self.goEvery(userConfig.Refresher.RefreshIntervalDuration(), self.gui.stopChan, func(_ bool) error {
-		self.gui.c.Refresh(types.RefreshOptions{Scope: []types.RefreshableView{types.FILES}})
+		self.gui.c.Refresh(types.RefreshOptions{Scope: []types.RefreshableView{types.FILES}, Background: true})
 		return nil
 	})
 }
@@ -184,7 +184,7 @@ func (self *BackgroundRoutineMgr) checkForExternalChanges() {
 
 	// No need to update the stored snapshot here; Refresh does that.
 	self.gui.c.Log.Info("External ref change detected — refreshing")
-	self.gui.c.Refresh(types.RefreshOptions{})
+	self.gui.c.Refresh(types.RefreshOptions{Background: true})
 }
 
 // returns a channel that can be used to trigger the callback immediately
@@ -226,7 +226,7 @@ func (self *BackgroundRoutineMgr) goEvery(interval time.Duration, stop chan stru
 func (self *BackgroundRoutineMgr) backgroundFetch() (err error) {
 	err = self.gui.git.Sync.FetchBackground()
 
-	return self.gui.helpers.BranchesHelper.PostFetchRefresh(err)
+	return self.gui.helpers.BranchesHelper.PostFetchRefresh(err, true)
 }
 
 func (self *BackgroundRoutineMgr) triggerImmediateFetch() {

pkg/gui/controllers/files_controller.go

@@ -1372,7 +1372,7 @@ func (self *FilesController) fetch() error {
 			return errors.New(self.c.Tr.PassUnameWrong)
 		}
 
-		return self.c.Helpers().BranchesHelper.PostFetchRefresh(err)
+		return self.c.Helpers().BranchesHelper.PostFetchRefresh(err, false)
 	})
 }
 

pkg/gui/controllers/helpers/branches_helper.go

@@ -285,7 +285,7 @@ func (self *BranchesHelper) deleteRemoteBranches(remoteBranches []*models.Remote
 	return nil
 }
 
-func (self *BranchesHelper) PostFetchRefresh(fetchErr error) error {
+func (self *BranchesHelper) PostFetchRefresh(fetchErr error, background bool) error {
 	scope := []types.RefreshableView{
 		types.BRANCHES, types.COMMITS, types.REMOTES, types.TAGS, types.PULL_REQUESTS,
 	}
@@ -293,7 +293,7 @@ func (self *BranchesHelper) PostFetchRefresh(fetchErr error) error {
 	if self.c.UserConfig().Git.AutoForwardBranches != "none" {
 		scope = append(scope, types.WORKTREES)
 	}
-	self.c.Refresh(types.RefreshOptions{Scope: scope, Mode: types.SYNC})
+	self.c.Refresh(types.RefreshOptions{Scope: scope, Mode: types.SYNC, Background: background})
 	if fetchErr != nil {
 		return fetchErr
 	}

pkg/gui/controllers/helpers/refresh_helper.go

@@ -200,7 +200,7 @@ func (self *RefreshHelper) Refresh(options types.RefreshOptions) {
 		if scopeSet.Includes(types.FILES) {
 			fileWg.Add(1)
 			refresh("files", func() {
-				_ = self.refreshFilesAndSubmodules()
+				_ = self.refreshFilesAndSubmodules(options.Background)
 				fileWg.Done()
 			})
 		}
@@ -624,7 +624,7 @@ func (self *RefreshHelper) refreshBranches(refreshWorktrees bool, keepBranchSele
 	self.refreshStatus()
 }
 
-func (self *RefreshHelper) refreshFilesAndSubmodules() error {
+func (self *RefreshHelper) refreshFilesAndSubmodules(background bool) error {
 	self.c.Mutexes().RefreshingFilesMutex.Lock()
 	self.c.State().SetIsRefreshingFiles(true)
 	defer func() {
@@ -636,7 +636,7 @@ func (self *RefreshHelper) refreshFilesAndSubmodules() error {
 		return err
 	}
 
-	if err := self.refreshStateFiles(); err != nil {
+	if err := self.refreshStateFiles(background); err != nil {
 		return err
 	}
 
@@ -649,7 +649,7 @@ func (self *RefreshHelper) refreshFilesAndSubmodules() error {
 	return nil
 }
 
-func (self *RefreshHelper) refreshStateFiles() error {
+func (self *RefreshHelper) refreshStateFiles(background bool) error {
 	fileTreeViewModel := self.c.Contexts().Files.FileTreeViewModel
 
 	prevConflictFileCount := 0
@@ -687,6 +687,7 @@ func (self *RefreshHelper) refreshStateFiles() error {
 	files := self.c.Git().Loaders.FileLoader.
 		GetStatusFiles(git_commands.GetStatusFileOptions{
 			ForceShowUntracked: self.c.Contexts().Files.ForceShowUntracked(),
+			Background:         background,
 		})
 
 	conflictFileCount := 0

pkg/gui/types/refresh.go

@@ -44,4 +44,12 @@ type RefreshOptions struct {
 	// keeps the selection index the same. Useful after checking out a detached
 	// head, and selecting index 0.
 	KeepBranchSelectionIndex bool
+
+	// 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.
+	Background bool
 }