diff --git a/pkg/commands/git.go b/pkg/commands/git.go index d9add2790..ba6e5a033 100644 --- a/pkg/commands/git.go +++ b/pkg/commands/git.go @@ -72,6 +72,10 @@ func NewGitCommand( return nil, utils.WrapError(err) } + // Pin the config reads to the repo directory like all other git commands + // (see NewGitCmdObjBuilder); the config commands run outside that builder. + gitConfig.SetDir(repoPaths.WorktreePath()) + return NewGitCommandAux( cmn, version, @@ -90,7 +94,7 @@ func NewGitCommandAux( repoPaths *git_commands.RepoPaths, pagerConfig *config.PagerConfig, ) *GitCommand { - cmd := NewGitCmdObjBuilder(cmn.Log, osCommand.Cmd) + cmd := NewGitCmdObjBuilder(cmn.Log, osCommand.Cmd, repoPaths.WorktreePath()) // here we're doing a bunch of dependency injection for each of our commands structs. // This is admittedly messy, but allows us to test each command struct in isolation, diff --git a/pkg/commands/git_cmd_obj_builder.go b/pkg/commands/git_cmd_obj_builder.go index 20d31c11c..9c3cd50d4 100644 --- a/pkg/commands/git_cmd_obj_builder.go +++ b/pkg/commands/git_cmd_obj_builder.go @@ -11,6 +11,15 @@ import ( type gitCmdObjBuilder struct { innerBuilder *oscommands.CmdObjBuilder + + // The directory of the repo (or worktree) this builder was created for; + // every command we produce runs there, regardless of the process's current + // working directory. The two are the same until the user switches to + // another repo: lazygit chdirs on a switch, but work still in flight for + // the previous repo (e.g. a background refresh spawning commands through + // the old builder) must keep running its commands against the repo it + // started in, not whichever one the process has since moved to. + repoDir string } var _ oscommands.ICmdObjBuilder = &gitCmdObjBuilder{} @@ -21,7 +30,7 @@ var _ oscommands.ICmdObjBuilder = &gitCmdObjBuilder{} // 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 { +func NewGitCmdObjBuilder(log *logrus.Entry, innerBuilder *oscommands.CmdObjBuilder, repoDir string) *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{ @@ -33,15 +42,16 @@ func NewGitCmdObjBuilder(log *logrus.Entry, innerBuilder *oscommands.CmdObjBuild return &gitCmdObjBuilder{ innerBuilder: updatedBuilder, + repoDir: repoDir, } } func (self *gitCmdObjBuilder) New(args []string) *oscommands.CmdObj { - return self.innerBuilder.New(args).AddEnvVars(defaultEnvVar) + return self.innerBuilder.New(args).AddEnvVars(defaultEnvVar).SetWd(self.repoDir) } func (self *gitCmdObjBuilder) NewShell(cmdStr string, shellFunctionsFile string) *oscommands.CmdObj { - return self.innerBuilder.NewShell(cmdStr, shellFunctionsFile).AddEnvVars(defaultEnvVar) + return self.innerBuilder.NewShell(cmdStr, shellFunctionsFile).AddEnvVars(defaultEnvVar).SetWd(self.repoDir) } func (self *gitCmdObjBuilder) Quote(str string) string { diff --git a/pkg/commands/git_cmd_obj_builder_test.go b/pkg/commands/git_cmd_obj_builder_test.go index ca7e54cfe..28e21501c 100644 --- a/pkg/commands/git_cmd_obj_builder_test.go +++ b/pkg/commands/git_cmd_obj_builder_test.go @@ -17,8 +17,25 @@ func TestGitCmdObjBuilderDisablesOptionalLocksByDefault(t *testing.T) { builder := NewGitCmdObjBuilder( utils.NewDummyLog(), oscommands.NewDummyCmdObjBuilder(oscommands.NewFakeRunner(t)), + "/path/to/repo", ) 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") } + +// Every command the builder produces runs in the directory of the repo the +// builder was created for, not in the process's current directory: lazygit +// chdirs when switching repos, and commands built for the previous repo after +// that (e.g. by a background refresh still in flight) must keep addressing the +// repo they were built for. +func TestGitCmdObjBuilderPinsCommandsToRepoDir(t *testing.T) { + builder := NewGitCmdObjBuilder( + utils.NewDummyLog(), + oscommands.NewDummyCmdObjBuilder(oscommands.NewFakeRunner(t)), + "/path/to/repo", + ) + + assert.Equal(t, "/path/to/repo", builder.New([]string{"git", "status"}).GetCmd().Dir) + assert.Equal(t, "/path/to/repo", builder.NewShell("git status", "").GetCmd().Dir) +} diff --git a/pkg/commands/git_commands/submodule.go b/pkg/commands/git_commands/submodule.go index 3b88e4fbb..d8c1208bc 100644 --- a/pkg/commands/git_commands/submodule.go +++ b/pkg/commands/git_commands/submodule.go @@ -28,10 +28,15 @@ func NewSubmoduleCommands(gitCommon *GitCommon) *SubmoduleCommands { } func (self *SubmoduleCommands) GetConfigs(parentModule *models.SubmoduleConfig) ([]*models.SubmoduleConfig, error) { - gitModulesPath := ".gitmodules" + // Resolve the path against the repo this commands object was created for + // rather than the process working directory, so that a read from a + // still-running refresh keeps addressing that repo after the user + // switched to another one. + dir := self.repoPaths.WorktreePath() if parentModule != nil { - gitModulesPath = filepath.Join(parentModule.FullPath(), gitModulesPath) + dir = filepath.Join(dir, parentModule.FullPath()) } + gitModulesPath := filepath.Join(dir, ".gitmodules") file, err := os.Open(gitModulesPath) if err != nil { if os.IsNotExist(err) { @@ -180,7 +185,7 @@ func (self *SubmoduleCommands) ConflictSideLog(path string, side string, otherSi func (self *SubmoduleCommands) Stash(submodule *models.SubmoduleConfig) error { // if the path does not exist then it hasn't yet been initialized so we'll swallow the error // because the intention here is to have no dirty worktree state - if _, err := os.Stat(submodule.Path); os.IsNotExist(err) { + if _, err := os.Stat(filepath.Join(self.repoPaths.WorktreePath(), submodule.FullPath())); os.IsNotExist(err) { self.Log.Infof("submodule path %s does not exist, returning", submodule.FullPath()) return nil } @@ -213,51 +218,51 @@ func (self *SubmoduleCommands) UpdateAll() error { return self.cmd.New(cmdArgs).Run() } +// runInParentModule runs the given command in the submodule's parent module's +// directory when the submodule is nested: its path arguments (and the +// .gitmodules file the config commands touch) are relative to the parent +// module. The directory is set on the command itself rather than by +// temporarily chdir-ing the process there, which would leak the parent +// module's directory into whatever other commands run concurrently (e.g. a +// background refresh's). +func (self *SubmoduleCommands) runInParentModule(submodule *models.SubmoduleConfig, cmdObj *oscommands.CmdObj) error { + if submodule.ParentModule != nil { + cmdObj.SetWd(submodule.ParentModule.FullPath()) + } + return cmdObj.Run() +} + func (self *SubmoduleCommands) Delete(submodule *models.SubmoduleConfig) error { // based on https://gist.github.com/myusuf3/7f645819ded92bda6677 - if submodule.ParentModule != nil { - wd, err := os.Getwd() - if err != nil { - return err - } - - err = os.Chdir(submodule.ParentModule.FullPath()) - if err != nil { - return err - } - - defer func() { _ = os.Chdir(wd) }() - } - - if err := self.cmd.New( + if err := self.runInParentModule(submodule, self.cmd.New( NewGitCmd("submodule"). Arg("deinit", "--force", "--", submodule.Path).ToArgv(), - ).Run(); err != nil { + )); err != nil { if !strings.Contains(err.Error(), "did not match any file(s) known to git") { return err } - if err := self.cmd.New( + if err := self.runInParentModule(submodule, self.cmd.New( NewGitCmd("config"). Arg("--file", ".gitmodules", "--remove-section", "submodule."+submodule.Path). ToArgv(), - ).Run(); err != nil { + )); err != nil { return err } - if err := self.cmd.New( + if err := self.runInParentModule(submodule, self.cmd.New( NewGitCmd("config"). Arg("--remove-section", "submodule."+submodule.Path). ToArgv(), - ).Run(); err != nil { + )); err != nil { return err } } - if err := self.cmd.New( + if err := self.runInParentModule(submodule, self.cmd.New( NewGitCmd("rm").Arg("--force", "-r", submodule.Path).ToArgv(), - ).Run(); err != nil { + )); err != nil { // if the directory isn't there then that's fine self.Log.Error(err) } @@ -282,20 +287,6 @@ func (self *SubmoduleCommands) Add(name string, path string, url string) error { } func (self *SubmoduleCommands) UpdateUrl(submodule *models.SubmoduleConfig, newUrl string) error { - if submodule.ParentModule != nil { - wd, err := os.Getwd() - if err != nil { - return err - } - - err = os.Chdir(submodule.ParentModule.FullPath()) - if err != nil { - return err - } - - defer func() { _ = os.Chdir(wd) }() - } - setUrlCmdStr := NewGitCmd("config"). Arg( "--file", ".gitmodules", "submodule."+submodule.Name+".url", newUrl, @@ -303,14 +294,14 @@ func (self *SubmoduleCommands) UpdateUrl(submodule *models.SubmoduleConfig, newU ToArgv() // the set-url command is only for later git versions so we're doing it manually here - if err := self.cmd.New(setUrlCmdStr).Run(); err != nil { + if err := self.runInParentModule(submodule, self.cmd.New(setUrlCmdStr)); err != nil { return err } syncCmdStr := NewGitCmd("submodule").Arg("sync", "--", submodule.Path). ToArgv() - if err := self.cmd.New(syncCmdStr).Run(); err != nil { + if err := self.runInParentModule(submodule, self.cmd.New(syncCmdStr)); err != nil { return err } diff --git a/pkg/commands/git_config/cached_git_config.go b/pkg/commands/git_config/cached_git_config.go index 256cd325b..17152ef9e 100644 --- a/pkg/commands/git_config/cached_git_config.go +++ b/pkg/commands/git_config/cached_git_config.go @@ -16,11 +16,19 @@ type IGitConfig interface { // this is for when you want to pass 'mykey' and check if the result is truthy GetBool(string) bool + // SetDir pins the config commands to the given repo directory, so that + // they keep reading that repo's local config even if the process working + // directory changes later (i.e. the user switches repos while this + // instance is still in use by in-flight work). Called once, before the + // first read. + SetDir(string) + DropCache() } type CachedGitConfig struct { cache map[string]string + dir string runGitConfigCmd func(*exec.Cmd) (string, error) log *logrus.Entry mutex sync.Mutex @@ -39,6 +47,13 @@ func NewCachedGitConfig(runGitConfigCmd func(*exec.Cmd) (string, error), log *lo } } +func (self *CachedGitConfig) SetDir(dir string) { + self.mutex.Lock() + defer self.mutex.Unlock() + + self.dir = dir +} + func (self *CachedGitConfig) Get(key string) string { self.mutex.Lock() defer self.mutex.Unlock() @@ -69,6 +84,7 @@ func (self *CachedGitConfig) GetGeneral(args string) string { func (self *CachedGitConfig) getGeneralAux(args string) string { cmd := getGitConfigGeneralCmd(args) + cmd.Dir = self.dir value, err := self.runGitConfigCmd(cmd) if err != nil { self.log.Debugf("Error getting git config value for args: %s. Error: %v", args, err.Error()) @@ -79,6 +95,7 @@ func (self *CachedGitConfig) getGeneralAux(args string) string { func (self *CachedGitConfig) getAux(key string) string { cmd := getGitConfigCmd(key) + cmd.Dir = self.dir value, err := self.runGitConfigCmd(cmd) if err != nil { self.log.Debugf("Error getting git config value for key: %s. Error: %v", key, err.Error()) diff --git a/pkg/commands/git_config/cached_git_config_test.go b/pkg/commands/git_config/cached_git_config_test.go index fd884df65..7b92eed1e 100644 --- a/pkg/commands/git_config/cached_git_config_test.go +++ b/pkg/commands/git_config/cached_git_config_test.go @@ -116,3 +116,20 @@ func TestGet(t *testing.T) { assert.Equal(t, "blah", result) assert.Equal(t, 1, count) } + +// The config commands run in the directory set by SetDir rather than in the +// process's current directory: lazygit chdirs when switching repos, and config +// reads issued for the previous repo after that must keep addressing the repo +// they were created for. +func TestSetDirPinsCommandsToDirectory(t *testing.T) { + real := NewCachedGitConfig( + func(cmd *exec.Cmd) (string, error) { + assert.Equal(t, "/path/to/repo", cmd.Dir) + return "blah", nil + }, + utils.NewDummyLog(), + ) + real.SetDir("/path/to/repo") + real.Get("commit.gpgsign") + real.GetGeneral("--local --get-regexp foo") +} diff --git a/pkg/commands/git_config/fake_git_config.go b/pkg/commands/git_config/fake_git_config.go index e82efcd1b..442c18644 100644 --- a/pkg/commands/git_config/fake_git_config.go +++ b/pkg/commands/git_config/fake_git_config.go @@ -28,5 +28,8 @@ func (self *FakeGitConfig) GetBool(key string) bool { return isTruthy(self.Get(key)) } +func (self *FakeGitConfig) SetDir(dir string) { +} + func (self *FakeGitConfig) DropCache() { } diff --git a/pkg/gui/background.go b/pkg/gui/background.go index 1e2db853f..180b2d445 100644 --- a/pkg/gui/background.go +++ b/pkg/gui/background.go @@ -6,7 +6,9 @@ import ( "sync/atomic" "time" + "github.com/jesseduffield/lazygit/pkg/commands" "github.com/jesseduffield/lazygit/pkg/gocui" + "github.com/jesseduffield/lazygit/pkg/gui/controllers/helpers" "github.com/jesseduffield/lazygit/pkg/gui/types" "github.com/jesseduffield/lazygit/pkg/utils" ) @@ -106,23 +108,35 @@ func (self *BackgroundRoutineMgr) startBackgroundFetch() { self.gui.waitForIntro.Wait() fetch := func(firstTimeOrRetriggered bool) error { - // Do this on the UI thread so that we don't have to deal with synchronization around the - // access of the repo state. - self.gui.onUIThread(func() error { - // There's a race here, where we might be recording the time stamp for a different repo - // than where the fetch actually ran. It's not very likely though, and not harmful if it - // does happen; guarding against it would be more effort than it's worth. + // Capture what the fetch needs from the gui's per-repo state in a + // single UI-thread hop: gui.git, gui.helpers and gui.State are all + // replaced on a repo switch (which runs on the UI thread), so reading + // them from this background goroutine would race the reassignment. + // Capturing them together also ties the fetch, the post-fetch + // refresh's generation baseline, and the recorded fetch time to the + // same repo. + var git *commands.GitCommand + var appStatusHelper *helpers.AppStatusHelper + var branchesHelper *helpers.BranchesHelper + var fetchGeneration int + if err := self.gui.g.OnUIThreadAndWaitBackground(func() error { + git = self.gui.git + appStatusHelper = self.gui.helpers.AppStatus + branchesHelper = self.gui.helpers.BranchesHelper + fetchGeneration = self.gui.c.State().GetRepoGeneration() self.gui.State.LastBackgroundFetchTime = time.Now() return nil - }) + }); err != nil { + return err + } if self.gui.UserConfig().Gui.ShowBottomLine || firstTimeOrRetriggered { - return self.gui.helpers.AppStatus.WithWaitingStatusImpl(self.gui.Tr.FetchingStatus, func(gocui.Task) error { - return self.backgroundFetch() + return appStatusHelper.WithWaitingStatusImpl(self.gui.Tr.FetchingStatus, func(gocui.Task) error { + return self.backgroundFetch(git, branchesHelper, fetchGeneration) }, nil) } - return self.backgroundFetch() + return self.backgroundFetch(git, branchesHelper, fetchGeneration) } // We want an immediate fetch at startup, and since goEvery starts by @@ -165,7 +179,20 @@ func (self *BackgroundRoutineMgr) startBackgroundExternalChangeDetection() { } func (self *BackgroundRoutineMgr) checkForExternalChanges() { - current, err := self.gui.git.Status.RefsSnapshot() + // Capture the per-repo objects in a UI-thread hop, like the background + // fetch does: gui.git and gui.helpers are replaced on a repo switch, so + // reading them from this background goroutine would race the reassignment. + var git *commands.GitCommand + var refreshHelper *helpers.RefreshHelper + if err := self.gui.g.OnUIThreadAndWaitBackground(func() error { + git = self.gui.git + refreshHelper = self.gui.helpers.Refresh + return nil + }); err != nil { + return + } + + current, err := git.Status.RefsSnapshot() if err != nil { // Transient error (e.g. git process couldn't start). Don't update the // stored snapshot; we'll retry next tick. @@ -173,7 +200,7 @@ func (self *BackgroundRoutineMgr) checkForExternalChanges() { return } - if !self.gui.helpers.Refresh.RefsSnapshotChangedSince(current) { + if !refreshHelper.RefsSnapshotChangedSince(current) { return } @@ -231,10 +258,14 @@ func (self *BackgroundRoutineMgr) goEvery(interval time.Duration, stop, retrigge }) } -func (self *BackgroundRoutineMgr) backgroundFetch() (err error) { - err = self.gui.git.Sync.FetchBackground() +// The parameters are captured by the caller before the fetch starts, not read +// here after it: the fetch is a network call during which the user may switch +// repos, and the post-fetch refresh needs to be able to tell (see +// PostFetchRefresh). +func (self *BackgroundRoutineMgr) backgroundFetch(git *commands.GitCommand, branchesHelper *helpers.BranchesHelper, fetchGeneration int) error { + err := git.Sync.FetchBackground() - return self.gui.helpers.BranchesHelper.PostFetchRefresh(err, true) + return branchesHelper.PostFetchRefresh(err, true, fetchGeneration) } func (self *BackgroundRoutineMgr) triggerImmediateFetch() { diff --git a/pkg/gui/controllers/files_controller.go b/pkg/gui/controllers/files_controller.go index bf73d4c8b..656da1bf5 100644 --- a/pkg/gui/controllers/files_controller.go +++ b/pkg/gui/controllers/files_controller.go @@ -1527,6 +1527,7 @@ func (self *FilesController) onClickMain(opts gocui.ViewMouseBindingOpts) error } func (self *FilesController) fetch() error { + fetchGeneration := self.c.State().GetRepoGeneration() return self.c.WithWaitingStatus(self.c.Tr.FetchingStatus, func(task gocui.Task) error { self.c.LogAction("Fetch") err := self.c.Git().Sync.Fetch(task) @@ -1535,7 +1536,7 @@ func (self *FilesController) fetch() error { return errors.New(self.c.Tr.PassUnameWrong) } - return self.c.Helpers().BranchesHelper.PostFetchRefresh(err, false) + return self.c.Helpers().BranchesHelper.PostFetchRefresh(err, false, fetchGeneration) }) } diff --git a/pkg/gui/controllers/helpers/branches_helper.go b/pkg/gui/controllers/helpers/branches_helper.go index 83735d87d..e87edb460 100644 --- a/pkg/gui/controllers/helpers/branches_helper.go +++ b/pkg/gui/controllers/helpers/branches_helper.go @@ -392,7 +392,11 @@ func (self *BranchesHelper) deleteRemoteBranches(remoteBranches []*models.Remote return nil } -func (self *BranchesHelper) PostFetchRefresh(fetchErr error, background bool) error { +// fetchGeneration must be the repo generation from when the fetch started, +// captured by the caller before running the fetch: the background fetch +// doesn't block repo switching and is a network call, so the window in which +// the user can switch repos spans the whole fetch, not just this refresh. +func (self *BranchesHelper) PostFetchRefresh(fetchErr error, background bool, fetchGeneration int) error { scope := []types.RefreshableView{ types.BRANCHES, types.COMMITS, types.REMOTES, types.TAGS, types.PULL_REQUESTS, } @@ -410,6 +414,12 @@ func (self *BranchesHelper) PostFetchRefresh(fetchErr error, background bool) er if fetchErr != nil { return nil } + // Then callbacks are not generation-guarded, so check explicitly: + // if the repo was switched since the fetch started, don't forward + // this repo's branches on the strength of another repo's fetch. + if self.c.State().GetRepoGeneration() != fetchGeneration { + return nil + } err := self.AutoForwardBranches(background) if background && err != nil { // The background poller discards this return value, so surface diff --git a/pkg/gui/controllers/helpers/refresh_helper.go b/pkg/gui/controllers/helpers/refresh_helper.go index a7c18aeb8..8ae62d9dd 100644 --- a/pkg/gui/controllers/helpers/refresh_helper.go +++ b/pkg/gui/controllers/helpers/refresh_helper.go @@ -1,12 +1,14 @@ package helpers import ( + "path/filepath" "strings" "sync" "sync/atomic" "time" "github.com/jesseduffield/generics/set" + "github.com/jesseduffield/lazygit/pkg/commands" "github.com/jesseduffield/lazygit/pkg/commands/git_commands" "github.com/jesseduffield/lazygit/pkg/commands/hosting_service" "github.com/jesseduffield/lazygit/pkg/commands/models" @@ -90,13 +92,29 @@ func (self *RefreshHelper) RefreshFromWorker(options types.RefreshOptions) { } type refreshEnv struct { - // whether this is a background refresh (which selects the dispatch variant that - // doesn't count towards lazygit being busy) + // Whether everything this refresh dispatches uses the background task + // variants, which don't count towards lazygit being busy — so the refresh + // doesn't block switching repos. Set for refreshes initiated by a + // background routine, and for foreground ones that opted in via + // RefreshOptions.DontBlockRepoSwitch. background bool + // Whether the refresh was initiated by an unattended background routine + // (RefreshOptions.Background) rather than by user activity. The files + // refresh uses this to decide whether git may take optional locks and + // persist its refreshed stat cache. + backgroundRoutine bool + // the repo generation captured when the refresh started generation int + // the git command instance captured when the refresh started. The refresh + // workers run their git commands through this rather than reading the live + // instance: a repo switch mid-refresh replaces the live instance (and the + // process cwd), while this one keeps addressing the repo the refresh was + // started for (its commands are pinned to that repo's directory). + git *commands.GitCommand + // When non-nil, each scope's UI-thread bounce is collected here instead of // being dispatched as it's produced, so they can all be applied in a single // frame once the whole refresh is done (see RefreshOptions.BatchUIUpdates). @@ -167,12 +185,31 @@ func (self *RefreshHelper) performRefresh(options types.RefreshOptions, calledFr panic("Refresh called from a worker, or RefreshFromWorker called from the UI thread") } - // Capture the repo generation once, here at the start, so every scope's - // bounce is guarded against the same baseline. - env := refreshEnv{ - background: options.Background, - generation: self.c.State().GetRepoGeneration(), + if options.Then != nil && options.DontBlockRepoSwitch { + // Then is not generation-guarded, so if a switch crossed the refresh it + // would run against the newly switched-to repo. A refresh carrying a + // Then must keep blocking switches. + panic("a refresh with a Then callback must not set DontBlockRepoSwitch") } + + // Capture the refresh's baseline once, here at the start: the repo + // generation that every scope's bounce is guarded against, and the git + // command instance the scopes run their commands through. The two are + // captured together on the UI thread so that they can't straddle a repo + // switch (which runs on the UI thread): pairing the old repo's instance + // with the new repo's generation would let a refresh compute data from + // the old repo and write it into the new repo's model unguarded. With a + // consistent pair, a switch-crossing refresh keeps running its commands + // against the repo it started in, and the generation guard drops its + // writes. + env := refreshEnv{ + background: options.Background || options.DontBlockRepoSwitch, + backgroundRoutine: options.Background, + } + self.captureOnUIThread(calledFromWorker, env.background, func() { + env.generation = self.c.State().GetRepoGeneration() + env.git = self.c.Git() + }) if options.BatchUIUpdates { env.batch = &refreshBounceBatch{} } @@ -226,7 +263,7 @@ func (self *RefreshHelper) performRefresh(options types.RefreshOptions, calledFr // of git's state changing externally while (or right after) we are // refreshing; the risk is one potential extra refresh, but capturing the // snapshot at the end would risk missing one, which is worse. - self.updateRefsSnapshotIfRelevant(scopeSet) + self.updateRefsSnapshotIfRelevant(scopeSet, env) wg := sync.WaitGroup{} refresh := func(name string, f func()) { @@ -515,12 +552,12 @@ func (self *RefreshHelper) RefsSnapshotChangedSince(snapshot string) bool { // We check just COMMITS and BRANCHES because the scope-expansion step at the // top of Refresh has already added these whenever REFLOG or BISECT_INFO are // in scope, and whenever a nil scope was passed. -func (self *RefreshHelper) updateRefsSnapshotIfRelevant(scopeSet *set.Set[types.RefreshableView]) { +func (self *RefreshHelper) updateRefsSnapshotIfRelevant(scopeSet *set.Set[types.RefreshableView], env refreshEnv) { if !scopeSet.Includes(types.COMMITS) && !scopeSet.Includes(types.BRANCHES) { return } - snapshot, err := self.c.Git().Status.RefsSnapshot() + snapshot, err := env.git.Status.RefsSnapshot() if err != nil { self.c.Log.Warnf("RefsSnapshot failed during refresh: %v", err) return @@ -704,15 +741,15 @@ func (self *RefreshHelper) refreshCommitsAndCommitFiles(captured capturedCommitS } } -func (self *RefreshHelper) determineCheckedOutRef() models.Ref { - if rebasedBranch := self.c.Git().Status.BranchBeingRebased(); rebasedBranch != "" { +func (self *RefreshHelper) determineCheckedOutRef(env refreshEnv) models.Ref { + if rebasedBranch := env.git.Status.BranchBeingRebased(); rebasedBranch != "" { // During a rebase we're on a detached head, so cannot determine the // branch name in the usual way. We need to read it from the // ".git/rebase-merge/head-name" file instead. return &models.Branch{Name: strings.TrimPrefix(rebasedBranch, "refs/heads/")} } - if bisectInfo := self.c.Git().Bisect.GetInfo(); bisectInfo.Bisecting() && bisectInfo.GetStartHash() != "" { + if bisectInfo := env.git.Bisect.GetInfo(); bisectInfo.Bisecting() && bisectInfo.GetStartHash() != "" { // Likewise, when we're bisecting we're on a detached head as well. In // this case we read the branch name from the ".git/BISECT_START" file. return &models.Branch{Name: bisectInfo.GetStartHash()} @@ -722,7 +759,7 @@ func (self *RefreshHelper) determineCheckedOutRef() models.Ref { // checked out. Note that if we're on a detached head (for reasons other // than rebasing or bisecting, i.e. it was explicitly checked out), then // this will return an empty string. - if branchName, err := self.c.Git().Branch.CurrentBranchName(); err == nil && branchName != "" { + if branchName, err := env.git.Branch.CurrentBranchName(); err == nil && branchName != "" { return &models.Branch{Name: branchName} } @@ -731,9 +768,9 @@ func (self *RefreshHelper) determineCheckedOutRef() models.Ref { } func (self *RefreshHelper) refreshCommitsWithLimit(captured capturedCommitState, commitSelection types.CommitSelectionBehavior, env refreshEnv) error { - checkedOutRef := self.determineCheckedOutRef() - refName, bisectInfo := self.refForLog() - commits, err := self.c.Git().Loaders.CommitLoader.GetCommits( + checkedOutRef := self.determineCheckedOutRef(env) + refName, bisectInfo := self.refForLog(env) + commits, err := env.git.Loaders.CommitLoader.GetCommits( git_commands.GetCommitsOptions{ Limit: captured.limitCommits, FilterPath: captured.filterPath, @@ -749,7 +786,7 @@ func (self *RefreshHelper) refreshCommitsWithLimit(captured capturedCommitState, if err != nil { return err } - workingTreeState := self.c.Git().Status.WorkingTreeState() + workingTreeState := env.git.Status.WorkingTreeState() self.onUIThreadUnlessRepoChanged(env, func() { self.c.Model().BisectInfo = bisectInfo @@ -902,7 +939,7 @@ func (self *RefreshHelper) refreshSubCommitsWithLimit(captured capturedSubCommit return nil } - commits, err := self.c.Git().Loaders.CommitLoader.GetCommits( + commits, err := env.git.Loaders.CommitLoader.GetCommits( git_commands.GetCommitsOptions{ Limit: captured.limitCommits, FilterPath: captured.filterPath, @@ -956,7 +993,7 @@ func (self *RefreshHelper) captureCommitFilesState() capturedCommitFilesState { } func (self *RefreshHelper) refreshCommitFilesContext(captured capturedCommitFilesState, env refreshEnv) error { - files, err := self.c.Git().Loaders.CommitFileLoader.GetFilesInDiff(captured.from, captured.to, captured.reverse) + files, err := env.git.Loaders.CommitFileLoader.GetFilesInDiff(captured.from, captured.to, captured.reverse) if err != nil { return err } @@ -975,11 +1012,11 @@ func (self *RefreshHelper) captureRebaseCommitState() (hashPool *utils.StringPoo } func (self *RefreshHelper) refreshRebaseCommits(hashPool *utils.StringPool, commits []*models.Commit, env refreshEnv) error { - updatedCommits, err := self.c.Git().Loaders.CommitLoader.MergeRebasingCommits(hashPool, commits) + updatedCommits, err := env.git.Loaders.CommitLoader.MergeRebasingCommits(hashPool, commits) if err != nil { return err } - workingTreeState := self.c.Git().Status.WorkingTreeState() + workingTreeState := env.git.Status.WorkingTreeState() self.onUIThreadUnlessRepoChanged(env, func() { self.c.Model().Commits = updatedCommits @@ -991,7 +1028,7 @@ func (self *RefreshHelper) refreshRebaseCommits(hashPool *utils.StringPool, comm } func (self *RefreshHelper) refreshTags(env refreshEnv) error { - tags, err := self.c.Git().Loaders.TagLoader.GetTags() + tags, err := env.git.Loaders.TagLoader.GetTags() if err != nil { return err } @@ -1004,8 +1041,8 @@ func (self *RefreshHelper) refreshTags(env refreshEnv) error { return nil } -func (self *RefreshHelper) refreshStateSubmoduleConfigs() ([]*models.SubmoduleConfig, error) { - return self.c.Git().Submodule.GetConfigs(nil) +func (self *RefreshHelper) refreshStateSubmoduleConfigs(env refreshEnv) ([]*models.SubmoduleConfig, error) { + return env.git.Submodule.GetConfigs(nil) } // self.refreshStatus is called at the end of this because that's when we can @@ -1013,14 +1050,25 @@ func (self *RefreshHelper) refreshStateSubmoduleConfigs() ([]*models.SubmoduleCo func (self *RefreshHelper) refreshBranches(captured capturedBranchState, refreshWorktrees bool, branchSelection types.BranchSelectionBehavior, loadBehindCounts bool, reflogCommits []*models.Commit, env refreshEnv) []*models.Branch { loadSeq := self.branchLoadSeq.Add(1) - branches, err := self.c.Git().Loaders.BranchLoader.Load( + branches, err := env.git.Loaders.BranchLoader.Load( reflogCommits, captured.mainBranches, captured.oldBranches, loadBehindCounts, func(f func() error) { self.onWorker(env.background, func(_ gocui.Task) error { - return f() + err := f() + if err != nil && self.c.State().GetRepoGeneration() != env.generation { + // An error returned from a worker is shown in a popup. Don't + // do that if the repo was switched while this worker was in + // flight: its results are dropped anyway, and the error + // concerns a repo the user has already left — e.g. failing to + // compute the behind-counts for a worktree that was deleted + // after switching away from it. + self.c.Log.Warnf("dropping error from a stale refresh worker after a repo switch: %v", err) + return nil + } + return err }) }, func() { @@ -1035,7 +1083,7 @@ func (self *RefreshHelper) refreshBranches(captured capturedBranchState, refresh var worktrees []*models.Worktree if refreshWorktrees { - worktrees = self.loadWorktrees() + worktrees = self.loadWorktrees(env) } self.onUIThreadUnlessRepoChanged(env, func() { @@ -1100,7 +1148,7 @@ func (self *RefreshHelper) refreshBranches(captured capturedBranchState, refresh } func (self *RefreshHelper) refreshFilesAndSubmodules(captured capturedFilesState, env refreshEnv) error { - configs, err := self.refreshStateSubmoduleConfigs() + configs, err := self.refreshStateSubmoduleConfigs(env) if err != nil { return err } @@ -1226,7 +1274,11 @@ func (self *RefreshHelper) refreshStateFiles(captured capturedFilesState, env re prevConflictFileCount++ } if file.HasInlineMergeConflicts { - hasConflicts, err := mergeconflicts.FileHasConflictMarkers(file.Path) + // Join with the refresh's repo root rather than relying on the + // process working directory, which may already point at another + // repo if the user switched while this refresh was in flight. + hasConflicts, err := mergeconflicts.FileHasConflictMarkers( + filepath.Join(env.git.RepoPaths.WorktreePath(), file.Path)) if err != nil { self.c.Log.Error(err) } else if !hasConflicts { @@ -1237,16 +1289,16 @@ func (self *RefreshHelper) refreshStateFiles(captured capturedFilesState, env re if len(pathsToStage) > 0 { self.c.LogAction(self.c.Tr.Actions.StageResolvedFiles) - if err := self.c.Git().WorkingTree.StageFiles(pathsToStage, nil); err != nil { + if err := env.git.WorkingTree.StageFiles(pathsToStage, nil); err != nil { return err } } } - files := self.c.Git().Loaders.FileLoader. + files := env.git.Loaders.FileLoader. GetStatusFiles(git_commands.GetStatusFileOptions{ ForceShowUntracked: captured.forceShowUntracked, - Background: env.background, + Background: env.backgroundRoutine, }) conflictFileCount := 0 @@ -1257,7 +1309,7 @@ func (self *RefreshHelper) refreshStateFiles(captured capturedFilesState, env re } repoState := self.c.State().GetRepoState() - workingTreeState := self.c.Git().Status.WorkingTreeState() + workingTreeState := env.git.Status.WorkingTreeState() if workingTreeState.None() { // No operation is in progress (any more), so forget that we started one. // This also covers an operation that was finished or aborted externally. @@ -1340,7 +1392,7 @@ func (self *RefreshHelper) refreshReflogCommits(captured capturedReflogState, en lastReflogCommit = existing[0] } - commits, onlyObtainedNewReflogCommits, err := self.c.Git().Loaders.ReflogCommitLoader. + commits, onlyObtainedNewReflogCommits, err := env.git.Loaders.ReflogCommitLoader. GetReflogCommits(captured.hashPool, lastReflogCommit, filterPath, filterAuthor) if err != nil { return nil, err @@ -1382,7 +1434,7 @@ func (self *RefreshHelper) refreshReflogCommits(captured capturedReflogState, en } func (self *RefreshHelper) refreshRemotes(prevSelectedRemote *models.Remote, env refreshEnv) ([]*models.Remote, error) { - remotes, err := self.c.Git().Loaders.RemoteLoader.GetRemotes() + remotes, err := env.git.Loaders.RemoteLoader.GetRemotes() if err != nil { return nil, err } @@ -1414,8 +1466,8 @@ func (self *RefreshHelper) refreshRemotes(prevSelectedRemote *models.Remote, env return remotes, nil } -func (self *RefreshHelper) loadWorktrees() []*models.Worktree { - worktrees, err := self.c.Git().Loaders.Worktrees.GetWorktrees() +func (self *RefreshHelper) loadWorktrees(env refreshEnv) []*models.Worktree { + worktrees, err := env.git.Loaders.Worktrees.GetWorktrees() if err != nil { self.c.Log.Error(err) return []*models.Worktree{} @@ -1424,7 +1476,7 @@ func (self *RefreshHelper) loadWorktrees() []*models.Worktree { } func (self *RefreshHelper) refreshWorktrees(env refreshEnv) { - worktrees := self.loadWorktrees() + worktrees := self.loadWorktrees(env) self.onUIThreadUnlessRepoChanged(env, func() { self.c.Model().Worktrees = worktrees @@ -1437,7 +1489,7 @@ func (self *RefreshHelper) refreshWorktrees(env refreshEnv) { } func (self *RefreshHelper) refreshStashEntries(filterPath string, env refreshEnv) { - stashEntries := self.c.Git().Loaders.StashLoader. + stashEntries := env.git.Loaders.StashLoader. GetStashEntries(filterPath) self.onUIThreadUnlessRepoChanged(env, func() { @@ -1449,8 +1501,8 @@ func (self *RefreshHelper) refreshStashEntries(filterPath string, env refreshEnv // never call this on its own, it should only be called from within refreshCommits() func (self *RefreshHelper) refreshStatus(env refreshEnv) { - workingTreeState := self.c.Git().Status.WorkingTreeState() - repoName := self.c.Git().RepoPaths.RepoName() + workingTreeState := env.git.Status.WorkingTreeState() + repoName := env.git.RepoPaths.RepoName() self.onUIThreadUnlessRepoChanged(env, func() { // Read the checked-out branch and the linked worktree name here on the UI @@ -1473,15 +1525,15 @@ func (self *RefreshHelper) refreshStatus(env refreshEnv) { // read to decide that. The caller writes the bisect info to the model (in its // bounce) rather than refForLog doing it, so the model write stays on the UI // thread. -func (self *RefreshHelper) refForLog() (string, *git_commands.BisectInfo) { - bisectInfo := self.c.Git().Bisect.GetInfo() +func (self *RefreshHelper) refForLog(env refreshEnv) (string, *git_commands.BisectInfo) { + bisectInfo := env.git.Bisect.GetInfo() if !bisectInfo.Started() { return "HEAD", bisectInfo } // need to see if our bisect's current commit is reachable from our 'new' ref. - if bisectInfo.Bisecting() && !self.c.Git().Bisect.ReachableFromStart(bisectInfo) { + if bisectInfo.Bisecting() && !env.git.Bisect.ReachableFromStart(bisectInfo) { return bisectInfo.GetNewHash(), bisectInfo } @@ -1525,18 +1577,18 @@ func (self *RefreshHelper) refreshGithubPullRequests(branches []*models.Branch, }) } - githubRemotes := getAuthenticatedGithubRemotes(self.getGithubRemotes(remotes), self.c.Git().GitHub.GetAuthToken) + githubRemotes := getAuthenticatedGithubRemotes(self.getGithubRemotes(remotes, env), env.git.GitHub.GetAuthToken) if len(githubRemotes) == 0 { clearPullRequests() return } - baseInfo := getGithubBaseRemote(githubRemotes, self.c.Git().GitHub.ConfiguredBaseRemoteName()) + baseInfo := getGithubBaseRemote(githubRemotes, env.git.GitHub.ConfiguredBaseRemoteName()) if baseInfo == nil { clearPullRequests() - if !self.githubBaseRemotePromptDismissed[self.c.Git().RepoPaths.RepoPath()] { - self.promptForBaseGithubRepo(githubRemotes, branches) + if !self.githubBaseRemotePromptDismissed[env.git.RepoPaths.RepoPath()] { + self.promptForBaseGithubRepo(githubRemotes) } return } @@ -1550,12 +1602,12 @@ type githubRemoteInfo struct { authToken string } -func (self *RefreshHelper) getGithubRemotes(remotes []*models.Remote) []githubRemoteInfo { +func (self *RefreshHelper) getGithubRemotes(remotes []*models.Remote, env refreshEnv) []githubRemoteInfo { return lo.FilterMap(remotes, func(remote *models.Remote, _ int) (githubRemoteInfo, bool) { if len(remote.Urls) == 0 { return githubRemoteInfo{}, false } - serviceInfo, err := self.c.Git().HostingService.GetServiceInfo(remote.Urls[0]) + serviceInfo, err := env.git.HostingService.GetServiceInfo(remote.Urls[0]) if err != nil || serviceInfo.Provider != "github" { return githubRemoteInfo{}, false } @@ -1612,7 +1664,7 @@ func getGithubBaseRemote(githubRemotes []githubRemoteInfo, configuredRemoteName return nil } -func (self *RefreshHelper) promptForBaseGithubRepo(githubRemotes []githubRemoteInfo, branches []*models.Branch) { +func (self *RefreshHelper) promptForBaseGithubRepo(githubRemotes []githubRemoteInfo) { menuItems := lo.Map(githubRemotes, func(info githubRemoteInfo, _ int) *types.MenuItem { return &types.MenuItem{ LabelColumns: []string{info.remote.Name, style.FgCyan.Sprint(info.serviceInfo.RepoName)}, @@ -1622,11 +1674,7 @@ func (self *RefreshHelper) promptForBaseGithubRepo(githubRemotes []githubRemoteI self.c.Log.Error(err) } - // This fetch runs on its own worker after the user picked a - // base remote, so it's not part of a performRefresh and has no - // ambient env; build a foreground one now, capturing the - // current generation as the guard baseline. - self.setGithubPullRequests(&info, branches, refreshEnv{generation: self.c.State().GetRepoGeneration()}) + self.c.RefreshFromWorker(types.RefreshOptions{Scope: []types.RefreshableView{types.PULL_REQUESTS}}) return nil }) }, @@ -1666,13 +1714,13 @@ func (self *RefreshHelper) setGithubPullRequests(baseInfo *githubRemoteInfo, bra return branch.UpstreamBranch }) - prs, err := self.c.Git().GitHub.FetchRecentPRs(branchNames, &baseInfo.serviceInfo, baseInfo.authToken) + prs, err := env.git.GitHub.FetchRecentPRs(branchNames, &baseInfo.serviceInfo, baseInfo.authToken) if err != nil { self.c.Log.Error("error fetching pull requests from GitHub: " + err.Error()) return } - self.savePullRequestsToCache(prs) + self.savePullRequestsToCache(prs, env) self.onUIThreadUnlessRepoChanged(env, func() { self.c.Model().PullRequests = prs @@ -1684,8 +1732,12 @@ func (self *RefreshHelper) setGithubPullRequests(baseInfo *githubRemoteInfo, bra }) } -func (self *RefreshHelper) savePullRequestsToCache(prs []*models.GithubPullRequest) { - repoPath := self.c.Git().RepoPaths.RepoPath() +func (self *RefreshHelper) savePullRequestsToCache(prs []*models.GithubPullRequest, env refreshEnv) { + // Key the cache by the repo the refresh was started for, not the live one: + // this runs on a worker, and if the user switched repos while the fetch was + // in flight, the live instance would file the old repo's pull requests + // under the new repo's path. + repoPath := env.git.RepoPaths.RepoPath() cached := lo.Map(prs, func(pr *models.GithubPullRequest, _ int) config.CachedPullRequest { return config.CachedPullRequest{ HeadRefName: pr.HeadRefName, diff --git a/pkg/gui/gui.go b/pkg/gui/gui.go index 533c01fbd..8776040e7 100644 --- a/pkg/gui/gui.go +++ b/pkg/gui/gui.go @@ -390,7 +390,7 @@ func (gui *Gui) onNewRepo(startArgs appTypes.StartArgs, contextKey types.Context } gui.c.Log.Info("Receiving focus - refreshing") - gui.helpers.Refresh.Refresh(types.RefreshOptions{}) + gui.helpers.Refresh.Refresh(types.RefreshOptions{DontBlockRepoSwitch: true}) return reloadErr } @@ -1031,7 +1031,7 @@ func (gui *Gui) runSubprocessWithSuspenseAndRefresh(subprocess *oscommands.CmdOb return err } - gui.c.Refresh(types.RefreshOptions{}) + gui.c.Refresh(types.RefreshOptions{DontBlockRepoSwitch: true}) return nil } @@ -1108,7 +1108,7 @@ func (gui *Gui) loadNewRepo() error { return err } - gui.c.Refresh(types.RefreshOptions{}) + gui.c.Refresh(types.RefreshOptions{DontBlockRepoSwitch: true}) if err := gui.os.UpdateWindowTitle(); err != nil { return err diff --git a/pkg/gui/types/refresh.go b/pkg/gui/types/refresh.go index 937c3a30e..c733e589e 100644 --- a/pkg/gui/types/refresh.go +++ b/pkg/gui/types/refresh.go @@ -94,4 +94,19 @@ type RefreshOptions struct { // fast. Background refreshes leave the suppression in place: not persisting // the stat-cache is the right trade-off for unattended work. Background bool + + // When true, this foreground refresh does not block switching repos while + // it is in flight. A refresh is switch-safe by construction — its git + // commands run against the repo it was started for, and the generation + // guard drops its model/view updates if the repo changed — but a refresh + // triggered by a user operation still blocks switching (its tasks count + // towards Busy()), because the operation's follow-up work isn't covered + // by those guards. A refresh that merely reloads state (on focus, after a + // repo switch, after returning from a subprocess) has no such follow-up, + // so it opts in here and a repo switch during it is allowed rather than + // refused with a toast. + // + // Must not be combined with Then: Then is not generation-guarded, so it + // would run against the newly switched-to repo. + DontBlockRepoSwitch bool }