diff --git a/internal/app/pull_create_test.go b/internal/app/pull_create_test.go index 5ba86863..f84eaa63 100644 --- a/internal/app/pull_create_test.go +++ b/internal/app/pull_create_test.go @@ -10,6 +10,7 @@ import ( "os" "path/filepath" "strings" + "sync" "testing" "gitea.dev/tea/internal/clierr" @@ -263,12 +264,15 @@ func TestPullCreateDerivesTitleAndBodyFromSingleCommit(t *testing.T) { func TestPullCreateEditorSeedsPullRequestTemplate(t *testing.T) { var payload map[string]any + var mu sync.Mutex var templateRequests []string server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch { case strings.HasPrefix(r.URL.Path, "/api/v1/repos/alice/project/raw/"): require.Equal(t, "main", r.URL.Query().Get("ref")) + mu.Lock() templateRequests = append(templateRequests, strings.TrimPrefix(r.URL.Path, "/api/v1/repos/alice/project/raw/")) + mu.Unlock() if r.URL.Path != "/api/v1/repos/alice/project/raw/.gitea/PULL_REQUEST_TEMPLATE.md" { w.WriteHeader(http.StatusNotFound) return @@ -289,7 +293,11 @@ func TestPullCreateEditorSeedsPullRequestTemplate(t *testing.T) { deps.Getenv = serverEnv(server.URL, map[string]string{"EDITOR": "editor"}) require.NoError(t, Execute(context.Background(), deps, []string{"pr", "create", "-R", "alice/project", "--title", "PR", "--head", "feature", "--base", "main", "--editor", "--json=number", "--no-input"})) require.Equal(t, "## Checklist\n", editor.initial) - require.Equal(t, []string{"PULL_REQUEST_TEMPLATE.md", "pull_request_template.md", ".gitea/PULL_REQUEST_TEMPLATE.md"}, templateRequests) + require.ElementsMatch(t, []string{ + "PULL_REQUEST_TEMPLATE.md", "pull_request_template.md", + ".gitea/PULL_REQUEST_TEMPLATE.md", ".gitea/pull_request_template.md", + ".github/PULL_REQUEST_TEMPLATE.md", ".github/pull_request_template.md", + }, templateRequests) require.Equal(t, "filled", payload["body"]) } diff --git a/internal/services/pulls.go b/internal/services/pulls.go index a999299a..e2fb2944 100644 --- a/internal/services/pulls.go +++ b/internal/services/pulls.go @@ -15,6 +15,7 @@ import ( "slices" "strconv" "strings" + "sync" "time" gitea "gitea.dev/sdk" @@ -474,20 +475,42 @@ var pullTemplatePaths = []string{ const maxPullTemplateBytes = 1 << 20 // Template returns the first pull request template found at ref, or "" when the repository has none. +// All paths are probed concurrently; the result is decided in priority order, so a failure is +// reported only when every higher-priority path is missing. func (s Pulls) Template(ctx context.Context, ref string) (string, error) { - for _, path := range pullTemplatePaths { - file, err := repoCall(ctx, s.Client, s.Repository, func(ctx context.Context, c *gitea.Client, owner, name string) (io.ReadCloser, *gitea.Response, error) { - return c.Repositories.GetFileReader(ctx, owner, name, ref, path) + type probe struct { + content string + missing bool + err error + } + probes := make([]probe, len(pullTemplatePaths)) + var wg sync.WaitGroup + for i, path := range pullTemplatePaths { + wg.Go(func() { + probes[i].content, probes[i].missing, probes[i].err = s.templateAt(ctx, ref, path) }) - if err != nil { - if IsNotFound(err) { - continue - } - return "", err + } + wg.Wait() + for _, p := range probes { + if !p.missing { + return p.content, p.err } - data, err := io.ReadAll(io.LimitReader(file, maxPullTemplateBytes)) - _ = file.Close() - return string(data), err } return "", nil } + +// templateAt reads one template path; missing reports a 404. +func (s Pulls) templateAt(ctx context.Context, ref, path string) (content string, missing bool, err error) { + file, err := repoCall(ctx, s.Client, s.Repository, func(ctx context.Context, c *gitea.Client, owner, name string) (io.ReadCloser, *gitea.Response, error) { + return c.Repositories.GetFileReader(ctx, owner, name, ref, path) + }) + if err != nil { + if IsNotFound(err) { + return "", true, nil + } + return "", false, err + } + data, err := io.ReadAll(io.LimitReader(file, maxPullTemplateBytes)) + _ = file.Close() + return string(data), false, err +} diff --git a/internal/services/pulls_test.go b/internal/services/pulls_test.go index 7edb7710..ab8718a6 100644 --- a/internal/services/pulls_test.go +++ b/internal/services/pulls_test.go @@ -11,7 +11,10 @@ import ( "net/http/httptest" "strconv" "strings" + "sync" + "sync/atomic" "testing" + "time" gitea "gitea.dev/sdk" "gitea.dev/tea/internal/transport" @@ -114,25 +117,28 @@ func TestPullTemplateLookup(t *testing.T) { status map[string]int requests int }{ - {"root first", "template", "", map[string]int{"PULL_REQUEST_TEMPLATE.md": http.StatusOK, ".gitea/pull_request_template.md": http.StatusOK}, 1}, - {"uppercase before lowercase", "template", "", map[string]int{".gitea/PULL_REQUEST_TEMPLATE.md": http.StatusOK}, 3}, - {"later match", "template", "", map[string]int{".github/pull_request_template.md": http.StatusOK}, 6}, + {"root first", "PULL_REQUEST_TEMPLATE.md", "", map[string]int{"PULL_REQUEST_TEMPLATE.md": http.StatusOK, ".gitea/pull_request_template.md": http.StatusOK}, 6}, + {"uppercase before lowercase", ".gitea/PULL_REQUEST_TEMPLATE.md", "", map[string]int{".gitea/PULL_REQUEST_TEMPLATE.md": http.StatusOK, ".gitea/pull_request_template.md": http.StatusOK}, 6}, + {"later match", ".github/pull_request_template.md", "", map[string]int{".github/pull_request_template.md": http.StatusOK}, 6}, {"docs is not searched", "", "", map[string]int{"docs/pull_request_template.md": http.StatusOK}, 6}, {"none", "", "", nil, 6}, - {"server error", "", "500", map[string]int{".gitea/pull_request_template.md": http.StatusInternalServerError}, 4}, + {"server error", "", "500", map[string]int{".gitea/pull_request_template.md": http.StatusInternalServerError}, 6}, + {"match before error", "pull_request_template.md", "", map[string]int{"pull_request_template.md": http.StatusOK, ".github/PULL_REQUEST_TEMPLATE.md": http.StatusInternalServerError}, 6}, + {"error before match", "", "500", map[string]int{"pull_request_template.md": http.StatusInternalServerError, ".github/PULL_REQUEST_TEMPLATE.md": http.StatusOK}, 6}, } { t.Run(tc.name, func(t *testing.T) { - requests := 0 + var requests atomic.Int32 server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - requests++ + requests.Add(1) assert.Equal(t, "release/1.0", r.URL.Query().Get("ref")) - status, ok := tc.status[strings.TrimPrefix(r.URL.Path, "/api/v1/repos/owner/repo/raw/")] + path := strings.TrimPrefix(r.URL.Path, "/api/v1/repos/owner/repo/raw/") + status, ok := tc.status[path] if !ok { w.WriteHeader(http.StatusNotFound) return } w.WriteHeader(status) - _, _ = w.Write([]byte("template")) + _, _ = w.Write([]byte(path)) })) defer server.Close() got, err := (Pulls{Client: transport.New(server.URL, "", false), Repository: "owner/repo"}).Template(context.Background(), "release/1.0") @@ -142,11 +148,32 @@ func TestPullTemplateLookup(t *testing.T) { require.NoError(t, err) } require.Equal(t, tc.want, got) - require.Equal(t, tc.requests, requests) + require.Equal(t, tc.requests, int(requests.Load())) }) } } +func TestPullTemplateProbesConcurrently(t *testing.T) { + // Every probe only completes once all of them are in flight. + var arrived sync.WaitGroup + arrived.Add(len(pullTemplatePaths)) + allArrived := make(chan struct{}) + go func() { arrived.Wait(); close(allArrived) }() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + arrived.Done() + select { + case <-allArrived: + case <-time.After(5 * time.Second): + t.Errorf("%s did not run concurrently with the other template probes", r.URL.Path) + } + w.WriteHeader(http.StatusNotFound) + })) + defer server.Close() + got, err := (Pulls{Client: transport.New(server.URL, "", false), Repository: "owner/repo"}).Template(context.Background(), "main") + require.NoError(t, err) + require.Empty(t, got) +} + func TestPullMergeExplainsRefusal(t *testing.T) { for _, tc := range []struct { name, pull, want string