From 7c911e29fc7642a66f3d88b5b7f28cc83df4bd0f Mon Sep 17 00:00:00 2001 From: abhinavgautam01 Date: Sat, 22 Aug 2026 11:31:21 +0530 Subject: [PATCH 1/3] Add push option to PR creation --- internal/cli/pr.go | 8 +++ internal/cli/pr_checkout_test.go | 18 ++++--- internal/cli/pr_test.go | 91 ++++++++++++++++++++++++++++++++ internal/git/git.go | 17 ++++++ internal/git/git_test.go | 62 ++++++++++++++++++++++ 5 files changed, 190 insertions(+), 6 deletions(-) diff --git a/internal/cli/pr.go b/internal/cli/pr.go index 7873d69..cabbca7 100644 --- a/internal/cli/pr.go +++ b/internal/cli/pr.go @@ -277,6 +277,7 @@ func prCreateCmd() *cobra.Command { flagHead string flagBase string flagDraft bool + flagPush bool flagReviewers []string flagAssignees []string flagLabels []string @@ -300,6 +301,12 @@ func prCreateCmd() *cobra.Command { return err } + if flagPush { + if err := git.PushBranch(cmd.Context(), "", resolve.RemoteName(), flagHead); err != nil { + return fmt.Errorf("pushing head branch: %w", err) + } + } + opts := forges.CreatePROpts{ Title: flagTitle, Body: flagBody, @@ -336,6 +343,7 @@ func prCreateCmd() *cobra.Command { cmd.Flags().StringVarP(&flagHead, "head", "H", "", "Head branch") cmd.Flags().StringVarP(&flagBase, "base", "B", "", "Base branch") cmd.Flags().BoolVarP(&flagDraft, "draft", "d", false, "Create as draft") + cmd.Flags().BoolVar(&flagPush, "push", false, "Push the head branch before creating the PR") cmd.Flags().StringSliceVarP(&flagReviewers, "reviewer", "r", nil, "Request a reviewer") cmd.Flags().StringSliceVarP(&flagAssignees, "assignee", "a", nil, "Assign to a user") cmd.Flags().StringSliceVarP(&flagLabels, "label", "l", nil, "Add a label") diff --git a/internal/cli/pr_checkout_test.go b/internal/cli/pr_checkout_test.go index a41f3d9..f9ee647 100644 --- a/internal/cli/pr_checkout_test.go +++ b/internal/cli/pr_checkout_test.go @@ -15,10 +15,14 @@ import ( // mockPRService implements forges.PullRequestService for testing. type mockPRService struct { - pr *forges.PullRequest - err error - listResult []forges.PullRequest - listErr error + pr *forges.PullRequest + err error + listResult []forges.PullRequest + listErr error + createResult *forges.PullRequest + createErr error + createOpts forges.CreatePROpts + createCalls int } func (m *mockPRService) Get(_ context.Context, _, _ string, _ int) (*forges.PullRequest, error) { @@ -29,8 +33,10 @@ func (m *mockPRService) List(_ context.Context, _, _ string, _ forges.ListPROpts return m.listResult, m.listErr } -func (m *mockPRService) Create(_ context.Context, _, _ string, _ forges.CreatePROpts) (*forges.PullRequest, error) { - return nil, nil +func (m *mockPRService) Create(_ context.Context, _, _ string, opts forges.CreatePROpts) (*forges.PullRequest, error) { + m.createCalls++ + m.createOpts = opts + return m.createResult, m.createErr } func (m *mockPRService) Update(_ context.Context, _, _ string, _ int, _ forges.UpdatePROpts) (*forges.PullRequest, error) { diff --git a/internal/cli/pr_test.go b/internal/cli/pr_test.go index 64877ce..b4af569 100644 --- a/internal/cli/pr_test.go +++ b/internal/cli/pr_test.go @@ -112,6 +112,82 @@ func TestPRCreateRequiresHead(t *testing.T) { } } +func TestPRCreatePushesHeadBranch(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not installed") + } + + dir := t.TempDir() + remoteDir := filepath.Join(t.TempDir(), "remote.git") + t.Chdir(dir) + mustGit(t, "", "init", "--bare", "-q", remoteDir) + mustGit(t, dir, "init", "-q") + mustGit(t, dir, "config", "user.email", "test@test.com") + mustGit(t, dir, "config", "user.name", "Test") + if err := os.WriteFile(filepath.Join(dir, "README"), []byte("test\n"), 0o644); err != nil { + t.Fatal(err) + } + mustGit(t, dir, "add", "README") + mustGit(t, dir, "commit", "-q", "-m", "initial commit") + mustGit(t, dir, "checkout", "-q", "-b", "feature") + mustGit(t, dir, "remote", "add", "origin", remoteDir) + + prs := &mockPRService{ + createResult: &forges.PullRequest{ + Number: 1, + HTMLURL: "https://example.com/pulls/1", + Base: forges.PRBranch{Ref: "main"}, + }, + } + resolve.SetTestForge(&mockForge{prService: prs}, "owner", "repo", "example.com") + t.Cleanup(resolve.ResetTestForge) + resolve.SetRemote("origin") + + cmd := prCreateCmd() + cmd.SetArgs([]string{"--title", "Test PR", "--head", "feature", "--push"}) + if err := cmd.Execute(); err != nil { + t.Fatalf("pr create: %v", err) + } + + if prs.createCalls != 1 { + t.Fatalf("Create calls = %d, want 1", prs.createCalls) + } + if prs.createOpts.Head != "feature" { + t.Fatalf("Create head = %q, want %q", prs.createOpts.Head, "feature") + } + localSHA := gitOutput(t, dir, "rev-parse", "refs/heads/feature") + remoteSHA := gitOutput(t, "", "--git-dir", remoteDir, "rev-parse", "refs/heads/feature") + if remoteSHA != localSHA { + t.Fatalf("remote branch SHA = %q, want %q", remoteSHA, localSHA) + } +} + +func TestPRCreatePushFailurePreventsCreate(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not installed") + } + + dir := t.TempDir() + t.Chdir(dir) + mustGit(t, dir, "init", "-q") + + prs := &mockPRService{} + resolve.SetTestForge(&mockForge{prService: prs}, "owner", "repo", "example.com") + t.Cleanup(resolve.ResetTestForge) + resolve.SetRemote("missing") + t.Cleanup(func() { resolve.SetRemote("origin") }) + + cmd := prCreateCmd() + cmd.SetArgs([]string{"--title", "Test PR", "--head", "feature", "--push"}) + err := cmd.Execute() + if err == nil || !strings.Contains(err.Error(), "pushing head branch") { + t.Fatalf("pr create error = %v, want push error", err) + } + if prs.createCalls != 0 { + t.Fatalf("Create calls = %d, want 0", prs.createCalls) + } +} + func TestPRMergeInvalidNumber(t *testing.T) { var buf bytes.Buffer rootCmd.SetOut(&buf) @@ -347,6 +423,21 @@ func mustGit(t *testing.T, dir string, args ...string) { } } +func gitOutput(t *testing.T, dir string, args ...string) string { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + cmd.Env = append(os.Environ(), + "GIT_CONFIG_GLOBAL=/dev/null", + "GIT_CONFIG_SYSTEM=/dev/null", + ) + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } + return strings.TrimSpace(string(out)) +} + func TestPRViewJSONFlagNotSupported(t *testing.T) { tests := []struct { name string diff --git a/internal/git/git.go b/internal/git/git.go index 5c47bcf..3bac187 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -131,6 +131,23 @@ func CurrentBranch(ctx context.Context, dir string) (string, error) { return branch, nil } +// PushBranch pushes a local branch to the same branch name on a remote and +// configures the local branch to track it. +func PushBranch(ctx context.Context, dir, remote, branch string) error { + if remote == "" { + return fmt.Errorf("empty remote name") + } + if branch == "" { + return fmt.Errorf("empty branch name") + } + + ref := "refs/heads/" + branch + if _, err := runGit(ctx, dir, "push", "--set-upstream", remote, ref+":"+ref); err != nil { + return fmt.Errorf("failed to push branch %q to remote %q: %w", branch, remote, err) + } + return nil +} + func branchConfigKey(branch, name string) string { return fmt.Sprintf("branch.%s.%s", branch, name) } diff --git a/internal/git/git_test.go b/internal/git/git_test.go index 361ea3b..a465ae0 100644 --- a/internal/git/git_test.go +++ b/internal/git/git_test.go @@ -2,7 +2,9 @@ package git import ( "context" + "os" "os/exec" + "path/filepath" "testing" "github.com/git-pkgs/forge" @@ -173,6 +175,66 @@ func TestCurrentBranch(t *testing.T) { } } +func TestPushBranch(t *testing.T) { + dir := initGitRepo(t) + remoteDir := filepath.Join(t.TempDir(), "remote.git") + mustGit(t, "", "init", "--bare", "-q", remoteDir) + mustGit(t, dir, "config", "user.name", "Test") + mustGit(t, dir, "config", "user.email", "test@example.com") + if err := os.WriteFile(filepath.Join(dir, "README"), []byte("test\n"), 0o644); err != nil { + t.Fatal(err) + } + mustGit(t, dir, "add", "README") + mustGit(t, dir, "commit", "-q", "-m", "initial commit") + mustGit(t, dir, "checkout", "-q", "-b", "feature") + mustGit(t, dir, "remote", "add", "origin", remoteDir) + + if err := PushBranch(context.Background(), dir, "origin", "feature"); err != nil { + t.Fatalf("PushBranch: %v", err) + } + + localSHA, err := runGit(context.Background(), dir, "rev-parse", "refs/heads/feature") + if err != nil { + t.Fatalf("reading local branch: %v", err) + } + remoteSHA, err := runGit(context.Background(), "", "--git-dir", remoteDir, "rev-parse", "refs/heads/feature") + if err != nil { + t.Fatalf("reading remote branch: %v", err) + } + if remoteSHA != localSHA { + t.Fatalf("remote branch SHA = %q, want %q", remoteSHA, localSHA) + } + + upstream, err := runGit(context.Background(), dir, "rev-parse", "--abbrev-ref", "feature@{upstream}") + if err != nil { + t.Fatalf("reading branch upstream: %v", err) + } + if upstream != "origin/feature" { + t.Fatalf("branch upstream = %q, want %q", upstream, "origin/feature") + } +} + +func TestPushBranchErrors(t *testing.T) { + tests := []struct { + name string + remote string + branch string + want string + }{ + {name: "empty remote", branch: "feature", want: "empty remote name"}, + {name: "empty branch", remote: "origin", want: "empty branch name"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := PushBranch(context.Background(), "", tt.remote, tt.branch) + if err == nil || err.Error() != tt.want { + t.Fatalf("PushBranch error = %v, want %q", err, tt.want) + } + }) + } +} + func TestSetAndGetPRNumber(t *testing.T) { dir := initGitRepo(t) ctx := context.Background() From 60e44ad884706be2fc4a695a086d80a2d2580b1b Mon Sep 17 00:00:00 2001 From: abhinavgautam01 Date: Sat, 22 Aug 2026 14:48:19 +0530 Subject: [PATCH 2/3] Handle qualified PR heads when pushing --- internal/cli/pr.go | 49 ++++++++++++++-- internal/cli/pr_test.go | 97 +++++++++++++++++++++++++++++++- internal/resolve/resolve.go | 22 ++++++++ internal/resolve/resolve_test.go | 35 ++++++++++++ 4 files changed, 196 insertions(+), 7 deletions(-) diff --git a/internal/cli/pr.go b/internal/cli/pr.go index cabbca7..cb9d49a 100644 --- a/internal/cli/pr.go +++ b/internal/cli/pr.go @@ -296,15 +296,24 @@ func prCreateCmd() *cobra.Command { return fmt.Errorf("--head is required") } - forge, owner, repoName, _, err := resolve.Repo(flagRepo, flagForgeType) + forge, owner, repoName, domain, err := resolve.Repo(flagRepo, flagForgeType) if err != nil { return err } + localHead := flagHead if flagPush { - if err := git.PushBranch(cmd.Context(), "", resolve.RemoteName(), flagHead); err != nil { + headOwner, localBranch, err := splitPRHead(flagHead) + if err != nil { + return err + } + if err := validatePushRemote(cmd.Context(), domain, owner, repoName, headOwner, localBranch); err != nil { + return err + } + if err := git.PushBranch(cmd.Context(), "", resolve.RemoteName(), localBranch); err != nil { return fmt.Errorf("pushing head branch: %w", err) } + localHead = localBranch } opts := forges.CreatePROpts{ @@ -324,8 +333,8 @@ func prCreateCmd() *cobra.Command { return fmt.Errorf("creating pull request: %w", err) } - if flagHead != "" && pr.Base.Ref != "" { - _ = git.SetBaseBranch(cmd.Context(), "", flagHead, pr.Base.Ref) + if localHead != "" && pr.Base.Ref != "" { + _ = git.SetBaseBranch(cmd.Context(), "", localHead, pr.Base.Ref) } p := printer() @@ -353,6 +362,38 @@ func prCreateCmd() *cobra.Command { return cmd } +func splitPRHead(head string) (owner, branch string, err error) { + owner, branch, qualified := strings.Cut(head, ":") + if !qualified { + return "", head, nil + } + if owner == "" || branch == "" { + return "", "", fmt.Errorf("invalid qualified head %q, expected OWNER:BRANCH", head) + } + return owner, branch, nil +} + +func validatePushRemote(ctx context.Context, domain, owner, repo, headOwner, branch string) error { + remote := resolve.RemoteName() + pushDomain, pushOwner, pushRepo, err := resolve.PushRemoteRepo(ctx, remote) + if err != nil { + // Local-path remotes cannot be mapped to a forge repository, but Git can + // still push to them. Let the push itself determine whether they work. + return nil + } + + if !strings.EqualFold(pushDomain, domain) || !strings.EqualFold(pushRepo, repo) { + return fmt.Errorf("push remote %q points to %s/%s/%s, not %s/%s/%s", remote, pushDomain, pushOwner, pushRepo, domain, owner, repo) + } + if headOwner == "" && !strings.EqualFold(pushOwner, owner) { + return fmt.Errorf("push remote %q belongs to %q; use --head %s:%s for a fork pull request", remote, pushOwner, pushOwner, branch) + } + if headOwner != "" && !strings.EqualFold(headOwner, pushOwner) { + return fmt.Errorf("head owner %q does not match push remote %q owner %q", headOwner, remote, pushOwner) + } + return nil +} + func prCloseCmd() *cobra.Command { return &cobra.Command{ Use: "close ", diff --git a/internal/cli/pr_test.go b/internal/cli/pr_test.go index b4af569..3e21728 100644 --- a/internal/cli/pr_test.go +++ b/internal/cli/pr_test.go @@ -144,7 +144,7 @@ func TestPRCreatePushesHeadBranch(t *testing.T) { resolve.SetRemote("origin") cmd := prCreateCmd() - cmd.SetArgs([]string{"--title", "Test PR", "--head", "feature", "--push"}) + cmd.SetArgs([]string{"--title", "Test PR", "--head", "forkowner:feature", "--push"}) if err := cmd.Execute(); err != nil { t.Fatalf("pr create: %v", err) } @@ -152,14 +152,105 @@ func TestPRCreatePushesHeadBranch(t *testing.T) { if prs.createCalls != 1 { t.Fatalf("Create calls = %d, want 1", prs.createCalls) } - if prs.createOpts.Head != "feature" { - t.Fatalf("Create head = %q, want %q", prs.createOpts.Head, "feature") + if prs.createOpts.Head != "forkowner:feature" { + t.Fatalf("Create head = %q, want %q", prs.createOpts.Head, "forkowner:feature") } localSHA := gitOutput(t, dir, "rev-parse", "refs/heads/feature") remoteSHA := gitOutput(t, "", "--git-dir", remoteDir, "rev-parse", "refs/heads/feature") if remoteSHA != localSHA { t.Fatalf("remote branch SHA = %q, want %q", remoteSHA, localSHA) } + base := gitOutput(t, dir, "config", "--local", "--get", "branch.feature.forge-merge-base") + if base != "main" { + t.Fatalf("cached base branch = %q, want %q", base, "main") + } +} + +func TestPRCreatePushRejectsUnqualifiedForkHead(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not installed") + } + + dir := t.TempDir() + t.Chdir(dir) + mustGit(t, dir, "init", "-q") + mustGit(t, dir, "remote", "add", "fork", "https://github.com/upstream/repo.git") + mustGit(t, dir, "remote", "set-url", "--push", "fork", "https://github.com/forkowner/repo.git") + + prs := &mockPRService{} + resolve.SetTestForge(&mockForge{prService: prs}, "upstream", "repo", "github.com") + t.Cleanup(resolve.ResetTestForge) + resolve.SetRemote("fork") + t.Cleanup(func() { resolve.SetRemote("origin") }) + + cmd := prCreateCmd() + cmd.SetArgs([]string{"--title", "Test PR", "--head", "feature", "--push"}) + err := cmd.Execute() + if err == nil || !strings.Contains(err.Error(), "use --head forkowner:feature") { + t.Fatalf("pr create error = %v, want qualified-head guidance", err) + } + if prs.createCalls != 0 { + t.Fatalf("Create calls = %d, want 0", prs.createCalls) + } +} + +func TestPRCreatePushRejectsMismatchedQualifiedOwner(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not installed") + } + + dir := t.TempDir() + t.Chdir(dir) + mustGit(t, dir, "init", "-q") + mustGit(t, dir, "remote", "add", "fork", "https://github.com/forkowner/repo.git") + + prs := &mockPRService{} + resolve.SetTestForge(&mockForge{prService: prs}, "upstream", "repo", "github.com") + t.Cleanup(resolve.ResetTestForge) + resolve.SetRemote("fork") + t.Cleanup(func() { resolve.SetRemote("origin") }) + + cmd := prCreateCmd() + cmd.SetArgs([]string{"--title", "Test PR", "--head", "otherowner:feature", "--push"}) + err := cmd.Execute() + if err == nil || !strings.Contains(err.Error(), "head owner \"otherowner\" does not match") { + t.Fatalf("pr create error = %v, want owner-mismatch error", err) + } + if prs.createCalls != 0 { + t.Fatalf("Create calls = %d, want 0", prs.createCalls) + } +} + +func TestSplitPRHead(t *testing.T) { + tests := []struct { + head string + wantOwner string + wantBranch string + wantErr bool + }{ + {head: "feature", wantBranch: "feature"}, + {head: "forkowner:feature", wantOwner: "forkowner", wantBranch: "feature"}, + {head: ":feature", wantErr: true}, + {head: "forkowner:", wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.head, func(t *testing.T) { + owner, branch, err := splitPRHead(tt.head) + if tt.wantErr { + if err == nil { + t.Fatal("expected error") + } + return + } + if err != nil { + t.Fatalf("splitPRHead: %v", err) + } + if owner != tt.wantOwner || branch != tt.wantBranch { + t.Fatalf("splitPRHead = %q, %q, want %q, %q", owner, branch, tt.wantOwner, tt.wantBranch) + } + }) + } } func TestPRCreatePushFailurePreventsCreate(t *testing.T) { diff --git a/internal/resolve/resolve.go b/internal/resolve/resolve.go index 3c4fea1..a96b5c4 100644 --- a/internal/resolve/resolve.go +++ b/internal/resolve/resolve.go @@ -7,6 +7,7 @@ import ( "net/url" "os" "os/exec" + "path/filepath" "strings" "github.com/git-pkgs/forge" @@ -246,6 +247,27 @@ func gitRemoteURL(name string) (string, error) { return strings.TrimSpace(string(out)), nil } +// PushRemoteRepo returns the forge domain, owner, and repository addressed by +// a remote's push URL. Git falls back to the fetch URL when no push URL is set. +func PushRemoteRepo(ctx context.Context, name string) (domain, owner, repo string, err error) { + out, err := exec.CommandContext(ctx, "git", "remote", "get-url", "--push", name).Output() + if err != nil { + return "", "", "", err + } + + rawURL := strings.TrimSpace(string(out)) + parsed, parseErr := url.Parse(rawURL) + if filepath.IsAbs(rawURL) || (parseErr == nil && parsed.Scheme == "file") { + return "", "", "", fmt.Errorf("push remote %q uses a local path", name) + } + + domain, owner, repo, err = forges.ParseRepoURL(rawURL) + if err != nil { + return "", "", "", err + } + return mapSSHHost(domain), owner, repo, nil +} + // OwnerForBranch returns the repository owner for the remote that the given // branch tracks. This is useful for determining which fork a branch was pushed // to when creating pull requests. diff --git a/internal/resolve/resolve_test.go b/internal/resolve/resolve_test.go index a9f2bd0..4e8911a 100644 --- a/internal/resolve/resolve_test.go +++ b/internal/resolve/resolve_test.go @@ -679,6 +679,41 @@ func TestOwnerForBranch(t *testing.T) { } } +func TestPushRemoteRepo(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not installed") + } + + dir := t.TempDir() + t.Chdir(dir) + mustGit(t, "init", "-q") + mustGit(t, "remote", "add", "origin", "https://github.com/mainowner/project.git") + mustGit(t, "remote", "set-url", "--push", "origin", "git@github.com:forkowner/project.git") + + domain, owner, repo, err := PushRemoteRepo(context.Background(), "origin") + if err != nil { + t.Fatalf("PushRemoteRepo: %v", err) + } + if domain != "github.com" || owner != "forkowner" || repo != "project" { + t.Fatalf("PushRemoteRepo = %q, %q, %q, want %q, %q, %q", domain, owner, repo, "github.com", "forkowner", "project") + } +} + +func TestPushRemoteRepoRejectsLocalPath(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not installed") + } + + dir := t.TempDir() + t.Chdir(dir) + mustGit(t, "init", "-q") + mustGit(t, "remote", "add", "origin", filepath.Join(t.TempDir(), "remote.git")) + + if _, _, _, err := PushRemoteRepo(context.Background(), "origin"); err == nil { + t.Fatal("expected local-path remote to be rejected") + } +} + func mustGit(t *testing.T, args ...string) { t.Helper() cmd := exec.Command("git", args...) From be77a447ace5cf41c03ce8caafba9509895037f5 Mon Sep 17 00:00:00 2001 From: abhinavgautam01 Date: Mon, 24 Aug 2026 14:29:49 +0530 Subject: [PATCH 3/3] Handle provider-specific fork PR pushes --- gitea/prs.go | 2 + github/prs.go | 2 + internal/cli/pr.go | 29 ++++++++++--- internal/cli/pr_checkout_test.go | 19 ++++---- internal/cli/pr_test.go | 74 +++++++++++++++++++++++++++++++- services.go | 6 +++ 6 files changed, 115 insertions(+), 17 deletions(-) diff --git a/gitea/prs.go b/gitea/prs.go index 3b127e4..f7fe901 100644 --- a/gitea/prs.go +++ b/gitea/prs.go @@ -13,6 +13,8 @@ type giteaPRService struct { client *gitea.Client } +func (*giteaPRService) SupportsQualifiedPRHeads() bool { return true } + func (f *giteaForge) PullRequests() forge.PullRequestService { return &giteaPRService{client: f.client} } diff --git a/github/prs.go b/github/prs.go index 70751dd..4fb1117 100644 --- a/github/prs.go +++ b/github/prs.go @@ -15,6 +15,8 @@ type gitHubPRService struct { client *github.Client } +func (*gitHubPRService) SupportsQualifiedPRHeads() bool { return true } + func (f *gitHubForge) PullRequests() forge.PullRequestService { return &gitHubPRService{client: f.client} } diff --git a/internal/cli/pr.go b/internal/cli/pr.go index cb9d49a..70d4033 100644 --- a/internal/cli/pr.go +++ b/internal/cli/pr.go @@ -300,6 +300,9 @@ func prCreateCmd() *cobra.Command { if err != nil { return err } + prService := forge.PullRequests() + qualifiedHeadProvider, ok := prService.(forges.QualifiedPRHeadProvider) + supportsQualifiedHeads := ok && qualifiedHeadProvider.SupportsQualifiedPRHeads() localHead := flagHead if flagPush { @@ -307,7 +310,10 @@ func prCreateCmd() *cobra.Command { if err != nil { return err } - if err := validatePushRemote(cmd.Context(), domain, owner, repoName, headOwner, localBranch); err != nil { + if headOwner != "" && !supportsQualifiedHeads { + return fmt.Errorf("qualified head %q is not supported by this forge; fork pushes require GitHub or Gitea", flagHead) + } + if err := validatePushRemote(cmd.Context(), domain, owner, repoName, headOwner, localBranch, supportsQualifiedHeads); err != nil { return err } if err := git.PushBranch(cmd.Context(), "", resolve.RemoteName(), localBranch); err != nil { @@ -328,7 +334,7 @@ func prCreateCmd() *cobra.Command { Milestone: flagMilestone, } - pr, err := forge.PullRequests().Create(cmd.Context(), owner, repoName, opts) + pr, err := prService.Create(cmd.Context(), owner, repoName, opts) if err != nil { return fmt.Errorf("creating pull request: %w", err) } @@ -373,7 +379,7 @@ func splitPRHead(head string) (owner, branch string, err error) { return owner, branch, nil } -func validatePushRemote(ctx context.Context, domain, owner, repo, headOwner, branch string) error { +func validatePushRemote(ctx context.Context, domain, owner, repo, headOwner, branch string, supportsQualifiedHeads bool) error { remote := resolve.RemoteName() pushDomain, pushOwner, pushRepo, err := resolve.PushRemoteRepo(ctx, remote) if err != nil { @@ -382,14 +388,23 @@ func validatePushRemote(ctx context.Context, domain, owner, repo, headOwner, bra return nil } - if !strings.EqualFold(pushDomain, domain) || !strings.EqualFold(pushRepo, repo) { + if !strings.EqualFold(pushDomain, domain) { return fmt.Errorf("push remote %q points to %s/%s/%s, not %s/%s/%s", remote, pushDomain, pushOwner, pushRepo, domain, owner, repo) } - if headOwner == "" && !strings.EqualFold(pushOwner, owner) { + if headOwner != "" { + if !strings.EqualFold(headOwner, pushOwner) { + return fmt.Errorf("head owner %q does not match push remote %q owner %q", headOwner, remote, pushOwner) + } + return nil + } + if !strings.EqualFold(pushOwner, owner) { + if !supportsQualifiedHeads { + return fmt.Errorf("push remote %q belongs to %q; fork pushes are not supported by this forge", remote, pushOwner) + } return fmt.Errorf("push remote %q belongs to %q; use --head %s:%s for a fork pull request", remote, pushOwner, pushOwner, branch) } - if headOwner != "" && !strings.EqualFold(headOwner, pushOwner) { - return fmt.Errorf("head owner %q does not match push remote %q owner %q", headOwner, remote, pushOwner) + if !strings.EqualFold(pushRepo, repo) { + return fmt.Errorf("push remote %q repository %q does not match target repository %q", remote, pushRepo, repo) } return nil } diff --git a/internal/cli/pr_checkout_test.go b/internal/cli/pr_checkout_test.go index f9ee647..6c90862 100644 --- a/internal/cli/pr_checkout_test.go +++ b/internal/cli/pr_checkout_test.go @@ -15,16 +15,19 @@ import ( // mockPRService implements forges.PullRequestService for testing. type mockPRService struct { - pr *forges.PullRequest - err error - listResult []forges.PullRequest - listErr error - createResult *forges.PullRequest - createErr error - createOpts forges.CreatePROpts - createCalls int + pr *forges.PullRequest + err error + listResult []forges.PullRequest + listErr error + createResult *forges.PullRequest + createErr error + createOpts forges.CreatePROpts + createCalls int + qualifiedHeads bool } +func (m *mockPRService) SupportsQualifiedPRHeads() bool { return m.qualifiedHeads } + func (m *mockPRService) Get(_ context.Context, _, _ string, _ int) (*forges.PullRequest, error) { return m.pr, m.err } diff --git a/internal/cli/pr_test.go b/internal/cli/pr_test.go index 3e21728..9f33c35 100644 --- a/internal/cli/pr_test.go +++ b/internal/cli/pr_test.go @@ -133,6 +133,7 @@ func TestPRCreatePushesHeadBranch(t *testing.T) { mustGit(t, dir, "remote", "add", "origin", remoteDir) prs := &mockPRService{ + qualifiedHeads: true, createResult: &forges.PullRequest{ Number: 1, HTMLURL: "https://example.com/pulls/1", @@ -166,6 +167,57 @@ func TestPRCreatePushesHeadBranch(t *testing.T) { } } +func TestPRCreatePushRejectsQualifiedHeadForUnsupportedForge(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not installed") + } + + dir := t.TempDir() + t.Chdir(dir) + mustGit(t, dir, "init", "-q") + + prs := &mockPRService{} + resolve.SetTestForge(&mockForge{prService: prs}, "upstream", "repo", "gitlab.com") + t.Cleanup(resolve.ResetTestForge) + + cmd := prCreateCmd() + cmd.SetArgs([]string{"--title", "Test PR", "--head", "forkowner:feature", "--push"}) + err := cmd.Execute() + if err == nil || !strings.Contains(err.Error(), "qualified head") { + t.Fatalf("pr create error = %v, want unsupported qualified-head error", err) + } + if prs.createCalls != 0 { + t.Fatalf("Create calls = %d, want 0", prs.createCalls) + } +} + +func TestPRCreatePushRejectsForkForUnsupportedForge(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not installed") + } + + dir := t.TempDir() + t.Chdir(dir) + mustGit(t, dir, "init", "-q") + mustGit(t, dir, "remote", "add", "fork", "https://gitlab.com/forkowner/repo.git") + + prs := &mockPRService{} + resolve.SetTestForge(&mockForge{prService: prs}, "upstream", "repo", "gitlab.com") + t.Cleanup(resolve.ResetTestForge) + resolve.SetRemote("fork") + t.Cleanup(func() { resolve.SetRemote("origin") }) + + cmd := prCreateCmd() + cmd.SetArgs([]string{"--title", "Test PR", "--head", "feature", "--push"}) + err := cmd.Execute() + if err == nil || !strings.Contains(err.Error(), "fork pushes are not supported") { + t.Fatalf("pr create error = %v, want unsupported fork-push error", err) + } + if prs.createCalls != 0 { + t.Fatalf("Create calls = %d, want 0", prs.createCalls) + } +} + func TestPRCreatePushRejectsUnqualifiedForkHead(t *testing.T) { if _, err := exec.LookPath("git"); err != nil { t.Skip("git not installed") @@ -177,7 +229,7 @@ func TestPRCreatePushRejectsUnqualifiedForkHead(t *testing.T) { mustGit(t, dir, "remote", "add", "fork", "https://github.com/upstream/repo.git") mustGit(t, dir, "remote", "set-url", "--push", "fork", "https://github.com/forkowner/repo.git") - prs := &mockPRService{} + prs := &mockPRService{qualifiedHeads: true} resolve.SetTestForge(&mockForge{prService: prs}, "upstream", "repo", "github.com") t.Cleanup(resolve.ResetTestForge) resolve.SetRemote("fork") @@ -194,6 +246,24 @@ func TestPRCreatePushRejectsUnqualifiedForkHead(t *testing.T) { } } +func TestValidatePushRemoteAllowsRenamedQualifiedFork(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not installed") + } + + dir := t.TempDir() + t.Chdir(dir) + mustGit(t, dir, "init", "-q") + mustGit(t, dir, "remote", "add", "fork", "https://github.com/forkowner/renamed-fork.git") + resolve.SetRemote("fork") + t.Cleanup(func() { resolve.SetRemote("origin") }) + + err := validatePushRemote(context.Background(), "github.com", "upstream", "repo", "forkowner", "feature", true) + if err != nil { + t.Fatalf("validatePushRemote: %v", err) + } +} + func TestPRCreatePushRejectsMismatchedQualifiedOwner(t *testing.T) { if _, err := exec.LookPath("git"); err != nil { t.Skip("git not installed") @@ -204,7 +274,7 @@ func TestPRCreatePushRejectsMismatchedQualifiedOwner(t *testing.T) { mustGit(t, dir, "init", "-q") mustGit(t, dir, "remote", "add", "fork", "https://github.com/forkowner/repo.git") - prs := &mockPRService{} + prs := &mockPRService{qualifiedHeads: true} resolve.SetTestForge(&mockForge{prService: prs}, "upstream", "repo", "github.com") t.Cleanup(resolve.ResetTestForge) resolve.SetRemote("fork") diff --git a/services.go b/services.go index 412bfb1..3006545 100644 --- a/services.go +++ b/services.go @@ -43,6 +43,12 @@ type PullRequestService interface { ListURL(repoHTMLURL string) string } +// QualifiedPRHeadProvider is implemented by pull request services that accept +// OWNER:BRANCH heads to identify a source repository in the same fork network. +type QualifiedPRHeadProvider interface { + SupportsQualifiedPRHeads() bool +} + // LabelService provides operations on repository labels. type LabelService interface { List(ctx context.Context, owner, repo string, opts ListLabelOpts) ([]Label, error)