diff --git a/.golangci.yml b/.golangci.yml index d050a586d..03c2a248e 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -1,16 +1,50 @@ version: "2" linters: + default: none enable: + - bodyclose + - copyloopvar + - durationcheck + - gocritic + - govet + - ineffassign + - nilerr - nolintlint - disable: - # The following linters are disabled purely because this config was migrated to v2 where they are in the default - # set, and we should have separate work to enable them if we truly want them. - - staticcheck - - errcheck + # To enable later due to too many issues, and confirm we need them: + # - gosec + # - staticcheck + # - errcheck exclusions: paths: - third-party + rules: + - path: _test\.go$ + linters: + - bodyclose + - gosec + settings: + gocritic: + disabled-checks: + - appendAssign + disabled-tags: + - style + gosec: + excludes: + - G110 + - G204 + - G301 + - G302 + - G304 + - G307 + - G404 + config: + G104: + os: + - Setenv + govet: + enable: + - httpresponse formatters: enable: diff --git a/api/client.go b/api/client.go index b30f5f164..e6ff59c59 100644 --- a/api/client.go +++ b/api/client.go @@ -152,6 +152,8 @@ func (c Client) RESTWithNext(hostname string, method string, p string, body io.R } // HandleHTTPError parses a http.Response into a HTTPError. +// +// The caller is responsible to close the response body stream. func HandleHTTPError(resp *http.Response) error { return handleResponse(ghAPI.HandleHTTPError(resp)) } @@ -196,12 +198,11 @@ func ScopesSuggestion(resp *http.Response) string { // EndpointNeedsScopes adds additional OAuth scopes to an HTTP response as if they were returned from the // server endpoint. This improves HTTP 4xx error messaging for endpoints that don't explicitly list the // OAuth scopes they need. -func EndpointNeedsScopes(resp *http.Response, s string) *http.Response { +func EndpointNeedsScopes(resp *http.Response, s string) { if resp.StatusCode >= 400 && resp.StatusCode < 500 { oldScopes := resp.Header.Get("X-Accepted-Oauth-Scopes") resp.Header.Set("X-Accepted-Oauth-Scopes", fmt.Sprintf("%s, %s", oldScopes, s)) } - return resp } func generateScopesSuggestion(statusCode int, endpointNeedsScopes, tokenHasScopes, hostname string) string { diff --git a/api/pull_request_test.go b/api/pull_request_test.go index 6c15cdddd..8ccfa976f 100644 --- a/api/pull_request_test.go +++ b/api/pull_request_test.go @@ -127,7 +127,6 @@ func TestChecksStatus_SummarisingCheckRuns(t *testing.T) { } for _, tt := range tests { - tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() @@ -180,7 +179,6 @@ func TestChecksStatus_SummarisingStatusContexts(t *testing.T) { } for _, tt := range tests { - tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() diff --git a/api/queries_repo.go b/api/queries_repo.go index e797cead4..4dadcbad3 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -1549,6 +1549,8 @@ func RepoExists(client *Client, repo ghrepo.Interface) (bool, error) { return false, err } + defer resp.Body.Close() + switch resp.StatusCode { case 200: return true, nil diff --git a/internal/safepaths/absolute_test.go b/internal/safepaths/absolute_test.go index 7bda40d89..8446fbfcc 100644 --- a/internal/safepaths/absolute_test.go +++ b/internal/safepaths/absolute_test.go @@ -83,7 +83,6 @@ func TestJoin(t *testing.T) { }, } for _, tt := range tests { - tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() joinedPath, err := tt.base.Join(tt.elems...) diff --git a/pkg/cmd/browse/browse_test.go b/pkg/cmd/browse/browse_test.go index cceba045a..b38fa57f0 100644 --- a/pkg/cmd/browse/browse_test.go +++ b/pkg/cmd/browse/browse_test.go @@ -696,7 +696,7 @@ func Test_parsePathFromFileArg(t *testing.T) { { name: "go to root of repository", currentDir: "pkg/cmd/browse/", - fileArg: filepath.Join("../../../"), + fileArg: filepath.FromSlash("../../../"), expectedPath: "", }, { diff --git a/pkg/cmd/codespace/ports.go b/pkg/cmd/codespace/ports.go index cd6656cf4..ef386dc86 100644 --- a/pkg/cmd/codespace/ports.go +++ b/pkg/cmd/codespace/ports.go @@ -329,7 +329,6 @@ func (a *App) ForwardPorts(ctx context.Context, selector *CodespaceSelector, por // them at the first failure, including cancellation of the context. group, ctx := errgroup.WithContext(ctx) for _, pair := range portPairs { - pair := pair group.Go(func() error { listen, _, err := codespaces.ListenTCP(pair.local, true) if err != nil { diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 70e7ceb42..cd90541f2 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -584,9 +584,8 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts sshOptions) (err erro continue } - cs := cs wg.Add(1) - go func() { + go func(cs *api.Codespace) { result := sshResult{} defer wg.Done() @@ -622,7 +621,7 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts sshOptions) (err erro result.codespace = cs sshUsers <- result - }() + }(cs) } go func() { diff --git a/pkg/cmd/gist/create/create.go b/pkg/cmd/gist/create/create.go index 4f51bed25..06b2336b6 100644 --- a/pkg/cmd/gist/create/create.go +++ b/pkg/cmd/gist/create/create.go @@ -286,7 +286,8 @@ func createGist(client *http.Client, hostname, description string, public bool, defer resp.Body.Close() if resp.StatusCode > 299 { - return nil, api.HandleHTTPError(api.EndpointNeedsScopes(resp, "gist")) + api.EndpointNeedsScopes(resp, "gist") + return nil, api.HandleHTTPError(resp) } result := &shared.Gist{} diff --git a/pkg/cmd/gist/list/list.go b/pkg/cmd/gist/list/list.go index 9dd2bfba5..7582ca2cb 100644 --- a/pkg/cmd/gist/list/list.go +++ b/pkg/cmd/gist/list/list.go @@ -293,7 +293,7 @@ func highlightMatch(s string, filter *regexp.Regexp, matched *bool, color, highl text = s[match[1]:matches[i+1][0]] } if _, err := out.WriteString(color(text)); err != nil { - return "", nil + return "", err } } diff --git a/pkg/cmd/pr/shared/templates.go b/pkg/cmd/pr/shared/templates.go index a3ffa13be..b8c9ea719 100644 --- a/pkg/cmd/pr/shared/templates.go +++ b/pkg/cmd/pr/shared/templates.go @@ -265,7 +265,8 @@ func (m *templateManager) fetch() error { gitClient := &git.Client{} dir, err = gitClient.ToplevelDir(context.Background()) if err != nil { - return nil // abort silently + //nolint:nilerr // intentional, abort silently + return nil } } diff --git a/pkg/cmd/repo/delete/http.go b/pkg/cmd/repo/delete/http.go index 659250446..23930e8e0 100644 --- a/pkg/cmd/repo/delete/http.go +++ b/pkg/cmd/repo/delete/http.go @@ -32,7 +32,8 @@ func deleteRepo(client *http.Client, repo ghrepo.Interface) error { defer resp.Body.Close() if resp.StatusCode > 299 { - return api.HandleHTTPError(api.EndpointNeedsScopes(resp, "delete_repo")) + api.EndpointNeedsScopes(resp, "delete_repo") + return api.HandleHTTPError(resp) } return nil diff --git a/pkg/cmd/repo/edit/edit.go b/pkg/cmd/repo/edit/edit.go index 94ad8868a..68c71c80f 100644 --- a/pkg/cmd/repo/edit/edit.go +++ b/pkg/cmd/repo/edit/edit.go @@ -526,6 +526,8 @@ func getTopics(ctx context.Context, httpClient *http.Client, repo ghrepo.Interfa if err != nil { return nil, err } + defer res.Body.Close() + if res.StatusCode != http.StatusOK { return nil, api.HandleHTTPError(res) } @@ -563,6 +565,7 @@ func setTopics(ctx context.Context, httpClient *http.Client, repo ghrepo.Interfa if err != nil { return err } + defer res.Body.Close() if res.StatusCode != http.StatusOK { return api.HandleHTTPError(res) diff --git a/pkg/cmd/repo/fork/fork.go b/pkg/cmd/repo/fork/fork.go index 3d62a73cc..b620291d6 100644 --- a/pkg/cmd/repo/fork/fork.go +++ b/pkg/cmd/repo/fork/fork.go @@ -347,7 +347,7 @@ func forkRun(opts *ForkOptions) error { forkedRepoURL := ghrepo.FormatRemoteURL(forkedRepo, protocol) dir, err := gitClient.Clone(ctx, forkedRepoURL, opts.GitArgs) if err == nil { - return dir, err + return dir, nil } var execError errWithExitCode if errors.As(err, &execError) && execError.ExitCode() == 128 { diff --git a/pkg/cmd/repo/garden/http.go b/pkg/cmd/repo/garden/http.go index af269fbfa..d968296f4 100644 --- a/pkg/cmd/repo/garden/http.go +++ b/pkg/cmd/repo/garden/http.go @@ -36,7 +36,7 @@ func getCommits(client *http.Client, repo ghrepo.Interface, maxCommits int) ([]* break } result := Result{} - resp, err := getResponse(client, repo.RepoHost(), pathF(page), &result) + links, err := getResponse(client, repo.RepoHost(), pathF(page), &result) if err != nil { return nil, err } @@ -52,8 +52,7 @@ func getCommits(client *http.Client, repo ghrepo.Interface, maxCommits int) ([]* Char: colorFunc(string(handle[0])), }) } - link := resp.Header["Link"] - if len(link) == 0 || !strings.Contains(link[0], "last") { + if len(links) == 0 || !strings.Contains(links[0], "last") { paginating = false } page++ @@ -68,7 +67,9 @@ func getCommits(client *http.Client, repo ghrepo.Interface, maxCommits int) ([]* return commits, nil } -func getResponse(client *http.Client, host, path string, data interface{}) (*http.Response, error) { +// getResponse performs the API call and returns the response's link header values. +// If the "Link" header is missing, the returned slice will be nil. +func getResponse(client *http.Client, host, path string, data interface{}) ([]string, error) { url := ghinstance.RESTPrefix(host) + path req, err := http.NewRequest("GET", url, nil) if err != nil { @@ -87,8 +88,10 @@ func getResponse(client *http.Client, host, path string, data interface{}) (*htt return nil, errors.New("api call failed") } + links := resp.Header["Link"] + if resp.StatusCode == http.StatusNoContent { - return resp, nil + return links, nil } b, err := io.ReadAll(resp.Body) @@ -101,5 +104,5 @@ func getResponse(client *http.Client, host, path string, data interface{}) (*htt return nil, err } - return resp, nil + return links, nil } diff --git a/pkg/cmd/root/help_topic_test.go b/pkg/cmd/root/help_topic_test.go index baa4f7f56..0113eccfb 100644 --- a/pkg/cmd/root/help_topic_test.go +++ b/pkg/cmd/root/help_topic_test.go @@ -60,7 +60,6 @@ func TestCmdHelpTopic(t *testing.T) { } for _, tt := range tests { - tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() diff --git a/pkg/cmd/run/download/download_test.go b/pkg/cmd/run/download/download_test.go index e0f5eb781..a90ae74b5 100644 --- a/pkg/cmd/run/download/download_test.go +++ b/pkg/cmd/run/download/download_test.go @@ -527,7 +527,7 @@ func Test_runDownload(t *testing.T) { }, }, expectedFiles: []string{ - filepath.Join("non-artifact-2-file"), + "non-artifact-2-file", }, }, { @@ -681,7 +681,7 @@ func Test_runDownload(t *testing.T) { }) }, expectedFiles: []string{ - filepath.Join("artifact-2-file"), + "artifact-2-file", }, }, { diff --git a/pkg/httpmock/stub.go b/pkg/httpmock/stub.go index f15423c84..a5444b2c8 100644 --- a/pkg/httpmock/stub.go +++ b/pkg/httpmock/stub.go @@ -249,6 +249,7 @@ func GraphQLQuery(body string, cb func(string, map[string]interface{})) Responde // ScopesResponder returns a response with a 200 status code and the given OAuth scopes. func ScopesResponder(scopes string) func(*http.Request) (*http.Response, error) { + //nolint:bodyclose return StatusScopesResponder(http.StatusOK, scopes) } diff --git a/pkg/search/searcher.go b/pkg/search/searcher.go index 5cc94ac9e..fc6050ba6 100644 --- a/pkg/search/searcher.go +++ b/pkg/search/searcher.go @@ -65,9 +65,6 @@ func NewSearcher(client *http.Client, host string, detector fd.Detector) Searche func (s searcher) Code(query Query) (CodeResult, error) { result := CodeResult{} - var resp *http.Response - var err error - // We will request either the query limit if it's less than 1 page, or our max page size. // This number doesn't change to keep a valid offset. // @@ -77,15 +74,11 @@ func (s searcher) Code(query Query) (CodeResult, error) { // If we were to request page #2 for 50 items, we would instead get items 50 to 99. numItemsToRetrieve := query.Limit query.Limit = min(numItemsToRetrieve, maxPerPage) + query.Page = 1 for numItemsToRetrieve > 0 { - query.Page = nextPage(resp) - if query.Page == 0 { - break - } - page := CodeResult{} - resp, err = s.search(query, &page) + link, err := s.search(query, &page) if err != nil { return result, err } @@ -99,6 +92,11 @@ func (s searcher) Code(query Query) (CodeResult, error) { result.Total = page.Total result.Items = append(result.Items, page.Items[:numItemsToAdd]...) numItemsToRetrieve = numItemsToRetrieve - numItemsToAdd + + query.Page = nextPage(link) + if query.Page == 0 { + break + } } return result, nil @@ -107,20 +105,13 @@ func (s searcher) Code(query Query) (CodeResult, error) { func (s searcher) Commits(query Query) (CommitsResult, error) { result := CommitsResult{} - var resp *http.Response - var err error - numItemsToRetrieve := query.Limit query.Limit = min(numItemsToRetrieve, maxPerPage) + query.Page = 1 for numItemsToRetrieve > 0 { - query.Page = nextPage(resp) - if query.Page == 0 { - break - } - page := CommitsResult{} - resp, err = s.search(query, &page) + link, err := s.search(query, &page) if err != nil { return result, err } @@ -130,6 +121,11 @@ func (s searcher) Commits(query Query) (CommitsResult, error) { result.Total = page.Total result.Items = append(result.Items, page.Items[:numItemsToAdd]...) numItemsToRetrieve = numItemsToRetrieve - numItemsToAdd + + query.Page = nextPage(link) + if query.Page == 0 { + break + } } return result, nil } @@ -137,20 +133,13 @@ func (s searcher) Commits(query Query) (CommitsResult, error) { func (s searcher) Repositories(query Query) (RepositoriesResult, error) { result := RepositoriesResult{} - var resp *http.Response - var err error - numItemsToRetrieve := query.Limit query.Limit = min(numItemsToRetrieve, maxPerPage) + query.Page = 1 for numItemsToRetrieve > 0 { - query.Page = nextPage(resp) - if query.Page == 0 { - break - } - page := RepositoriesResult{} - resp, err = s.search(query, &page) + link, err := s.search(query, &page) if err != nil { return result, err } @@ -160,6 +149,11 @@ func (s searcher) Repositories(query Query) (RepositoriesResult, error) { result.Total = page.Total result.Items = append(result.Items, page.Items[:numItemsToAdd]...) numItemsToRetrieve = numItemsToRetrieve - numItemsToAdd + + query.Page = nextPage(link) + if query.Page == 0 { + break + } } return result, nil } @@ -167,20 +161,12 @@ func (s searcher) Repositories(query Query) (RepositoriesResult, error) { func (s searcher) Issues(query Query) (IssuesResult, error) { result := IssuesResult{} - var resp *http.Response - var err error - numItemsToRetrieve := query.Limit query.Limit = min(numItemsToRetrieve, maxPerPage) - + query.Page = 1 for numItemsToRetrieve > 0 { - query.Page = nextPage(resp) - if query.Page == 0 { - break - } - page := IssuesResult{} - resp, err = s.search(query, &page) + link, err := s.search(query, &page) if err != nil { return result, err } @@ -190,11 +176,18 @@ func (s searcher) Issues(query Query) (IssuesResult, error) { result.Total = page.Total result.Items = append(result.Items, page.Items[:numItemsToAdd]...) numItemsToRetrieve = numItemsToRetrieve - numItemsToAdd + + query.Page = nextPage(link) + if query.Page == 0 { + break + } } return result, nil } -// search makes a single-page REST search request for code, commits, issues, prs, or repos. +// search makes a single-page REST search request for code, commits, issues, prs, or repos, +// and returns the link header from response for further pagination calls. If the link header +// is not set on the response, empty string is returned. // // The result argument is populated with the following information: // @@ -203,7 +196,7 @@ func (s searcher) Issues(query Query) (IssuesResult, error) { // - Items: the actual matching search results, up to 100 max items per page // // For more information, see https://docs.github.com/en/rest/search/search?apiVersion=2022-11-28. -func (s searcher) search(query Query, result interface{}) (*http.Response, error) { +func (s searcher) search(query Query, result interface{}) (string, error) { path := fmt.Sprintf("%ssearch/%s", ghinstance.RESTPrefix(s.host), query.Kind) qs := url.Values{} qs.Set("page", strconv.Itoa(query.Page)) @@ -216,7 +209,7 @@ func (s searcher) search(query Query, result interface{}) (*http.Response, error // issues. features, err := s.detector.SearchFeatures() if err != nil { - return nil, err + return "", err } if !features.AdvancedIssueSearchAPI { @@ -242,7 +235,7 @@ func (s searcher) search(query Query, result interface{}) (*http.Response, error url := fmt.Sprintf("%s?%s", path, qs.Encode()) req, err := http.NewRequest("GET", url, nil) if err != nil { - return nil, err + return "", err } req.Header.Set("Content-Type", "application/json; charset=utf-8") req.Header.Set("Accept", "application/vnd.github.v3+json") @@ -252,19 +245,22 @@ func (s searcher) search(query Query, result interface{}) (*http.Response, error resp, err := s.client.Do(req) if err != nil { - return nil, err + return "", err } defer resp.Body.Close() + + link := resp.Header.Get("Link") + success := resp.StatusCode >= 200 && resp.StatusCode < 300 if !success { - return resp, handleHTTPError(resp) + return link, handleHTTPError(resp) } decoder := json.NewDecoder(resp.Body) err = decoder.Decode(result) if err != nil { - return resp, err + return link, err } - return resp, nil + return link, nil } // URL returns URL to the global search in web GUI (i.e. github.com/search). @@ -317,16 +313,17 @@ func handleHTTPError(resp *http.Response) error { return httpError } -// https://docs.github.com/en/rest/using-the-rest-api/using-pagination-in-the-rest-api -func nextPage(resp *http.Response) (page int) { - if resp == nil { - return 1 - } - +// nextPage extracts the next page number from an API response's link header. if +// the provided link header is empty or there is no next page, zero is returned. +// +// See API [docs] on pagination for more information. +// +// [docs]: https://docs.github.com/en/rest/using-the-rest-api/using-pagination-in-the-rest-api +func nextPage(link string) (page int) { // When using pagination, responses get a "Link" field in their header. // When a next page is available, "Link" contains a link to the next page // tagged with rel="next". - for _, m := range linkRE.FindAllStringSubmatch(resp.Header.Get("Link"), -1) { + for _, m := range linkRE.FindAllStringSubmatch(link, -1) { if !(len(m) > 2 && m[2] == "next") { continue }