diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cd4e5a48..479fa2f3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -26,7 +26,16 @@ jobs: - name: Check goreleaser file uses: goreleaser/goreleaser-action@v6.2.1 with: - version: latest + # Pinned: goreleaser v2.16.0 (2026-05-24) started treating the + # deprecated `brews:` key in .goreleaser.yml as a hard error, + # breaking CI on every push. v2.15.4 (2026-04-21, the last + # release before the strict check) still accepts `brews:` with + # only a warning AND understands the current `formats:` syntax + # under `archives.format_overrides`. Unpin once .goreleaser.yml + # is updated (likely either drop the brews block or migrate it + # to whatever goreleaser eventually settles on as the homebrew + # successor). + version: 'v2.15.4' args: check #- name: Upload coverage diff --git a/config/config.go b/config/config.go index a3de0976..bc9adc28 100644 --- a/config/config.go +++ b/config/config.go @@ -53,6 +53,10 @@ type UserConfig struct { CreateDraftPRs bool `default:"false" yaml:"createDraftPRs"` PreserveTitleAndBody bool `default:"false" yaml:"preserveTitleAndBody"` + // MaintainerCanModify is passed to GitHub when opening cross-fork PRs. + // When true (the default) upstream maintainers can push to the PR head + // branch on the fork. Has no effect for same-fork PRs. + MaintainerCanModify bool `default:"true" yaml:"maintainerCanModify"` NoRebase bool `default:"false" yaml:"noRebase"` NoFetch bool `default:"false" yaml:"noFetch"` DeleteMergedBranches bool `default:"false" yaml:"deleteMergedBranches"` diff --git a/config/config_parser/remote_source.go b/config/config_parser/remote_source.go index bacdb810..dc0bad30 100644 --- a/config/config_parser/remote_source.go +++ b/config/config_parser/remote_source.go @@ -1,7 +1,6 @@ package config_parser import ( - "fmt" "regexp" "strings" @@ -38,27 +37,16 @@ func (s *remoteSource) Load(_ interface{}) { } } +// originPushURLRegex captures the URL portion of an `origin ... (push)` line +// from `git remote -v` output. The URL itself is parsed by git.ParseRepoURL. +var originPushURLRegex = regexp.MustCompile(`^origin\s+(\S+)\s+\(push\)`) + func getRepoDetailsFromRemote(remote string) (string, string, string, bool) { - // Allows "https://", "ssh://" or no protocol at all (this means ssh) - protocolFormat := `(?:(https://)|(ssh://))?` - // This may or may not be present in the address - userFormat := `(git@)?` - // "/" is expected in "http://" or "ssh://" protocol, when no protocol given - // it should be ":" - repoFormat := `(?P[a-z0-9._\-]+)(/|:)(?P[\w-]+)/(?P[\w-]+)` - // This is neither required in https access nor in ssh one - suffixFormat := `(.git)?` - regexFormat := fmt.Sprintf(`^origin\s+%s%s%s%s \(push\)`, - protocolFormat, userFormat, repoFormat, suffixFormat) - regex := regexp.MustCompile(regexFormat) - matches := regex.FindStringSubmatch(remote) - if matches != nil { - githubHostIndex := regex.SubexpIndex("githubHost") - repoOwnerIndex := regex.SubexpIndex("repoOwner") - repoNameIndex := regex.SubexpIndex("repoName") - return matches[githubHostIndex], matches[repoOwnerIndex], matches[repoNameIndex], true + matches := originPushURLRegex.FindStringSubmatch(remote) + if matches == nil { + return "", "", "", false } - return "", "", "", false + return git.ParseRepoURL(matches[1]) } func check(err error) { diff --git a/config/config_test.go b/config/config_test.go index 0664b78d..0542c108 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -37,12 +37,13 @@ func TestDefaultConfig(t *testing.T) { ShowPrTitlesInStack: false, }, User: &UserConfig{ - ShowPRLink: true, - LogGitCommands: false, - LogGitHubCalls: false, - StatusBitsHeader: true, - StatusBitsEmojis: true, - BranchPrefix: "spr", + ShowPRLink: true, + LogGitCommands: false, + LogGitHubCalls: false, + StatusBitsHeader: true, + StatusBitsEmojis: true, + MaintainerCanModify: true, + BranchPrefix: "spr", }, State: &InternalState{ MergeCheckCommit: map[string]string{}, diff --git a/git/helpers.go b/git/helpers.go index ef847d27..d3902788 100644 --- a/git/helpers.go +++ b/git/helpers.go @@ -34,6 +34,47 @@ func BranchNameRegex(branchPrefix string) *regexp.Regexp { return regexp.MustCompile(regexp.QuoteMeta(branchPrefix) + `/([a-zA-Z0-9_\-/\.]+)/([a-f0-9]{8})$`) } +// repoURLRegex matches the supported forms of a GitHub-style remote URL and +// captures the host, owner, and name. Used by ParseRepoURL. +// +// Forms accepted: +// - https://host/owner/name[.git] +// - ssh://[git@]host/owner/name[.git] +// - [git@]host:owner/name[.git] +var repoURLRegex = regexp.MustCompile( + `^` + + `(?:https://|ssh://)?` + + `(?:git@)?` + + `(?P[a-z0-9._\-]+)` + + `(?:/|:)` + + `(?P[\w-]+)/(?P[\w-]+)` + + `(?:\.git)?` + + `/?$`, +) + +// ParseRepoURL extracts (host, owner, name) from a git remote URL. +// Returns ok=false if the URL is not recognizable as a GitHub-style repo URL. +func ParseRepoURL(remoteURL string) (host, owner, name string, ok bool) { + matches := repoURLRegex.FindStringSubmatch(strings.TrimSpace(remoteURL)) + if matches == nil { + return "", "", "", false + } + return matches[repoURLRegex.SubexpIndex("host")], + matches[repoURLRegex.SubexpIndex("owner")], + matches[repoURLRegex.SubexpIndex("name")], + true +} + +// GetRemoteURL returns the push URL for the given git remote, or an error if +// the remote is not configured. +func GetRemoteURL(gitcmd GitInterface, remoteName string) (string, error) { + var output string + if err := gitcmd.Git("remote get-url "+remoteName, &output); err != nil { + return "", err + } + return strings.TrimSpace(output), nil +} + // GetLocalTopCommit returns the top unmerged commit in the stack // // return nil if there are no unmerged commits in the stack diff --git a/git/helpers_external_test.go b/git/helpers_external_test.go new file mode 100644 index 00000000..d9328d6e --- /dev/null +++ b/git/helpers_external_test.go @@ -0,0 +1,35 @@ +// Tests in package git_test rather than git so they can use git/mockgit +// without an import cycle. +package git_test + +import ( + "testing" + + "github.com/ejoffe/spr/git" + "github.com/ejoffe/spr/git/mockgit" + "github.com/stretchr/testify/require" +) + +func TestGetRemoteURL(t *testing.T) { + t.Run("returns trimmed URL on success", func(t *testing.T) { + mock := mockgit.NewMockGit(t) + mock.ExpectRemoteGetURL("origin", "git@github.com:r2/d2.git") + + url, err := git.GetRemoteURL(mock, "origin") + + require.NoError(t, err) + require.Equal(t, "git@github.com:r2/d2.git", url) + mock.ExpectationsMet() + }) + + t.Run("passes the remote name through verbatim", func(t *testing.T) { + mock := mockgit.NewMockGit(t) + mock.ExpectRemoteGetURL("upstream", "https://github.com/ejoffe/spr.git") + + url, err := git.GetRemoteURL(mock, "upstream") + + require.NoError(t, err) + require.Equal(t, "https://github.com/ejoffe/spr.git", url) + mock.ExpectationsMet() + }) +} diff --git a/git/helpers_test.go b/git/helpers_test.go index 9e733896..819030da 100644 --- a/git/helpers_test.go +++ b/git/helpers_test.go @@ -47,6 +47,42 @@ func TestBranchNameRegexNoMatch(t *testing.T) { } } +func TestParseRepoURL(t *testing.T) { + tests := []struct { + name string + url string + host string + owner string + repoName string + ok bool + }{ + {name: "https with .git", url: "https://github.com/r2/d2.git", host: "github.com", owner: "r2", repoName: "d2", ok: true}, + {name: "https without .git", url: "https://github.com/r2/d2", host: "github.com", owner: "r2", repoName: "d2", ok: true}, + {name: "ssh form", url: "git@github.com:r2/d2.git", host: "github.com", owner: "r2", repoName: "d2", ok: true}, + {name: "ssh form no suffix", url: "git@github.com:r2/d2", host: "github.com", owner: "r2", repoName: "d2", ok: true}, + {name: "ssh:// protocol", url: "ssh://git@github.com/r2/d2.git", host: "github.com", owner: "r2", repoName: "d2", ok: true}, + {name: "enterprise host", url: "git@gh.enterprise.com:r2/d2.git", host: "gh.enterprise.com", owner: "r2", repoName: "d2", ok: true}, + {name: "dashes in owner and name", url: "https://github.com/r-2/d-2.git", host: "github.com", owner: "r-2", repoName: "d-2", ok: true}, + {name: "underscores in name", url: "https://github.com/r2/d2_a.git", host: "github.com", owner: "r2", repoName: "d2_a", ok: true}, + {name: "case sensitive", url: "https://github.com/R2/D2.git", host: "github.com", owner: "R2", repoName: "D2", ok: true}, + {name: "trailing slash", url: "https://github.com/r2/d2/", host: "github.com", owner: "r2", repoName: "d2", ok: true}, + {name: "leading and trailing whitespace", url: " https://github.com/r2/d2.git\n", host: "github.com", owner: "r2", repoName: "d2", ok: true}, + + {name: "empty", url: "", ok: false}, + {name: "garbage", url: "not a url", ok: false}, + {name: "missing owner", url: "https://github.com//d2.git", ok: false}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + host, owner, name, ok := ParseRepoURL(tc.url) + assert.Equal(t, tc.ok, ok) + assert.Equal(t, tc.host, host) + assert.Equal(t, tc.owner, owner) + assert.Equal(t, tc.repoName, name) + }) + } +} + func TestBranchNameFromCommit(t *testing.T) { tests := []struct { name string diff --git a/git/mockgit/mockgit.go b/git/mockgit/mockgit.go index dadc95ec..f7fe6d47 100644 --- a/git/mockgit/mockgit.go +++ b/git/mockgit/mockgit.go @@ -155,6 +155,12 @@ func (m *Mock) ExpectRemote(remote string) { m.expect("git remote -v").respond(response) } +// ExpectRemoteGetURL queues an expectation for `git remote get-url ` +// and a canned URL response. +func (m *Mock) ExpectRemoteGetURL(remoteName, url string) { + m.expect("git remote get-url " + remoteName).respond(url + "\n") +} + func (m *Mock) ExpectFixup(commitHash string) { m.expect("git commit --fixup " + commitHash) m.expect("git rebase -i --autosquash --autostash origin/master") diff --git a/github/githubclient/client.go b/github/githubclient/client.go index ce6bd054..a0f15168 100644 --- a/github/githubclient/client.go +++ b/github/githubclient/client.go @@ -242,16 +242,69 @@ func (c *client) GetInfo(ctx context.Context, gitcmd git.GitInterface) *github.G } info := &github.GitHubInfo{ - UserName: loginName, - RepositoryID: repoID, - LocalBranch: git.GetLocalBranchName(gitcmd), - PullRequests: pullRequests, + UserName: loginName, + RepositoryID: repoID, + HeadRepositoryID: resolveHeadRepositoryID(ctx, gitcmd, c.config.Repo, repoID, c.starGetRepoLookup), + LocalBranch: git.GetLocalBranchName(gitcmd), + PullRequests: pullRequests, } log.Debug().Interface("Info", info).Msg("GetInfo") return info } +// repoIDLookup resolves an (owner, name) pair to the repo's GitHub node ID. +// Extracted as a separate type so resolveHeadRepositoryID is testable without +// stubbing the full genclient.Client interface. +type repoIDLookup func(ctx context.Context, owner, name string) (string, error) + +// resolveHeadRepositoryID returns the fork's GitHub node ID when spr is +// configured for cross-fork PRs (i.e. the URL of GitHubRemote points at a +// different (owner, name) than the configured GitHubRepoOwner/Name target). +// Returns "" for same-fork operation or when fork detection fails — in the +// latter case spr falls back to same-fork behavior and GitHub will surface +// any resulting error on the CreatePullRequest call. +func resolveHeadRepositoryID(ctx context.Context, gitcmd git.GitInterface, + repoCfg *config.RepoConfig, upstreamRepoID string, lookup repoIDLookup) string { + remoteURL, err := git.GetRemoteURL(gitcmd, repoCfg.GitHubRemote) + if err != nil { + log.Warn().Err(err).Str("remote", repoCfg.GitHubRemote). + Msg("cross-fork detection: failed to read remote URL") + return "" + } + _, headOwner, headName, ok := git.ParseRepoURL(remoteURL) + if !ok { + log.Warn().Str("remote", repoCfg.GitHubRemote).Str("url", remoteURL). + Msg("cross-fork detection: unparseable remote URL") + return "" + } + if headOwner == repoCfg.GitHubRepoOwner && headName == repoCfg.GitHubRepoName { + return "" + } + headRepoID, err := lookup(ctx, headOwner, headName) + if err != nil { + log.Warn().Err(err).Str("owner", headOwner).Str("name", headName). + Msg("cross-fork detection: failed to resolve fork repo ID") + return "" + } + if headRepoID == "" || headRepoID == upstreamRepoID { + return "" + } + return headRepoID +} + +// starGetRepoLookup adapts the genclient StarGetRepo call to a repoIDLookup. +func (c *client) starGetRepoLookup(ctx context.Context, owner, name string) (string, error) { + resp, err := c.api.StarGetRepo(ctx, owner, name) + if err != nil { + return "", err + } + if resp == nil || resp.Repository == nil { + return "", nil + } + return resp.Repository.Id, nil +} + func matchPullRequestStack( repoConfig *config.RepoConfig, branchPrefix string, @@ -409,6 +462,26 @@ func (c *client) GetAssignableUsers(ctx context.Context) []github.RepoAssignee { return users } +// buildCreatePullRequestInput assembles the GraphQL input for opening a PR. +// Routes through the fork via HeadRepositoryId when GitHubInfo signals +// cross-fork mode (HeadRepositoryID set and different from RepositoryID). +func buildCreatePullRequestInput(info *github.GitHubInfo, user *config.UserConfig, + headRefName, baseRefName, title, body string) genclient.CreatePullRequestInput { + input := genclient.CreatePullRequestInput{ + RepositoryId: info.RepositoryID, + BaseRefName: baseRefName, + HeadRefName: headRefName, + Title: title, + Body: &body, + Draft: &user.CreateDraftPRs, + } + if info.HeadRepositoryID != "" && info.HeadRepositoryID != info.RepositoryID { + input.HeadRepositoryId = &info.HeadRepositoryID + input.MaintainerCanModify = &user.MaintainerCanModify + } + return input +} + func (c *client) CreatePullRequest(ctx context.Context, gitcmd git.GitInterface, info *github.GitHubInfo, commit git.Commit, prevCommit *git.Commit) *github.PullRequest { @@ -425,14 +498,9 @@ func (c *client) CreatePullRequest(ctx context.Context, gitcmd git.GitInterface, templatizer := config_fetcher.PRTemplatizer(c.config, gitcmd) body := templatizer.Body(info, commit, nil) - resp, err := c.api.CreatePullRequest(ctx, genclient.CreatePullRequestInput{ - RepositoryId: info.RepositoryID, - BaseRefName: baseRefName, - HeadRefName: headRefName, - Title: templatizer.Title(info, commit), - Body: &body, - Draft: &c.config.User.CreateDraftPRs, - }) + input := buildCreatePullRequestInput(info, c.config.User, headRefName, baseRefName, + templatizer.Title(info, commit), body) + resp, err := c.api.CreatePullRequest(ctx, input) check(err) pr := &github.PullRequest{ diff --git a/github/githubclient/client_test.go b/github/githubclient/client_test.go index 9df27e25..51bf8c06 100644 --- a/github/githubclient/client_test.go +++ b/github/githubclient/client_test.go @@ -1,10 +1,13 @@ package githubclient import ( + "context" + "errors" "testing" "github.com/ejoffe/spr/config" "github.com/ejoffe/spr/git" + "github.com/ejoffe/spr/git/mockgit" "github.com/ejoffe/spr/github" "github.com/ejoffe/spr/github/githubclient/fezzik_types" "github.com/stretchr/testify/require" @@ -753,3 +756,179 @@ func TestComputeRequiredCheckStatus(t *testing.T) { }) } } + +func TestBuildCreatePullRequestInput(t *testing.T) { + const ( + upstreamID = "upstream-repo-id" + forkID = "fork-repo-id" + headRef = "spr/master/deadbeef" + baseRef = "master" + title = "feat: thing" + body = "body text" + ) + + t.Run("same-fork omits HeadRepositoryId and MaintainerCanModify", func(t *testing.T) { + info := &github.GitHubInfo{RepositoryID: upstreamID} + user := &config.UserConfig{CreateDraftPRs: false, MaintainerCanModify: true} + + input := buildCreatePullRequestInput(info, user, headRef, baseRef, title, body) + + require.Equal(t, upstreamID, input.RepositoryId) + require.Equal(t, headRef, input.HeadRefName) + require.Equal(t, baseRef, input.BaseRefName) + require.Equal(t, title, input.Title) + require.NotNil(t, input.Body) + require.Equal(t, body, *input.Body) + require.Nil(t, input.HeadRepositoryId, "same-fork must not set HeadRepositoryId") + require.Nil(t, input.MaintainerCanModify, "same-fork must not set MaintainerCanModify") + }) + + t.Run("cross-fork sets HeadRepositoryId and MaintainerCanModify", func(t *testing.T) { + info := &github.GitHubInfo{RepositoryID: upstreamID, HeadRepositoryID: forkID} + user := &config.UserConfig{CreateDraftPRs: false, MaintainerCanModify: true} + + input := buildCreatePullRequestInput(info, user, headRef, baseRef, title, body) + + require.Equal(t, upstreamID, input.RepositoryId) + require.NotNil(t, input.HeadRepositoryId) + require.Equal(t, forkID, *input.HeadRepositoryId) + require.Equal(t, headRef, input.HeadRefName, + "HeadRefName stays bare — cross-fork is encoded via HeadRepositoryId, not owner: prefix") + require.NotNil(t, input.MaintainerCanModify) + require.True(t, *input.MaintainerCanModify) + }) + + t.Run("cross-fork honours MaintainerCanModify=false", func(t *testing.T) { + info := &github.GitHubInfo{RepositoryID: upstreamID, HeadRepositoryID: forkID} + user := &config.UserConfig{MaintainerCanModify: false} + + input := buildCreatePullRequestInput(info, user, headRef, baseRef, title, body) + + require.NotNil(t, input.MaintainerCanModify) + require.False(t, *input.MaintainerCanModify) + }) + + t.Run("HeadRepositoryID equal to RepositoryID is treated as same-fork", func(t *testing.T) { + info := &github.GitHubInfo{RepositoryID: upstreamID, HeadRepositoryID: upstreamID} + user := &config.UserConfig{MaintainerCanModify: true} + + input := buildCreatePullRequestInput(info, user, headRef, baseRef, title, body) + + require.Nil(t, input.HeadRepositoryId) + require.Nil(t, input.MaintainerCanModify) + }) +} + +func TestResolveHeadRepositoryID(t *testing.T) { + const ( + upstreamID = "upstream-repo-id" + forkID = "fork-repo-id" + ) + + upstreamCfg := func() *config.RepoConfig { + return &config.RepoConfig{ + GitHubRepoOwner: "ejoffe", + GitHubRepoName: "spr", + GitHubRemote: "origin", + } + } + + // trackingLookup wraps a repoIDLookup and records whether it was called. + type lookupCall struct { + owner, name string + } + trackingLookup := func(returnID string, returnErr error) (repoIDLookup, *[]lookupCall) { + calls := &[]lookupCall{} + fn := func(_ context.Context, owner, name string) (string, error) { + *calls = append(*calls, lookupCall{owner, name}) + return returnID, returnErr + } + return fn, calls + } + + t.Run("same fork: no lookup, returns empty", func(t *testing.T) { + mock := mockgit.NewMockGit(t) + mock.ExpectRemoteGetURL("origin", "git@github.com:ejoffe/spr.git") + lookup, calls := trackingLookup(forkID, nil) + + got := resolveHeadRepositoryID(context.Background(), mock, upstreamCfg(), upstreamID, lookup) + + require.Empty(t, got) + require.Empty(t, *calls, "same-fork must skip the lookup call") + mock.ExpectationsMet() + }) + + t.Run("cross-fork: looks up and returns fork ID", func(t *testing.T) { + mock := mockgit.NewMockGit(t) + mock.ExpectRemoteGetURL("origin", "git@github.com:jucor/spr.git") + lookup, calls := trackingLookup(forkID, nil) + + got := resolveHeadRepositoryID(context.Background(), mock, upstreamCfg(), upstreamID, lookup) + + require.Equal(t, forkID, got) + require.Equal(t, []lookupCall{{owner: "jucor", name: "spr"}}, *calls) + mock.ExpectationsMet() + }) + + t.Run("lookup returns upstream ID: defensive empty", func(t *testing.T) { + mock := mockgit.NewMockGit(t) + mock.ExpectRemoteGetURL("origin", "git@github.com:jucor/spr.git") + // Pathological case: GitHub returns the upstream's ID when asked for the fork. + // Treat as same-fork so we don't accidentally pass HeadRepositoryId=upstream. + lookup, _ := trackingLookup(upstreamID, nil) + + got := resolveHeadRepositoryID(context.Background(), mock, upstreamCfg(), upstreamID, lookup) + + require.Empty(t, got) + mock.ExpectationsMet() + }) + + t.Run("lookup errors: falls back to empty", func(t *testing.T) { + mock := mockgit.NewMockGit(t) + mock.ExpectRemoteGetURL("origin", "git@github.com:jucor/spr.git") + lookup, calls := trackingLookup("", errors.New("graphql 403")) + + got := resolveHeadRepositoryID(context.Background(), mock, upstreamCfg(), upstreamID, lookup) + + require.Empty(t, got, "lookup error must not panic; falls back to same-fork") + require.Len(t, *calls, 1) + mock.ExpectationsMet() + }) + + t.Run("lookup returns empty ID: empty result", func(t *testing.T) { + mock := mockgit.NewMockGit(t) + mock.ExpectRemoteGetURL("origin", "git@github.com:jucor/spr.git") + lookup, _ := trackingLookup("", nil) + + got := resolveHeadRepositoryID(context.Background(), mock, upstreamCfg(), upstreamID, lookup) + + require.Empty(t, got) + mock.ExpectationsMet() + }) + + t.Run("unparseable URL: no lookup, returns empty", func(t *testing.T) { + mock := mockgit.NewMockGit(t) + mock.ExpectRemoteGetURL("origin", "not a real url") + lookup, calls := trackingLookup(forkID, nil) + + got := resolveHeadRepositoryID(context.Background(), mock, upstreamCfg(), upstreamID, lookup) + + require.Empty(t, got) + require.Empty(t, *calls, "unparseable URL must skip the lookup call") + mock.ExpectationsMet() + }) + + t.Run("respects non-default GitHubRemote name", func(t *testing.T) { + mock := mockgit.NewMockGit(t) + mock.ExpectRemoteGetURL("myfork", "https://github.com/jucor/spr") + cfg := upstreamCfg() + cfg.GitHubRemote = "myfork" + lookup, calls := trackingLookup(forkID, nil) + + got := resolveHeadRepositoryID(context.Background(), mock, cfg, upstreamID, lookup) + + require.Equal(t, forkID, got) + require.Equal(t, []lookupCall{{owner: "jucor", name: "spr"}}, *calls) + mock.ExpectationsMet() + }) +} diff --git a/github/interface.go b/github/interface.go index 18c25da6..02d341f7 100644 --- a/github/interface.go +++ b/github/interface.go @@ -36,8 +36,14 @@ type GitHubInterface interface { type GitHubInfo struct { UserName string RepositoryID string - LocalBranch string - PullRequests []*PullRequest + // HeadRepositoryID is the GitHub node ID of the repository that owns the + // PR head branches (the fork). Empty for same-fork PRs, in which case + // branches live in RepositoryID. When non-empty and different from + // RepositoryID, spr is operating in cross-fork mode: branches are pushed + // to the fork and PRs are opened against RepositoryID (the upstream). + HeadRepositoryID string + LocalBranch string + PullRequests []*PullRequest } type RepoAssignee struct { diff --git a/readme.md b/readme.md index 30fcee29..e42b8a8b 100644 --- a/readme.md +++ b/readme.md @@ -214,9 +214,9 @@ Configuration is created automatically on first run. Repository config lives in | `requireChecks` | bool | `true` | Require checks to pass in order to merge | | `requiredChecks` | list | | List of check names that must pass. When set, only these checks are evaluated; all others are ignored | | `requireApproval` | bool | `true` | Require PR approval in order to merge | -| `githubRepoOwner` | str | | GitHub owner (auto-detected from git remote) | -| `githubRepoName` | str | | GitHub repository name (auto-detected from git remote) | -| `githubRemote` | str | `origin` | Git remote name to use | +| `githubRepoOwner` | str | | GitHub owner of the **PR target** (auto-detected from the git remote; override to open cross-fork PRs — see [Working from a fork](#working-from-a-fork)) | +| `githubRepoName` | str | | GitHub repository name of the **PR target** (auto-detected from the git remote; override for cross-fork PRs) | +| `githubRemote` | str | `origin` | Git remote name spr pushes branches to (may be the fork) | | `githubBranch` | str | `main` | Target branch for pull requests | | `githubHost` | str | `github.com` | GitHub host (update for GitHub Enterprise) | | `mergeMethod` | str | `rebase` | Merge method: `rebase`, `squash`, or `merge` | @@ -260,12 +260,41 @@ defaultReviewers: | `statusBitsEmojis` | bool | `true` | Use emoji status bits | | `createDraftPRs` | bool | `false` | Create new PRs as drafts | | `preserveTitleAndBody` | bool | `false` | Don't overwrite PR title and body on update | +| `maintainerCanModify` | bool | `true` | For cross-fork PRs, allow upstream maintainers to push to the head branch on the fork. No effect for same-fork PRs | | `noRebase` | bool | `false` | Skip rebasing on `git spr update` | | `deleteMergedBranches` | bool | `false` | Delete branches after PRs are merged | | `branchPrefix` | str | `spr` | Prefix for spr-managed branch names | +### Working from a fork + +spr supports opening PRs from a fork into an upstream repository (e.g. you've cloned `your-org/their-project` as `you/their-project` and want to submit PRs back). Branches are pushed to the fork; PRs are opened against the upstream. + +Point `githubRepoOwner` / `githubRepoName` at the **upstream** in `.spr.yml`, and leave `githubRemote` at the fork (the default `origin` if that's how you cloned). spr detects that the two differ and routes the PR through GitHub's `headRepository` field. + +Example: clone of `jucor/spr` (a fork of `ejoffe/spr`) targeting upstream master: + +```yaml +# .spr.yml +githubRepoOwner: ejoffe # PR target +githubRepoName: spr # PR target +githubRemote: origin # pushes go to your fork +githubBranch: master +``` + +``` +$ git remote -v +origin git@github.com:jucor/spr.git (push) +upstream git@github.com:ejoffe/spr.git (push) +``` + +You need push access to the fork (you do — it's yours). GitHub allows anyone who can push to a fork to open a PR against the parent, so no special permission on the upstream is required. The `maintainerCanModify` user-config flag (default `true`) lets upstream maintainers push to the PR head branch on your fork. + +If owner/name match the remote URL, spr operates in same-fork mode (the previous behavior, unchanged). + +**Trade-off when stacking PRs cross-fork:** GitHub requires a PR's base branch to live in the same repo where the PR is filed. In same-fork mode (the only mode before this change) spr stacks PRs by setting each PR's base to the previous PR's head branch — that gives every PR a single-commit "Files changed" view. In cross-fork mode the previous PR's head lives in your fork, not in the upstream, so it can't be a valid base; every PR in the stack has to base on the upstream's target branch (usually `master`). The visible consequence: **each subsequent PR's "Files changed" tab shows the cumulative diff** from all predecessors, not just its own commit. The commits tab still shows commits individually. Code-review bots (e.g. Copilot) review the cumulative diff and may re-flag earlier-PR issues — for an N-deep stack expect roughly N× the review noise. Each PR collapses back to its own commit once its predecessor lands at upstream, so the noise window is bounded by your merge cadence. The only way to recover single-commit diffs across forks before merge is for the upstream maintainer to grant the contributor write access to the upstream repo, which lets the contributor push branches there directly — independent of (and orthogonal to) the `maintainerCanModify` flag above. + ## How it compares spr is similar to [Graphite](https://graphite.dev), [ghstack](https://github.com/ezyang/ghstack), and [Gerrit](https://www.gerritcodereview.com/)'s stacked review model -- but works purely with GitHub's native pull requests. No extra service, no custom merge bot, no lock-in.