From 47471c501b4ae6ecd8c265bb7940c623de03b453 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Thu, 30 Jul 2026 12:33:47 +0200 Subject: [PATCH] Move Github PR cache out of state.yml into a separate file Having the cache in state.yml causes this file to be rewritten every 60s, which is annoying if you have a lazygit running in the background somewhere without even realizing it, and it keeps overwriting the foreground lazygit's newer command shell history and recent repos list with its stale data. State.yml should only contain things that change in response to user actions, not periodically. --- pkg/config/app_config.go | 83 ++++++----- pkg/config/dummies.go | 11 +- pkg/config/github_pull_request_cache.go | 124 +++++++++++++++ pkg/config/github_pull_request_cache_test.go | 141 ++++++++++++++++++ pkg/gui/controllers/helpers/refresh_helper.go | 7 +- pkg/gui/gui.go | 5 +- 6 files changed, 319 insertions(+), 52 deletions(-) create mode 100644 pkg/config/github_pull_request_cache.go create mode 100644 pkg/config/github_pull_request_cache_test.go diff --git a/pkg/config/app_config.go b/pkg/config/app_config.go index a0be00329..ade614f7d 100644 --- a/pkg/config/app_config.go +++ b/pkg/config/app_config.go @@ -21,17 +21,18 @@ import ( // AppConfig contains the base configuration fields required for lazygit. type AppConfig struct { - debug bool `long:"debug" env:"DEBUG" default:"false"` - version string `long:"version" env:"VERSION" default:"unversioned"` - buildDate string `long:"build-date" env:"BUILD_DATE"` - name string `long:"name" env:"NAME" default:"lazygit"` - buildSource string `long:"build-source" env:"BUILD_SOURCE" default:""` - userConfig *UserConfig - globalUserConfigFiles []*ConfigFile - userConfigFiles []*ConfigFile - userConfigDir string - tempDir string - appState *AppState + debug bool `long:"debug" env:"DEBUG" default:"false"` + version string `long:"version" env:"VERSION" default:"unversioned"` + buildDate string `long:"build-date" env:"BUILD_DATE"` + name string `long:"name" env:"NAME" default:"lazygit"` + buildSource string `long:"build-source" env:"BUILD_SOURCE" default:""` + userConfig *UserConfig + globalUserConfigFiles []*ConfigFile + userConfigFiles []*ConfigFile + userConfigDir string + tempDir string + appState *AppState + githubPullRequestCache *githubPullRequestCache } type AppConfigurer interface { @@ -51,6 +52,8 @@ type AppConfigurer interface { GetAppState() *AppState SaveAppState() error + GetCachedGithubPullRequests(repoPath string) ([]CachedPullRequest, error) + SaveCachedGithubPullRequests(repoPath string, pullRequests []CachedPullRequest) error } type ConfigFilePolicy int @@ -107,19 +110,21 @@ func NewAppConfig( if err != nil { return nil, err } + githubPullRequestCache := loadGithubPullRequestCache() appConfig := &AppConfig{ - name: name, - version: version, - buildDate: date, - debug: debuggingFlag, - buildSource: buildSource, - userConfig: userConfig, - globalUserConfigFiles: configFiles, - userConfigFiles: configFiles, - userConfigDir: configDir, - tempDir: tempDir, - appState: appState, + name: name, + version: version, + buildDate: date, + debug: debuggingFlag, + buildSource: buildSource, + userConfig: userConfig, + globalUserConfigFiles: configFiles, + userConfigFiles: configFiles, + userConfigDir: configDir, + tempDir: tempDir, + appState: appState, + githubPullRequestCache: githubPullRequestCache, } return appConfig, nil @@ -666,6 +671,20 @@ func (c *AppConfig) GetAppState() *AppState { return c.appState } +func (c *AppConfig) GetCachedGithubPullRequests(repoPath string) ([]CachedPullRequest, error) { + if c.githubPullRequestCache == nil { + return nil, nil + } + return c.githubPullRequestCache.get(repoPath), c.githubPullRequestCache.takeLoadError() +} + +func (c *AppConfig) SaveCachedGithubPullRequests(repoPath string, pullRequests []CachedPullRequest) error { + if c.githubPullRequestCache == nil { + return nil + } + return c.githubPullRequestCache.save(repoPath, pullRequests) +} + func (c *AppConfig) GetUserConfigPaths() []string { return lo.FilterMap(c.userConfigFiles, func(f *ConfigFile, _ int) (string, bool) { return f.Path, f.exists @@ -837,28 +856,10 @@ type AppState struct { ShellCommandsHistory []string `yaml:"customcommandshistory"` HideCommandLog bool - - // Cache of GitHub pull requests per repo path, so that PR info can be - // shown instantly on startup before the async refresh completes. - GithubPullRequests map[string][]CachedPullRequest `yaml:"githubPullRequests"` -} - -// CachedPullRequest stores the essential fields of a GitHub pull request -// for persisting in the app state cache. -type CachedPullRequest struct { - HeadRefName string `yaml:"headRefName"` - Number int `yaml:"number"` - Title string `yaml:"title"` - State string `yaml:"state"` - ChecksState string `yaml:"checksState,omitempty"` - Url string `yaml:"url"` - HeadRepositoryOwner string `yaml:"headRepositoryOwner"` } func getDefaultAppState() *AppState { - return &AppState{ - GithubPullRequests: make(map[string][]CachedPullRequest), - } + return &AppState{} } func LogPath() (string, error) { diff --git a/pkg/config/dummies.go b/pkg/config/dummies.go index 5bc349fa0..b872fac29 100644 --- a/pkg/config/dummies.go +++ b/pkg/config/dummies.go @@ -9,11 +9,12 @@ func NewDummyAppConfig() *AppConfig { userConfig := GetDefaultConfig() userConfig.Keybinding.MergeLegacyAltKeybindings() appConfig := &AppConfig{ - name: "lazygit", - version: "unversioned", - debug: false, - userConfig: userConfig, - appState: &AppState{}, + name: "lazygit", + version: "unversioned", + debug: false, + userConfig: userConfig, + appState: &AppState{}, + githubPullRequestCache: newGithubPullRequestCache(""), } _ = yaml.Unmarshal([]byte{}, appConfig.appState) return appConfig diff --git a/pkg/config/github_pull_request_cache.go b/pkg/config/github_pull_request_cache.go new file mode 100644 index 000000000..3ea8d0841 --- /dev/null +++ b/pkg/config/github_pull_request_cache.go @@ -0,0 +1,124 @@ +package config + +import ( + "encoding/json" + "fmt" + "os" + "path/filepath" + "sync" +) + +const githubPullRequestsCacheFileName = "github_pull_requests.json" + +// CachedPullRequest stores the essential fields of a GitHub pull request. +type CachedPullRequest struct { + HeadRefName string `json:"headRefName"` + Number int `json:"number"` + Title string `json:"title"` + State string `json:"state"` + ChecksState string `json:"checksState,omitempty"` + Url string `json:"url"` + HeadRepositoryOwner string `json:"headRepositoryOwner"` +} + +type githubPullRequestCache struct { + mutex sync.Mutex + path string + pullRequestsByRepoPath map[string][]CachedPullRequest + loadErr error +} + +func loadGithubPullRequestCache() *githubPullRequestCache { + path, err := githubPullRequestCachePath() + if err != nil { + cache := newGithubPullRequestCache("") + cache.loadErr = err + return cache + } + + cache := newGithubPullRequestCache(path) + cache.load() + return cache +} + +func githubPullRequestCachePath() (string, error) { + path, err := stateFilePath(stateFileName) + if err != nil { + return "", err + } + + return filepath.Join(filepath.Dir(path), githubPullRequestsCacheFileName), nil +} + +func newGithubPullRequestCache(path string) *githubPullRequestCache { + return &githubPullRequestCache{ + path: path, + pullRequestsByRepoPath: make(map[string][]CachedPullRequest), + } +} + +func (c *githubPullRequestCache) load() { + if c.path == "" { + return + } + + content, err := os.ReadFile(c.path) + if err != nil { + if !os.IsNotExist(err) { + c.loadErr = fmt.Errorf("reading GitHub pull request cache: %w", err) + } + return + } + if len(content) == 0 { + return + } + + if err := json.Unmarshal(content, &c.pullRequestsByRepoPath); err != nil { + c.pullRequestsByRepoPath = make(map[string][]CachedPullRequest) + c.loadErr = fmt.Errorf("parsing GitHub pull request cache: %w", err) + } else if c.pullRequestsByRepoPath == nil { + c.pullRequestsByRepoPath = make(map[string][]CachedPullRequest) + } +} + +func (c *githubPullRequestCache) get(repoPath string) []CachedPullRequest { + c.mutex.Lock() + defer c.mutex.Unlock() + + return append([]CachedPullRequest(nil), c.pullRequestsByRepoPath[repoPath]...) +} + +// takeLoadError returns the error, if any, that occurred while loading the +// cache from disk, clearing it so that it is reported only once. +func (c *githubPullRequestCache) takeLoadError() error { + c.mutex.Lock() + defer c.mutex.Unlock() + + loadErr := c.loadErr + c.loadErr = nil + return loadErr +} + +func (c *githubPullRequestCache) save(repoPath string, pullRequests []CachedPullRequest) error { + c.mutex.Lock() + defer c.mutex.Unlock() + + c.pullRequestsByRepoPath[repoPath] = append([]CachedPullRequest(nil), pullRequests...) + if c.path == "" { + return nil + } + + content, err := json.MarshalIndent(c.pullRequestsByRepoPath, "", " ") + if err != nil { + return err + } + content = append(content, '\n') + + // Apparently when people have read-only permissions they prefer us to fail + // silently, so don't propagate permission errors. + if err := os.WriteFile(c.path, content, 0o644); err != nil && !os.IsPermission(err) { + return err + } + + return nil +} diff --git a/pkg/config/github_pull_request_cache_test.go b/pkg/config/github_pull_request_cache_test.go new file mode 100644 index 000000000..aa10d1acb --- /dev/null +++ b/pkg/config/github_pull_request_cache_test.go @@ -0,0 +1,141 @@ +package config + +import ( + "fmt" + "os" + "path/filepath" + "sync" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestGithubPullRequestCachePath(t *testing.T) { + stateDir := t.TempDir() + t.Setenv("CONFIG_DIR", stateDir) + + path, err := githubPullRequestCachePath() + + assert.NoError(t, err) + assert.Equal(t, filepath.Join(stateDir, githubPullRequestsCacheFileName), path) +} + +func TestGithubPullRequestCache(t *testing.T) { + path := filepath.Join(t.TempDir(), githubPullRequestsCacheFileName) + cache := newGithubPullRequestCache(path) + repoOnePullRequests := []CachedPullRequest{{ + HeadRefName: "first-branch", + Number: 1, + Title: "First pull request", + State: "OPEN", + Url: "https://github.com/owner/repo/pull/1", + HeadRepositoryOwner: "owner", + }} + repoTwoPullRequests := []CachedPullRequest{{ + HeadRefName: "second-branch", + Number: 2, + Title: "Second pull request", + State: "MERGED", + Url: "https://github.com/other/repo/pull/2", + HeadRepositoryOwner: "other", + }} + + assert.NoError(t, cache.save("/repo/one", repoOnePullRequests)) + assert.NoError(t, cache.save("/repo/two", repoTwoPullRequests)) + + content, err := os.ReadFile(path) + assert.NoError(t, err) + assert.Equal(t, `{ + "/repo/one": [ + { + "headRefName": "first-branch", + "number": 1, + "title": "First pull request", + "state": "OPEN", + "url": "https://github.com/owner/repo/pull/1", + "headRepositoryOwner": "owner" + } + ], + "/repo/two": [ + { + "headRefName": "second-branch", + "number": 2, + "title": "Second pull request", + "state": "MERGED", + "url": "https://github.com/other/repo/pull/2", + "headRepositoryOwner": "other" + } + ] +} +`, string(content)) + + reloadedCache := newGithubPullRequestCache(path) + reloadedCache.load() + assert.Equal(t, repoOnePullRequests, reloadedCache.get("/repo/one")) + assert.Equal(t, repoTwoPullRequests, reloadedCache.get("/repo/two")) + assert.NoError(t, reloadedCache.takeLoadError()) +} + +func TestGithubPullRequestCacheIgnoresMalformedContent(t *testing.T) { + path := filepath.Join(t.TempDir(), githubPullRequestsCacheFileName) + assert.NoError(t, os.WriteFile(path, []byte("{"), 0o644)) + + cache := newGithubPullRequestCache(path) + cache.load() + + assert.ErrorContains(t, cache.takeLoadError(), "parsing GitHub pull request cache") + assert.Empty(t, cache.get("/repo")) + assert.NoError(t, cache.save("/repo", []CachedPullRequest{{Number: 1}})) + + reloadedCache := newGithubPullRequestCache(path) + reloadedCache.load() + assert.Equal(t, []CachedPullRequest{{Number: 1}}, reloadedCache.get("/repo")) + assert.NoError(t, reloadedCache.takeLoadError()) +} + +func TestGithubPullRequestCacheDoesNotModifyAppState(t *testing.T) { + stateDir := t.TempDir() + t.Setenv("CONFIG_DIR", stateDir) + statePath := filepath.Join(stateDir, stateFileName) + stateContent := []byte("recentrepos:\n - /repo\n") + assert.NoError(t, os.WriteFile(statePath, stateContent, 0o644)) + + cache := loadGithubPullRequestCache() + assert.NoError(t, cache.save("/repo", []CachedPullRequest{{Number: 1}})) + + actualStateContent, err := os.ReadFile(statePath) + assert.NoError(t, err) + assert.Equal(t, stateContent, actualStateContent) + _, err = os.Stat(filepath.Join(stateDir, githubPullRequestsCacheFileName)) + assert.NoError(t, err) +} + +func TestGithubPullRequestCacheSerializesConcurrentSaves(t *testing.T) { + path := filepath.Join(t.TempDir(), githubPullRequestsCacheFileName) + cache := newGithubPullRequestCache(path) + const repoCount = 20 + var waitGroup sync.WaitGroup + errs := make(chan error, repoCount) + + for index := range repoCount { + waitGroup.Add(1) + go func() { + defer waitGroup.Done() + repoPath := fmt.Sprintf("/repo/%d", index) + errs <- cache.save(repoPath, []CachedPullRequest{{Number: index}}) + }() + } + waitGroup.Wait() + close(errs) + for err := range errs { + assert.NoError(t, err) + } + + reloadedCache := newGithubPullRequestCache(path) + reloadedCache.load() + for index := range repoCount { + repoPath := fmt.Sprintf("/repo/%d", index) + assert.Equal(t, []CachedPullRequest{{Number: index}}, reloadedCache.get(repoPath)) + } + assert.NoError(t, reloadedCache.takeLoadError()) +} diff --git a/pkg/gui/controllers/helpers/refresh_helper.go b/pkg/gui/controllers/helpers/refresh_helper.go index 29fb66be2..3db9ab9dc 100644 --- a/pkg/gui/controllers/helpers/refresh_helper.go +++ b/pkg/gui/controllers/helpers/refresh_helper.go @@ -1776,10 +1776,7 @@ func (self *RefreshHelper) savePullRequestsToCache(prs []*models.GithubPullReque } }) - appState := self.c.GetAppState() - if appState.GithubPullRequests == nil { - appState.GithubPullRequests = make(map[string][]config.CachedPullRequest) + if err := self.c.GetConfig().SaveCachedGithubPullRequests(repoPath, cached); err != nil { + self.c.Log.Warnf("error saving GitHub pull request cache: %v", err) } - appState.GithubPullRequests[repoPath] = cached - self.c.SaveAppStateAndLogError() } diff --git a/pkg/gui/gui.go b/pkg/gui/gui.go index f536d5b64..7713cbd57 100644 --- a/pkg/gui/gui.go +++ b/pkg/gui/gui.go @@ -666,7 +666,10 @@ func (gui *Gui) resetState(startArgs appTypes.StartArgs) types.Context { func (gui *Gui) loadCachedPullRequests() []*models.GithubPullRequest { repoPath := gui.git.RepoPaths.RepoPath() - cachedPRs := gui.c.GetAppState().GithubPullRequests[repoPath] + cachedPRs, err := gui.Config.GetCachedGithubPullRequests(repoPath) + if err != nil { + gui.Log.Warnf("error loading GitHub pull request cache: %v", err) + } return lo.Map(cachedPRs, func(cached config.CachedPullRequest, _ int) *models.GithubPullRequest { return &models.GithubPullRequest{