diff --git a/cmd/amend/main.go b/cmd/amend/main.go index 3418d7ad..61717401 100644 --- a/cmd/amend/main.go +++ b/cmd/amend/main.go @@ -10,6 +10,7 @@ import ( "github.com/ejoffe/spr/git/realgit" "github.com/ejoffe/spr/github/githubclient" "github.com/ejoffe/spr/spr" + "github.com/ejoffe/spr/vcs" "github.com/jessevdk/go-flags" "github.com/rs/zerolog" "github.com/rs/zerolog/log" @@ -62,7 +63,8 @@ func main() { client := githubclient.NewGitHubClient(ctx, cfg) gitcmd = realgit.NewGitCmd(cfg) - sd := spr.NewStackedPR(cfg, client, gitcmd) + vcsOps := vcs.NewVCSOperations(cfg, gitcmd) + sd := spr.NewStackedPR(cfg, client, gitcmd, vcsOps) sd.AmendCommit(ctx) if opts.Update { diff --git a/cmd/spr/main.go b/cmd/spr/main.go index fc03ce53..d048eaf5 100644 --- a/cmd/spr/main.go +++ b/cmd/spr/main.go @@ -5,6 +5,7 @@ import ( "context" "fmt" "os" + "os/exec" "strings" "github.com/ejoffe/rake" @@ -13,6 +14,7 @@ import ( "github.com/ejoffe/spr/git/realgit" "github.com/ejoffe/spr/github/githubclient" "github.com/ejoffe/spr/spr" + "github.com/ejoffe/spr/vcs" "github.com/rs/zerolog" "github.com/rs/zerolog/log" @@ -88,9 +90,21 @@ func main() { } gitcmd = realgit.NewGitCmd(cfg) + // Check for --no-jj flag or SPR_NOJJ env var before creating VCS operations. + // This must happen before app.Run() since vcsOps is created here. + for _, arg := range os.Args[1:] { + if arg == "--no-jj" { + cfg.User.NoJJ = true + } + } + if os.Getenv("SPR_NOJJ") == "true" { + cfg.User.NoJJ = true + } + ctx := context.Background() client := githubclient.NewGitHubClient(ctx, cfg) - stackedpr := spr.NewStackedPR(cfg, client, gitcmd) + vcsOps := vcs.NewVCSOperations(cfg, gitcmd) + stackedpr := spr.NewStackedPR(cfg, client, gitcmd, vcsOps) detailFlag := &cli.BoolFlag{ Name: "detail", @@ -146,6 +160,12 @@ VERSION: fork of {{.Version}} Value: false, Usage: "Show runtime debug info", }, + &cli.BoolFlag{ + Name: "no-jj", + Value: false, + Usage: "Disable jj (Jujutsu) mode even in jj-colocated repos", + EnvVars: []string{"SPR_NOJJ"}, + }, }, Before: func(c *cli.Context) error { if c.IsSet("debug") { @@ -307,6 +327,25 @@ VERSION: fork of {{.Version}} return nil }, }, + { + Name: "jj-setup", + Usage: "Register 'jj spr' alias for use in Jujutsu repos", + Action: func(c *cli.Context) error { + cmd := exec.Command("jj", "config", "set", "--user", + "aliases.spr", `["util", "exec", "--", "git-spr"]`) + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + err := cmd.Run() + if err != nil { + return cli.Exit(fmt.Sprintf("Failed to set jj alias: %s", err), 1) + } + fmt.Println("jj alias registered. You can now use:") + fmt.Println(" jj spr update") + fmt.Println(" jj spr status") + fmt.Println(" jj spr merge") + return nil + }, + }, { Name: "version", Usage: "Show version info", diff --git a/config/config.go b/config/config.go index 8d363c8a..1a9d3936 100644 --- a/config/config.go +++ b/config/config.go @@ -39,7 +39,11 @@ type RepoConfig struct { ForceFetchTags bool `default:"false" yaml:"forceFetchTags"` + MultiCommitPRs bool `default:"false" yaml:"multiCommitPRs"` + ConcatCommitMessages bool `default:"true" yaml:"concatCommitMessages"` + ShowPrTitlesInStack bool `default:"false" yaml:"showPrTitlesInStack"` + ShowStackNumberInTitle bool `default:"false" yaml:"showStackNumberInTitle"` BranchPushIndividually bool `default:"false" yaml:"branchPushIndividually"` } @@ -53,6 +57,7 @@ type UserConfig struct { CreateDraftPRs bool `default:"false" yaml:"createDraftPRs"` PreserveTitleAndBody bool `default:"false" yaml:"preserveTitleAndBody"` NoRebase bool `default:"false" yaml:"noRebase"` + NoJJ bool `default:"false" yaml:"noJJ"` DeleteMergedBranches bool `default:"false" yaml:"deleteMergedBranches"` ShortPRLink bool `default:"false" yaml:"shortPRLink"` ShowCommitID bool `default:"false" yaml:"showCommitID"` diff --git a/config/config_test.go b/config/config_test.go index b322c581..f02022e1 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -34,6 +34,7 @@ func TestDefaultConfig(t *testing.T) { PRTemplatePath: "", PRTemplateInsertStart: "", PRTemplateInsertEnd: "", + ConcatCommitMessages: true, ShowPrTitlesInStack: false, }, User: &UserConfig{ diff --git a/git/commit.go b/git/commit.go index 60ccfe0f..e34e5af5 100644 --- a/git/commit.go +++ b/git/commit.go @@ -20,6 +20,10 @@ type Commit struct { // CommitHash is the git commit hash, this gets updated everytime the commit is amended. CommitHash string + // ChangeID is the jj (Jujutsu) change ID. Only populated in jj-colocated repos. + // Unlike CommitHash, the ChangeID is stable across rewrites done via jj commands. + ChangeID string + // Subject is the subject of the commit message. Subject string @@ -28,4 +32,8 @@ type Commit struct { // WIP is true if the commit is still work in progress. WIP bool + + // Branches holds local branch names (git) or bookmark names (jj) pointing at this commit. + // Only populated when MultiCommitPRs is enabled. + Branches []string } diff --git a/git/helpers.go b/git/helpers.go index 5b7c4c2c..bffc1ff2 100644 --- a/git/helpers.go +++ b/git/helpers.go @@ -31,6 +31,56 @@ func BranchNameFromCommit(cfg *config.Config, commit Commit) string { var BranchNameRegex = regexp.MustCompile(`spr/([a-zA-Z0-9_\-/\.]+)/([a-f0-9]{8})$`) +// IsSPRBranch returns true if branchName is an spr-managed remote branch (e.g. "spr/main/abc12345"). +func IsSPRBranch(branchName string) bool { + return BranchNameRegex.MatchString(branchName) +} + +// GetLocalBranchMap returns a map from commit hash to local branch names. +// It filters out spr/* branches and remote tracking branches. +func GetLocalBranchMap(gitcmd GitInterface) map[string][]string { + var output string + err := gitcmd.Git("for-each-ref --format=%(objectname) %(refname:short) refs/heads/", &output) + if err != nil { + return nil + } + result := make(map[string][]string) + for _, line := range strings.Split(strings.TrimSpace(output), "\n") { + if line == "" { + continue + } + parts := strings.SplitN(line, " ", 2) + if len(parts) != 2 { + continue + } + hash := parts[0] + branch := parts[1] + if IsSPRBranch(branch) { + continue + } + result[hash] = append(result[hash], branch) + } + return result +} + +// AnnotateCommitsWithBranches populates the Branches field on each commit +// using the given branch map. It excludes the target branch name. +func AnnotateCommitsWithBranches(commits []Commit, branchMap map[string][]string, targetBranch string) { + if branchMap == nil { + return + } + for i := range commits { + branches := branchMap[commits[i].CommitHash] + var filtered []string + for _, b := range branches { + if b != targetBranch && !IsSPRBranch(b) { + filtered = append(filtered, b) + } + } + commits[i].Branches = filtered + } +} + // 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_test.go b/git/helpers_test.go index 45fa566c..2d8e143c 100644 --- a/git/helpers_test.go +++ b/git/helpers_test.go @@ -1,6 +1,47 @@ package git -import "testing" +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestIsSPRBranch(t *testing.T) { + assert.True(t, IsSPRBranch("spr/main/deadbeef")) + assert.True(t, IsSPRBranch("spr/master/abc12345")) + assert.False(t, IsSPRBranch("my-feature")) + assert.False(t, IsSPRBranch("main")) + assert.False(t, IsSPRBranch("spr-related")) +} + +func TestAnnotateCommitsWithBranches(t *testing.T) { + commits := []Commit{ + {CommitHash: "hash1", CommitID: "00000001"}, + {CommitHash: "hash2", CommitID: "00000002"}, + {CommitHash: "hash3", CommitID: "00000003"}, + } + + branchMap := map[string][]string{ + "hash1": {"feature-a", "spr/main/abc12345"}, + "hash2": {"main"}, + "hash3": {"feature-b"}, + } + + AnnotateCommitsWithBranches(commits, branchMap, "main") + + // feature-a kept, spr branch filtered + assert.Equal(t, []string{"feature-a"}, commits[0].Branches) + // main filtered (target branch) + assert.Len(t, commits[1].Branches, 0) + // feature-b kept + assert.Equal(t, []string{"feature-b"}, commits[2].Branches) +} + +func TestAnnotateCommitsWithBranches_NilMap(t *testing.T) { + commits := []Commit{{CommitHash: "hash1", CommitID: "00000001"}} + AnnotateCommitsWithBranches(commits, nil, "main") + assert.Nil(t, commits[0].Branches) +} func TestBranchNameRegex(t *testing.T) { tests := []struct { diff --git a/git/mockgit/mockgit.go b/git/mockgit/mockgit.go index 1316d071..77e1d7d8 100644 --- a/git/mockgit/mockgit.go +++ b/git/mockgit/mockgit.go @@ -79,6 +79,11 @@ func (m *Mock) ExpectFetch() { m.expect("git rebase origin/master --autostash") } +func (m *Mock) ExpectFetchTags() { + m.expect("git fetch --tags --force") + m.expect("git rebase origin/master --autostash") +} + func (m *Mock) ExpectDeleteBranch(branchName string) { m.expect(fmt.Sprintf("git DeleteRemoteBranch(%s)", branchName)) } @@ -117,6 +122,7 @@ func (m *Mock) ExpectLocalBranch(name string) { m.expect("git branch --no-color").respond(name) } + func (m *Mock) expect(cmd string, args ...interface{}) *Mock { m.expectedCmd = append(m.expectedCmd, fmt.Sprintf(cmd, args...)) m.response = append(m.response, &commitResponse{valid: false}) diff --git a/github/githubclient/client.go b/github/githubclient/client.go index 2cec7c18..f37e3a91 100644 --- a/github/githubclient/client.go +++ b/github/githubclient/client.go @@ -173,7 +173,7 @@ type client struct { api genclient.Client } -func (c *client) GetInfo(ctx context.Context, gitcmd git.GitInterface) *github.GitHubInfo { +func (c *client) GetInfo(ctx context.Context, gitcmd git.GitInterface, localCommits []git.Commit) *github.GitHubInfo { if c.config.User.LogGitHubCalls { fmt.Printf("> github fetch pull requests\n") } @@ -200,7 +200,7 @@ func (c *client) GetInfo(ctx context.Context, gitcmd git.GitInterface) *github.G } targetBranch := c.config.Repo.GitHubBranch - localCommitStack := git.GetLocalCommitStack(c.config, gitcmd) + localCommitStack := localCommits pullRequests := matchPullRequestStack(c.config.Repo, targetBranch, localCommitStack, pullRequestConnection) for _, pr := range pullRequests { @@ -378,6 +378,21 @@ func (c *client) GetAssignableUsers(ctx context.Context) []github.RepoAssignee { return users } +// formatTitle adds a [Stack N/M] prefix to the title if configured. +func (c *client) formatTitle(title string, commit git.Commit, info *github.GitHubInfo) string { + if !c.config.Repo.ShowStackNumberInTitle { + return title + } + total := len(info.PullRequests) + for i, pr := range info.PullRequests { + if pr.Commit.CommitID == commit.CommitID { + return fmt.Sprintf("[Stack %d/%d] %s", i+1, total, title) + } + } + // Commit not yet in PR list (new PR being created) — count it + return fmt.Sprintf("[Stack %d/%d] %s", total+1, total+1, title) +} + func (c *client) CreatePullRequest(ctx context.Context, gitcmd git.GitInterface, info *github.GitHubInfo, commit git.Commit, prevCommit *git.Commit) *github.PullRequest { @@ -398,7 +413,7 @@ func (c *client) CreatePullRequest(ctx context.Context, gitcmd git.GitInterface, RepositoryId: info.RepositoryID, BaseRefName: baseRefName, HeadRefName: headRefName, - Title: templatizer.Title(info, commit), + Title: c.formatTitle(templatizer.Title(info, commit), commit, info), Body: &body, Draft: &c.config.User.CreateDraftPRs, }) @@ -445,7 +460,7 @@ func (c *client) UpdatePullRequest(ctx context.Context, gitcmd git.GitInterface, Interface("PR", pr).Msg("UpdatePullRequest") templatizer := config_fetcher.PRTemplatizer(c.config, gitcmd) - title := templatizer.Title(info, commit) + title := c.formatTitle(templatizer.Title(info, commit), commit, info) body := templatizer.Body(info, commit, pr) input := genclient.UpdatePullRequestInput{ PullRequestId: pr.ID, diff --git a/github/githubclient/client_test.go b/github/githubclient/client_test.go index c2cbd588..9da7bd06 100644 --- a/github/githubclient/client_test.go +++ b/github/githubclient/client_test.go @@ -1,6 +1,7 @@ package githubclient import ( + "context" "testing" "github.com/ejoffe/spr/config" @@ -528,3 +529,63 @@ func TestMatchPullRequestStack(t *testing.T) { }) } } + +// TestGetInfoShouldAcceptLocalCommits verifies that GetInfo accepts a +// localCommits parameter so the caller can provide commits from VCSOperations +// (e.g. jj log) instead of GetInfo fetching them via git.GetLocalCommitStack. +// +// RED: currently GetInfo only takes (ctx, gitcmd) — no commits parameter. +// GREEN: after fix, GetInfo takes (ctx, gitcmd, localCommits). +func TestGetInfoShouldAcceptLocalCommits(t *testing.T) { + // Define the interface we expect GetInfo to satisfy after the fix. + type getInfoWithCommits interface { + GetInfo(ctx context.Context, gitcmd git.GitInterface, localCommits []git.Commit) *github.GitHubInfo + } + + cfg := config.EmptyConfig() + c := &client{config: cfg} + + _, ok := interface{}(c).(getInfoWithCommits) + require.True(t, ok, + "GetInfo should accept a localCommits []git.Commit parameter so jj mode can provide commits") +} + +func TestFormatTitle_StackNumber(t *testing.T) { + prs := []*github.PullRequest{ + {Commit: git.Commit{CommitID: "aaa"}}, + {Commit: git.Commit{CommitID: "bbb"}}, + {Commit: git.Commit{CommitID: "ccc"}}, + } + info := &github.GitHubInfo{PullRequests: prs} + + t.Run("disabled", func(t *testing.T) { + cfg := config.EmptyConfig() + c := &client{config: cfg} + got := c.formatTitle("my title", git.Commit{CommitID: "bbb"}, info) + require.Equal(t, "my title", got) + }) + + t.Run("enabled_middle", func(t *testing.T) { + cfg := config.EmptyConfig() + cfg.Repo.ShowStackNumberInTitle = true + c := &client{config: cfg} + got := c.formatTitle("my title", git.Commit{CommitID: "bbb"}, info) + require.Equal(t, "[Stack 2/3] my title", got) + }) + + t.Run("enabled_first", func(t *testing.T) { + cfg := config.EmptyConfig() + cfg.Repo.ShowStackNumberInTitle = true + c := &client{config: cfg} + got := c.formatTitle("my title", git.Commit{CommitID: "aaa"}, info) + require.Equal(t, "[Stack 1/3] my title", got) + }) + + t.Run("enabled_new_commit", func(t *testing.T) { + cfg := config.EmptyConfig() + cfg.Repo.ShowStackNumberInTitle = true + c := &client{config: cfg} + got := c.formatTitle("my title", git.Commit{CommitID: "zzz"}, info) + require.Equal(t, "[Stack 4/4] my title", got) + }) +} diff --git a/github/interface.go b/github/interface.go index 18c25da6..8b502c32 100644 --- a/github/interface.go +++ b/github/interface.go @@ -8,8 +8,9 @@ import ( ) type GitHubInterface interface { - // GetInfo returns the list of pull requests from GitHub which match the local stack of commits - GetInfo(ctx context.Context, gitcmd git.GitInterface) *GitHubInfo + // GetInfo returns the list of pull requests from GitHub which match the local stack of commits. + // localCommits is the commit stack provided by VCSOperations (git log or jj log). + GetInfo(ctx context.Context, gitcmd git.GitInterface, localCommits []git.Commit) *GitHubInfo // GetAssignableUsers returns a list of valid GitHub users that can review the pull request GetAssignableUsers(ctx context.Context) []RepoAssignee @@ -38,6 +39,10 @@ type GitHubInfo struct { RepositoryID string LocalBranch string PullRequests []*PullRequest + + // GroupMap maps tip commit-id to all commits in a multi-commit PR group. + // Nil when MultiCommitPRs is disabled (single-commit mode). + GroupMap map[string][]git.Commit } type RepoAssignee struct { diff --git a/github/mockclient/mockclient.go b/github/mockclient/mockclient.go index bda9399d..859b079d 100644 --- a/github/mockclient/mockclient.go +++ b/github/mockclient/mockclient.go @@ -33,7 +33,7 @@ type MockClient struct { Synchronized bool // When true code is executed without goroutines. Allows test to be deterministic } -func (c *MockClient) GetInfo(ctx context.Context, gitcmd git.GitInterface) *github.GitHubInfo { +func (c *MockClient) GetInfo(ctx context.Context, gitcmd git.GitInterface, localCommits []git.Commit) *github.GitHubInfo { fmt.Printf("HUB: GetInfo\n") c.verifyExpectation(expectation{ op: getInfoOP, diff --git a/github/template/config_fetcher/config.go b/github/template/config_fetcher/config.go index b6e6b423..984b7331 100644 --- a/github/template/config_fetcher/config.go +++ b/github/template/config_fetcher/config.go @@ -13,7 +13,7 @@ import ( func PRTemplatizer(c *config.Config, gitcmd git.GitInterface) template.PRTemplatizer { switch c.Repo.PRTemplateType { case "stack": - return template_stack.NewStackTemplatizer(c.Repo.ShowPrTitlesInStack) + return template_stack.NewStackTemplatizer(c.Repo.ShowPrTitlesInStack, c.Repo.ConcatCommitMessages) case "basic": return template_basic.NewBasicTemplatizer() case "why_what": diff --git a/github/template/helpers.go b/github/template/helpers.go index 6926c2be..8a11c849 100644 --- a/github/template/helpers.go +++ b/github/template/helpers.go @@ -3,6 +3,7 @@ package template import ( "bytes" "fmt" + "strings" "github.com/ejoffe/spr/git" "github.com/ejoffe/spr/github" @@ -21,10 +22,110 @@ func ManualMergeNotice() string { "Do not merge manually using the UI - doing so may have unexpected results.*" } +// PRMetadata holds optional PR title/body overrides parsed from commit messages. +type PRMetadata struct { + Title string // from spr-pr-title: marker, or "" + Description string // from spr-pr-body: marker, or "" +} + +// ParsePRMetadata extracts optional spr-pr-title: and spr-pr-body: markers +// from a commit message body. +// +// spr-pr-title: is a single-line value. +// spr-pr-body: spans from the marker to ---end-spr-pr-body or end of text +// (whichever comes first), excluding other trailers like commit-id:. +func ParsePRMetadata(body string) PRMetadata { + var meta PRMetadata + lines := strings.Split(body, "\n") + + inBody := false + var bodyLines []string + + for _, line := range lines { + trimmed := strings.TrimSpace(line) + + if strings.HasPrefix(trimmed, "spr-pr-title:") { + meta.Title = strings.TrimSpace(strings.TrimPrefix(trimmed, "spr-pr-title:")) + continue + } + + if strings.HasPrefix(trimmed, "spr-pr-body:") { + inBody = true + rest := strings.TrimSpace(strings.TrimPrefix(trimmed, "spr-pr-body:")) + if rest != "" { + bodyLines = append(bodyLines, rest) + } + continue + } + + if inBody { + if trimmed == "---end-spr-pr-body" { + inBody = false + continue + } + // Stop at known trailers + if strings.HasPrefix(trimmed, "commit-id:") { + inBody = false + continue + } + bodyLines = append(bodyLines, line) + } + } + + if len(bodyLines) > 0 { + meta.Description = strings.TrimSpace(strings.Join(bodyLines, "\n")) + } + + return meta +} + +// FormatGroupCommits formats a list of commits for inclusion in a multi-commit PR body. +// Each commit is rendered as a bullet with subject, plus body if non-empty. +func FormatGroupCommits(commits []git.Commit) string { + var buf bytes.Buffer + for _, c := range commits { + // Strip spr metadata lines from display + cleanBody := StripSPRMetadata(c.Body) + if cleanBody != "" { + buf.WriteString(fmt.Sprintf("### %s\n\n%s\n\n", c.Subject, cleanBody)) + } else { + buf.WriteString(fmt.Sprintf("### %s\n\n", c.Subject)) + } + } + return buf.String() +} + +// stripSPRMetadata removes spr-pr-title:, spr-pr-body:, commit-id:, and +// ---end-spr-pr-body lines from a commit body for cleaner display. +func StripSPRMetadata(body string) string { + lines := strings.Split(body, "\n") + var result []string + inBody := false + for _, line := range lines { + trimmed := strings.TrimSpace(line) + if strings.HasPrefix(trimmed, "spr-pr-title:") || + strings.HasPrefix(trimmed, "commit-id:") { + continue + } + if strings.HasPrefix(trimmed, "spr-pr-body:") { + inBody = true + continue + } + if inBody { + if trimmed == "---end-spr-pr-body" { + inBody = false + } + continue + } + result = append(result, line) + } + return strings.TrimSpace(strings.Join(result, "\n")) +} + func FormatStackMarkdown(commit git.Commit, stack []*github.PullRequest, showPrTitlesInStack bool) string { var buf bytes.Buffer for i := len(stack) - 1; i >= 0; i-- { - isCurrent := stack[i].Commit == commit + isCurrent := stack[i].Commit.CommitID == commit.CommitID var suffix string if isCurrent { suffix = " ⬅" diff --git a/github/template/helpers_test.go b/github/template/helpers_test.go new file mode 100644 index 00000000..67683891 --- /dev/null +++ b/github/template/helpers_test.go @@ -0,0 +1,107 @@ +package template + +import ( + "testing" + + "github.com/ejoffe/spr/git" + "github.com/stretchr/testify/assert" +) + +func TestParsePRMetadata_TitleOnly(t *testing.T) { + body := "some text\nspr-pr-title: My Custom Title\ncommit-id:abc12345" + meta := ParsePRMetadata(body) + assert.Equal(t, "My Custom Title", meta.Title) + assert.Equal(t, "", meta.Description) +} + +func TestParsePRMetadata_TitleAndBody(t *testing.T) { + body := `some text + +spr-pr-title: My PR Title +spr-pr-body: This is the PR description. + +It has multiple paragraphs. +---end-spr-pr-body + +commit-id:abc12345` + + meta := ParsePRMetadata(body) + assert.Equal(t, "My PR Title", meta.Title) + assert.Equal(t, "This is the PR description.\n\nIt has multiple paragraphs.", meta.Description) +} + +func TestParsePRMetadata_BodyWithoutEndMarker(t *testing.T) { + body := `spr-pr-body: Description text. + +More text. + +commit-id:abc12345` + + meta := ParsePRMetadata(body) + assert.Equal(t, "", meta.Title) + assert.Equal(t, "Description text.\n\nMore text.", meta.Description) +} + +func TestParsePRMetadata_NoMarkers(t *testing.T) { + body := "just a normal commit body\n\ncommit-id:abc12345" + meta := ParsePRMetadata(body) + assert.Equal(t, "", meta.Title) + assert.Equal(t, "", meta.Description) +} + +func TestParsePRMetadata_Empty(t *testing.T) { + meta := ParsePRMetadata("") + assert.Equal(t, "", meta.Title) + assert.Equal(t, "", meta.Description) +} + +func TestParsePRMetadata_BodyOnSameLine(t *testing.T) { + body := "spr-pr-body: Single line description\n---end-spr-pr-body\ncommit-id:abc12345" + meta := ParsePRMetadata(body) + assert.Equal(t, "Single line description", meta.Description) +} + +func TestFormatGroupCommits_MultipleCommits(t *testing.T) { + commits := []git.Commit{ + {Subject: "Add feature A", Body: "Details about A\n\ncommit-id:00000001"}, + {Subject: "Add feature B", Body: "commit-id:00000002"}, + } + + result := FormatGroupCommits(commits) + assert.Contains(t, result, "### Add feature A") + assert.Contains(t, result, "Details about A") + assert.Contains(t, result, "### Add feature B") + // commit-id should be stripped + assert.NotContains(t, result, "commit-id:") +} + +func TestFormatGroupCommits_SingleCommit(t *testing.T) { + commits := []git.Commit{ + {Subject: "Only commit", Body: ""}, + } + + result := FormatGroupCommits(commits) + assert.Contains(t, result, "### Only commit") +} + +func TestStripSPRMetadata(t *testing.T) { + body := `Some description + +spr-pr-title: Title +spr-pr-body: Body text +---end-spr-pr-body + +commit-id:abc12345` + + result := StripSPRMetadata(body) + assert.Equal(t, "Some description", result) + assert.NotContains(t, result, "spr-pr-title") + assert.NotContains(t, result, "spr-pr-body") + assert.NotContains(t, result, "commit-id") +} + +func TestStripSPRMetadata_NoMetadata(t *testing.T) { + body := "Just a normal body\n\nWith paragraphs" + result := StripSPRMetadata(body) + assert.Equal(t, "Just a normal body\n\nWith paragraphs", result) +} diff --git a/github/template/template_basic/template.go b/github/template/template_basic/template.go index d253df07..aa0d2b94 100644 --- a/github/template/template_basic/template.go +++ b/github/template/template_basic/template.go @@ -13,11 +13,35 @@ func NewBasicTemplatizer() *BasicTemplatizer { } func (t *BasicTemplatizer) Title(info *github.GitHubInfo, commit git.Commit) string { + if info.GroupMap != nil { + meta := template.ParsePRMetadata(commit.Body) + if meta.Title != "" { + return meta.Title + } + } return commit.Subject } func (t *BasicTemplatizer) Body(info *github.GitHubInfo, commit git.Commit, pr *github.PullRequest) string { - body := commit.Body + var body string + if info.GroupMap != nil { + groupCommits := info.GroupMap[commit.CommitID] + meta := template.ParsePRMetadata(commit.Body) + if meta.Description != "" { + body = meta.Description + } else { + body = template.StripSPRMetadata(commit.Body) + } + if len(groupCommits) > 1 { + if body != "" { + body += "\n\n" + } + body += "---\n**Commits**:\n\n" + body += template.FormatGroupCommits(groupCommits) + } + } else { + body = commit.Body + } body += "\n\n" body += template.ManualMergeNotice() return body diff --git a/github/template/template_custom/template.go b/github/template/template_custom/template.go index c6453bea..e3d78713 100644 --- a/github/template/template_custom/template.go +++ b/github/template/template_custom/template.go @@ -33,6 +33,12 @@ func NewCustomTemplatizer( } func (t *CustomTemplatizer) Title(info *github.GitHubInfo, commit git.Commit) string { + if info.GroupMap != nil { + meta := template.ParsePRMetadata(commit.Body) + if meta.Title != "" { + return meta.Title + } + } return commit.Subject } diff --git a/github/template/template_stack/template.go b/github/template/template_stack/template.go index 5d342657..6e94841a 100644 --- a/github/template/template_stack/template.go +++ b/github/template/template_stack/template.go @@ -1,6 +1,7 @@ package template_stack import ( + "github.com/ejoffe/spr/config" "github.com/ejoffe/spr/git" "github.com/ejoffe/spr/github" "github.com/ejoffe/spr/github/template" @@ -8,18 +9,41 @@ import ( type StackTemplatizer struct { showPrTitlesInStack bool + concatCommitMessages bool } -func NewStackTemplatizer(showPrTitlesInStack bool) *StackTemplatizer { - return &StackTemplatizer{showPrTitlesInStack: showPrTitlesInStack} +func NewStackTemplatizer(showPrTitlesInStack bool, concatCommitMessages ...bool) *StackTemplatizer { + concat := true // default + if len(concatCommitMessages) > 0 { + concat = concatCommitMessages[0] + } + return &StackTemplatizer{ + showPrTitlesInStack: showPrTitlesInStack, + concatCommitMessages: concat, + } } func (t *StackTemplatizer) Title(info *github.GitHubInfo, commit git.Commit) string { + // In multi-commit mode, check for spr-pr-title: marker in tip commit + if info.GroupMap != nil { + meta := template.ParsePRMetadata(commit.Body) + if meta.Title != "" { + return meta.Title + } + } return commit.Subject } func (t *StackTemplatizer) Body(info *github.GitHubInfo, commit git.Commit, pr *github.PullRequest) string { - body := commit.Body + var body string + + if info.GroupMap != nil { + // Multi-commit mode: use PR metadata and/or concatenated commit messages + groupCommits := info.GroupMap[commit.CommitID] + body = t.multiCommitBody(commit, groupCommits) + } else { + body = commit.Body + } // Always show stack section and notice body += "\n\n" @@ -30,3 +54,37 @@ func (t *StackTemplatizer) Body(info *github.GitHubInfo, commit git.Commit, pr * body += template.ManualMergeNotice() return body } + +// multiCommitBody builds the PR body for a multi-commit PR. +// Priority: spr-pr-body marker > tip commit body > empty. +// Then optionally appends concatenated commit messages. +func (t *StackTemplatizer) multiCommitBody(tipCommit git.Commit, groupCommits []git.Commit) string { + meta := template.ParsePRMetadata(tipCommit.Body) + + var body string + if meta.Description != "" { + body = meta.Description + } else { + // Fall back to tip commit body (stripped of spr metadata) + body = template.StripSPRMetadata(tipCommit.Body) + } + + if t.concatCommitMessages && len(groupCommits) > 1 { + if body != "" { + body += "\n\n" + } + body += "---\n" + body += "**Commits**:\n\n" + body += template.FormatGroupCommits(groupCommits) + } + + return body +} + +// NewStackTemplatizerFromConfig creates a StackTemplatizer from full config. +func NewStackTemplatizerFromConfig(cfg *config.RepoConfig) *StackTemplatizer { + return &StackTemplatizer{ + showPrTitlesInStack: cfg.ShowPrTitlesInStack, + concatCommitMessages: cfg.ConcatCommitMessages, + } +} diff --git a/github/template/template_why_what/template.go b/github/template/template_why_what/template.go index c81f49ad..cfe2ff67 100644 --- a/github/template/template_why_what/template.go +++ b/github/template/template_why_what/template.go @@ -17,6 +17,12 @@ func NewWhyWhatTemplatizer() *WhyWhatTemplatizer { } func (t *WhyWhatTemplatizer) Title(info *github.GitHubInfo, commit git.Commit) string { + if info.GroupMap != nil { + meta := template.ParsePRMetadata(commit.Body) + if meta.Title != "" { + return meta.Title + } + } return commit.Subject } diff --git a/readme.md b/readme.md index 05f791a9..f4318175 100644 --- a/readme.md +++ b/readme.md @@ -29,7 +29,7 @@ Commands | `git spr check` | | Run pre-merge checks (configured by `mergeCheck`) | | `git spr version` | | Show version info | -**Global flags:** `--detail` (show status bit headers), `--verbose` (log git commands and GitHub API calls), `--debug`, `--profile` +**Global flags:** `--detail` (show status bit headers), `--verbose` (log git commands and GitHub API calls), `--no-jj` (disable jj mode), `--debug`, `--profile` Installation ------------ @@ -230,10 +230,44 @@ User specific configuration is saved to .spr.yml in the user home directory. | createDraftPRs | bool | false | new pull requests are created as draft | | preserveTitleAndBody | bool | false | updating pull requests will not overwrite the pr title and body | | noRebase | bool | false | when true spr update will not rebase on top of origin | +| noJJ | bool | false | disable jj (Jujutsu) mode even in jj-colocated repos (also `--no-jj` flag or `SPR_NOJJ` env var) | | deleteMergedBranches | bool | false | delete branches after prs are merged | | shortPRLink | bool | false | show pull request links as clickable PR- instead of full URL | | showCommitID | bool | false | show first 8 characters of commit hash for each pull request | +Jujutsu (jj) Support +-------------------- +spr supports [Jujutsu](https://jj-vcs.github.io/jj/) colocated repositories. When spr detects a `.jj/` directory in your repo root, it automatically uses jj-native commands for history-rewriting operations (`jj describe`, `jj rebase`, `jj squash`, `jj edit`) instead of `git rebase`. This preserves jj change IDs across all spr operations. + +Everything else (push, branch management, GitHub API calls) continues to use git, which works identically in colocated repos. + +**Setup:** +```shell +# 1. Initialize jj in your existing git repo (if not already) +jj git init --colocate + +# 2. Register the jj alias so you can use "jj spr" instead of "git spr" +git spr jj-setup +``` + +The `jj-setup` command adds a jj alias so spr can be invoked as `jj spr`: +```shell +jj spr update # create/update PRs +jj spr status # show PR status +jj spr merge # merge PRs +jj spr amend # amend a commit in the stack +``` + +You can also set up the alias manually: +```shell +jj config set --user aliases.spr '["util", "exec", "--", "git-spr"]' +``` + +**Opt-out:** If you have a `.jj/` directory but want spr to use git mode: +- CLI flag: `jj spr update --no-jj` +- Environment variable: `SPR_NOJJ=true` +- Config: add `noJJ: true` to `~/.spr.yml` + Happy Coding! ------------- If you find a bug, feel free to open an issue. Pull requests are welcome. diff --git a/spr/grouping.go b/spr/grouping.go new file mode 100644 index 00000000..4528b103 --- /dev/null +++ b/spr/grouping.go @@ -0,0 +1,101 @@ +package spr + +import ( + "github.com/ejoffe/spr/git" + "github.com/rs/zerolog/log" +) + +// PRGroup represents a group of consecutive commits that form a single PR. +// The group is delimited by a local branch (git) or bookmark (jj) at the tip commit. +type PRGroup struct { + // LocalBranch is the local branch/bookmark name marking the tip of this PR group. + LocalBranch string + + // Commits in this group, ordered bottom-first (oldest first). + Commits []git.Commit +} + +// TipCommit returns the top (most recent) commit in the group — the one +// the branch/bookmark points to. +func (g PRGroup) TipCommit() git.Commit { + return g.Commits[len(g.Commits)-1] +} + +// IsWIP returns true if the tip commit is marked as work in progress. +func (g PRGroup) IsWIP() bool { + return g.TipCommit().WIP +} + +// GroupCommitsIntoPRs groups a linear commit stack by branch/bookmark boundaries. +// +// Every commit that has a qualifying local branch (not spr/*, not targetBranch) +// marks the end of a PR group. Commits above the last branch are treated as WIP +// and returned separately. +// +// Returns: +// - groups: PR groups ordered bottom-first, each with its branch name and commits +// - wipCommits: commits above the last branch (no PR will be created for these) +func GroupCommitsIntoPRs(commits []git.Commit, targetBranch string) (groups []PRGroup, wipCommits []git.Commit) { + var currentCommits []git.Commit + + for _, c := range commits { + currentCommits = append(currentCommits, c) + + branch := qualifyingBranch(c, targetBranch) + if branch != "" { + groups = append(groups, PRGroup{ + LocalBranch: branch, + Commits: currentCommits, + }) + currentCommits = nil + } + } + + // Any remaining commits after the last branch are WIP + wipCommits = currentCommits + return groups, wipCommits +} + +// qualifyingBranch returns the first qualifying branch name on a commit, +// or "" if none. Qualifying means: not an spr/* branch and not the target branch. +func qualifyingBranch(c git.Commit, targetBranch string) string { + if len(c.Branches) == 0 { + return "" + } + var selected string + count := 0 + for _, b := range c.Branches { + if b == targetBranch || git.IsSPRBranch(b) { + continue + } + count++ + if selected == "" { + selected = b + } + } + if count > 1 { + log.Warn().Str("selected", selected).Int("total", count). + Msg("multiple qualifying branches on commit; using first one") + } + return selected +} + +// TipCommits extracts the tip commit from each group, suitable for use +// in place of the flat localCommits list in the existing spr flow. +func TipCommits(groups []PRGroup) []git.Commit { + tips := make([]git.Commit, len(groups)) + for i, g := range groups { + tips[i] = g.TipCommit() + } + return tips +} + +// BuildGroupMap creates a map from tip commit-id to the full list of commits +// in that group. Used by templates to generate multi-commit PR bodies. +func BuildGroupMap(groups []PRGroup) map[string][]git.Commit { + m := make(map[string][]git.Commit, len(groups)) + for _, g := range groups { + m[g.TipCommit().CommitID] = g.Commits + } + return m +} diff --git a/spr/grouping_test.go b/spr/grouping_test.go new file mode 100644 index 00000000..c61735d1 --- /dev/null +++ b/spr/grouping_test.go @@ -0,0 +1,181 @@ +package spr + +import ( + "testing" + + "github.com/ejoffe/spr/git" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestGroupCommitsIntoPRs_BasicGrouping(t *testing.T) { + // 5 commits, branches at c2 and c4 → 2 groups + commits := []git.Commit{ + {CommitID: "00000001", Subject: "c1"}, + {CommitID: "00000002", Subject: "c2", Branches: []string{"feature-a"}}, + {CommitID: "00000003", Subject: "c3"}, + {CommitID: "00000004", Subject: "c4", Branches: []string{"feature-b"}}, + {CommitID: "00000005", Subject: "c5"}, + } + + groups, wip := GroupCommitsIntoPRs(commits, "main") + + require.Len(t, groups, 2) + assert.Equal(t, "feature-a", groups[0].LocalBranch) + assert.Len(t, groups[0].Commits, 2) + assert.Equal(t, "00000001", groups[0].Commits[0].CommitID) + assert.Equal(t, "00000002", groups[0].Commits[1].CommitID) + assert.Equal(t, "00000002", groups[0].TipCommit().CommitID) + + assert.Equal(t, "feature-b", groups[1].LocalBranch) + assert.Len(t, groups[1].Commits, 2) + assert.Equal(t, "00000003", groups[1].Commits[0].CommitID) + assert.Equal(t, "00000004", groups[1].Commits[1].CommitID) + + // c5 is WIP (above last branch) + require.Len(t, wip, 1) + assert.Equal(t, "00000005", wip[0].CommitID) +} + +func TestGroupCommitsIntoPRs_NoBranches(t *testing.T) { + commits := []git.Commit{ + {CommitID: "00000001", Subject: "c1"}, + {CommitID: "00000002", Subject: "c2"}, + } + + groups, wip := GroupCommitsIntoPRs(commits, "main") + + assert.Len(t, groups, 0) + assert.Len(t, wip, 2) +} + +func TestGroupCommitsIntoPRs_SingleBranchAtTop(t *testing.T) { + commits := []git.Commit{ + {CommitID: "00000001", Subject: "c1"}, + {CommitID: "00000002", Subject: "c2"}, + {CommitID: "00000003", Subject: "c3", Branches: []string{"my-feature"}}, + } + + groups, wip := GroupCommitsIntoPRs(commits, "main") + + require.Len(t, groups, 1) + assert.Equal(t, "my-feature", groups[0].LocalBranch) + assert.Len(t, groups[0].Commits, 3) + assert.Len(t, wip, 0) +} + +func TestGroupCommitsIntoPRs_FiltersSPRBranches(t *testing.T) { + commits := []git.Commit{ + {CommitID: "00000001", Subject: "c1", Branches: []string{"spr/main/abc12345"}}, + {CommitID: "00000002", Subject: "c2", Branches: []string{"real-branch"}}, + } + + groups, wip := GroupCommitsIntoPRs(commits, "main") + + require.Len(t, groups, 1) + assert.Equal(t, "real-branch", groups[0].LocalBranch) + assert.Len(t, groups[0].Commits, 2) + assert.Len(t, wip, 0) +} + +func TestGroupCommitsIntoPRs_FiltersTargetBranch(t *testing.T) { + commits := []git.Commit{ + {CommitID: "00000001", Subject: "c1", Branches: []string{"main"}}, + {CommitID: "00000002", Subject: "c2"}, + } + + groups, wip := GroupCommitsIntoPRs(commits, "main") + + assert.Len(t, groups, 0) + assert.Len(t, wip, 2) +} + +func TestGroupCommitsIntoPRs_MultipleBranchesOnSameCommit(t *testing.T) { + commits := []git.Commit{ + {CommitID: "00000001", Subject: "c1", Branches: []string{"branch-a", "branch-b"}}, + } + + groups, wip := GroupCommitsIntoPRs(commits, "main") + + require.Len(t, groups, 1) + assert.Equal(t, "branch-a", groups[0].LocalBranch) // uses first + assert.Len(t, wip, 0) +} + +func TestGroupCommitsIntoPRs_WIPAtTop(t *testing.T) { + commits := []git.Commit{ + {CommitID: "00000001", Subject: "c1", Branches: []string{"feature"}}, + {CommitID: "00000002", Subject: "WIP c2"}, + {CommitID: "00000003", Subject: "WIP c3"}, + } + + groups, wip := GroupCommitsIntoPRs(commits, "main") + + require.Len(t, groups, 1) + assert.Equal(t, "feature", groups[0].LocalBranch) + assert.Len(t, wip, 2) +} + +func TestGroupCommitsIntoPRs_EmptyInput(t *testing.T) { + groups, wip := GroupCommitsIntoPRs(nil, "main") + assert.Len(t, groups, 0) + assert.Len(t, wip, 0) +} + +func TestTipCommits(t *testing.T) { + groups := []PRGroup{ + { + LocalBranch: "a", + Commits: []git.Commit{ + {CommitID: "00000001"}, + {CommitID: "00000002"}, + }, + }, + { + LocalBranch: "b", + Commits: []git.Commit{ + {CommitID: "00000003"}, + }, + }, + } + + tips := TipCommits(groups) + require.Len(t, tips, 2) + assert.Equal(t, "00000002", tips[0].CommitID) + assert.Equal(t, "00000003", tips[1].CommitID) +} + +func TestBuildGroupMap(t *testing.T) { + groups := []PRGroup{ + { + LocalBranch: "a", + Commits: []git.Commit{ + {CommitID: "00000001"}, + {CommitID: "00000002"}, + }, + }, + } + + gm := BuildGroupMap(groups) + require.Len(t, gm, 1) + commits, ok := gm["00000002"] + require.True(t, ok) + assert.Len(t, commits, 2) +} + +func TestPRGroup_IsWIP(t *testing.T) { + group := PRGroup{ + Commits: []git.Commit{ + {CommitID: "00000001", Subject: "normal"}, + {CommitID: "00000002", Subject: "WIP stuff", WIP: true}, + }, + } + assert.True(t, group.IsWIP()) + + group2 := PRGroup{ + Commits: []git.Commit{ + {CommitID: "00000001", Subject: "normal"}, + }, + } + assert.False(t, group2.IsWIP()) +} diff --git a/spr/spr.go b/spr/spr.go index d5f12688..de9e5137 100644 --- a/spr/spr.go +++ b/spr/spr.go @@ -9,7 +9,6 @@ import ( "os" "os/exec" "os/signal" - "path/filepath" "strconv" "strings" "sync" @@ -21,14 +20,23 @@ import ( "github.com/ejoffe/spr/config/config_parser" "github.com/ejoffe/spr/git" "github.com/ejoffe/spr/github" + "github.com/ejoffe/spr/vcs" ) // NewStackedPR constructs and returns a new stackediff instance. -func NewStackedPR(config *config.Config, github github.GitHubInterface, gitcmd git.GitInterface) *stackediff { +// If vcsOps is nil, a default git-based implementation is created. +func NewStackedPR(config *config.Config, github github.GitHubInterface, gitcmd git.GitInterface, vcsOps ...vcs.VCSOperations) *stackediff { + var ops vcs.VCSOperations + if len(vcsOps) > 0 && vcsOps[0] != nil { + ops = vcsOps[0] + } else { + ops = vcs.NewGitOps(config, gitcmd) + } return &stackediff{ config: config, github: github, gitcmd: gitcmd, + vcsOps: ops, profiletimer: profiletimer.StartNoopTimer(), output: os.Stdout, @@ -40,6 +48,7 @@ type stackediff struct { config *config.Config github github.GitHubInterface gitcmd git.GitInterface + vcsOps vcs.VCSOperations profiletimer profiletimer.Timer DetailEnabled bool @@ -48,11 +57,27 @@ type stackediff struct { synchronized bool // When true code is executed without goroutines. Allows test to be deterministic } +// confirmIfIncompleteStack checks whether the working copy position might cause +// spr to see an incomplete stack, and if so, warns the user and asks for +// confirmation. Returns true if the user wants to proceed (or no warning). +func (sd *stackediff) confirmIfIncompleteStack() bool { + warning := sd.vcsOps.CheckStackCompleteness() + if warning == "" { + return true + } + fmt.Fprintf(sd.output, "%s\n", warning) + fmt.Fprintf(sd.output, "Continue anyway? [y/N] ") + reader := bufio.NewReader(sd.input) + line, _ := reader.ReadString('\n') + line = strings.TrimSpace(strings.ToLower(line)) + return line == "y" || line == "yes" +} + // AmendCommit enables one to easily amend a commit in the middle of a stack // // of commits. A list of commits is printed and one can be chosen to be amended. func (sd *stackediff) AmendCommit(ctx context.Context) { - localCommits := git.GetLocalCommitStack(sd.config, sd.gitcmd) + localCommits := sd.vcsOps.GetLocalCommitStack(sd.config, sd.gitcmd) if len(localCommits) == 0 { fmt.Fprintf(sd.output, "No commits to amend\n") return @@ -79,20 +104,16 @@ func (sd *stackediff) AmendCommit(ctx context.Context) { } commitIndex = commitIndex - 1 check(err) - sd.gitcmd.MustGit("commit --fixup "+localCommits[commitIndex].CommitHash, nil) - - rebaseCmd := fmt.Sprintf("rebase -i --autosquash --autostash %s/%s", - sd.config.Repo.GitHubRemote, sd.config.Repo.GitHubBranch) - sd.gitcmd.MustGit(rebaseCmd, nil) + err = sd.vcsOps.AmendInto(localCommits[commitIndex]) + check(err) } func (sd *stackediff) editStatePath() string { - return filepath.Join(sd.gitcmd.RootDir(), ".git", "spr_edit_state") + return sd.vcsOps.EditStatePath() } func (sd *stackediff) isEditing() bool { - _, err := os.Stat(sd.editStatePath()) - return err == nil + return sd.vcsOps.IsEditing() } // EditCommit starts an interactive edit session on a commit in the stack. @@ -107,7 +128,7 @@ func (sd *stackediff) EditCommit(ctx context.Context) { return } - localCommits := git.GetLocalCommitStack(sd.config, sd.gitcmd) + localCommits := sd.vcsOps.GetLocalCommitStack(sd.config, sd.gitcmd) if len(localCommits) == 0 { fmt.Fprintf(sd.output, "No commits to edit\n") return @@ -136,23 +157,8 @@ func (sd *stackediff) EditCommit(ctx context.Context) { targetCommit := localCommits[commitIndex] - // Write state file so --done knows we're in an edit session - stateContent := fmt.Sprintf("commit_id=%s\ncommit_subject=%s\n", targetCommit.CommitID, targetCommit.Subject) - err = os.WriteFile(sd.editStatePath(), []byte(stateContent), 0644) - check(err) - - // Use the spr binary itself as the sequence editor to rewrite 'pick' to 'edit' - // for the target commit. Git invokes the editor as: - exe, err := os.Executable() - check(err) - editorCmd := fmt.Sprintf("%s _edit-sequence %s", exe, targetCommit.CommitHash[:7]) - - rebaseCmd := fmt.Sprintf("rebase -i --autostash %s/%s", - sd.config.Repo.GitHubRemote, sd.config.Repo.GitHubBranch) - err = sd.gitcmd.GitWithEditor(rebaseCmd, nil, editorCmd) + err = sd.vcsOps.EditStart(targetCommit) if err != nil { - // Clean up state file on failure - os.Remove(sd.editStatePath()) fmt.Fprintf(sd.output, "Failed to start edit session: %s\n", err) return } @@ -171,26 +177,13 @@ func (sd *stackediff) EditCommitDone(ctx context.Context, update bool) { return } - // Stage all changes - sd.gitcmd.MustGit("add -A", nil) - - // Amend the current commit (no-edit keeps the original message) - err := sd.gitcmd.Git("commit --amend --no-edit", nil) + err := sd.vcsOps.EditFinish() if err != nil { - fmt.Fprintf(sd.output, "Failed to amend commit: %s\n", err) + fmt.Fprintf(sd.output, "Edit finish failed: %s\n", err) fmt.Fprintf(sd.output, "Resolve any issues and try again.\n") return } - // Continue the rebase to replay the remaining commits - err = sd.gitcmd.Git("rebase --continue", nil) - if err != nil { - fmt.Fprintf(sd.output, "Rebase conflict detected. Resolve conflicts and run 'git spr edit --done' again.\n") - return - } - - // Clean up state file - os.Remove(sd.editStatePath()) fmt.Fprintf(sd.output, "Stack restored successfully.\n") if update { @@ -205,13 +198,12 @@ func (sd *stackediff) EditCommitAbort(ctx context.Context) { return } - err := sd.gitcmd.Git("rebase --abort", nil) + err := sd.vcsOps.EditAbort() if err != nil { fmt.Fprintf(sd.output, "Failed to abort: %s\n", err) return } - os.Remove(sd.editStatePath()) fmt.Fprintf(sd.output, "Edit session aborted.\n") } @@ -264,6 +256,9 @@ func alignLocalCommits(commits []git.Commit, prs []*github.PullRequest) []git.Co // In the case where commits are reordered, the corresponding pull requests // will also be reordered to match the commit stack order. func (sd *stackediff) UpdatePullRequests(ctx context.Context, reviewers []string, count *uint) { + if !sd.confirmIfIncompleteStack() { + return + } sd.profiletimer.Step("UpdatePullRequests::Start") reviewers = append(sd.config.Repo.DefaultReviewers, reviewers...) githubInfo := sd.fetchAndGetGitHubInfo(ctx) @@ -271,7 +266,29 @@ func (sd *stackediff) UpdatePullRequests(ctx context.Context, reviewers []string return } sd.profiletimer.Step("UpdatePullRequests::FetchAndGetGitHubInfo") - localCommits := alignLocalCommits(git.GetLocalCommitStack(sd.config, sd.gitcmd), githubInfo.PullRequests) + + allLocalCommits := sd.vcsOps.GetLocalCommitStack(sd.config, sd.gitcmd) + + var localCommits []git.Commit + if sd.config.Repo.MultiCommitPRs { + groups, wipCommits := GroupCommitsIntoPRs(allLocalCommits, sd.config.Repo.GitHubBranch) + if len(groups) == 0 { + if len(wipCommits) > 0 { + fmt.Fprintf(sd.output, "warning: no local branches/bookmarks found in stack — create branches to define PR boundaries\n") + fmt.Fprintf(sd.output, " (%d commit(s) treated as WIP)\n", len(wipCommits)) + } else { + fmt.Fprintf(sd.output, "pull request stack is empty\n") + } + return + } + if len(wipCommits) > 0 { + fmt.Fprintf(sd.output, "note: %d commit(s) above last branch treated as WIP\n", len(wipCommits)) + } + localCommits = TipCommits(groups) + githubInfo.GroupMap = BuildGroupMap(groups) + } else { + localCommits = alignLocalCommits(allLocalCommits, githubInfo.PullRequests) + } sd.profiletimer.Step("UpdatePullRequests::GetLocalCommitStack") // close prs for deleted commits @@ -412,13 +429,24 @@ func (sd *stackediff) UpdatePullRequests(ctx context.Context, reviewers []string // We than close all the pull requests which are below the merged request, as // their commits have already been merged. func (sd *stackediff) MergePullRequests(ctx context.Context, count *uint) { + if !sd.confirmIfIncompleteStack() { + return + } sd.profiletimer.Step("MergePullRequests::Start") - githubInfo := sd.github.GetInfo(ctx, sd.gitcmd) + allLocalCommits := sd.vcsOps.GetLocalCommitStack(sd.config, sd.gitcmd) + + var localCommits []git.Commit + if sd.config.Repo.MultiCommitPRs { + groups, _ := GroupCommitsIntoPRs(allLocalCommits, sd.config.Repo.GitHubBranch) + localCommits = TipCommits(groups) + } else { + localCommits = allLocalCommits + } + githubInfo := sd.github.GetInfo(ctx, sd.gitcmd, localCommits) sd.profiletimer.Step("MergePullRequests::getGitHubInfo") // MergeCheck if sd.config.Repo.MergeCheck != "" { - localCommits := git.GetLocalCommitStack(sd.config, sd.gitcmd) if len(localCommits) > 0 { lastCommit := localCommits[len(localCommits)-1] checkedCommit, found := sd.config.State.MergeCheckCommit[githubInfo.Key()] @@ -493,8 +521,20 @@ func (sd *stackediff) MergePullRequests(ctx context.Context, count *uint) { // prints out the status of each. It does not make any updates locally or // remotely on github. func (sd *stackediff) StatusPullRequests(ctx context.Context) { + if !sd.confirmIfIncompleteStack() { + return + } sd.profiletimer.Step("StatusPullRequests::Start") - githubInfo := sd.github.GetInfo(ctx, sd.gitcmd) + allLocalCommits := sd.vcsOps.GetLocalCommitStack(sd.config, sd.gitcmd) + + var localCommits []git.Commit + if sd.config.Repo.MultiCommitPRs { + groups, _ := GroupCommitsIntoPRs(allLocalCommits, sd.config.Repo.GitHubBranch) + localCommits = TipCommits(groups) + } else { + localCommits = allLocalCommits + } + githubInfo := sd.github.GetInfo(ctx, sd.gitcmd, localCommits) if len(githubInfo.PullRequests) == 0 { fmt.Fprintf(sd.output, "pull request stack is empty\n") @@ -515,7 +555,8 @@ func (sd *stackediff) SyncStack(ctx context.Context) { sd.profiletimer.Step("SyncStack::Start") defer sd.profiletimer.Step("SyncStack::End") - githubInfo := sd.github.GetInfo(ctx, sd.gitcmd) + localCommits := sd.vcsOps.GetLocalCommitStack(sd.config, sd.gitcmd) + githubInfo := sd.github.GetInfo(ctx, sd.gitcmd, localCommits) if len(githubInfo.PullRequests) == 0 { fmt.Fprintf(sd.output, "pull request stack is empty\n") @@ -537,13 +578,13 @@ func (sd *stackediff) RunMergeCheck(ctx context.Context) { return } - localCommits := git.GetLocalCommitStack(sd.config, sd.gitcmd) + localCommits := sd.vcsOps.GetLocalCommitStack(sd.config, sd.gitcmd) if len(localCommits) == 0 { fmt.Println("no local commits - nothing to check") return } - githubInfo := sd.github.GetInfo(ctx, sd.gitcmd) + githubInfo := sd.github.GetInfo(ctx, sd.gitcmd, localCommits) sigch := make(chan os.Signal, 1) signal.Notify(sigch, os.Interrupt, syscall.SIGTERM) @@ -623,18 +664,12 @@ func sortPullRequestsByLocalCommitOrder(pullRequests []*github.PullRequest, loca } func (sd *stackediff) fetchAndGetGitHubInfo(ctx context.Context) *github.GitHubInfo { - if sd.config.Repo.ForceFetchTags { - sd.gitcmd.MustGit("fetch --tags --force", nil) - } else { - sd.gitcmd.MustGit("fetch", nil) - } - rebaseCommand := fmt.Sprintf("rebase %s/%s --autostash", - sd.config.Repo.GitHubRemote, sd.config.Repo.GitHubBranch) - err := sd.gitcmd.Git(rebaseCommand, nil) + err := sd.vcsOps.FetchAndRebase(sd.config) if err != nil { return nil } - info := sd.github.GetInfo(ctx, sd.gitcmd) + localCommits := sd.vcsOps.GetLocalCommitStack(sd.config, sd.gitcmd) + info := sd.github.GetInfo(ctx, sd.gitcmd, localCommits) if git.BranchNameRegex.FindString(info.LocalBranch) != "" { fmt.Printf("error: don't run spr in a remote pr branch\n") fmt.Printf(" this could lead to weird duplicate pull requests getting created\n") @@ -655,15 +690,11 @@ func (sd *stackediff) fetchAndGetGitHubInfo(ctx context.Context) *github.GitHubI func (sd *stackediff) syncCommitStackToGitHub(ctx context.Context, commits []git.Commit, info *github.GitHubInfo, ) bool { - var output string - sd.gitcmd.MustGit("status --porcelain --untracked-files=no", &output) - if output != "" { - err := sd.gitcmd.Git("stash", nil) - if err != nil { - return false - } - defer sd.gitcmd.MustGit("stash pop", nil) + cleanup, err := sd.vcsOps.PrepareForPush() + if err != nil { + return false } + defer cleanup() commitUpdated := func(c git.Commit, info *github.GitHubInfo) bool { for _, pr := range info.PullRequests { @@ -684,24 +715,9 @@ func (sd *stackediff) syncCommitStackToGitHub(ctx context.Context, } } - var refNames []string - for _, commit := range updatedCommits { - branchName := git.BranchNameFromCommit(sd.config, commit) - refNames = append(refNames, - commit.CommitHash+":refs/heads/"+branchName) - } - if len(updatedCommits) > 0 { - if sd.config.Repo.BranchPushIndividually { - for _, refName := range refNames { - pushCommand := fmt.Sprintf("push --force %s %s", sd.config.Repo.GitHubRemote, refName) - sd.gitcmd.MustGit(pushCommand, nil) - } - } else { - pushCommand := fmt.Sprintf("push --force --atomic %s ", sd.config.Repo.GitHubRemote) - pushCommand += strings.Join(refNames, " ") - sd.gitcmd.MustGit(pushCommand, nil) - } + err := sd.vcsOps.PushBranches(sd.config, updatedCommits, sd.config.Repo.BranchPushIndividually) + check(err) } sd.profiletimer.Step("SyncCommitStack::PushBranches") return true diff --git a/spr/spr_test.go b/spr/spr_test.go index acddcc8a..97ec6253 100644 --- a/spr/spr_test.go +++ b/spr/spr_test.go @@ -75,6 +75,7 @@ func testSPRBasicFlowFourCommitsQueue(t *testing.T, sync bool) { } // 'git spr status' :: StatusPullRequest + gitmock.ExpectLogAndRespond([]*git.Commit{}) githubmock.ExpectGetInfo() s.StatusPullRequests(ctx) assert.Equal("pull request stack is empty\n", output.String()) @@ -86,11 +87,13 @@ func testSPRBasicFlowFourCommitsQueue(t *testing.T, sync bool) { githubmock.ExpectGetInfo() gitmock.ExpectFetch() gitmock.ExpectLogAndRespond([]*git.Commit{&c1}) + gitmock.ExpectLogAndRespond([]*git.Commit{&c1}) gitmock.ExpectPushCommits([]*git.Commit{&c1}) githubmock.ExpectCreatePullRequest(c1, nil) githubmock.ExpectGetAssignableUsers() githubmock.ExpectAddReviewers([]string{mockclient.NobodyUserID}) githubmock.ExpectUpdatePullRequest(c1, nil) + gitmock.ExpectLogAndRespond([]*git.Commit{}) githubmock.ExpectGetInfo() s.UpdatePullRequests(ctx, []string{mockclient.NobodyLogin}, nil) fmt.Printf("OUT: %s\n", output.String()) @@ -103,12 +106,14 @@ func testSPRBasicFlowFourCommitsQueue(t *testing.T, sync bool) { githubmock.ExpectGetInfo() gitmock.ExpectFetch() gitmock.ExpectLogAndRespond([]*git.Commit{&c2, &c1}) + gitmock.ExpectLogAndRespond([]*git.Commit{&c2, &c1}) gitmock.ExpectPushCommits([]*git.Commit{&c2}) githubmock.ExpectCreatePullRequest(c2, &c1) githubmock.ExpectGetAssignableUsers() githubmock.ExpectAddReviewers([]string{mockclient.NobodyUserID}) githubmock.ExpectUpdatePullRequest(c1, nil) githubmock.ExpectUpdatePullRequest(c2, &c1) + gitmock.ExpectLogAndRespond([]*git.Commit{}) githubmock.ExpectGetInfo() s.UpdatePullRequests(ctx, []string{mockclient.NobodyLogin}, nil) lines := strings.Split(output.String(), "\n") @@ -124,6 +129,7 @@ func testSPRBasicFlowFourCommitsQueue(t *testing.T, sync bool) { githubmock.ExpectGetInfo() gitmock.ExpectFetch() gitmock.ExpectLogAndRespond([]*git.Commit{&c4, &c3, &c2, &c1}) + gitmock.ExpectLogAndRespond([]*git.Commit{&c4, &c3, &c2, &c1}) gitmock.ExpectPushCommits([]*git.Commit{&c3, &c4}) // For the first "create" call we should call GetAssignableUsers @@ -139,6 +145,7 @@ func testSPRBasicFlowFourCommitsQueue(t *testing.T, sync bool) { githubmock.ExpectUpdatePullRequest(c2, &c1) githubmock.ExpectUpdatePullRequest(c3, &c2) githubmock.ExpectUpdatePullRequest(c4, &c3) + gitmock.ExpectLogAndRespond([]*git.Commit{}) githubmock.ExpectGetInfo() s.UpdatePullRequests(ctx, []string{mockclient.NobodyLogin}, nil) lines = strings.Split(output.String(), "\n") @@ -156,6 +163,7 @@ func testSPRBasicFlowFourCommitsQueue(t *testing.T, sync bool) { output.Reset() // 'git spr merge' :: MergePullRequest :: commits=[a1, a2] + gitmock.ExpectLogAndRespond([]*git.Commit{}) githubmock.ExpectGetInfo() githubmock.ExpectUpdatePullRequest(c2, nil) githubmock.ExpectMergePullRequest(c2, genclient.PullRequestMergeMethod_REBASE) @@ -182,7 +190,9 @@ func testSPRBasicFlowFourCommitsQueue(t *testing.T, sync bool) { gitmock.ExpectFetch() gitmock.ExpectLogAndRespond([]*git.Commit{&c4, &c3, &c2, &c1}) + gitmock.ExpectLogAndRespond([]*git.Commit{&c4, &c3, &c2, &c1}) gitmock.ExpectStatus() + gitmock.ExpectLogAndRespond([]*git.Commit{&c4, &c3, &c2, &c1}) s.UpdatePullRequests(ctx, []string{mockclient.NobodyLogin}, nil) lines = strings.Split(output.String(), "\n") @@ -200,6 +210,7 @@ func testSPRBasicFlowFourCommitsQueue(t *testing.T, sync bool) { output.Reset() // 'git spr merge' :: MergePullRequest :: commits=[a2, a3, a4] + gitmock.ExpectLogAndRespond([]*git.Commit{&c4, &c3, &c2, &c1}) githubmock.ExpectGetInfo() githubmock.ExpectUpdatePullRequest(c4, nil) githubmock.ExpectMergePullRequest(c4, genclient.PullRequestMergeMethod_REBASE) @@ -256,6 +267,7 @@ func testSPRBasicFlowFourCommits(t *testing.T, sync bool) { } // 'git spr status' :: StatusPullRequest + gitmock.ExpectLogAndRespond([]*git.Commit{}) githubmock.ExpectGetInfo() s.StatusPullRequests(ctx) assert.Equal("pull request stack is empty\n", output.String()) @@ -267,11 +279,13 @@ func testSPRBasicFlowFourCommits(t *testing.T, sync bool) { githubmock.ExpectGetInfo() gitmock.ExpectFetch() gitmock.ExpectLogAndRespond([]*git.Commit{&c1}) + gitmock.ExpectLogAndRespond([]*git.Commit{&c1}) gitmock.ExpectPushCommits([]*git.Commit{&c1}) githubmock.ExpectCreatePullRequest(c1, nil) githubmock.ExpectGetAssignableUsers() githubmock.ExpectAddReviewers([]string{mockclient.NobodyUserID}) githubmock.ExpectUpdatePullRequest(c1, nil) + gitmock.ExpectLogAndRespond([]*git.Commit{}) githubmock.ExpectGetInfo() s.UpdatePullRequests(ctx, []string{mockclient.NobodyLogin}, nil) fmt.Printf("OUT: %s\n", output.String()) @@ -284,12 +298,14 @@ func testSPRBasicFlowFourCommits(t *testing.T, sync bool) { githubmock.ExpectGetInfo() gitmock.ExpectFetch() gitmock.ExpectLogAndRespond([]*git.Commit{&c2, &c1}) + gitmock.ExpectLogAndRespond([]*git.Commit{&c2, &c1}) gitmock.ExpectPushCommits([]*git.Commit{&c2}) githubmock.ExpectCreatePullRequest(c2, &c1) githubmock.ExpectGetAssignableUsers() githubmock.ExpectAddReviewers([]string{mockclient.NobodyUserID}) githubmock.ExpectUpdatePullRequest(c1, nil) githubmock.ExpectUpdatePullRequest(c2, &c1) + gitmock.ExpectLogAndRespond([]*git.Commit{}) githubmock.ExpectGetInfo() s.UpdatePullRequests(ctx, []string{mockclient.NobodyLogin}, nil) lines := strings.Split(output.String(), "\n") @@ -305,6 +321,7 @@ func testSPRBasicFlowFourCommits(t *testing.T, sync bool) { githubmock.ExpectGetInfo() gitmock.ExpectFetch() gitmock.ExpectLogAndRespond([]*git.Commit{&c4, &c3, &c2, &c1}) + gitmock.ExpectLogAndRespond([]*git.Commit{&c4, &c3, &c2, &c1}) gitmock.ExpectPushCommits([]*git.Commit{&c3, &c4}) // For the first "create" call we should call GetAssignableUsers @@ -320,6 +337,7 @@ func testSPRBasicFlowFourCommits(t *testing.T, sync bool) { githubmock.ExpectUpdatePullRequest(c2, &c1) githubmock.ExpectUpdatePullRequest(c3, &c2) githubmock.ExpectUpdatePullRequest(c4, &c3) + gitmock.ExpectLogAndRespond([]*git.Commit{}) githubmock.ExpectGetInfo() s.UpdatePullRequests(ctx, []string{mockclient.NobodyLogin}, nil) lines = strings.Split(output.String(), "\n") @@ -337,6 +355,7 @@ func testSPRBasicFlowFourCommits(t *testing.T, sync bool) { output.Reset() // 'git spr merge' :: MergePullRequest :: commits=[a1, a2, a3, a4] + gitmock.ExpectLogAndRespond([]*git.Commit{&c4, &c3, &c2, &c1}) githubmock.ExpectGetInfo() githubmock.ExpectUpdatePullRequest(c4, nil) githubmock.ExpectMergePullRequest(c4, genclient.PullRequestMergeMethod_REBASE) @@ -386,11 +405,13 @@ func testSPRBasicFlowDeleteBranch(t *testing.T, sync bool) { githubmock.ExpectGetInfo() gitmock.ExpectFetch() gitmock.ExpectLogAndRespond([]*git.Commit{&c1}) + gitmock.ExpectLogAndRespond([]*git.Commit{&c1}) gitmock.ExpectPushCommits([]*git.Commit{&c1}) githubmock.ExpectCreatePullRequest(c1, nil) githubmock.ExpectGetAssignableUsers() githubmock.ExpectAddReviewers([]string{mockclient.NobodyUserID}) githubmock.ExpectUpdatePullRequest(c1, nil) + gitmock.ExpectLogAndRespond([]*git.Commit{}) githubmock.ExpectGetInfo() s.UpdatePullRequests(ctx, []string{mockclient.NobodyLogin}, nil) fmt.Printf("OUT: %s\n", output.String()) @@ -403,12 +424,14 @@ func testSPRBasicFlowDeleteBranch(t *testing.T, sync bool) { githubmock.ExpectGetInfo() gitmock.ExpectFetch() gitmock.ExpectLogAndRespond([]*git.Commit{&c2, &c1}) + gitmock.ExpectLogAndRespond([]*git.Commit{&c2, &c1}) gitmock.ExpectPushCommits([]*git.Commit{&c2}) githubmock.ExpectCreatePullRequest(c2, &c1) githubmock.ExpectGetAssignableUsers() githubmock.ExpectAddReviewers([]string{mockclient.NobodyUserID}) githubmock.ExpectUpdatePullRequest(c1, nil) githubmock.ExpectUpdatePullRequest(c2, &c1) + gitmock.ExpectLogAndRespond([]*git.Commit{}) githubmock.ExpectGetInfo() s.UpdatePullRequests(ctx, []string{mockclient.NobodyLogin}, nil) lines := strings.Split(output.String(), "\n") @@ -421,6 +444,7 @@ func testSPRBasicFlowDeleteBranch(t *testing.T, sync bool) { output.Reset() // 'git spr merge' :: MergePullRequest :: commits=[a1, a2] + gitmock.ExpectLogAndRespond([]*git.Commit{}) githubmock.ExpectGetInfo() githubmock.ExpectUpdatePullRequest(c2, nil) githubmock.ExpectMergePullRequest(c2, genclient.PullRequestMergeMethod_REBASE) @@ -474,6 +498,7 @@ func testSPRMergeCount(t *testing.T, sync bool) { githubmock.ExpectGetInfo() gitmock.ExpectFetch() gitmock.ExpectLogAndRespond([]*git.Commit{&c4, &c3, &c2, &c1}) + gitmock.ExpectLogAndRespond([]*git.Commit{&c4, &c3, &c2, &c1}) gitmock.ExpectPushCommits([]*git.Commit{&c1, &c2, &c3, &c4}) // For the first "create" call we should call GetAssignableUsers githubmock.ExpectCreatePullRequest(c1, nil) @@ -489,6 +514,7 @@ func testSPRMergeCount(t *testing.T, sync bool) { githubmock.ExpectUpdatePullRequest(c2, &c1) githubmock.ExpectUpdatePullRequest(c3, &c2) githubmock.ExpectUpdatePullRequest(c4, &c3) + gitmock.ExpectLogAndRespond([]*git.Commit{}) githubmock.ExpectGetInfo() s.UpdatePullRequests(ctx, []string{mockclient.NobodyLogin}, nil) lines := strings.Split(output.String(), "\n") @@ -504,6 +530,7 @@ func testSPRMergeCount(t *testing.T, sync bool) { output.Reset() // 'git spr merge --count 2' :: MergePullRequest :: commits=[a1, a2, a3, a4] + gitmock.ExpectLogAndRespond([]*git.Commit{}) githubmock.ExpectGetInfo() githubmock.ExpectUpdatePullRequest(c2, nil) githubmock.ExpectMergePullRequest(c2, genclient.PullRequestMergeMethod_REBASE) @@ -543,6 +570,7 @@ func testSPRAmendCommit(t *testing.T, sync bool) { } // 'git spr state' :: StatusPullRequest + gitmock.ExpectLogAndRespond([]*git.Commit{}) githubmock.ExpectGetInfo() s.StatusPullRequests(ctx) assert.Equal("pull request stack is empty\n", output.String()) @@ -554,11 +582,13 @@ func testSPRAmendCommit(t *testing.T, sync bool) { githubmock.ExpectGetInfo() gitmock.ExpectFetch() gitmock.ExpectLogAndRespond([]*git.Commit{&c2, &c1}) + gitmock.ExpectLogAndRespond([]*git.Commit{&c2, &c1}) gitmock.ExpectPushCommits([]*git.Commit{&c1, &c2}) githubmock.ExpectCreatePullRequest(c1, nil) githubmock.ExpectCreatePullRequest(c2, &c1) githubmock.ExpectUpdatePullRequest(c1, nil) githubmock.ExpectUpdatePullRequest(c2, &c1) + gitmock.ExpectLogAndRespond([]*git.Commit{}) githubmock.ExpectGetInfo() s.UpdatePullRequests(ctx, nil, nil) fmt.Printf("OUT: %s\n", output.String()) @@ -575,9 +605,11 @@ func testSPRAmendCommit(t *testing.T, sync bool) { githubmock.ExpectGetInfo() gitmock.ExpectFetch() gitmock.ExpectLogAndRespond([]*git.Commit{&c2, &c1}) + gitmock.ExpectLogAndRespond([]*git.Commit{&c2, &c1}) gitmock.ExpectPushCommits([]*git.Commit{&c2}) githubmock.ExpectUpdatePullRequest(c1, nil) githubmock.ExpectUpdatePullRequest(c2, &c1) + gitmock.ExpectLogAndRespond([]*git.Commit{}) githubmock.ExpectGetInfo() s.UpdatePullRequests(ctx, nil, nil) lines = strings.Split(output.String(), "\n") @@ -595,9 +627,11 @@ func testSPRAmendCommit(t *testing.T, sync bool) { githubmock.ExpectGetInfo() gitmock.ExpectFetch() gitmock.ExpectLogAndRespond([]*git.Commit{&c2, &c1}) + gitmock.ExpectLogAndRespond([]*git.Commit{&c2, &c1}) gitmock.ExpectPushCommits([]*git.Commit{&c1, &c2}) githubmock.ExpectUpdatePullRequest(c1, nil) githubmock.ExpectUpdatePullRequest(c2, &c1) + gitmock.ExpectLogAndRespond([]*git.Commit{}) githubmock.ExpectGetInfo() s.UpdatePullRequests(ctx, nil, nil) lines = strings.Split(output.String(), "\n") @@ -609,6 +643,7 @@ func testSPRAmendCommit(t *testing.T, sync bool) { output.Reset() // 'git spr merge' :: MergePullRequest :: commits=[a1, a2] + gitmock.ExpectLogAndRespond([]*git.Commit{}) githubmock.ExpectGetInfo() githubmock.ExpectUpdatePullRequest(c2, nil) githubmock.ExpectMergePullRequest(c2, genclient.PullRequestMergeMethod_REBASE) @@ -663,6 +698,7 @@ func testSPRReorderCommit(t *testing.T, sync bool) { } // 'git spr status' :: StatusPullRequest + gitmock.ExpectLogAndRespond([]*git.Commit{}) githubmock.ExpectGetInfo() s.StatusPullRequests(ctx) assert.Equal("pull request stack is empty\n", output.String()) @@ -674,6 +710,7 @@ func testSPRReorderCommit(t *testing.T, sync bool) { githubmock.ExpectGetInfo() gitmock.ExpectFetch() gitmock.ExpectLogAndRespond([]*git.Commit{&c4, &c3, &c2, &c1}) + gitmock.ExpectLogAndRespond([]*git.Commit{&c4, &c3, &c2, &c1}) gitmock.ExpectPushCommits([]*git.Commit{&c1, &c2, &c3, &c4}) githubmock.ExpectCreatePullRequest(c1, nil) githubmock.ExpectCreatePullRequest(c2, &c1) @@ -683,6 +720,7 @@ func testSPRReorderCommit(t *testing.T, sync bool) { githubmock.ExpectUpdatePullRequest(c2, &c1) githubmock.ExpectUpdatePullRequest(c3, &c2) githubmock.ExpectUpdatePullRequest(c4, &c3) + gitmock.ExpectLogAndRespond([]*git.Commit{}) githubmock.ExpectGetInfo() s.UpdatePullRequests(ctx, nil, nil) fmt.Printf("OUT: %s\n", output.String()) @@ -699,6 +737,7 @@ func testSPRReorderCommit(t *testing.T, sync bool) { githubmock.ExpectGetInfo() gitmock.ExpectFetch() gitmock.ExpectLogAndRespond([]*git.Commit{&c3, &c1, &c4, &c2}) + gitmock.ExpectLogAndRespond([]*git.Commit{&c3, &c1, &c4, &c2}) githubmock.ExpectUpdatePullRequest(c1, nil) githubmock.ExpectUpdatePullRequest(c2, nil) githubmock.ExpectUpdatePullRequest(c3, nil) @@ -713,6 +752,7 @@ func testSPRReorderCommit(t *testing.T, sync bool) { githubmock.ExpectUpdatePullRequest(c4, &c2) githubmock.ExpectUpdatePullRequest(c1, &c4) githubmock.ExpectUpdatePullRequest(c3, &c1) + gitmock.ExpectLogAndRespond([]*git.Commit{}) githubmock.ExpectGetInfo() s.UpdatePullRequests(ctx, nil, nil) fmt.Printf("OUT: %s\n", output.String()) @@ -730,6 +770,7 @@ func testSPRReorderCommit(t *testing.T, sync bool) { githubmock.ExpectGetInfo() gitmock.ExpectFetch() gitmock.ExpectLogAndRespond([]*git.Commit{&c1, &c2, &c3, &c4, &c5}) + gitmock.ExpectLogAndRespond([]*git.Commit{&c1, &c2, &c3, &c4, &c5}) githubmock.ExpectUpdatePullRequest(c1, nil) githubmock.ExpectUpdatePullRequest(c2, nil) githubmock.ExpectUpdatePullRequest(c3, nil) @@ -746,6 +787,7 @@ func testSPRReorderCommit(t *testing.T, sync bool) { githubmock.ExpectUpdatePullRequest(c3, &c4) githubmock.ExpectUpdatePullRequest(c2, &c3) githubmock.ExpectUpdatePullRequest(c1, &c2) + gitmock.ExpectLogAndRespond([]*git.Commit{}) githubmock.ExpectGetInfo() s.UpdatePullRequests(ctx, nil, nil) fmt.Printf("OUT: %s\n", output.String()) @@ -797,6 +839,7 @@ func testSPRDeleteCommit(t *testing.T, sync bool) { } // 'git spr status' :: StatusPullRequest + gitmock.ExpectLogAndRespond([]*git.Commit{}) githubmock.ExpectGetInfo() s.StatusPullRequests(ctx) assert.Equal("pull request stack is empty\n", output.String()) @@ -808,6 +851,7 @@ func testSPRDeleteCommit(t *testing.T, sync bool) { githubmock.ExpectGetInfo() gitmock.ExpectFetch() gitmock.ExpectLogAndRespond([]*git.Commit{&c4, &c3, &c2, &c1}) + gitmock.ExpectLogAndRespond([]*git.Commit{&c4, &c3, &c2, &c1}) gitmock.ExpectPushCommits([]*git.Commit{&c1, &c2, &c3, &c4}) githubmock.ExpectCreatePullRequest(c1, nil) githubmock.ExpectCreatePullRequest(c2, &c1) @@ -817,6 +861,7 @@ func testSPRDeleteCommit(t *testing.T, sync bool) { githubmock.ExpectUpdatePullRequest(c2, &c1) githubmock.ExpectUpdatePullRequest(c3, &c2) githubmock.ExpectUpdatePullRequest(c4, &c3) + gitmock.ExpectLogAndRespond([]*git.Commit{&c4, &c3, &c2, &c1}) githubmock.ExpectGetInfo() s.UpdatePullRequests(ctx, nil, nil) @@ -834,6 +879,7 @@ func testSPRDeleteCommit(t *testing.T, sync bool) { githubmock.ExpectGetInfo() gitmock.ExpectFetch() gitmock.ExpectLogAndRespond([]*git.Commit{&c4, &c1}) + gitmock.ExpectLogAndRespond([]*git.Commit{&c4, &c1}) githubmock.ExpectCommentPullRequest(c2) githubmock.ExpectClosePullRequest(c2) githubmock.ExpectCommentPullRequest(c3) @@ -844,6 +890,7 @@ func testSPRDeleteCommit(t *testing.T, sync bool) { githubmock.ExpectUpdatePullRequest(c1, nil) githubmock.ExpectUpdatePullRequest(c4, &c1) gitmock.ExpectPushCommits([]*git.Commit{&c1, &c4}) + gitmock.ExpectLogAndRespond([]*git.Commit{}) githubmock.ExpectGetInfo() s.UpdatePullRequests(ctx, nil, nil) fmt.Printf("OUT: %s\n", output.String()) diff --git a/vcs/detect.go b/vcs/detect.go new file mode 100644 index 00000000..f2cb027a --- /dev/null +++ b/vcs/detect.go @@ -0,0 +1,13 @@ +package vcs + +import ( + "os" + "path/filepath" +) + +// IsJJColocated returns true if the repository at rootDir is a jj-colocated +// repo (has both .jj/ and .git/ directories). +func IsJJColocated(rootDir string) bool { + _, err := os.Stat(filepath.Join(rootDir, ".jj")) + return err == nil +} diff --git a/vcs/detect_test.go b/vcs/detect_test.go new file mode 100644 index 00000000..75efb5c4 --- /dev/null +++ b/vcs/detect_test.go @@ -0,0 +1,70 @@ +package vcs + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/ejoffe/spr/config" + "github.com/stretchr/testify/require" +) + +func TestIsJJColocated_NoJJDir(t *testing.T) { + dir := t.TempDir() + require.False(t, IsJJColocated(dir)) +} + +func TestIsJJColocated_WithJJDir(t *testing.T) { + dir := t.TempDir() + err := os.Mkdir(filepath.Join(dir, ".jj"), 0755) + require.NoError(t, err) + require.True(t, IsJJColocated(dir)) +} + +func TestIsJJColocated_EmptyString(t *testing.T) { + require.False(t, IsJJColocated("")) +} + +func TestNewVCSOperations_GitRepo(t *testing.T) { + cfg := config.EmptyConfig() + dir := t.TempDir() + gitmock := &mockRootDirOnly{rootDir: dir} + ops := NewVCSOperations(cfg, gitmock) + _, isGit := ops.(*GitOps) + require.True(t, isGit, "should return GitOps for non-jj repo") +} + +func TestNewVCSOperations_JJRepo(t *testing.T) { + cfg := config.EmptyConfig() + dir := t.TempDir() + os.Mkdir(filepath.Join(dir, ".jj"), 0755) + gitmock := &mockRootDirOnly{rootDir: dir} + ops := NewVCSOperations(cfg, gitmock) + _, isJj := ops.(*JjOps) + require.True(t, isJj, "should return JjOps for jj-colocated repo") +} + +func TestNewVCSOperations_JJRepo_NoJJFlag(t *testing.T) { + cfg := config.EmptyConfig() + cfg.User.NoJJ = true + dir := t.TempDir() + os.Mkdir(filepath.Join(dir, ".jj"), 0755) + gitmock := &mockRootDirOnly{rootDir: dir} + ops := NewVCSOperations(cfg, gitmock) + _, isGit := ops.(*GitOps) + require.True(t, isGit, "should return GitOps when NoJJ is true even with .jj/ present") +} + +// mockRootDirOnly implements git.GitInterface with only RootDir meaningful. +type mockRootDirOnly struct { + rootDir string +} + +func (m *mockRootDirOnly) GitWithEditor(args string, output *string, editorCmd string) error { + return nil +} +func (m *mockRootDirOnly) Git(args string, output *string) error { return nil } +func (m *mockRootDirOnly) MustGit(args string, output *string) {} +func (m *mockRootDirOnly) RootDir() string { return m.rootDir } +func (m *mockRootDirOnly) DeleteRemoteBranch(ctx context.Context, branch string) error { return nil } diff --git a/vcs/git_ops.go b/vcs/git_ops.go new file mode 100644 index 00000000..09bce6ec --- /dev/null +++ b/vcs/git_ops.go @@ -0,0 +1,283 @@ +package vcs + +import ( + "fmt" + "os" + "path/filepath" + "regexp" + "strings" + + "github.com/ejoffe/spr/config" + "github.com/ejoffe/spr/git" + "github.com/rs/zerolog/log" +) + +// GitOps implements VCSOperations using standard git commands. +// This is a pure extraction of the existing logic from spr.go and helpers.go. +type GitOps struct { + cfg *config.Config + gitcmd git.GitInterface +} + +// NewGitOps creates a git-based VCSOperations implementation. +func NewGitOps(cfg *config.Config, gitcmd git.GitInterface) *GitOps { + return &GitOps{cfg: cfg, gitcmd: gitcmd} +} + +// FetchAndRebase fetches from remote and rebases the local stack. +// When MultiCommitPRs is enabled, saves and restores local branch positions +// across the rebase using commit-id trailers for matching. +func (g *GitOps) FetchAndRebase(cfg *config.Config) error { + if cfg.Repo.ForceFetchTags { + g.gitcmd.MustGit("fetch --tags --force", nil) + } else { + g.gitcmd.MustGit("fetch", nil) + } + + // Save branch→commitID mapping before rebase (multi-commit mode only) + var branchCommitIDs map[string]string + if cfg.Repo.MultiCommitPRs { + branchCommitIDs = g.getBranchCommitIDMap(cfg) + } + + rebaseCommand := fmt.Sprintf("rebase %s/%s --autostash", + cfg.Repo.GitHubRemote, cfg.Repo.GitHubBranch) + err := g.gitcmd.Git(rebaseCommand, nil) + + // After rebase, update branch pointers (multi-commit mode only) + if cfg.Repo.MultiCommitPRs && len(branchCommitIDs) > 0 { + g.updateBranchesAfterRebase(cfg, branchCommitIDs) + } + + return err +} + +// getBranchCommitIDMap returns a map of branch-name → commit-id for all +// local branches in the trunk..HEAD range. It reads each branch's commit +// message to extract the commit-id trailer. +func (g *GitOps) getBranchCommitIDMap(cfg *config.Config) map[string]string { + commitIDRegex := regexp.MustCompile(`commit-id:\s*([a-f0-9]{8})`) + + branchMap := git.GetLocalBranchMap(g.gitcmd) + if len(branchMap) == 0 { + return nil + } + + // Get the set of commit hashes in our stack range + stackRange := fmt.Sprintf("%s/%s..HEAD", cfg.Repo.GitHubRemote, cfg.Repo.GitHubBranch) + var logOutput string + err := g.gitcmd.Git(fmt.Sprintf("log --format=%%H %s", stackRange), &logOutput) + if err != nil { + return nil + } + stackHashes := make(map[string]bool) + for _, line := range strings.Split(strings.TrimSpace(logOutput), "\n") { + if line != "" { + stackHashes[line] = true + } + } + + result := make(map[string]string) + for hash, branches := range branchMap { + if !stackHashes[hash] { + continue + } + // Read commit message to get commit-id + var msg string + err := g.gitcmd.Git(fmt.Sprintf("log -1 --format=%%B %s", hash), &msg) + if err != nil { + continue + } + matches := commitIDRegex.FindStringSubmatch(msg) + if matches != nil { + for _, branch := range branches { + if branch != cfg.Repo.GitHubBranch && !git.IsSPRBranch(branch) { + result[branch] = matches[1] + } + } + } + } + return result +} + +// updateBranchesAfterRebase moves local branches to their new positions +// after a rebase, matching by commit-id trailers. +func (g *GitOps) updateBranchesAfterRebase(cfg *config.Config, oldBranches map[string]string) { + commitIDRegex := regexp.MustCompile(`commit-id:\s*([a-f0-9]{8})`) + + // Build commitID → newHash map from the rebased stack + stackRange := fmt.Sprintf("%s/%s..HEAD", cfg.Repo.GitHubRemote, cfg.Repo.GitHubBranch) + var logOutput string + err := g.gitcmd.Git(fmt.Sprintf("log --format=%%H%%n%%B%s %s", "%x00", stackRange), &logOutput) + if err != nil { + return + } + + commitIDToHash := make(map[string]string) + for _, entry := range strings.Split(logOutput, "\x00") { + entry = strings.TrimSpace(entry) + if entry == "" { + continue + } + lines := strings.SplitN(entry, "\n", 2) + if len(lines) < 1 { + continue + } + hash := strings.TrimSpace(lines[0]) + body := "" + if len(lines) > 1 { + body = lines[1] + } + matches := commitIDRegex.FindStringSubmatch(body) + if matches != nil { + commitIDToHash[matches[1]] = hash + } + } + + // Move each branch to its new position + for branch, commitID := range oldBranches { + newHash, found := commitIDToHash[commitID] + if !found { + log.Debug().Str("branch", branch).Str("commitID", commitID). + Msg("skipping branch update: commit-id not found after rebase (commit may have been dropped)") + continue + } + err := g.gitcmd.Git(fmt.Sprintf("branch -f %s %s", branch, newHash), nil) + if err != nil { + log.Warn().Str("branch", branch).Err(err).Msg("failed to update branch after rebase") + } else { + log.Debug().Str("branch", branch).Str("newHash", newHash[:8]). + Msg("updated branch position after rebase") + } + } +} + +// GetLocalCommitStack returns the local commit stack using git log. +// Delegates to the existing git.GetLocalCommitStack function. +// When MultiCommitPRs is enabled, annotates commits with local branch names. +func (g *GitOps) GetLocalCommitStack(cfg *config.Config, gitcmd git.GitInterface) []git.Commit { + commits := git.GetLocalCommitStack(cfg, gitcmd) + if cfg.Repo.MultiCommitPRs { + branchMap := git.GetLocalBranchMap(gitcmd) + git.AnnotateCommitsWithBranches(commits, branchMap, cfg.Repo.GitHubBranch) + } + return commits +} + +// AmendInto creates a fixup commit and autosquashes it into the target. +// Extracted from spr.go AmendCommit(). +func (g *GitOps) AmendInto(commit git.Commit) error { + g.gitcmd.MustGit("commit --fixup "+commit.CommitHash, nil) + rebaseCmd := fmt.Sprintf("rebase -i --autosquash --autostash %s/%s", + g.cfg.Repo.GitHubRemote, g.cfg.Repo.GitHubBranch) + g.gitcmd.MustGit(rebaseCmd, nil) + return nil +} + +// EditStart begins an interactive edit session on a commit. +// Extracted from spr.go EditCommit(). +func (g *GitOps) EditStart(commit git.Commit) error { + // Write state file + stateContent := fmt.Sprintf("commit_id=%s\ncommit_subject=%s\n", commit.CommitID, commit.Subject) + err := os.WriteFile(g.EditStatePath(), []byte(stateContent), 0644) + if err != nil { + return err + } + + // Use the spr binary as the sequence editor to rewrite 'pick' to 'edit' + exe, err := os.Executable() + if err != nil { + os.Remove(g.EditStatePath()) + return err + } + editorCmd := fmt.Sprintf("%s _edit-sequence %s", exe, commit.CommitHash[:7]) + + rebaseCmd := fmt.Sprintf("rebase -i --autostash %s/%s", + g.cfg.Repo.GitHubRemote, g.cfg.Repo.GitHubBranch) + err = g.gitcmd.GitWithEditor(rebaseCmd, nil, editorCmd) + if err != nil { + os.Remove(g.EditStatePath()) + return err + } + return nil +} + +// EditFinish completes an edit session by amending and continuing the rebase. +// Extracted from spr.go EditCommitDone(). +func (g *GitOps) EditFinish() error { + g.gitcmd.MustGit("add -A", nil) + err := g.gitcmd.Git("commit --amend --no-edit", nil) + if err != nil { + return fmt.Errorf("failed to amend commit: %w", err) + } + err = g.gitcmd.Git("rebase --continue", nil) + if err != nil { + return fmt.Errorf("rebase conflict detected: %w", err) + } + os.Remove(g.EditStatePath()) + return nil +} + +// EditAbort cancels the current edit session. +// Extracted from spr.go EditCommitAbort(). +func (g *GitOps) EditAbort() error { + err := g.gitcmd.Git("rebase --abort", nil) + if err != nil { + return fmt.Errorf("failed to abort rebase: %w", err) + } + os.Remove(g.EditStatePath()) + return nil +} + +// PrepareForPush stashes uncommitted changes and returns a cleanup function. +// Extracted from spr.go syncCommitStackToGitHub(). +func (g *GitOps) PrepareForPush() (func(), error) { + var output string + g.gitcmd.MustGit("status --porcelain --untracked-files=no", &output) + if output != "" { + err := g.gitcmd.Git("stash", nil) + if err != nil { + return nil, err + } + return func() { g.gitcmd.MustGit("stash pop", nil) }, nil + } + return func() {}, nil +} + +// IsEditing returns true if an edit session is in progress. +func (g *GitOps) IsEditing() bool { + _, err := os.Stat(g.EditStatePath()) + return err == nil +} + +// EditStatePath returns the path to the edit state file. +func (g *GitOps) EditStatePath() string { + return filepath.Join(g.gitcmd.RootDir(), ".git", "spr_edit_state") +} + +// PushBranches force-pushes spr branches for updated commits via git. +func (g *GitOps) PushBranches(cfg *config.Config, commits []git.Commit, individually bool) error { + var refNames []string + for _, commit := range commits { + branchName := git.BranchNameFromCommit(cfg, commit) + refNames = append(refNames, commit.CommitHash+":refs/heads/"+branchName) + } + if individually { + for _, refName := range refNames { + pushCommand := fmt.Sprintf("push --force %s %s", cfg.Repo.GitHubRemote, refName) + g.gitcmd.MustGit(pushCommand, nil) + } + } else { + pushCommand := fmt.Sprintf("push --force --atomic %s %s", + cfg.Repo.GitHubRemote, strings.Join(refNames, " ")) + g.gitcmd.MustGit(pushCommand, nil) + } + return nil +} + +// CheckStackCompleteness is a no-op for git. The detached-HEAD case is already +// caught by the branch name check in fetchAndGetGitHubInfo. +func (g *GitOps) CheckStackCompleteness() string { + return "" +} + diff --git a/vcs/git_ops_test.go b/vcs/git_ops_test.go new file mode 100644 index 00000000..aac1e4e1 --- /dev/null +++ b/vcs/git_ops_test.go @@ -0,0 +1,89 @@ +package vcs + +import ( + "testing" + + "github.com/ejoffe/spr/config" + "github.com/ejoffe/spr/git/mockgit" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func makeGitTestConfig() *config.Config { + cfg := config.EmptyConfig() + cfg.Repo.GitHubRemote = "origin" + cfg.Repo.GitHubBranch = "master" + cfg.Repo.MergeMethod = "rebase" + return cfg +} + +func TestGitOpsFetchAndRebase(t *testing.T) { + cfg := makeGitTestConfig() + gitmock := mockgit.NewMockGit(t) + ops := NewGitOps(cfg, gitmock) + + gitmock.ExpectFetch() // expects git fetch + git rebase origin/master --autostash + + err := ops.FetchAndRebase(cfg) + require.NoError(t, err) + gitmock.ExpectationsMet() +} + +func TestGitOpsFetchAndRebase_ForceTags(t *testing.T) { + cfg := makeGitTestConfig() + cfg.Repo.ForceFetchTags = true + gitmock := mockgit.NewMockGit(t) + ops := NewGitOps(cfg, gitmock) + + // ExpectFetch expects "git fetch" but with force tags it's "git fetch --tags --force" + // We need a custom expectation + gitmock.ExpectFetchTags() + + err := ops.FetchAndRebase(cfg) + require.NoError(t, err) + gitmock.ExpectationsMet() +} + +func TestGitOpsPrepareForPush_Clean(t *testing.T) { + cfg := makeGitTestConfig() + gitmock := mockgit.NewMockGit(t) + ops := NewGitOps(cfg, gitmock) + + gitmock.ExpectStatus() // returns empty (clean) + + cleanup, err := ops.PrepareForPush() + require.NoError(t, err) + require.NotNil(t, cleanup) + cleanup() // should not panic, no stash pop expected + gitmock.ExpectationsMet() +} + +func TestGitOpsIsEditing_NoStateFile(t *testing.T) { + cfg := makeGitTestConfig() + gitmock := mockgit.NewMockGit(t) + ops := NewGitOps(cfg, gitmock) + + assert.False(t, ops.IsEditing()) +} + +func TestGitOpsEditStatePath(t *testing.T) { + cfg := makeGitTestConfig() + gitmock := mockgit.NewMockGit(t) + ops := NewGitOps(cfg, gitmock) + + // mockgit.RootDir() returns "" + assert.Contains(t, ops.EditStatePath(), "spr_edit_state") +} + +// --- CheckStackCompleteness --- + +func TestGitOpsCheckStackCompleteness_Noop(t *testing.T) { + cfg := makeGitTestConfig() + gitmock := mockgit.NewMockGit(t) + ops := NewGitOps(cfg, gitmock) + + // Git mode is a no-op — detached HEAD is caught by fetchAndGetGitHubInfo + warning := ops.CheckStackCompleteness() + assert.Equal(t, "", warning) + gitmock.ExpectationsMet() +} diff --git a/vcs/interface.go b/vcs/interface.go new file mode 100644 index 00000000..a26f62f3 --- /dev/null +++ b/vcs/interface.go @@ -0,0 +1,75 @@ +package vcs + +import ( + "github.com/ejoffe/spr/config" + "github.com/ejoffe/spr/git" +) + +// VCSOperations abstracts the version control operations that differ between +// git and jj (Jujutsu). Operations like push, fetch, and branch management +// stay on git.GitInterface; only history-rewriting operations are abstracted here. +type VCSOperations interface { + // FetchAndRebase fetches from remote and rebases local stack onto updated trunk. + // Git: git fetch + git rebase origin/main --autostash + // jj: jj git fetch + jj rebase -b @ -d main@origin + FetchAndRebase(cfg *config.Config) error + + // GetLocalCommitStack returns unmerged commits (bottom-first), adding + // commit-id trailers if missing. + // Git: git log origin/main..HEAD, then git rebase -i with spr_reword_helper if needed + // jj: jj log -r 'trunk()..@' --reversed, then jj describe for missing trailers + GetLocalCommitStack(cfg *config.Config, gitcmd git.GitInterface) []git.Commit + + // AmendInto squashes working copy changes into a specific commit in the stack. + // Git: git commit --fixup + git rebase -i --autosquash --autostash + // jj: jj squash --into + AmendInto(commit git.Commit) error + + // EditStart checks out a commit for editing (interactive edit session). + // Git: git rebase -i with 'edit' stop + // jj: jj edit + EditStart(commit git.Commit) error + + // EditFinish completes an edit session. + // Git: git add -A + git commit --amend --no-edit + git rebase --continue + // jj: jj new (changes are auto-captured) + EditFinish() error + + // EditAbort cancels an edit session. + // Git: git rebase --abort + // jj: jj op restore + EditAbort() error + + // PrepareForPush saves working state before push and returns a cleanup func. + // Git: git stash / git stash pop + // jj: no-op (working copy is always a commit) + PrepareForPush() (cleanup func(), err error) + + // PushBranches force-pushes spr branches for updated commits. + // Git: git push --force --atomic origin :refs/heads/ ... + // jj: jj bookmark set -r + jj git push --bookmark 'glob:spr/*' + PushBranches(cfg *config.Config, commits []git.Commit, individually bool) error + + // IsEditing returns true if an edit session is in progress. + IsEditing() bool + + // EditStatePath returns the path to the edit state file. + EditStatePath() string + + // CheckStackCompleteness checks whether the current working copy position + // might cause spr to see an incomplete stack. Returns a non-empty warning + // string if there are commits that would be excluded (e.g. @ has descendants + // in jj, or HEAD is detached in git). Returns "" if everything looks fine. + CheckStackCompleteness() string +} + +// NewVCSOperations creates a VCSOperations implementation appropriate for the +// current repository. If a .jj/ directory exists (jj-colocated repo) and +// the user has not set noJJ, returns a jj implementation. +// Otherwise returns a git implementation. +func NewVCSOperations(cfg *config.Config, gitcmd git.GitInterface) VCSOperations { + if !cfg.User.NoJJ && IsJJColocated(gitcmd.RootDir()) { + return NewJjOps(cfg, NewJjCmd(gitcmd.RootDir()), gitcmd) + } + return NewGitOps(cfg, gitcmd) +} diff --git a/vcs/jj_cmd.go b/vcs/jj_cmd.go new file mode 100644 index 00000000..d6da8173 --- /dev/null +++ b/vcs/jj_cmd.go @@ -0,0 +1,66 @@ +package vcs + +import ( + "fmt" + "os/exec" + "strings" + + "github.com/rs/zerolog/log" +) + +// JjCmd executes jj commands, mirroring the git/realgit/realcmd.go pattern. +type JjCmd struct { + rootdir string +} + +// NewJjCmd creates a new jj command executor. +func NewJjCmd(rootDir string) *JjCmd { + return &JjCmd{rootdir: rootDir} +} + +// JjInterface abstracts jj command execution for testing. +type JjInterface interface { + Jj(args string, output *string) error + MustJj(args string, output *string) + JjArgs(args []string, output *string) error +} + +// Jj executes a jj command with the given arguments. +func (c *JjCmd) Jj(args string, output *string) error { + log.Debug().Msgf("jj %s", args) + cmdArgs := strings.Fields(args) + cmd := exec.Command("jj", cmdArgs...) + cmd.Dir = c.rootdir + out, err := cmd.CombinedOutput() + if output != nil { + *output = strings.TrimRight(string(out), "\n") + } + if err != nil { + return fmt.Errorf("jj %s: %w\n%s", args, err, string(out)) + } + return nil +} + +// MustJj executes a jj command and panics on error. +func (c *JjCmd) MustJj(args string, output *string) { + err := c.Jj(args, output) + if err != nil { + panic(err) + } +} + +// JjArgs executes a jj command with pre-split arguments (for messages with spaces). +func (c *JjCmd) JjArgs(args []string, output *string) error { + log.Debug().Msgf("jj %v", args) + cmd := exec.Command("jj", args...) + cmd.Dir = c.rootdir + + out, err := cmd.CombinedOutput() + if output != nil { + *output = strings.TrimRight(string(out), "\n") + } + if err != nil { + return fmt.Errorf("jj %v: %w\n%s", args, err, string(out)) + } + return nil +} diff --git a/vcs/jj_cmd_test.go b/vcs/jj_cmd_test.go new file mode 100644 index 00000000..d0c52294 --- /dev/null +++ b/vcs/jj_cmd_test.go @@ -0,0 +1,33 @@ +package vcs + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestStringsFieldsSplitsTemplates demonstrates that strings.Fields +// (used by JjCmd.Jj) incorrectly splits jj template arguments containing +// spaces. Commands with templates must use JjArgs to preserve argument boundaries. +func TestStringsFieldsSplitsTemplates(t *testing.T) { + template := `commit_id ++ "\x1f" ++ change_id ++ "\x1f" ++ empty ++ "\x1f" ++ description ++ "\x1e"` + cmdStr := `log --no-graph --reversed --color=never -r "trunk()..@" -T '` + template + `'` + + fields := strings.Fields(cmdStr) + + // Find the -T flag + tIdx := -1 + for i, f := range fields { + if f == "-T" { + tIdx = i + break + } + } + assert.NotEqual(t, -1, tIdx, "-T flag should be present") + + // strings.Fields splits the template — the next field is just "'commit_id", not the full template. + // This proves that Jj() (which uses strings.Fields) cannot be used for commands with templates. + assert.False(t, strings.Contains(fields[tIdx+1], "description"), + "strings.Fields splits template args — commands with templates must use JjArgs instead") +} diff --git a/vcs/jj_ops.go b/vcs/jj_ops.go new file mode 100644 index 00000000..e24f9b8a --- /dev/null +++ b/vcs/jj_ops.go @@ -0,0 +1,272 @@ +package vcs + +import ( + "fmt" + "os" + "path/filepath" + "strings" + + "github.com/ejoffe/spr/config" + "github.com/ejoffe/spr/git" + "github.com/google/uuid" +) + +// JjOps implements VCSOperations using jj (Jujutsu) commands. +// Git commands are still used for push operations (via gitcmd). +type JjOps struct { + cfg *config.Config + jjcmd JjInterface + gitcmd git.GitInterface +} + +// NewJjOps creates a jj-based VCSOperations implementation. +func NewJjOps(cfg *config.Config, jjcmd JjInterface, gitcmd git.GitInterface) *JjOps { + return &JjOps{cfg: cfg, jjcmd: jjcmd, gitcmd: gitcmd} +} + +// FetchAndRebase fetches from remote and rebases using jj commands. +// Preserves jj change IDs (unlike git rebase which destroys them). +func (j *JjOps) FetchAndRebase(cfg *config.Config) error { + if cfg.User.NoRebase { + // Only fetch, skip rebase (same semantics as git NoRebase) + return j.jjcmd.Jj("git fetch", nil) + } + + err := j.jjcmd.Jj("git fetch", nil) + if err != nil { + return err + } + + // Rebase current stack onto updated trunk + remote := cfg.Repo.GitHubRemote + branch := cfg.Repo.GitHubBranch + rebaseCmd := fmt.Sprintf("rebase -b @ -d %s@%s", branch, remote) + return j.jjcmd.Jj(rebaseCmd, nil) +} + +// GetLocalCommitStack returns unmerged commits using jj log. +// If any commits lack commit-id trailers, adds them via jj describe. +// When MultiCommitPRs is enabled, uses the extended template that includes bookmarks. +func (j *JjOps) GetLocalCommitStack(cfg *config.Config, gitcmd git.GitInterface) []git.Commit { + useExtended := cfg.Repo.MultiCommitPRs + + var template string + if useExtended { + template = `commit_id ++ "\x1f" ++ change_id ++ "\x1f" ++ empty ++ "\x1f" ++ bookmarks ++ "\x1f" ++ description ++ "\x1e"` + } else { + template = `commit_id ++ "\x1f" ++ change_id ++ "\x1f" ++ empty ++ "\x1f" ++ description ++ "\x1e"` + } + + var output string + err := j.jjcmd.JjArgs([]string{"log", "--no-graph", "--reversed", "--color=never", "-r", "trunk()..@", "-T", template}, &output) + if err != nil { + panic(err) + } + + var parsed []parsedJjCommit + var valid bool + if useExtended { + parsed, valid = parseJjLogOutputExtended(output) + } else { + parsed, valid = parseJjLogOutput(output) + } + + if !valid { + // Add commit-id trailers to commits that lack them + for i, p := range parsed { + if p.sprCommitID == "" && !p.empty { + newID := uuid.New().String()[:8] + newDesc := strings.TrimRight(p.description, "\n") + newDesc += "\n\ncommit-id:" + newID + err := j.jjcmd.JjArgs([]string{"describe", "-r", p.changeID, "-m", newDesc}, nil) + if err != nil { + panic(fmt.Sprintf("failed to add commit-id to %s: %v", p.changeID, err)) + } + parsed[i].sprCommitID = newID + } + } + + // Re-read commit hashes since jj describe changes them + err = j.jjcmd.JjArgs([]string{"log", "--no-graph", "--reversed", "--color=never", "-r", "trunk()..@", "-T", template}, &output) + if err != nil { + panic(err) + } + if useExtended { + parsed, valid = parseJjLogOutputExtended(output) + } else { + parsed, valid = parseJjLogOutput(output) + } + if !valid { + panic("unable to add commit-id trailers via jj describe") + } + } + + // Convert to []git.Commit + var commits []git.Commit + for _, p := range parsed { + c := git.Commit{ + CommitID: p.sprCommitID, + CommitHash: p.commitHash, + ChangeID: p.changeID, + Subject: p.subject, + Body: p.body, + WIP: p.wip, + } + if useExtended { + // Filter out target branch from bookmarks + var filtered []string + for _, bm := range p.bookmarks { + if bm != cfg.Repo.GitHubBranch { + filtered = append(filtered, bm) + } + } + c.Branches = filtered + } + commits = append(commits, c) + } + return commits +} + +// AmendInto squashes working copy changes into a specific commit. +// Uses jj squash which preserves change IDs. +func (j *JjOps) AmendInto(commit git.Commit) error { + if commit.ChangeID == "" { + return fmt.Errorf("cannot amend: commit %s has no jj change ID", commit.CommitID) + } + return j.jjcmd.Jj(fmt.Sprintf("squash --into %s", commit.ChangeID), nil) +} + +// EditStart begins an edit session by checking out the target commit. +// Uses jj edit which preserves change IDs. +func (j *JjOps) EditStart(commit git.Commit) error { + if commit.ChangeID == "" { + return fmt.Errorf("cannot edit: commit %s has no jj change ID", commit.CommitID) + } + + // Save operation ID for abort + var opID string + j.jjcmd.MustJj("op log --no-graph -n 1 -T 'id.short(16)'", &opID) + + // Save current @ for finish + var currentAt string + j.jjcmd.MustJj("log --no-graph -r @ -T change_id", ¤tAt) + + // Write state file + stateContent := fmt.Sprintf("vcs=jj\nchange_id=%s\noriginal_at=%s\nop_id=%s\ncommit_id=%s\ncommit_subject=%s\n", + commit.ChangeID, strings.TrimSpace(currentAt), strings.TrimSpace(opID), + commit.CommitID, commit.Subject) + err := os.WriteFile(j.EditStatePath(), []byte(stateContent), 0644) + if err != nil { + return err + } + + // Move working copy to the target commit + err = j.jjcmd.Jj("edit "+commit.ChangeID, nil) + if err != nil { + os.Remove(j.EditStatePath()) + return err + } + return nil +} + +// EditFinish completes an edit session. +// In jj, changes to the edited commit are automatically captured. +// We just need to return to where we were. +func (j *JjOps) EditFinish() error { + state, err := j.readEditState() + if err != nil { + return err + } + + // Return to where we were before the edit + err = j.jjcmd.Jj("new "+state["original_at"], nil) + if err != nil { + return fmt.Errorf("failed to return from edit: %w", err) + } + + os.Remove(j.EditStatePath()) + return nil +} + +// EditAbort cancels an edit session by restoring the operation state. +func (j *JjOps) EditAbort() error { + state, err := j.readEditState() + if err != nil { + return err + } + + err = j.jjcmd.Jj("op restore "+state["op_id"], nil) + if err != nil { + return fmt.Errorf("failed to restore operation: %w", err) + } + + os.Remove(j.EditStatePath()) + return nil +} + +// PushBranches sets jj bookmarks for each updated commit and pushes via jj git push. +// This ensures jj tracks the branches, keeping the commits mutable. +func (j *JjOps) PushBranches(cfg *config.Config, commits []git.Commit, individually bool) error { + for _, commit := range commits { + branchName := git.BranchNameFromCommit(cfg, commit) + err := j.jjcmd.JjArgs([]string{"bookmark", "set", branchName, "-r", commit.CommitHash, "--allow-backwards"}, nil) + if err != nil { + return fmt.Errorf("failed to set bookmark %s: %w", branchName, err) + } + } + remote := cfg.Repo.GitHubRemote + branchGlob := "glob:spr/" + cfg.Repo.GitHubBranch + "/*" + return j.jjcmd.JjArgs([]string{"git", "push", "--remote", remote, "--bookmark", branchGlob}, nil) +} + +// PrepareForPush is a no-op for jj — the working copy is always a commit. +func (j *JjOps) PrepareForPush() (func(), error) { + return func() {}, nil +} + +// IsEditing returns true if an edit session is in progress. +func (j *JjOps) IsEditing() bool { + _, err := os.Stat(j.EditStatePath()) + return err == nil +} + +// EditStatePath returns the path to the edit state file. +func (j *JjOps) EditStatePath() string { + if j.gitcmd != nil { + return filepath.Join(j.gitcmd.RootDir(), ".git", "spr_edit_state") + } + return "" +} + +// CheckStackCompleteness warns if @ is not at the top of the stack. +// In jj, this happens when the user has done 'jj edit' to a mid-stack commit, +// causing 'trunk()..@' to miss commits above @. +func (j *JjOps) CheckStackCompleteness() string { + var output string + err := j.jjcmd.JjArgs([]string{"log", "--no-graph", "--color=never", "-r", "children(@) & trunk()..@+", "-T", `change_id ++ "\n"`}, &output) + if err != nil { + return "" + } + output = strings.TrimSpace(output) + if output == "" { + return "" + } + lines := strings.Split(output, "\n") + return fmt.Sprintf("warning: @ is not at the top of your stack — %d commit(s) above @ will be excluded from spr operations", len(lines)) +} + +// readEditState reads the key=value state file. +func (j *JjOps) readEditState() (map[string]string, error) { + data, err := os.ReadFile(j.EditStatePath()) + if err != nil { + return nil, fmt.Errorf("no edit session in progress: %w", err) + } + state := make(map[string]string) + for _, line := range strings.Split(string(data), "\n") { + parts := strings.SplitN(line, "=", 2) + if len(parts) == 2 { + state[parts[0]] = parts[1] + } + } + return state, nil +} diff --git a/vcs/jj_ops_test.go b/vcs/jj_ops_test.go new file mode 100644 index 00000000..b8e7e027 --- /dev/null +++ b/vcs/jj_ops_test.go @@ -0,0 +1,286 @@ +package vcs + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/ejoffe/spr/config" + "github.com/ejoffe/spr/git" + "github.com/ejoffe/spr/git/mockgit" + "github.com/ejoffe/spr/vcs/mockjj" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func makeJjTestConfig() *config.Config { + cfg := config.EmptyConfig() + cfg.Repo.GitHubRemote = "origin" + cfg.Repo.GitHubBranch = "main" + cfg.Repo.MergeMethod = "squash" + return cfg +} + +// --- FetchAndRebase --- + +func TestJjOpsFetchAndRebase(t *testing.T) { + cfg := makeJjTestConfig() + jjmock := mockjj.NewMockJj(t) + ops := NewJjOps(cfg, jjmock, nil) + + jjmock.ExpectFetch() + jjmock.ExpectRebase("origin", "main") + + err := ops.FetchAndRebase(cfg) + require.NoError(t, err) + jjmock.ExpectationsMet() +} + +func TestJjOpsFetchAndRebase_NoRebase(t *testing.T) { + cfg := makeJjTestConfig() + cfg.User.NoRebase = true + jjmock := mockjj.NewMockJj(t) + ops := NewJjOps(cfg, jjmock, nil) + + // Only fetch, no rebase + jjmock.ExpectFetch() + + err := ops.FetchAndRebase(cfg) + require.NoError(t, err) + jjmock.ExpectationsMet() +} + +// --- GetLocalCommitStack --- + +func TestJjOpsGetLocalCommitStack_AllHaveIDs(t *testing.T) { + cfg := makeJjTestConfig() + jjmock := mockjj.NewMockJj(t) + ops := NewJjOps(cfg, jjmock, nil) + + c1 := &git.Commit{ + CommitID: "00000001", + CommitHash: "c100000000000000000000000000000000000000", + ChangeID: "jjchange1", + Subject: "test commit 1", + } + c2 := &git.Commit{ + CommitID: "00000002", + CommitHash: "c200000000000000000000000000000000000000", + ChangeID: "jjchange2", + Subject: "test commit 2", + } + + jjmock.ExpectLogAndRespond([]*git.Commit{c1, c2}) + + commits := ops.GetLocalCommitStack(cfg, nil) + require.Len(t, commits, 2) + assert.Equal(t, "00000001", commits[0].CommitID) + assert.Equal(t, "jjchange1", commits[0].ChangeID) + assert.Equal(t, "00000002", commits[1].CommitID) + assert.Equal(t, "jjchange2", commits[1].ChangeID) + jjmock.ExpectationsMet() +} + +func TestJjOpsGetLocalCommitStack_WIPCommit(t *testing.T) { + cfg := makeJjTestConfig() + jjmock := mockjj.NewMockJj(t) + ops := NewJjOps(cfg, jjmock, nil) + + c1 := &git.Commit{ + CommitID: "00000001", + CommitHash: "c100000000000000000000000000000000000000", + ChangeID: "jjchange1", + Subject: "WIP not ready yet", + } + + jjmock.ExpectLogAndRespond([]*git.Commit{c1}) + + commits := ops.GetLocalCommitStack(cfg, nil) + require.Len(t, commits, 1) + assert.True(t, commits[0].WIP) + jjmock.ExpectationsMet() +} + +// --- AmendInto --- + +func TestJjOpsAmendInto(t *testing.T) { + cfg := makeJjTestConfig() + jjmock := mockjj.NewMockJj(t) + ops := NewJjOps(cfg, jjmock, nil) + + commit := git.Commit{ + CommitID: "00000001", + CommitHash: "c100000000000000000000000000000000000000", + ChangeID: "jjchange1", + } + + jjmock.ExpectSquash("jjchange1") + + err := ops.AmendInto(commit) + require.NoError(t, err) + jjmock.ExpectationsMet() +} + +func TestJjOpsAmendInto_NoChangeID(t *testing.T) { + cfg := makeJjTestConfig() + jjmock := mockjj.NewMockJj(t) + ops := NewJjOps(cfg, jjmock, nil) + + commit := git.Commit{ + CommitID: "00000001", + CommitHash: "c100000000000000000000000000000000000000", + // No ChangeID + } + + err := ops.AmendInto(commit) + require.Error(t, err) + assert.Contains(t, err.Error(), "no jj change ID") + jjmock.ExpectationsMet() +} + +// --- EditStart / EditFinish / EditAbort --- + +func TestJjOpsEditStart(t *testing.T) { + cfg := makeJjTestConfig() + jjmock := mockjj.NewMockJj(t) + gitmock := mockgit.NewMockGit(t) + ops := NewJjOps(cfg, jjmock, gitmock) + + // Create a temp dir so EditStatePath works + tmpDir := t.TempDir() + gitDir := filepath.Join(tmpDir, ".git") + os.Mkdir(gitDir, 0755) + // Override gitcmd.RootDir to use tmpDir + ops.gitcmd = &mockRootDir{rootDir: tmpDir} + + commit := git.Commit{ + CommitID: "00000001", + CommitHash: "c100000000000000000000000000000000000000", + ChangeID: "jjchange1", + Subject: "test commit 1", + } + + jjmock.ExpectOpLog("op123456abcdef") + jjmock.ExpectLogAt("currentchange") + jjmock.ExpectEdit("jjchange1") + + err := ops.EditStart(commit) + require.NoError(t, err) + assert.True(t, ops.IsEditing()) + + // Verify state file contents + data, err := os.ReadFile(ops.EditStatePath()) + require.NoError(t, err) + assert.Contains(t, string(data), "original_at=currentchange") + assert.Contains(t, string(data), "op_id=op123456abcdef") + assert.Contains(t, string(data), "change_id=jjchange1") + + jjmock.ExpectationsMet() +} + +func TestJjOpsEditFinish(t *testing.T) { + cfg := makeJjTestConfig() + jjmock := mockjj.NewMockJj(t) + ops := NewJjOps(cfg, jjmock, nil) + + // Create state file manually + tmpDir := t.TempDir() + gitDir := filepath.Join(tmpDir, ".git") + os.Mkdir(gitDir, 0755) + ops.gitcmd = &mockRootDir{rootDir: tmpDir} + + stateContent := "vcs=jj\nchange_id=jjchange1\noriginal_at=prevchange\nop_id=op123456\ncommit_id=00000001\n" + os.WriteFile(ops.EditStatePath(), []byte(stateContent), 0644) + + jjmock.ExpectNew("prevchange") + + err := ops.EditFinish() + require.NoError(t, err) + assert.False(t, ops.IsEditing()) // state file cleaned up + jjmock.ExpectationsMet() +} + +func TestJjOpsEditAbort(t *testing.T) { + cfg := makeJjTestConfig() + jjmock := mockjj.NewMockJj(t) + ops := NewJjOps(cfg, jjmock, nil) + + tmpDir := t.TempDir() + gitDir := filepath.Join(tmpDir, ".git") + os.Mkdir(gitDir, 0755) + ops.gitcmd = &mockRootDir{rootDir: tmpDir} + + stateContent := "vcs=jj\nchange_id=jjchange1\noriginal_at=prevchange\nop_id=op123456\ncommit_id=00000001\n" + os.WriteFile(ops.EditStatePath(), []byte(stateContent), 0644) + + jjmock.ExpectOpRestore("op123456") + + err := ops.EditAbort() + require.NoError(t, err) + assert.False(t, ops.IsEditing()) + jjmock.ExpectationsMet() +} + +func TestJjOpsEditStart_NoChangeID(t *testing.T) { + cfg := makeJjTestConfig() + jjmock := mockjj.NewMockJj(t) + ops := NewJjOps(cfg, jjmock, nil) + + commit := git.Commit{CommitID: "00000001", CommitHash: "c100000000000000000000000000000000000000"} + + err := ops.EditStart(commit) + require.Error(t, err) + assert.Contains(t, err.Error(), "no jj change ID") + jjmock.ExpectationsMet() +} + +// --- PrepareForPush --- + +func TestJjOpsPrepareForPush_IsNoop(t *testing.T) { + cfg := makeJjTestConfig() + ops := NewJjOps(cfg, nil, nil) + + cleanup, err := ops.PrepareForPush() + require.NoError(t, err) + require.NotNil(t, cleanup) + cleanup() // should not panic +} + +// --- CheckStackCompleteness --- + +func TestJjOpsCheckStackCompleteness_AtTop(t *testing.T) { + cfg := makeJjTestConfig() + jjmock := mockjj.NewMockJj(t) + ops := NewJjOps(cfg, jjmock, nil) + + jjmock.ExpectCheckChildren("") + + warning := ops.CheckStackCompleteness() + assert.Equal(t, "", warning) + jjmock.ExpectationsMet() +} + +func TestJjOpsCheckStackCompleteness_MidStack(t *testing.T) { + cfg := makeJjTestConfig() + jjmock := mockjj.NewMockJj(t) + ops := NewJjOps(cfg, jjmock, nil) + + jjmock.ExpectCheckChildren("jjchange_above1\njjchange_above2") + + warning := ops.CheckStackCompleteness() + assert.Contains(t, warning, "2 commit(s) above @") + jjmock.ExpectationsMet() +} + +// --- mockRootDir implements git.GitInterface just for RootDir --- + +type mockRootDir struct { + rootDir string +} + +func (m *mockRootDir) GitWithEditor(args string, output *string, editorCmd string) error { return nil } +func (m *mockRootDir) Git(args string, output *string) error { return nil } +func (m *mockRootDir) MustGit(args string, output *string) {} +func (m *mockRootDir) RootDir() string { return m.rootDir } +func (m *mockRootDir) DeleteRemoteBranch(ctx context.Context, branch string) error { return nil } diff --git a/vcs/jj_parse.go b/vcs/jj_parse.go new file mode 100644 index 00000000..ae9ce290 --- /dev/null +++ b/vcs/jj_parse.go @@ -0,0 +1,172 @@ +package vcs + +import ( + "regexp" + "strings" + + "github.com/ejoffe/spr/git" +) + +// parsedJjCommit is the intermediate representation of a commit from jj log output. +type parsedJjCommit struct { + commitHash string // git SHA (from jj's commit_id template keyword) + changeID string // jj change ID + empty bool + description string + sprCommitID string // extracted from commit-id: trailer, may be "" + subject string + body string + wip bool + bookmarks []string // jj bookmarks pointing at this commit +} + +// parseJjLogOutputExtended parses output from the extended template that includes bookmarks: +// +// jj log ... -T 'commit_id ++ "\x1f" ++ change_id ++ "\x1f" ++ empty ++ "\x1f" ++ +// bookmarks ++ "\x1f" ++ description ++ "\x1e"' +// +// Fields: commit_id, change_id, empty, bookmarks, description (5 fields). +// Returns the parsed commits and true if all non-empty commits have commit-id trailers. +func parseJjLogOutputExtended(output string) ([]parsedJjCommit, bool) { + commitIDRegex := regexp.MustCompile(`commit-id:\s*([a-f0-9]{8})`) + + records := strings.Split(output, "\x1e") + var commits []parsedJjCommit + valid := true + + for _, record := range records { + record = strings.TrimSpace(record) + if record == "" { + continue + } + + fields := strings.SplitN(record, "\x1f", 5) + if len(fields) < 5 { + continue + } + + commitHash := strings.TrimSpace(fields[0]) + changeID := strings.TrimSpace(fields[1]) + isEmpty := strings.TrimSpace(fields[2]) == "true" + bookmarksRaw := strings.TrimSpace(fields[3]) + description := fields[4] + + if isEmpty && strings.TrimSpace(description) == "" { + continue + } + + // Parse bookmarks (space-separated in jj output, may have trailing markers like *) + var bookmarks []string + if bookmarksRaw != "" { + for _, bm := range strings.Fields(bookmarksRaw) { + // Strip trailing markers like * (indicating divergent bookmarks) + bm = strings.TrimRight(bm, "*") + if bm != "" && !git.IsSPRBranch(bm) { + bookmarks = append(bookmarks, bm) + } + } + } + + lines := strings.SplitN(strings.TrimSpace(description), "\n", 2) + subject := "" + body := "" + if len(lines) > 0 { + subject = strings.TrimSpace(lines[0]) + } + if len(lines) > 1 { + body = strings.TrimSpace(lines[1]) + } + + var sprCommitID string + matches := commitIDRegex.FindStringSubmatch(description) + if matches != nil { + sprCommitID = matches[1] + } else if !isEmpty { + valid = false + } + + commits = append(commits, parsedJjCommit{ + commitHash: commitHash, + changeID: changeID, + empty: isEmpty, + description: description, + sprCommitID: sprCommitID, + subject: subject, + body: body, + wip: strings.HasPrefix(subject, "WIP"), + bookmarks: bookmarks, + }) + } + + return commits, valid +} + +// parseJjLogOutput parses output from: +// +// jj log --no-graph --reversed --color=never -r 'trunk()..@' +// -T 'commit_id ++ "\x1f" ++ change_id ++ "\x1f" ++ empty ++ "\x1f" ++ description ++ "\x1e"' +// +// Fields are separated by \x1f (unit separator), records by \x1e (record separator). +// Returns the parsed commits and true if all non-empty commits have commit-id trailers. +func parseJjLogOutput(output string) ([]parsedJjCommit, bool) { + commitIDRegex := regexp.MustCompile(`commit-id:\s*([a-f0-9]{8})`) + + records := strings.Split(output, "\x1e") + var commits []parsedJjCommit + valid := true + + for _, record := range records { + record = strings.TrimSpace(record) + if record == "" { + continue + } + + fields := strings.SplitN(record, "\x1f", 4) + if len(fields) < 4 { + continue + } + + commitHash := strings.TrimSpace(fields[0]) + changeID := strings.TrimSpace(fields[1]) + isEmpty := strings.TrimSpace(fields[2]) == "true" + description := fields[3] + + // Skip empty commits with no description (working copy placeholder) + if isEmpty && strings.TrimSpace(description) == "" { + continue + } + + // Parse subject and body from description + lines := strings.SplitN(strings.TrimSpace(description), "\n", 2) + subject := "" + body := "" + if len(lines) > 0 { + subject = strings.TrimSpace(lines[0]) + } + if len(lines) > 1 { + body = strings.TrimSpace(lines[1]) + } + + // Extract commit-id trailer + var sprCommitID string + matches := commitIDRegex.FindStringSubmatch(description) + if matches != nil { + sprCommitID = matches[1] + } else if !isEmpty { + valid = false + } + + commits = append(commits, parsedJjCommit{ + commitHash: commitHash, + changeID: changeID, + empty: isEmpty, + description: description, + sprCommitID: sprCommitID, + subject: subject, + body: body, + wip: strings.HasPrefix(subject, "WIP"), + }) + } + + return commits, valid +} diff --git a/vcs/jj_parse_test.go b/vcs/jj_parse_test.go new file mode 100644 index 00000000..9a3160d0 --- /dev/null +++ b/vcs/jj_parse_test.go @@ -0,0 +1,155 @@ +package vcs + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestParseJjLogOutput_SingleCommit(t *testing.T) { + input := "c100000000000000000000000000000000000000\x1fmychangeid1234\x1ffalse\x1ftest commit 1\n\ncommit-id:00000001\n\x1e" + commits, valid := parseJjLogOutput(input) + require.True(t, valid) + require.Len(t, commits, 1) + assert.Equal(t, "00000001", commits[0].sprCommitID) + assert.Equal(t, "mychangeid1234", commits[0].changeID) + assert.Equal(t, "c100000000000000000000000000000000000000", commits[0].commitHash) + assert.Equal(t, "test commit 1", commits[0].subject) + assert.False(t, commits[0].wip) + assert.False(t, commits[0].empty) +} + +func TestParseJjLogOutput_MultipleCommits(t *testing.T) { + input := "c100000000000000000000000000000000000000\x1fchange1\x1ffalse\x1fcommit 1\n\ncommit-id:00000001\n\x1e" + + "c200000000000000000000000000000000000000\x1fchange2\x1ffalse\x1fcommit 2\n\ncommit-id:00000002\n\x1e" + + "c300000000000000000000000000000000000000\x1fchange3\x1ffalse\x1fcommit 3\n\ncommit-id:00000003\n\x1e" + commits, valid := parseJjLogOutput(input) + require.True(t, valid) + require.Len(t, commits, 3) + assert.Equal(t, "00000001", commits[0].sprCommitID) + assert.Equal(t, "00000002", commits[1].sprCommitID) + assert.Equal(t, "00000003", commits[2].sprCommitID) + assert.Equal(t, "change1", commits[0].changeID) + assert.Equal(t, "change2", commits[1].changeID) + assert.Equal(t, "change3", commits[2].changeID) +} + +func TestParseJjLogOutput_MissingCommitID(t *testing.T) { + input := "c100000000000000000000000000000000000000\x1fchange1\x1ffalse\x1fcommit without trailer\n\x1e" + commits, valid := parseJjLogOutput(input) + require.False(t, valid) // invalid because non-empty commit lacks commit-id + require.Len(t, commits, 1) + assert.Equal(t, "", commits[0].sprCommitID) + assert.Equal(t, "change1", commits[0].changeID) + assert.Equal(t, "commit without trailer", commits[0].subject) +} + +func TestParseJjLogOutput_EmptyCommitSkipped(t *testing.T) { + input := "c100000000000000000000000000000000000000\x1fchange1\x1ftrue\x1f\x1e" + + "c200000000000000000000000000000000000000\x1fchange2\x1ffalse\x1freal commit\n\ncommit-id:00000001\n\x1e" + commits, valid := parseJjLogOutput(input) + require.True(t, valid) + require.Len(t, commits, 1) // empty commit skipped + assert.Equal(t, "change2", commits[0].changeID) +} + +func TestParseJjLogOutput_WIPPrefix(t *testing.T) { + input := "c100000000000000000000000000000000000000\x1fchange1\x1ffalse\x1fWIP work in progress\n\ncommit-id:00000001\n\x1e" + commits, valid := parseJjLogOutput(input) + require.True(t, valid) + require.Len(t, commits, 1) + assert.True(t, commits[0].wip) + assert.Equal(t, "WIP work in progress", commits[0].subject) +} + +func TestParseJjLogOutput_MultiLineBody(t *testing.T) { + input := "c100000000000000000000000000000000000000\x1fchange1\x1ffalse\x1fFix the bug\n\nThis is a detailed\ndescription of the fix.\n\ncommit-id:deadbeef\n\x1e" + commits, valid := parseJjLogOutput(input) + require.True(t, valid) + require.Len(t, commits, 1) + assert.Equal(t, "Fix the bug", commits[0].subject) + assert.Equal(t, "deadbeef", commits[0].sprCommitID) + assert.Contains(t, commits[0].body, "detailed") + assert.Contains(t, commits[0].body, "description of the fix") +} + +func TestParseJjLogOutput_EmptyInput(t *testing.T) { + commits, valid := parseJjLogOutput("") + require.True(t, valid) // no commits = valid (nothing to check) + require.Len(t, commits, 0) +} + +func TestParseJjLogOutput_CommitIDWithSpace(t *testing.T) { + // commit-id: with a space after colon (spr regex allows this) + input := "c100000000000000000000000000000000000000\x1fchange1\x1ffalse\x1ftest commit\n\ncommit-id: abcdef01\n\x1e" + commits, valid := parseJjLogOutput(input) + require.True(t, valid) + require.Len(t, commits, 1) + assert.Equal(t, "abcdef01", commits[0].sprCommitID) +} + +func TestParseJjLogOutput_MixedValidAndInvalid(t *testing.T) { + // First commit has trailer, second doesn't + input := "c100000000000000000000000000000000000000\x1fchange1\x1ffalse\x1fcommit 1\n\ncommit-id:00000001\n\x1e" + + "c200000000000000000000000000000000000000\x1fchange2\x1ffalse\x1fcommit 2 no trailer\n\x1e" + commits, valid := parseJjLogOutput(input) + require.False(t, valid) // invalid because second commit lacks trailer + require.Len(t, commits, 2) + assert.Equal(t, "00000001", commits[0].sprCommitID) + assert.Equal(t, "", commits[1].sprCommitID) +} + +// --- Extended template tests (with bookmarks) --- + +func TestParseJjLogOutputExtended_WithBookmarks(t *testing.T) { + // 5 fields: commit_id, change_id, empty, bookmarks, description + input := "c100000000000000000000000000000000000000\x1fchange1\x1ffalse\x1ffeature-a\x1fcommit 1\n\ncommit-id:00000001\n\x1e" + + "c200000000000000000000000000000000000000\x1fchange2\x1ffalse\x1f\x1fcommit 2\n\ncommit-id:00000002\n\x1e" + commits, valid := parseJjLogOutputExtended(input) + require.True(t, valid) + require.Len(t, commits, 2) + assert.Equal(t, []string{"feature-a"}, commits[0].bookmarks) + assert.Len(t, commits[1].bookmarks, 0) +} + +func TestParseJjLogOutputExtended_MultipleBookmarks(t *testing.T) { + input := "c100000000000000000000000000000000000000\x1fchange1\x1ffalse\x1fbranch-a branch-b\x1fcommit 1\n\ncommit-id:00000001\n\x1e" + commits, valid := parseJjLogOutputExtended(input) + require.True(t, valid) + require.Len(t, commits, 1) + assert.Equal(t, []string{"branch-a", "branch-b"}, commits[0].bookmarks) +} + +func TestParseJjLogOutputExtended_FiltersSPRBookmarks(t *testing.T) { + input := "c100000000000000000000000000000000000000\x1fchange1\x1ffalse\x1fspr/main/abc12345 real-branch\x1fcommit 1\n\ncommit-id:00000001\n\x1e" + commits, valid := parseJjLogOutputExtended(input) + require.True(t, valid) + require.Len(t, commits, 1) + assert.Equal(t, []string{"real-branch"}, commits[0].bookmarks) +} + +func TestParseJjLogOutputExtended_DivergentBookmarkMarker(t *testing.T) { + // jj marks divergent bookmarks with trailing * + input := "c100000000000000000000000000000000000000\x1fchange1\x1ffalse\x1fmy-branch*\x1fcommit 1\n\ncommit-id:00000001\n\x1e" + commits, valid := parseJjLogOutputExtended(input) + require.True(t, valid) + require.Len(t, commits, 1) + assert.Equal(t, []string{"my-branch"}, commits[0].bookmarks) +} + +func TestParseJjLogOutputExtended_NoBookmarks(t *testing.T) { + input := "c100000000000000000000000000000000000000\x1fchange1\x1ffalse\x1f\x1fcommit 1\n\ncommit-id:00000001\n\x1e" + commits, valid := parseJjLogOutputExtended(input) + require.True(t, valid) + require.Len(t, commits, 1) + assert.Len(t, commits[0].bookmarks, 0) +} + +func TestParseJjLogOutputExtended_MissingCommitID(t *testing.T) { + input := "c100000000000000000000000000000000000000\x1fchange1\x1ffalse\x1fmy-branch\x1fcommit without trailer\n\x1e" + commits, valid := parseJjLogOutputExtended(input) + require.False(t, valid) + require.Len(t, commits, 1) + assert.Equal(t, []string{"my-branch"}, commits[0].bookmarks) +} diff --git a/vcs/mockjj/mockjj.go b/vcs/mockjj/mockjj.go new file mode 100644 index 00000000..ad530b46 --- /dev/null +++ b/vcs/mockjj/mockjj.go @@ -0,0 +1,171 @@ +package mockjj + +import ( + "fmt" + "strings" + "testing" + + "github.com/ejoffe/spr/git" + "github.com/stretchr/testify/require" +) + +// NewMockJj creates a new mock jj executor. +func NewMockJj(t *testing.T) *Mock { + return &Mock{ + assert: require.New(t), + } +} + +// Mock implements vcs.JjInterface with expectation-based command verification, +// following the same pattern as git/mockgit/mockgit.go. +type Mock struct { + assert *require.Assertions + expectedCmd []string + response []responder +} + +// Jj verifies the command matches expectations and returns the configured response. +func (m *Mock) Jj(args string, output *string) error { + fmt.Printf("CMD: jj %s\n", args) + + m.assert.NotEmpty(m.expectedCmd, fmt.Sprintf("Unexpected command: jj %s\n", args)) + + expected := m.expectedCmd[0] + actual := "jj " + args + m.assert.Equal(expected, actual) + + if m.response[0].Valid() { + m.assert.NotNil(output) + *output = m.response[0].Output() + } else if output != nil { + *output = "" + } + + m.expectedCmd = m.expectedCmd[1:] + m.response = m.response[1:] + + return nil +} + +// MustJj calls Jj and panics on error. +func (m *Mock) MustJj(args string, output *string) { + err := m.Jj(args, output) + if err != nil { + panic(err) + } +} + +// JjArgs verifies commands with pre-split arguments. +func (m *Mock) JjArgs(args []string, output *string) error { + return m.Jj(strings.Join(args, " "), output) +} + +// ExpectationsMet verifies all expected commands were called. +func (m *Mock) ExpectationsMet() { + m.assert.Empty(m.expectedCmd, fmt.Sprintf("expected additional jj commands: %v", m.expectedCmd)) + m.assert.Empty(m.response, fmt.Sprintf("expected additional jj responses: %v", m.response)) +} + +// ExpectFetch expects a jj git fetch command. +func (m *Mock) ExpectFetch() { + m.expect("jj git fetch") +} + +// ExpectRebase expects a jj rebase command. +func (m *Mock) ExpectRebase(remote, branch string) { + m.expect(fmt.Sprintf("jj rebase -b @ -d %s@%s", branch, remote)) +} + +// ExpectLogAndRespond expects the jj log command and returns formatted output. +func (m *Mock) ExpectLogAndRespond(commits []*git.Commit) { + template := `commit_id ++ "\x1f" ++ change_id ++ "\x1f" ++ empty ++ "\x1f" ++ description ++ "\x1e"` + m.expect(fmt.Sprintf(`jj log --no-graph --reversed --color=never -r trunk()..@ -T %s`, template)). + respond(formatJjLogResponse(commits)) +} + +// ExpectDescribe expects a jj describe command for a specific change. +func (m *Mock) ExpectDescribe(changeID, message string) { + m.expect(fmt.Sprintf("jj describe -r %s -m %s", changeID, message)) +} + +// ExpectSquash expects a jj squash --into command. +func (m *Mock) ExpectSquash(changeID string) { + m.expect(fmt.Sprintf("jj squash --into %s", changeID)) +} + +// ExpectEdit expects a jj edit command. +func (m *Mock) ExpectEdit(changeID string) { + m.expect(fmt.Sprintf("jj edit %s", changeID)) +} + +// ExpectNew expects a jj new command. +func (m *Mock) ExpectNew(changeID string) { + m.expect(fmt.Sprintf("jj new %s", changeID)) +} + +// ExpectOpLog expects a jj op log command and returns an operation ID. +func (m *Mock) ExpectOpLog(opID string) { + m.expect("jj op log --no-graph -n 1 -T 'id.short(16)'").respond(opID) +} + +// ExpectOpRestore expects a jj op restore command. +func (m *Mock) ExpectOpRestore(opID string) { + m.expect(fmt.Sprintf("jj op restore %s", opID)) +} + +// ExpectLogAt expects a jj log command for the current change ID. +func (m *Mock) ExpectLogAt(changeID string) { + m.expect("jj log --no-graph -r @ -T change_id").respond(changeID) +} + +// ExpectCheckChildren expects the children(@) completeness check and responds +// with the given output (empty string means no children, i.e. @ is at the top). +func (m *Mock) ExpectCheckChildren(response string) { + m.expect(`jj log --no-graph --color=never -r children(@) & trunk()..@+ -T change_id ++ "\n"`).respond(response) +} + +func (m *Mock) expect(cmd string) *Mock { + m.expectedCmd = append(m.expectedCmd, cmd) + m.response = append(m.response, &stringResponse{valid: false}) + return m +} + +func (m *Mock) respond(response string) { + m.response[len(m.response)-1] = &stringResponse{ + valid: true, + output: response, + } +} + +type responder interface { + Valid() bool + Output() string +} + +type stringResponse struct { + valid bool + output string +} + +func (r *stringResponse) Valid() bool { return r.valid } +func (r *stringResponse) Output() string { return r.output } + +// formatJjLogResponse formats commits as jj log output with field/record separators. +func formatJjLogResponse(commits []*git.Commit) string { + var b strings.Builder + for _, c := range commits { + desc := c.Subject + if c.Body != "" { + desc += "\n\n" + c.Body + } + if c.CommitID != "" { + desc += "\n\ncommit-id:" + c.CommitID + } + changeID := c.ChangeID + if changeID == "" { + changeID = "jjchange_" + c.CommitID + } + fmt.Fprintf(&b, "%s\x1f%s\x1ffalse\x1f%s\n\x1e", c.CommitHash, changeID, desc) + } + return b.String() +}