diff --git a/AGENTS.md b/AGENTS.md index 399a86718..476d6e03a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -27,6 +27,8 @@ while still being meaningful and self-contained. - **Every commit must compile and pass all tests.** No "WIP" commits, no commits that leave the tree broken and rely on a follow-up to fix it. +- **Every commit must be `gofumpt`-formatted.** Run `make format` before + committing. - **Commit messages explain _why_, not _what_.** The diff already shows what changed; the message should capture the motivation, the constraint, or the bug being fixed. If the reason is obvious from a one-line subject, no body @@ -38,6 +40,28 @@ while still being meaningful and self-contained. - **Do not use conventional commits** (no `feat:`/`fix:`/`chore:` prefixes). Match the plain English imperative style of the existing history. +## Iterate with `fixup!` commits + +When refining work that's already committed — adjusting an approach, +incorporating an idea from elsewhere, fixing something that belongs to the +same logical unit — create a fixup against the target commit +(`git commit --fixup=`) so the history collapses cleanly under +`git rebase --autosquash`. Don't pile follow-up commits on top with the +intent of squashing them later. + +If the changes don't map cleanly onto existing commits — say they cut +across several of them, or restructure something at a different layer +than any existing commit naturally owns — stop and ask the user how to +proceed. Resetting the branch and redoing the work is sometimes the right +call, but it's the user's call to make. + +After writing a fixup, re-read the target commit's message. If anything in +that message has become inaccurate or misleading because of the fixup, use +an `amend!` commit instead (its subject is `amend! ` and +its body becomes the target's new full message after autosquash). A plain +`fixup!` keeps the original message verbatim, so message drift stays in +unless you explicitly correct it. + ## Prefer the cleaner design over the smaller diff When a task could be implemented either by tacking onto existing code or by @@ -88,3 +112,10 @@ Use this pattern only where it makes sense; don't apply it by default. Prefer `assert.Equal` (and friends) over hand-rolled `if` checks. The failure messages are more useful and the intent is clearer at a glance. + +## Don't search outside the working tree + +Never run `find` (or similar) from `/` or other paths outside the project. All +third-party code we use is vendored under `vendor/`, so dependency sources are +reachable from inside the working tree — search there instead of the host +filesystem. diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 000000000..3f1ed7b4c --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1 @@ +Before doing anything else, read AGENTS.md and follow it. diff --git a/README.md b/README.md index 044386655..549366864 100644 --- a/README.md +++ b/README.md @@ -230,7 +230,7 @@ If you press `shift+w` on a commit (or branch/ref) a menu will open that allows ### Show GitHub pull requests -In the branches panel, lazygit can show which of your branches have an associated GitHub pull request by showing a GitHub icon next to the branch name; its color shows the state of the PR (open, merged, etc.). For those that have one, you can press `shift-G` to open the PR in the browser. There is no configuration needed to enable this, but it requires the [`gh`](https://cli.github.com/) tool to be installed, and you need to do `gh auth login` once to allow lazygit to access GitHub. +In the branches panel, lazygit can show which of your branches have an associated GitHub pull request by showing a GitHub icon next to the branch name; its color shows the state of the PR (open, merged, etc.). For those that have one, you can press `shift-G` to open the PR in the browser. There is no configuration needed to enable this for github.com, but it requires the [`gh`](https://cli.github.com/) tool to be installed, and you need to do `gh auth login` once to allow lazygit to access GitHub. For GitHub Enterprise, also run `gh auth login --hostname ` and add a [`services` entry](docs/Config.md#custom-pull-request-urls) for the host with the `github` provider. ## Tutorials diff --git a/docs-master/Config.md b/docs-master/Config.md index 42eff4666..05d3b6d20 100644 --- a/docs-master/Config.md +++ b/docs-master/Config.md @@ -1117,6 +1117,8 @@ Where: - `provider` is one of `github`, `bitbucket`, `bitbucketServer`, `azuredevops`, `gitlab`, `gitea` or `codeberg` - `webDomain` is the URL where your git service exposes a web interface and APIs, e.g. `gitservice.work.com` +For the `github` provider, configuring an entry here also enables the pull-request icons in the branches panel for that host (e.g. a GitHub Enterprise Server instance). Lazygit picks up the auth token via the same mechanisms as the `gh` CLI: the `GH_ENTERPRISE_TOKEN` / `GITHUB_ENTERPRISE_TOKEN` environment variables, or `gh auth login --hostname `. + ## Predefined commit message prefix In situations where certain naming pattern is used for branches and commits, pattern can be used to populate commit message with prefix that is parsed from the branch name. diff --git a/pkg/commands/git_commands/github.go b/pkg/commands/git_commands/github.go index 85893615d..e05472ef1 100644 --- a/pkg/commands/git_commands/github.go +++ b/pkg/commands/git_commands/github.go @@ -138,19 +138,16 @@ func fetchPullRequestsQuery(branches []string, owner string, repo string) (strin return queryString, variables } -func (self *GitHubCommands) GetAuthToken() string { - defaultHost, _ := auth.DefaultHost() - token, _ := auth.TokenForHost(defaultHost) +func (self *GitHubCommands) GetAuthToken(host string) string { + token, _ := auth.TokenForHost(host) return token } -// FetchRecentPRs fetches recent pull requests using GraphQL. -func (self *GitHubCommands) FetchRecentPRs(branches []string, baseRemote *models.Remote, token string) ([]*models.GithubPullRequest, error) { - repoOwner, repoName, err := self.GetBaseRepoOwnerAndName(baseRemote) - if err != nil { - return nil, err - } - +// FetchRecentPRs fetches recent pull requests using GraphQL. serviceInfo +// identifies the GitHub instance (github.com or a GitHub Enterprise Server) +// and the owner/repo to query against. +func (self *GitHubCommands) FetchRecentPRs(branches []string, serviceInfo *hosting_service.ServiceInfo, token string) ([]*models.GithubPullRequest, error) { + endpoint := graphQLEndpoint(serviceInfo.WebDomain) t := time.Now() var g errgroup.Group @@ -171,7 +168,7 @@ func (self *GitHubCommands) FetchRecentPRs(branches []string, baseRemote *models // Launch a goroutine for each chunk of branches g.Go(func() error { - prs, err := self.fetchRecentPRsAux(repoOwner, repoName, branchChunk, token) + prs, err := self.fetchRecentPRsAux(endpoint, serviceInfo.Owner, serviceInfo.Repository, branchChunk, token) if err != nil { return err } @@ -181,7 +178,7 @@ func (self *GitHubCommands) FetchRecentPRs(branches []string, baseRemote *models } // Wait for all goroutines, then close the channel so the range loop exits - err = g.Wait() + err := g.Wait() close(results) if err != nil { return nil, err @@ -198,14 +195,14 @@ func (self *GitHubCommands) FetchRecentPRs(branches []string, baseRemote *models return allPRs, nil } -func (self *GitHubCommands) fetchRecentPRsAux(repoOwner string, repoName string, branches []string, token string) ([]*models.GithubPullRequest, error) { +func (self *GitHubCommands) fetchRecentPRsAux(endpoint string, repoOwner string, repoName string, branches []string, token string) ([]*models.GithubPullRequest, error) { queryString, variables := fetchPullRequestsQuery(branches, repoOwner, repoName) bodyBytes, err := json.Marshal(graphQLRequest{Query: queryString, Variables: variables}) if err != nil { return nil, err } - req, err := http.NewRequest("POST", "https://api.github.com/graphql", bytes.NewBuffer(bodyBytes)) + req, err := http.NewRequest("POST", endpoint, bytes.NewBuffer(bodyBytes)) if err != nil { return nil, err } @@ -336,45 +333,12 @@ func getRemotesToOwnersMap(remotes []*models.Remote) map[string]string { return res } -func (self *GitHubCommands) InGithubRepo(remotes []*models.Remote) bool { - if len(remotes) == 0 { - return false +// graphQLEndpoint returns the GraphQL API URL for a GitHub host. github.com +// uses a dedicated api. subdomain; GitHub Enterprise Server hangs the API off +// the web host under /api/graphql. +func graphQLEndpoint(host string) string { + if auth.NormalizeHostname(host) == "github.com" { + return "https://api.github.com/graphql" } - - remote := getMainRemote(remotes) - - if len(remote.Urls) == 0 { - return false - } - - url := remote.Urls[0] - return strings.Contains(strings.ToLower(url), "github.com") -} - -func getMainRemote(remotes []*models.Remote) *models.Remote { - for _, remote := range remotes { - if remote.Name == "origin" { - return remote - } - } - - // need to sort remotes by name so that this is deterministic - return lo.MinBy(remotes, func(a, b *models.Remote) bool { - return a.Name < b.Name - }) -} - -func (self *GitHubCommands) GetBaseRepoOwnerAndName(baseRemote *models.Remote) (string, string, error) { - if len(baseRemote.Urls) == 0 { - return "", "", fmt.Errorf("No URLs found for remote") - } - - url := baseRemote.Urls[0] - - repoInfo, err := hosting_service.GetRepoInfoFromURL(url) - if err != nil { - return "", "", err - } - - return repoInfo.Owner, repoInfo.Repository, nil + return "https://" + host + "/api/graphql" } diff --git a/pkg/commands/git_commands/github_test.go b/pkg/commands/git_commands/github_test.go index d9d55ffd1..b332ba12a 100644 --- a/pkg/commands/git_commands/github_test.go +++ b/pkg/commands/git_commands/github_test.go @@ -57,6 +57,25 @@ func TestGetRepoInfoFromURL(t *testing.T) { } } +func TestGraphQLEndpoint(t *testing.T) { + cases := []struct { + host string + expected string + }{ + {"github.com", "https://api.github.com/graphql"}, + {"www.github.com", "https://api.github.com/graphql"}, + {"GITHUB.com", "https://api.github.com/graphql"}, + {"ghe.example.com", "https://ghe.example.com/api/graphql"}, + {"ghe.example.com:8443", "https://ghe.example.com:8443/api/graphql"}, + } + + for _, c := range cases { + t.Run(c.host, func(t *testing.T) { + assert.Equal(t, c.expected, graphQLEndpoint(c.host)) + }) + } +} + func TestGenerateGithubPullRequestMap(t *testing.T) { cases := []struct { name string diff --git a/pkg/commands/git_commands/hosting_service.go b/pkg/commands/git_commands/hosting_service.go index 7d9772127..f43b93e90 100644 --- a/pkg/commands/git_commands/hosting_service.go +++ b/pkg/commands/git_commands/hosting_service.go @@ -21,8 +21,8 @@ func (self *HostingService) GetCommitURL(commitSha string) (string, error) { return self.getHostingServiceMgr(self.config.GetRemoteURL()).GetCommitURL(commitSha) } -func (self *HostingService) GetRepoNameFromRemoteURL(remoteURL string) (string, error) { - return self.getHostingServiceMgr(remoteURL).GetRepoName() +func (self *HostingService) GetServiceInfo(remoteURL string) (hosting_service.ServiceInfo, error) { + return self.getHostingServiceMgr(remoteURL).GetServiceInfo() } // getting this on every request rather than storing it in state in case our remoteURL changes diff --git a/pkg/commands/hosting_service/definitions.go b/pkg/commands/hosting_service/definitions.go index 130bf0481..09fa191c8 100644 --- a/pkg/commands/hosting_service/definitions.go +++ b/pkg/commands/hosting_service/definitions.go @@ -1,10 +1,12 @@ package hosting_service +import "regexp" + // if you want to make a custom regex for a given service feel free to test it out // at https://regex101.com using the flavor Golang -var defaultUrlRegexStrings = []string{ - `^(?:https?|ssh)://[^/]+/(?P.*)/(?P.*?)(?:\.git)?$`, - `^(.*?@)?.*:/*(?P.*)/(?P.*?)(?:\.git)?$`, +var defaultUrlRegexps = []*regexp.Regexp{ + regexp.MustCompile(`^(?:https?|ssh)://[^/]+/(?P.*)/(?P.*?)(?:\.git)?$`), + regexp.MustCompile(`^(.*?@)?.*:/*(?P.*)/(?P.*?)(?:\.git)?$`), } var ( @@ -19,7 +21,7 @@ var githubServiceDef = ServiceDefinition{ pullRequestURLIntoDefaultBranch: "/compare/{{.From}}?expand=1", pullRequestURLIntoTargetBranch: "/compare/{{.To}}...{{.From}}?expand=1", commitURL: "/commit/{{.CommitHash}}", - regexStrings: defaultUrlRegexStrings, + urlRegexps: defaultUrlRegexps, repoURLTemplate: defaultRepoURLTemplate, repoNameTemplate: defaultRepoNameTemplate, } @@ -29,9 +31,9 @@ var bitbucketServiceDef = ServiceDefinition{ pullRequestURLIntoDefaultBranch: "/pull-requests/new?source={{.From}}&t=1", pullRequestURLIntoTargetBranch: "/pull-requests/new?source={{.From}}&dest={{.To}}&t=1", commitURL: "/commits/{{.CommitHash}}", - regexStrings: []string{ - `^(?:https?|ssh)://.*/(?P.*)/(?P.*?)(?:\.git)?$`, - `^.*@.*:/*(?P.*)/(?P.*?)(?:\.git)?$`, + urlRegexps: []*regexp.Regexp{ + regexp.MustCompile(`^(?:https?|ssh)://.*/(?P.*)/(?P.*?)(?:\.git)?$`), + regexp.MustCompile(`^.*@.*:/*(?P.*)/(?P.*?)(?:\.git)?$`), }, repoURLTemplate: defaultRepoURLTemplate, repoNameTemplate: defaultRepoNameTemplate, @@ -42,7 +44,7 @@ var gitLabServiceDef = ServiceDefinition{ pullRequestURLIntoDefaultBranch: "/-/merge_requests/new?merge_request%5Bsource_branch%5D={{.From}}", pullRequestURLIntoTargetBranch: "/-/merge_requests/new?merge_request%5Bsource_branch%5D={{.From}}&merge_request%5Btarget_branch%5D={{.To}}", commitURL: "/-/commit/{{.CommitHash}}", - regexStrings: defaultUrlRegexStrings, + urlRegexps: defaultUrlRegexps, repoURLTemplate: defaultRepoURLTemplate, repoNameTemplate: defaultRepoNameTemplate, } @@ -52,11 +54,11 @@ var azdoServiceDef = ServiceDefinition{ pullRequestURLIntoDefaultBranch: "/pullrequestcreate?sourceRef={{.From}}", pullRequestURLIntoTargetBranch: "/pullrequestcreate?sourceRef={{.From}}&targetRef={{.To}}", commitURL: "/commit/{{.CommitHash}}", - regexStrings: []string{ - `^.+@vs-ssh\.visualstudio\.com[:/](?:v3/)?(?P[^/]+)/(?P[^/]+)/(?P[^/]+?)(?:\.git)?$`, - `^git@ssh.dev.azure.com.*/(?P.*)/(?P.*)/(?P.*?)(?:\.git)?$`, - `^https://.*@dev.azure.com/(?P.*?)/(?P.*?)/_git/(?P.*?)(?:\.git)?$`, - `^https://.*/(?P.*?)/(?P.*?)/_git/(?P.*?)(?:\.git)?$`, + urlRegexps: []*regexp.Regexp{ + regexp.MustCompile(`^.+@vs-ssh\.visualstudio\.com[:/](?:v3/)?(?P[^/]+)/(?P[^/]+)/(?P[^/]+?)(?:\.git)?$`), + regexp.MustCompile(`^git@ssh.dev.azure.com.*/(?P.*)/(?P.*)/(?P.*?)(?:\.git)?$`), + regexp.MustCompile(`^https://.*@dev.azure.com/(?P.*?)/(?P.*?)/_git/(?P.*?)(?:\.git)?$`), + regexp.MustCompile(`^https://.*/(?P.*?)/(?P.*?)/_git/(?P.*?)(?:\.git)?$`), }, repoURLTemplate: "https://{{.webDomain}}/{{.org}}/{{.project}}/_git/{{.repo}}", repoNameTemplate: "{{.org}}/{{.project}}/{{.repo}}", @@ -67,9 +69,9 @@ var bitbucketServerServiceDef = ServiceDefinition{ pullRequestURLIntoDefaultBranch: "/pull-requests?create&sourceBranch={{.From}}", pullRequestURLIntoTargetBranch: "/pull-requests?create&targetBranch={{.To}}&sourceBranch={{.From}}", commitURL: "/commits/{{.CommitHash}}", - regexStrings: []string{ - `^ssh://git@.*/(?P.*)/(?P.*?)(?:\.git)?$`, - `^https://.*/scm/(?P.*)/(?P.*?)(?:\.git)?$`, + urlRegexps: []*regexp.Regexp{ + regexp.MustCompile(`^ssh://git@.*/(?P.*)/(?P.*?)(?:\.git)?$`), + regexp.MustCompile(`^https://.*/scm/(?P.*)/(?P.*?)(?:\.git)?$`), }, repoURLTemplate: "https://{{.webDomain}}/projects/{{.project}}/repos/{{.repo}}", repoNameTemplate: "{{.project}}/{{.repo}}", @@ -80,7 +82,7 @@ var giteaServiceDef = ServiceDefinition{ pullRequestURLIntoDefaultBranch: "/compare/{{.From}}", pullRequestURLIntoTargetBranch: "/compare/{{.To}}...{{.From}}", commitURL: "/commit/{{.CommitHash}}", - regexStrings: defaultUrlRegexStrings, + urlRegexps: defaultUrlRegexps, repoURLTemplate: defaultRepoURLTemplate, } @@ -89,7 +91,7 @@ var codebergServiceDef = ServiceDefinition{ pullRequestURLIntoDefaultBranch: "/compare/{{.From}}", pullRequestURLIntoTargetBranch: "/compare/{{.To}}...{{.From}}", commitURL: "/commit/{{.CommitHash}}", - regexStrings: defaultUrlRegexStrings, + urlRegexps: defaultUrlRegexps, repoURLTemplate: defaultRepoURLTemplate, } diff --git a/pkg/commands/hosting_service/hosting_service.go b/pkg/commands/hosting_service/hosting_service.go index 620d0d0a7..ff2641441 100644 --- a/pkg/commands/hosting_service/hosting_service.go +++ b/pkg/commands/hosting_service/hosting_service.go @@ -73,6 +73,42 @@ func (self *HostingServiceMgr) GetRepoName() (string, error) { return repoName, nil } +// ServiceInfo holds the resolved hosting service for a remote URL. Owner +// comes from the "owner" named regex capture, which only exists for +// owner/repo-shaped providers (github, gitlab, bitbucket, gitea, codeberg); +// it's empty for azuredevops and bitbucketServer, whose URLs are organised +// differently. Repository is populated for every provider, but RepoName may +// have more than two segments (e.g. "org/project/repo" for azuredevops). +type ServiceInfo struct { + Provider string // e.g. "github" + WebDomain string // e.g. "github.com", or "git.acme.com" for an on-prem instance + Owner string // e.g. "jesseduffield" + Repository string // e.g. "lazygit" + RepoName string // e.g. "jesseduffield/lazygit" +} + +// GetServiceInfo identifies which hosting service the configured remote URL +// belongs to and returns enough information to talk to its web/API host. +func (self *HostingServiceMgr) GetServiceInfo() (ServiceInfo, error) { + serviceDomain, err := self.getServiceDomain(self.remoteURL) + if err != nil { + return ServiceInfo{}, err + } + + matches, err := serviceDomain.serviceDefinition.parseRemoteUrl(self.remoteURL) + if err != nil { + return ServiceInfo{}, err + } + + return ServiceInfo{ + Provider: serviceDomain.serviceDefinition.provider, + WebDomain: serviceDomain.webDomain, + Owner: matches["owner"], + Repository: matches["repo"], + RepoName: utils.ResolvePlaceholderString(serviceDomain.serviceDefinition.repoNameTemplate, matches), + }, nil +} + func (self *HostingServiceMgr) getService() (*Service, error) { serviceDomain, err := self.getServiceDomain(self.remoteURL) if err != nil { @@ -159,7 +195,7 @@ type ServiceDefinition struct { pullRequestURLIntoDefaultBranch string pullRequestURLIntoTargetBranch string commitURL string - regexStrings []string + urlRegexps []*regexp.Regexp // can expect 'webdomain' to be passed in. Otherwise, you get to pick what we match in the regex repoURLTemplate string @@ -186,8 +222,7 @@ func (self ServiceDefinition) getRepoNameFromRemoteURL(url string) (string, erro } func (self ServiceDefinition) parseRemoteUrl(url string) (map[string]string, error) { - for _, regexStr := range self.regexStrings { - re := regexp.MustCompile(regexStr) + for _, re := range self.urlRegexps { matches := utils.FindNamedMatches(re, url) if matches != nil { return matches, nil @@ -206,8 +241,7 @@ type RepoInformation struct { // GetRepoInfoFromURL parses a remote URL (SSH or HTTPS) and extracts the // owner and repository name using the default URL regex patterns. func GetRepoInfoFromURL(url string) (RepoInformation, error) { - for _, regexStr := range defaultUrlRegexStrings { - re := regexp.MustCompile(regexStr) + for _, re := range defaultUrlRegexps { matches := utils.FindNamedMatches(re, url) if matches != nil { return RepoInformation{ diff --git a/pkg/commands/hosting_service/hosting_service_test.go b/pkg/commands/hosting_service/hosting_service_test.go index c2fabcd0d..f150f22eb 100644 --- a/pkg/commands/hosting_service/hosting_service_test.go +++ b/pkg/commands/hosting_service/hosting_service_test.go @@ -577,3 +577,107 @@ func TestGetPullRequestURL(t *testing.T) { }) } } + +func TestGetServiceInfo(t *testing.T) { + scenarios := []struct { + name string + remoteURL string + configServiceDomains map[string]string + expected ServiceInfo + }{ + { + name: "github.com SSH", + remoteURL: "git@github.com:jesseduffield/lazygit.git", + expected: ServiceInfo{ + Provider: "github", + WebDomain: "github.com", + Owner: "jesseduffield", + Repository: "lazygit", + RepoName: "jesseduffield/lazygit", + }, + }, + { + name: "github enterprise with same git and web host", + remoteURL: "git@github.example.com:my-org/my-repo.git", + configServiceDomains: map[string]string{ + "github.example.com": "github:github.example.com", + }, + expected: ServiceInfo{ + Provider: "github", + WebDomain: "github.example.com", + Owner: "my-org", + Repository: "my-repo", + RepoName: "my-org/my-repo", + }, + }, + { + name: "github enterprise with distinct git and web hosts", + remoteURL: "git@git.example.com:my-org/my-repo.git", + configServiceDomains: map[string]string{ + "git.example.com": "github:ghe.example.com", + }, + expected: ServiceInfo{ + Provider: "github", + WebDomain: "ghe.example.com", + Owner: "my-org", + Repository: "my-repo", + RepoName: "my-org/my-repo", + }, + }, + { + name: "github enterprise with web host port", + remoteURL: "git@git.example.com:my-org/my-repo.git", + configServiceDomains: map[string]string{ + "git.example.com": "github:ghe.example.com:8443", + }, + expected: ServiceInfo{ + Provider: "github", + WebDomain: "ghe.example.com:8443", + Owner: "my-org", + Repository: "my-repo", + RepoName: "my-org/my-repo", + }, + }, + { + // azuredevops uses org/project/repo named captures rather than + // owner/repo, so Owner is unpopulated and RepoName has three + // segments rather than the usual two. + name: "azuredevops", + remoteURL: "https://myorg@dev.azure.com/myorg/myproject/_git/myrepo", + expected: ServiceInfo{ + Provider: "azuredevops", + WebDomain: "dev.azure.com", + Repository: "myrepo", + RepoName: "myorg/myproject/myrepo", + }, + }, + { + // bitbucketServer uses project/repo named captures, so Owner is + // unpopulated and RepoName is project/repo rather than owner/repo. + name: "bitbucketServer", + remoteURL: "https://mycompany.bitbucket.com/scm/myproject/myrepo.git", + configServiceDomains: map[string]string{ + "mycompany.bitbucket.com": "bitbucketServer:mycompany.bitbucket.com", + }, + expected: ServiceInfo{ + Provider: "bitbucketServer", + WebDomain: "mycompany.bitbucket.com", + Repository: "myrepo", + RepoName: "myproject/myrepo", + }, + }, + } + + for _, s := range scenarios { + t.Run(s.name, func(t *testing.T) { + tr := i18n.EnglishTranslationSet() + log := &fakes.FakeFieldLogger{} + mgr := NewHostingServiceMgr(log, tr, s.remoteURL, s.configServiceDomains) + + info, err := mgr.GetServiceInfo() + + assert.NoError(t, err) + assert.Equal(t, s.expected, info) + }) + } +} diff --git a/pkg/config/user_config_validation.go b/pkg/config/user_config_validation.go index 23215eab6..7eeab32dd 100644 --- a/pkg/config/user_config_validation.go +++ b/pkg/config/user_config_validation.go @@ -1,6 +1,7 @@ package config import ( + "errors" "fmt" "log" "reflect" @@ -8,6 +9,8 @@ import ( "strings" "github.com/jesseduffield/lazygit/pkg/constants" + "github.com/jesseduffield/lazygit/pkg/utils" + "github.com/samber/lo" ) func (config *UserConfig) Validate() error { @@ -49,6 +52,22 @@ func (config *UserConfig) Validate() error { if err := validateCustomCommands(config.CustomCommands); err != nil { return err } + if err := validateSpinner(config.Gui.Spinner); err != nil { + return err + } + return nil +} + +func validateSpinner(spinner SpinnerConfig) error { + if len(spinner.Frames) == 0 { + return errors.New("gui.spinner.frames must not be empty.") + } + firstWidth := utils.StringWidth(spinner.Frames[0]) + if lo.SomeBy(spinner.Frames, func(frame string) bool { + return utils.StringWidth(frame) != firstWidth + }) { + return errors.New("All gui.spinner.frames entries must have the same width.") + } return nil } diff --git a/pkg/config/user_config_validation_test.go b/pkg/config/user_config_validation_test.go index 7fbfd00d4..a2841684d 100644 --- a/pkg/config/user_config_validation_test.go +++ b/pkg/config/user_config_validation_test.go @@ -289,3 +289,33 @@ func TestUserConfigValidate_enums(t *testing.T) { }) } } + +func TestUserConfigValidate_spinnerFrames(t *testing.T) { + scenarios := []struct { + name string + frames []string + valid bool + }{ + {name: "empty", frames: []string{}, valid: false}, + {name: "single frame", frames: []string{"|"}, valid: true}, + {name: "all same width", frames: []string{"|", "/", "-", "\\"}, valid: true}, + {name: "all same width, multi-char", frames: []string{". ", ".. ", "..."}, valid: true}, + {name: "all same width, wide runes", frames: []string{"⠋", "⠙", "⠹"}, valid: true}, + {name: "differing widths", frames: []string{"|", "//"}, valid: false}, + {name: "first differs from rest", frames: []string{"||", "/", "-"}, valid: false}, + } + + for _, s := range scenarios { + t.Run(s.name, func(t *testing.T) { + config := GetDefaultConfig() + config.Gui.Spinner.Frames = s.frames + err := config.Validate() + + if s.valid { + assert.NoError(t, err) + } else { + assert.Error(t, err) + } + }) + } +} diff --git a/pkg/gocui/flush_test.go b/pkg/gocui/flush_test.go new file mode 100644 index 000000000..59bae427c --- /dev/null +++ b/pkg/gocui/flush_test.go @@ -0,0 +1,292 @@ +package gocui + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +func newTestGui(t *testing.T) *Gui { + t.Helper() + g, err := NewGui(NewGuiOpts{ + OutputMode: OutputNormal, + Headless: true, + Width: 80, + Height: 24, + }) + assert.NoError(t, err) + t.Cleanup(func() { g.Close() }) + return g +} + +// setupViews creates a few views and does an initial full flush so all views +// start in a clean (non-tainted) state. +func setupViews(t *testing.T, g *Gui) (*View, *View) { + t.Helper() + + status, _ := g.SetView("status", 0, 22, 40, 24, 0) + status.Frame = false + main, _ := g.SetView("main", 0, 0, 80, 22, 0) + + // Initial content + status.SetContent("Ready") + main.SetContent("hello world") + + // Full flush to draw everything and clear tainted flags + assert.NoError(t, g.flush()) + + return status, main +} + +// pushContentOnly pushes a content-only event directly to the channel +// (synchronous, deterministic — unlike Update which spawns a goroutine). +func pushContentOnly(g *Gui, f func(*Gui) error) { + g.userEvents <- userEvent{f: f, task: g.NewTask(), contentOnly: true} +} + +// pushRegular pushes a regular event directly to the channel. +func pushRegular(g *Gui, f func(*Gui) error) { + g.userEvents <- userEvent{f: f, task: g.NewTask(), contentOnly: false} +} + +func TestFlushContentOnly_SkipsUntaintedViews(t *testing.T) { + g := newTestGui(t) + status, main := setupViews(t, g) + + // After initial flush, both views should be untainted + assert.False(t, status.IsTainted(), "status view should not be tainted after flush") + assert.False(t, main.IsTainted(), "main view should not be tainted after flush") + + // Modify only the status view + status.SetContent("Fetching /") + + assert.True(t, status.IsTainted(), "status view should be tainted after SetContent") + assert.False(t, main.IsTainted(), "main view should not be tainted (was not modified)") + + // flushContentOnly should succeed and clear status tainted flag + assert.NoError(t, g.flushContentOnly(g.views)) + + assert.False(t, status.IsTainted(), "status view should not be tainted after flushContentOnly") + assert.False(t, main.IsTainted(), "main view should not be tainted after flushContentOnly") +} + +func TestFlushContentOnly_WritesCorrectContent(t *testing.T) { + g := newTestGui(t) + status, _ := setupViews(t, g) + + status.SetContent("Fetching |") + assert.NoError(t, g.flushContentOnly(g.views)) + + assert.Equal(t, "Fetching |", status.Buffer()) +} + +func TestProcessEvent_ContentOnlyEvent_SkipsTaintedCheck(t *testing.T) { + g := newTestGui(t) + status, main := setupViews(t, g) + + // Send a content-only event that modifies only the status view + pushContentOnly(g, func(gui *Gui) error { + status.SetContent("Fetching /") + return nil + }) + + assert.NoError(t, g.processEvent()) + + // status was modified and drawn → tainted cleared + assert.False(t, status.IsTainted(), "status should not be tainted after processEvent with contentOnly") + // main was NOT modified → should still be untainted + assert.False(t, main.IsTainted(), "main should not be tainted after processEvent with contentOnly") +} + +func TestProcessEvent_RegularEvent_UsesFullFlush(t *testing.T) { + g := newTestGui(t) + status, _ := setupViews(t, g) + + // Regular event (not content-only) should trigger full flush + pushRegular(g, func(gui *Gui) error { + status.SetContent("Fetching \\") + return nil + }) + + assert.NoError(t, g.processEvent()) + + assert.False(t, status.IsTainted(), "status should not be tainted after full flush") +} + +func TestProcessEvent_MixedBatch_UsesFullFlush(t *testing.T) { + g := newTestGui(t) + status, main := setupViews(t, g) + + // Queue a content-only event followed by a regular event. + // processEvent picks up the first; processRemainingEvents picks up + // the second. Since the second is not contentOnly, full flush runs. + pushContentOnly(g, func(gui *Gui) error { + status.SetContent("Fetching -") + return nil + }) + pushRegular(g, func(gui *Gui) error { + main.SetContent("updated main") + return nil + }) + + assert.NoError(t, g.processEvent()) + + // Both views were modified and should have been drawn by full flush + assert.False(t, status.IsTainted(), "status should not be tainted after full flush") + assert.False(t, main.IsTainted(), "main should not be tainted after full flush") +} + +func TestProcessEvent_RegularThenContentOnly_UsesFullFlush(t *testing.T) { + g := newTestGui(t) + status, main := setupViews(t, g) + + // Even if a regular event comes first and the remaining are contentOnly, + // the batch must use full flush. + pushRegular(g, func(gui *Gui) error { + main.SetContent("new main content") + return nil + }) + pushContentOnly(g, func(gui *Gui) error { + status.SetContent("Fetching |") + return nil + }) + + assert.NoError(t, g.processEvent()) + + assert.False(t, status.IsTainted(), "status should not be tainted after full flush") + assert.False(t, main.IsTainted(), "main should not be tainted after full flush") +} + +func TestProcessRemainingEvents_AllContentOnly_ReturnsTrue(t *testing.T) { + g := newTestGui(t) + status, _ := setupViews(t, g) + + pushContentOnly(g, func(gui *Gui) error { + status.SetContent("a") + return nil + }) + pushContentOnly(g, func(gui *Gui) error { + status.SetContent("b") + return nil + }) + + contentOnly, err := g.processRemainingEvents() + assert.NoError(t, err) + assert.True(t, contentOnly, "should return true when all events are contentOnly") +} + +func TestProcessRemainingEvents_MixedEvents_ReturnsFalse(t *testing.T) { + g := newTestGui(t) + status, _ := setupViews(t, g) + + pushContentOnly(g, func(gui *Gui) error { + status.SetContent("a") + return nil + }) + pushRegular(g, func(gui *Gui) error { + status.SetContent("b") + return nil + }) + + contentOnly, err := g.processRemainingEvents() + assert.NoError(t, err) + assert.False(t, contentOnly, "should return false when any event is not contentOnly") +} + +func TestProcessRemainingEvents_EmptyQueue_ReturnsTrue(t *testing.T) { + g := newTestGui(t) + + contentOnly, err := g.processRemainingEvents() + assert.NoError(t, err) + assert.True(t, contentOnly, "should return true when no events are queued") +} + +// Ensure an overlapping view that is not tainted does not get overdrawn +func TestFlushContentOnly_DoesNotOverdrawHigherZViews(t *testing.T) { + g := newTestGui(t) + + // Base view + list, _ := g.SetView("list", 0, 0, 79, 23, 0) + list.Frame = false + list.SetContent(strings.Repeat("LIST LINE FILLER FILLER FILLER FILLER FILLER FILLER FILLER FILLER FILLER\n", 22)) + + // Overlapping 'popup' + popup, _ := g.SetView("popup", 20, 8, 60, 16, 0) + popup.Frame = false + popupLine := strings.Repeat("P", 60) + popup.SetContent(strings.Repeat(popupLine+"\n", 16)) + + // Full flush — popup ends up on top. + assert.NoError(t, g.flush()) + + cellAt := func(x, y int) string { + s, _, _ := g.screen.Get(x, y) + return s + } + + // Taint only the list view + list.SetContent(strings.Repeat(strings.Repeat("X", 80)+"\n", 22)) + assert.True(t, list.IsTainted(), "list should be tainted after SetContent") + assert.False(t, popup.IsTainted(), "popup should not be tainted") + + // flushContentOnly is what spinner ticks ultimately invoke. + assert.NoError(t, g.flushContentOnly(g.views)) + + assert.Equal(t, "P", cellAt(21, 9), + "popup region must still show popup content after flushContentOnly; "+ + "if this fails the popup-overdraw bug is present") + + // Additional checks to be sure + assert.Equal(t, "P", cellAt(40, 11), "interior popup cell should still show popup content") + assert.Equal(t, "P", cellAt(58, 14), "near-edge popup cell should still show popup content") + + // Ensure tainted view was updated + assert.Equal(t, "X", cellAt(5, 5), "list cell outside popup should show new list content") + assert.Equal(t, "X", cellAt(70, 20), "list cell outside popup should show new list content") +} + +// Ensure transitive overlap: with views in z-order [a, b, c] where b overlaps a +// and c overlaps b but c does NOT overlap a, tainting a must redraw all three — +// otherwise b's redraw paints over c. +func TestFlushContentOnly_RedrawsTransitivelyOverlappingViews(t *testing.T) { + g := newTestGui(t) + + // Geometry: b straddles a and c; a and c are disjoint. + // a: (0,0)-(40,10) b: (30,5)-(60,15) c: (50,12)-(75,20) + a, _ := g.SetView("a", 0, 0, 40, 10, 0) + a.Frame = false + a.SetContent(strings.Repeat(strings.Repeat("A", 60)+"\n", 20)) + + b, _ := g.SetView("b", 30, 5, 60, 15, 0) + b.Frame = false + b.SetContent(strings.Repeat(strings.Repeat("B", 60)+"\n", 20)) + + c, _ := g.SetView("c", 50, 12, 75, 20, 0) + c.Frame = false + c.SetContent(strings.Repeat(strings.Repeat("C", 60)+"\n", 20)) + + assert.NoError(t, g.flush()) + + cellAt := func(x, y int) string { + s, _, _ := g.screen.Get(x, y) + return s + } + + // Taint only a. + a.SetContent(strings.Repeat(strings.Repeat("X", 60)+"\n", 20)) + assert.True(t, a.IsTainted()) + assert.False(t, b.IsTainted()) + assert.False(t, c.IsTainted()) + + assert.NoError(t, g.flushContentOnly(g.views)) + + // a redrawn (direct). + assert.Equal(t, "X", cellAt(5, 5), "a should be redrawn (tainted)") + // b redrawn (overlaps a). + assert.Equal(t, "B", cellAt(45, 7), "b should be redrawn (overlaps a)") + // c redrawn transitively (overlaps b, which overlaps a). Without the + // transitive case, b's redraw would paint over c at this cell. + assert.Equal(t, "C", cellAt(55, 14), + "c should be redrawn transitively; if 'B' here, b's redraw painted over c") +} diff --git a/pkg/gocui/gui.go b/pkg/gocui/gui.go index dded5dd4a..645329d6b 100644 --- a/pkg/gocui/gui.go +++ b/pkg/gocui/gui.go @@ -13,7 +13,9 @@ import ( "github.com/gdamore/tcell/v3" "github.com/go-errors/errors" + "github.com/jesseduffield/generics/set" "github.com/rivo/uniseg" + "github.com/samber/lo" ) // OutputMode represents an output mode, which determines how colors @@ -604,6 +606,10 @@ func (g *Gui) SetRenderSearchStatusFunc(renderSearchStatusFunc func(*View, int, type userEvent struct { f func(*Gui) error task Task + // Signals that this event only modifies view content (e.g. SetContent). + // When all events in a batch are contentOnly, processEvent + // can skip the expensive layout() call in flush(). + contentOnly bool } // Update executes the passed function. This method can be called safely from a @@ -630,6 +636,12 @@ func (g *Gui) updateAsyncAux(f func(*Gui) error, task Task) { g.userEvents <- userEvent{f: f, task: task} } +// Like Update, but signals that the callback only modifies content. +func (g *Gui) UpdateContentOnly(f func(*Gui) error) { + task := g.NewTask() + g.userEvents <- userEvent{f: f, task: task, contentOnly: true} +} + // Calls a function in a goroutine. Handles panics gracefully and tracks // number of background tasks. // Always use this when you want to spawn a goroutine and you want lazygit to @@ -743,6 +755,8 @@ func (g *Gui) handleError(err error) error { } func (g *Gui) processEvent() error { + contentOnly := false + select { case ev := <-g.gEvents: task := g.NewTask() @@ -752,6 +766,7 @@ func (g *Gui) processEvent() error { return err } case ev := <-g.userEvents: + contentOnly = ev.contentOnly defer func() { ev.task.Done() }() if err := g.handleError(ev.f(g)); err != nil { @@ -759,32 +774,38 @@ func (g *Gui) processEvent() error { } } - if err := g.processRemainingEvents(); err != nil { - return err - } - if err := g.flush(); err != nil { + remainingContentOnly, err := g.processRemainingEvents() + if err != nil { return err } + contentOnly = contentOnly && remainingContentOnly - return nil + if contentOnly { + return g.flushContentOnly(g.views) + } + return g.flush() } // processRemainingEvents handles the remaining events in the events pool. -func (g *Gui) processRemainingEvents() error { +// Returns true if all processed events were content-only. +func (g *Gui) processRemainingEvents() (bool, error) { + contentOnly := true for { select { case ev := <-g.gEvents: + contentOnly = false if err := g.handleError(g.handleEvent(&ev)); err != nil { - return err + return false, err } case ev := <-g.userEvents: + contentOnly = ev.contentOnly && contentOnly err := g.handleError(ev.f(g)) ev.task.Done() if err != nil { - return err + return false, err } default: - return nil + return contentOnly, nil } } } @@ -1148,27 +1169,62 @@ func (g *Gui) flush() error { return nil } -func (g *Gui) ForceLayoutAndRedraw() error { - return g.flush() -} - -// force redrawing one or more views outside of the normal main loop. Useful during longer -// operations that block the main thread, to update a spinner in a status view. -func (g *Gui) ForceRedrawViews(views ...*View) error { - for _, m := range g.managers { - if err := m.Layout(g); err != nil { +// Redraws only tainted views and skips the layout pass. +// tcell's cell-level dirty tracking ensures only +// actually-changed cells are emitted to the terminal. +// Will also redraw any views that overlap tainted views +func (g *Gui) flushContentOnly(views []*View) error { + for _, v := range viewsToRedrawContentOnly(views) { + if err := g.draw(v); err != nil { return err } } - for _, v := range views { - v.draw() - } - Screen.Show() return nil } +func viewsToRedrawContentOnly(views []*View) []*View { + redrawIndexes := set.New[int]() + + for i, v := range views { + if !v.tainted && !redrawIndexes.Includes(i) { + continue + } + + redrawIndexes.Add(i) + + for j, above := range views[i+1:] { + aboveIndex := i + 1 + j + if !redrawIndexes.Includes(aboveIndex) && rectsOverlap(v, above) { + redrawIndexes.Add(aboveIndex) + } + } + } + + return lo.FilterMap(views, func(view *View, i int) (*View, bool) { + return view, redrawIndexes.Includes(i) + }) +} + +// Reports whether two views' rectangles share at least one cell. +func rectsOverlap(a, b *View) bool { + ax0, ay0, ax1, ay1 := a.Dimensions() + bx0, by0, bx1, by1 := b.Dimensions() + return ax0 <= bx1 && ax1 >= bx0 && ay0 <= by1 && ay1 >= by0 +} + +func (g *Gui) ForceLayoutAndRedraw() error { + return g.flush() +} + +// Redraws only tainted views outside of the normal main +// loop, without a layout pass. Useful during longer operations that block the +// main thread, e.g. to update a spinner in a status view. +func (g *Gui) ForceFlushViewsContentOnly(views []*View) error { + return g.flushContentOnly(views) +} + // draw manages the cursor and calls the draw function of a view. func (g *Gui) draw(v *View) error { if g.suspended { diff --git a/pkg/gui/context/list_context_trait.go b/pkg/gui/context/list_context_trait.go index 98833fdb2..597fc99df 100644 --- a/pkg/gui/context/list_context_trait.go +++ b/pkg/gui/context/list_context_trait.go @@ -124,7 +124,6 @@ func (self *ListContextTrait) HandleRender() { content := self.renderLines(-1, -1) self.GetViewTrait().SetContent(content) } - self.c.Render() self.setFooter() } diff --git a/pkg/gui/context/suggestions_context.go b/pkg/gui/context/suggestions_context.go index eafe7fb7c..fb69b34d9 100644 --- a/pkg/gui/context/suggestions_context.go +++ b/pkg/gui/context/suggestions_context.go @@ -67,10 +67,17 @@ func NewSuggestionsContext( } func (self *SuggestionsContext) SetSuggestions(suggestions []*types.Suggestion) { - self.State.Suggestions = suggestions - self.SetSelection(0) - self.c.ResetViewOrigin(self.GetView()) - self.HandleRender() + // SetSuggestions is invoked from AsyncHandler (a worker goroutine) when + // the prompt input changes, as well as from prepareConfirmationPanel on + // the UI thread. Bounce to the UI thread either way so the worker path + // keeps flushing once HandleRender stops calling Render() itself. + self.c.OnUIThread(func() error { + self.State.Suggestions = suggestions + self.SetSelection(0) + self.c.ResetViewOrigin(self.GetView()) + self.HandleRender() + return nil + }) } func (self *SuggestionsContext) RefreshSuggestions() { diff --git a/pkg/gui/controllers/commits_files_controller.go b/pkg/gui/controllers/commits_files_controller.go index 7cde6c15d..b12819c39 100644 --- a/pkg/gui/controllers/commits_files_controller.go +++ b/pkg/gui/controllers/commits_files_controller.go @@ -476,7 +476,11 @@ func (self *CommitFilesController) toggleForPatch(selectedNodes []*filetree.Comm self.c.Git().Patch.PatchBuilder.Reset() } - self.c.PostRefreshUpdate(self.context()) + self.c.OnUIThread(func() error { + self.c.PostRefreshUpdate(self.context()) + return nil + }) + return nil }) } diff --git a/pkg/gui/controllers/helpers/app_status_helper.go b/pkg/gui/controllers/helpers/app_status_helper.go index 89de221b8..fa402962c 100644 --- a/pkg/gui/controllers/helpers/app_status_helper.go +++ b/pkg/gui/controllers/helpers/app_status_helper.go @@ -6,6 +6,7 @@ import ( "github.com/jesseduffield/lazygit/pkg/gocui" "github.com/jesseduffield/lazygit/pkg/gui/status" "github.com/jesseduffield/lazygit/pkg/gui/types" + "github.com/jesseduffield/lazygit/pkg/utils" ) type AppStatusHelper struct { @@ -93,13 +94,24 @@ func (self *AppStatusHelper) renderAppStatus() { self.c.OnWorker(func(_ gocui.Task) error { ticker := time.NewTicker(time.Millisecond * time.Duration(self.c.UserConfig().Gui.Spinner.Rate)) defer ticker.Stop() + prevAppStatus := "" for range ticker.C { appStatus, color := self.statusMgr().GetStatusString(self.c.UserConfig()) - self.c.Views().AppStatus.FgColor = color - self.c.OnUIThread(func() error { + + update := self.c.OnUIThreadContentOnly + if utils.StringWidth(appStatus) != utils.StringWidth(prevAppStatus) { + // Need a full layout whenever the width of the status string changes. This can't + // happen during normal spinning because we validate that all spinner frames have + // the same width, so typically this will only be triggered at the beginning and end + // of a status, or if the status string changes midway for some reason. + update = self.c.OnUIThread + } + update(func() error { + self.c.Views().AppStatus.FgColor = color self.c.SetViewContent(self.c.Views().AppStatus, appStatus) return nil }) + prevAppStatus = appStatus if appStatus == "" { break @@ -111,7 +123,7 @@ func (self *AppStatusHelper) renderAppStatus() { func (self *AppStatusHelper) renderAppStatusSync(stop chan struct{}) { go func() { - ticker := time.NewTicker(time.Millisecond * 50) + ticker := time.NewTicker(time.Millisecond * time.Duration(self.c.UserConfig().Gui.Spinner.Rate)) defer ticker.Stop() // Forcing a re-layout and redraw after we added the waiting status; @@ -136,7 +148,7 @@ func (self *AppStatusHelper) renderAppStatusSync(stop chan struct{}) { self.c.Views().AppStatus, self.c.Views().Options, self.c.Views().Information, self.c.Views().StatusSpacer1, self.c.Views().StatusSpacer2, } - _ = self.c.GocuiGui().ForceRedrawViews(bottomLineViews...) + _ = self.c.GocuiGui().ForceFlushViewsContentOnly(bottomLineViews) case <-stop: break outer } diff --git a/pkg/gui/controllers/helpers/cherry_pick_helper.go b/pkg/gui/controllers/helpers/cherry_pick_helper.go index 359d1cbc2..079fdedcf 100644 --- a/pkg/gui/controllers/helpers/cherry_pick_helper.go +++ b/pkg/gui/controllers/helpers/cherry_pick_helper.go @@ -40,6 +40,14 @@ func (self *CherryPickHelper) CopyRange(commitsList []*models.Commit, context ty return err } + // After a paste the buffer is hidden but not cleared, so the user + // thinks they're starting fresh. Clear it before adding so the new + // copy replaces the old one. + if self.getData().DidPaste { + self.getData().CherryPickedCommits = nil + self.getData().DidPaste = false + } + commitSet := self.getData().SelectedHashSet() allCommitsCopied := lo.EveryBy(commitsList[startIdx:endIdx+1], func(commit *models.Commit) bool { @@ -59,8 +67,6 @@ func (self *CherryPickHelper) CopyRange(commitsList []*models.Commit, context ty } } - self.getData().DidPaste = false - self.rerender() return nil } diff --git a/pkg/gui/controllers/helpers/inline_status_helper.go b/pkg/gui/controllers/helpers/inline_status_helper.go index 13c112811..02afcdd50 100644 --- a/pkg/gui/controllers/helpers/inline_status_helper.go +++ b/pkg/gui/controllers/helpers/inline_status_helper.go @@ -149,7 +149,7 @@ func (self *InlineStatusHelper) stop(opts InlineStatusOpts) { } func (self *InlineStatusHelper) renderContext(contextKey types.ContextKey) { - self.c.OnUIThread(func() error { + self.c.OnUIThreadContentOnly(func() error { self.c.ContextForKey(contextKey).HandleRender() return nil }) diff --git a/pkg/gui/controllers/helpers/refresh_helper.go b/pkg/gui/controllers/helpers/refresh_helper.go index 4a61bde18..2922b4403 100644 --- a/pkg/gui/controllers/helpers/refresh_helper.go +++ b/pkg/gui/controllers/helpers/refresh_helper.go @@ -1,13 +1,13 @@ package helpers import ( - "fmt" "strings" "sync" "time" "github.com/jesseduffield/generics/set" "github.com/jesseduffield/lazygit/pkg/commands/git_commands" + "github.com/jesseduffield/lazygit/pkg/commands/hosting_service" "github.com/jesseduffield/lazygit/pkg/commands/models" "github.com/jesseduffield/lazygit/pkg/config" "github.com/jesseduffield/lazygit/pkg/gocui" @@ -532,9 +532,12 @@ func (self *RefreshHelper) refreshBranches(refreshWorktrees bool, keepBranchSele // Need to re-render the commits view because the visualization of local // branch heads might have changed - self.c.Mutexes().LocalCommitsMutex.Lock() - self.c.Contexts().LocalCommits.HandleRender() - self.c.Mutexes().LocalCommitsMutex.Unlock() + self.c.OnUIThread(func() error { + self.c.Mutexes().LocalCommitsMutex.Lock() + self.c.Contexts().LocalCommits.HandleRender() + self.c.Mutexes().LocalCommitsMutex.Unlock() + return nil + }) self.refreshStatus() } @@ -780,22 +783,27 @@ func (self *RefreshHelper) refForLog() string { } func (self *RefreshHelper) refreshView(context types.Context) { - // Re-applying the filter must be done before re-rendering the view, so that - // the filtered list model is up to date for rendering. - self.searchHelper.ReApplyFilter(context) + // refreshView is called from the worker goroutine that drives async + // refreshes, so bounce to the UI thread before mutating view content. + self.c.OnUIThread(func() error { + // Re-applying the filter must be done before re-rendering the view, so that + // the filtered list model is up to date for rendering. + self.searchHelper.ReApplyFilter(context) - self.c.PostRefreshUpdate(context) + self.c.PostRefreshUpdate(context) - self.c.AfterLayout(func() error { - // Re-applying the search must be done after re-rendering the view though, - // so that the "x of y" status is shown correctly. - // - // Also, it must be done after layout, because otherwise FocusPoint - // hasn't been called yet (see ListContextTrait.FocusLine), which means - // that the scroll position might be such that the entire visible - // content is outside the viewport. And this would cause problems in - // searchModelCommits. - self.searchHelper.ReApplySearch(context) + self.c.AfterLayout(func() error { + // Re-applying the search must be done after re-rendering the view though, + // so that the "x of y" status is shown correctly. + // + // Also, it must be done after layout, because otherwise FocusPoint + // hasn't been called yet (see ListContextTrait.FocusLine), which means + // that the scroll position might be such that the entire visible + // content is outside the viewport. And this would cause problems in + // searchModelCommits. + self.searchHelper.ReApplySearch(context) + return nil + }) return nil }) } @@ -804,39 +812,31 @@ func (self *RefreshHelper) refreshGithubPullRequests() { self.c.Mutexes().RefreshingPullRequestsMutex.Lock() defer self.c.Mutexes().RefreshingPullRequestsMutex.Unlock() - if !self.c.Git().GitHub.InGithubRepo(self.c.Model().Remotes) { + githubRemotes := getAuthenticatedGithubRemotes(self.getGithubRemotes(), self.c.Git().GitHub.GetAuthToken) + if len(githubRemotes) == 0 { self.c.Model().PullRequests = nil self.c.Model().PullRequestsMap = nil return } - authToken := self.c.Git().GitHub.GetAuthToken() - if authToken == "" { - self.c.Model().PullRequests = nil - self.c.Model().PullRequestsMap = nil - return - } - - githubRemotes := self.getGithubRemotes() - baseRemote := getGithubBaseRemote(githubRemotes, self.c.Git().GitHub.ConfiguredBaseRemoteName()) - if baseRemote == nil { + baseInfo := getGithubBaseRemote(githubRemotes, self.c.Git().GitHub.ConfiguredBaseRemoteName()) + if baseInfo == nil { self.c.Model().PullRequests = nil self.c.Model().PullRequestsMap = nil - if len(githubRemotes) > 0 && !self.githubBaseRemotePromptDismissed[self.c.Git().RepoPaths.RepoPath()] { - self.promptForBaseGithubRepo(authToken, githubRemotes) + if !self.githubBaseRemotePromptDismissed[self.c.Git().RepoPaths.RepoPath()] { + self.promptForBaseGithubRepo(githubRemotes) } return } - if err := self.setGithubPullRequests(authToken, baseRemote); err != nil { - self.c.LogAction(fmt.Sprintf("Error fetching pull requests from GitHub: %s", err.Error())) - } + self.setGithubPullRequests(baseInfo) } type githubRemoteInfo struct { - remote *models.Remote - repoName string + remote *models.Remote + serviceInfo hosting_service.ServiceInfo + authToken string } func (self *RefreshHelper) getGithubRemotes() []githubRemoteInfo { @@ -844,23 +844,44 @@ func (self *RefreshHelper) getGithubRemotes() []githubRemoteInfo { if len(remote.Urls) == 0 { return githubRemoteInfo{}, false } - repoName, err := self.c.Git().HostingService.GetRepoNameFromRemoteURL(remote.Urls[0]) - if err != nil { + serviceInfo, err := self.c.Git().HostingService.GetServiceInfo(remote.Urls[0]) + if err != nil || serviceInfo.Provider != "github" { return githubRemoteInfo{}, false } - return githubRemoteInfo{remote: remote, repoName: repoName}, true + return githubRemoteInfo{remote: remote, serviceInfo: serviceInfo}, true }) } -func getGithubBaseRemote(githubRemotes []githubRemoteInfo, configuredRemoteName string) *models.Remote { - findRemoteByName := func(name string) *models.Remote { +// getAuthenticatedGithubRemotes drops remotes for which no auth token is +// available and attaches the resolved token to the rest. Token lookups are +// cached by host so that multiple remotes pointing at the same instance +// (e.g. origin + a fork on github.com) only trigger one lookup. +func getAuthenticatedGithubRemotes(githubRemotes []githubRemoteInfo, getAuthToken func(host string) string) []githubRemoteInfo { + tokensByHost := map[string]string{} + return lo.FilterMap(githubRemotes, func(info githubRemoteInfo, _ int) (githubRemoteInfo, bool) { + host := info.serviceInfo.WebDomain + token, cached := tokensByHost[host] + if !cached { + token = getAuthToken(host) + tokensByHost[host] = token + } + if token == "" { + return githubRemoteInfo{}, false + } + info.authToken = token + return info, true + }) +} + +func getGithubBaseRemote(githubRemotes []githubRemoteInfo, configuredRemoteName string) *githubRemoteInfo { + findRemoteByName := func(name string) *githubRemoteInfo { info, ok := lo.Find(githubRemotes, func(info githubRemoteInfo) bool { return info.remote.Name == name }) if !ok { return nil } - return info.remote + return &info } if configuredRemoteName != "" { @@ -868,31 +889,29 @@ func getGithubBaseRemote(githubRemotes []githubRemoteInfo, configuredRemoteName } if len(githubRemotes) == 1 { - return githubRemotes[0].remote + return &githubRemotes[0] } // Not sure if "upstream" is really a common convention for the name of the remote that PRs are // made against, but if it exists it's pretty likely to be the one we want. - if remote := findRemoteByName("upstream"); remote != nil { - return remote + if info := findRemoteByName("upstream"); info != nil { + return info } return nil } -func (self *RefreshHelper) promptForBaseGithubRepo(authToken string, githubRemotes []githubRemoteInfo) { +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.repoName)}, + LabelColumns: []string{info.remote.Name, style.FgCyan.Sprint(info.serviceInfo.RepoName)}, OnPress: func() error { return self.c.WithWaitingStatus(self.c.Tr.FetchingPullRequests, func(gocui.Task) error { if err := self.c.Git().GitHub.SetConfiguredBaseRemoteName(info.remote.Name); err != nil { self.c.Log.Error(err) } - if err := self.setGithubPullRequests(authToken, info.remote); err != nil { - self.c.LogAction(fmt.Sprintf("Error fetching pull requests from GitHub: %s", err.Error())) - } + self.setGithubPullRequests(&info) return nil }) }, @@ -920,9 +939,9 @@ func (self *RefreshHelper) rebuildPullRequestsMap() { ) } -func (self *RefreshHelper) setGithubPullRequests(authToken string, baseRemote *models.Remote) error { +func (self *RefreshHelper) setGithubPullRequests(baseInfo *githubRemoteInfo) { if len(self.c.Model().Branches) == 0 { - return nil + return } branches := lo.Filter(self.c.Model().Branches, func(branch *models.Branch, _ int) bool { @@ -932,17 +951,20 @@ func (self *RefreshHelper) setGithubPullRequests(authToken string, baseRemote *m return branch.UpstreamBranch }) - prs, err := self.c.Git().GitHub.FetchRecentPRs(branchNames, baseRemote, authToken) + prs, err := self.c.Git().GitHub.FetchRecentPRs(branchNames, &baseInfo.serviceInfo, baseInfo.authToken) if err != nil { - return err + self.c.Log.Error("error fetching pull requests from GitHub: " + err.Error()) + return } self.c.Model().PullRequests = prs self.savePullRequestsToCache(prs) self.rebuildPullRequestsMap() - self.c.PostRefreshUpdate(self.c.Contexts().Branches) - return nil + self.c.OnUIThread(func() error { + self.c.PostRefreshUpdate(self.c.Contexts().Branches) + return nil + }) } func (self *RefreshHelper) savePullRequestsToCache(prs []*models.GithubPullRequest) { diff --git a/pkg/gui/controllers/helpers/refresh_helper_test.go b/pkg/gui/controllers/helpers/refresh_helper_test.go index dea3b8b81..cebd044c4 100644 --- a/pkg/gui/controllers/helpers/refresh_helper_test.go +++ b/pkg/gui/controllers/helpers/refresh_helper_test.go @@ -3,6 +3,7 @@ package helpers import ( "testing" + "github.com/jesseduffield/lazygit/pkg/commands/hosting_service" "github.com/jesseduffield/lazygit/pkg/commands/models" "github.com/samber/lo" "github.com/stretchr/testify/assert" @@ -60,14 +61,64 @@ func TestGetGithubBaseRemote(t *testing.T) { assert.Nil(t, result) } else { assert.NotNil(t, result) - assert.Equal(t, c.expected, result.Name) + assert.Equal(t, c.expected, result.remote.Name) } }) } } +func TestGetAuthenticatedGithubRemotes(t *testing.T) { + githubRemotes := []githubRemoteInfo{ + makeGithubRemoteInfo("origin", "github.com"), + makeGithubRemoteInfo("fork", "github.com"), + makeGithubRemoteInfo("enterprise", "ghe.example.com"), + makeGithubRemoteInfo("missing-auth", "no-token.example.com"), + } + + callsByHost := map[string]int{} + result := getAuthenticatedGithubRemotes(githubRemotes, func(host string) string { + callsByHost[host]++ + switch host { + case "github.com": + return "github-token" + case "ghe.example.com": + return "ghe-token" + default: + return "" + } + }) + + assert.Equal(t, []githubRemoteInfo{ + makeAuthenticatedGithubRemoteInfo("origin", "github.com", "github-token"), + makeAuthenticatedGithubRemoteInfo("fork", "github.com", "github-token"), + makeAuthenticatedGithubRemoteInfo("enterprise", "ghe.example.com", "ghe-token"), + }, result) + // Two remotes share github.com; the lookup runs only once. + assert.Equal(t, map[string]int{ + "github.com": 1, + "ghe.example.com": 1, + "no-token.example.com": 1, + }, callsByHost) +} + func makeGithubRemoteInfoList(names ...string) []githubRemoteInfo { return lo.Map(names, func(name string, _ int) githubRemoteInfo { - return githubRemoteInfo{remote: &models.Remote{Name: name}, repoName: name} + return makeGithubRemoteInfo(name, name) }) } + +func makeGithubRemoteInfo(name string, webDomain string) githubRemoteInfo { + return githubRemoteInfo{ + remote: &models.Remote{Name: name}, + serviceInfo: hosting_service.ServiceInfo{ + RepoName: name, + WebDomain: webDomain, + }, + } +} + +func makeAuthenticatedGithubRemoteInfo(name string, webDomain string, authToken string) githubRemoteInfo { + info := makeGithubRemoteInfo(name, webDomain) + info.authToken = authToken + return info +} diff --git a/pkg/gui/gui.go b/pkg/gui/gui.go index d5104e4f4..c1af24d9f 100644 --- a/pkg/gui/gui.go +++ b/pkg/gui/gui.go @@ -428,6 +428,8 @@ func (gui *Gui) onNewRepo(startArgs appTypes.StartArgs, contextKey types.Context gui.c.Context().Push(contextToPush, types.OnFocusOpts{}) + gui.render() + return nil } @@ -1202,6 +1204,12 @@ func (gui *Gui) onUIThread(f func() error) { }) } +func (gui *Gui) onUIThreadContentOnly(f func() error) { + gui.g.UpdateContentOnly(func(*gocui.Gui) error { + return f() + }) +} + func (gui *Gui) onWorker(f func(gocui.Task) error) { gui.g.OnWorker(f) } diff --git a/pkg/gui/gui_common.go b/pkg/gui/gui_common.go index 13f49c46b..07945c350 100644 --- a/pkg/gui/gui_common.go +++ b/pkg/gui/gui_common.go @@ -120,6 +120,10 @@ func (self *guiCommon) OnUIThread(f func() error) { self.gui.onUIThread(f) } +func (self *guiCommon) OnUIThreadContentOnly(f func() error) { + self.gui.onUIThreadContentOnly(f) +} + func (self *guiCommon) OnWorker(f func(gocui.Task) error) { self.gui.onWorker(f) } diff --git a/pkg/gui/layout.go b/pkg/gui/layout.go index 21d83ca04..dacd93f68 100644 --- a/pkg/gui/layout.go +++ b/pkg/gui/layout.go @@ -5,6 +5,7 @@ import ( "github.com/jesseduffield/lazygit/pkg/gocui" "github.com/jesseduffield/lazygit/pkg/gui/types" + "github.com/jesseduffield/lazygit/pkg/utils" "github.com/samber/lo" ) @@ -23,7 +24,11 @@ func (gui *Gui) layout(g *gocui.Gui) error { informationStr := gui.informationStr() - appStatus := gui.helpers.AppStatus.GetStatusString() + var appStatus string + appStatusView, err := g.View("appStatus") + if err == nil { + appStatus = utils.Decolorise(appStatusView.Buffer()) + } viewDimensions := gui.getWindowDimensions(informationStr, appStatus) diff --git a/pkg/gui/types/common.go b/pkg/gui/types/common.go index 0afbd5ed8..5856bb5e0 100644 --- a/pkg/gui/types/common.go +++ b/pkg/gui/types/common.go @@ -71,6 +71,10 @@ type IGuiCommon interface { // Only necessary to call if you're not already on the UI thread i.e. you're inside a goroutine. // All controller handlers are executed on the UI thread. OnUIThread(f func() error) + // Like OnUIThread, but signals that the callback only modifies view + // content (e.g. spinner), allows the event loop to skip + // the expensive layout recalculation when only content changed. + OnUIThreadContentOnly(f func() error) // Runs a function in a goroutine. Use this whenever you want to run a goroutine and keep track of the fact // that lazygit is still busy. See docs/dev/Busy.md OnWorker(f func(gocui.Task) error) diff --git a/pkg/integration/tests/cherry_pick/cherry_pick_range_after_paste.go b/pkg/integration/tests/cherry_pick/cherry_pick_range_after_paste.go new file mode 100644 index 000000000..4f1cf180a --- /dev/null +++ b/pkg/integration/tests/cherry_pick/cherry_pick_range_after_paste.go @@ -0,0 +1,106 @@ +package cherry_pick + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var CherryPickRangeAfterPaste = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Regression test: range-copy multiple commits after a previous paste", + ExtraCmdArgs: []string{}, + Skip: false, + SetupConfig: func(config *config.AppConfig) { + config.GetUserConfig().Git.LocalBranchSortOrder = "recency" + }, + SetupRepo: func(shell *Shell) { + shell. + EmptyCommit("base"). + NewBranch("target"). + NewBranch("source"). + EmptyCommit("one"). + EmptyCommit("two"). + EmptyCommit("three"). + EmptyCommit("four"). + EmptyCommit("five"). + Checkout("target") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Branches(). + Focus(). + Lines( + Contains("target").IsSelected(), + Contains("source"), + Contains("master"), + ). + SelectNextItem(). + PressEnter() + + t.Views().SubCommits(). + IsFocused(). + Lines( + Contains("five").IsSelected(), + Contains("four"), + Contains("three"), + Contains("two"), + Contains("one"), + Contains("base"), + ). + Press(keys.Commits.CherryPickCopy) + + t.Views().Commits(). + Focus(). + Lines( + Contains("base").IsSelected(), + ). + Press(keys.Commits.PasteCommits). + Tap(func() { + t.ExpectPopup().Alert(). + Title(Equals("Cherry-pick")). + Content(Equals("Are you sure you want to cherry-pick the 1 copied commit(s) onto this branch?")). + Confirm() + }). + Lines( + Contains("five"), + Contains("base").IsSelected(), + ). + Tap(func() { + // After paste, CherryPicking.DidPaste is true, so it looks to the user as if no + // commits are copied: + t.Views().Information().Content(DoesNotContain("commits copied")) + }) + + t.Views().Branches(). + Focus(). + NavigateToLine(Contains("source")). + PressEnter() + + t.Views().SubCommits(). + IsFocused(). + NavigateToLine(Contains("four")). + Press(keys.Universal.RangeSelectDown). + Press(keys.Universal.RangeSelectDown). + Press(keys.Commits.CherryPickCopy). + Tap(func() { + t.Views().Information().Content(Contains("3 commits copied")) + }) + + t.Views().Commits(). + Focus(). + NavigateToLine(Contains("base")). + Press(keys.Commits.PasteCommits). + Tap(func() { + t.ExpectPopup().Alert(). + Title(Equals("Cherry-pick")). + Content(Equals("Are you sure you want to cherry-pick the 3 copied commit(s) onto this branch?")). + Confirm() + }) + + t.Views().Commits().Lines( + Contains("four"), + Contains("three"), + Contains("two"), + Contains("five"), + Contains("base").IsSelected(), + ) + }, +}) diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go index 09d487852..4b96c4d42 100644 --- a/pkg/integration/tests/test_list.go +++ b/pkg/integration/tests/test_list.go @@ -97,6 +97,7 @@ var tests = []*components.IntegrationTest{ cherry_pick.CherryPickDuringRebase, cherry_pick.CherryPickMerge, cherry_pick.CherryPickRange, + cherry_pick.CherryPickRangeAfterPaste, commit.AddCoAuthor, commit.AddCoAuthorRange, commit.AddCoAuthorWhileCommitting, diff --git a/scripts/check_for_fixups.sh b/scripts/check_for_fixups.sh index d2e8cb08e..5184d4820 100755 --- a/scripts/check_for_fixups.sh +++ b/scripts/check_for_fixups.sh @@ -2,7 +2,7 @@ # We will have only done a shallow clone, so the git log will consist only of # commits on the current PR -commits=$(git log --format="%h %s" | egrep '(^fixup!|^squash!|^amend!|WIP|DROPME)') +commits=$(git log --format="%s" | egrep '(^fixup!|^squash!|^amend!|WIP|DROPME)') if [ -z "$commits" ]; then echo "No fixup commits found."