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{