Skip to content

Commit 49dfab9

Browse files
committed
feat: add Jujutsu (jj) compatibility via VCSOperations abstraction
Adds end-to-end Jujutsu support for stacked PRs in colocated git+jj repos, preserving jj change IDs across all spr operations and using jj-native primitives (auto-rebase, conflicts as commits, non-blocking edits) instead of mimicking git's rebase-session model. Core architecture: - New vcs.VCSOperations interface — the abstraction boundary for history-rewriting operations (FetchAndRebase, GetLocalCommitStack, AmendInto, EditStart/Finish/Abort, Fetch, PushBranches, etc.). - vcs/git_ops.go: git implementation (extracted from spr.go, behavior- preserving). Improved EditFinish uses .git/REBASE_HEAD to distinguish conflict-resolution from initial edit stop and stages only tracked changes (subsumes upstream's 606df43 and 0767a45 fixes). - vcs/jj_ops.go: jj implementation. Uses jj-native commands: jj git fetch + jj rebase, jj describe (for auto-added commit-id trailers), jj squash --into, jj edit, jj bookmark set + jj git push. - Commit.ChangeID field (empty in git mode, populated in jj mode). - vcs/detect.go: factory chooses JjOps when .jj/ exists and User.NoJJ is false; respects --no-jj flag and SPR_NOJJ env var. Auto-revset (the key correctness fix): - JjOps.GetLocalCommitStack queries trunk()..(@:: | ::@) — the connected component containing @, excluding trunk — rather than the position-dependent trunk()..@. Result: spr finds the full stack regardless of where @ is, so 'spr update' from a mid-stack @ no longer silently truncates the stack and closes PRs for the missing commits. - JjOps.CheckStackCompleteness detects multi-head ambiguity via heads(trunk()..(@:: | ::@)) and refuses non-linear stacks with a clear error pointing at jj rebase / jj edit as the fix. - checkStackUsable (renamed from confirmIfIncompleteStack, used as guard in update/merge/status/check/amend) blocks rather than prompts on ambiguity — no safe 'continue anyway' answer exists. jj-mode behavioral divergences: - spr edit (jj): announces the action then runs 'jj edit <change-id>'. No state file, no op-id snapshot, no rebase-session machinery — jj is non-blocking and 'jj undo' handles revert. --done and --abort are echo-only deprecations that point users at native jj. - spr sync (jj): runs 'jj git fetch' (the literal 'git cherry-pick' was broken in jj). No rebase promise — that's spr update's job. VCS-layer integrations: - Configurable branch prefix (upstream feature b01881d): jj_ops threads cfg.User.BranchPrefix through PushBranches' bookmark glob. - Stdout/stderr separation in vcs/jj_cmd.go: replaced cmd.CombinedOutput() with separate capture so jj's stderr messages (e.g. 'Rebased 1 descendant commits onto updated working copy') don't corrupt parsed log output. Caught by the conflicted-push integration test. Testing: - Two suites: unit (mockjj-based, fast) and integration (real jj via the new vcs/jjtest package + a colocated test repo). - Integration build tag //go:build integration; SPR_SKIP_JJ_INTEGRATION=1 env var to skip cleanly. - jjtest.NewRepo creates a colocated git+jj repo in t.TempDir() with a bare-repo origin; helpers for stack topology (AddCommit, Edit, Fork, InsertAbove, SnapshotOpLog, AssertOpsSince). - Headline regression: TestSprIntegration_StatusFromMidStack_ShowsFullStack pins that GetLocalCommitStack from mid-stack @ returns the whole stack. - TestJjIntegration_PushBranches_ConflictedCommit_Refused pins jj's natural refusal to push conflicts (this is the test that revealed the stderr bug). - 100% statement coverage on jj-related production code (jj_ops.go, jj_cmd.go, jj_parse.go). - Op-log assertions verify that spr commands do exactly the expected jj operations and no destructive ones. Coverage tooling: - scripts/coverage-merge/main.go merges two coverprofile files (max hits per block, within and across profiles) and reports per-file attribution (unit %, integ %, merged %, plus 'only in X' sections). - Makefile targets: test-unit, test-integration, test-all, coverage-html. - CI installs pinned JJ_VERSION (0.40.0), runs both suites with -coverpkg=./..., merges, uploads as artifact. Documentation: - readme.md: new Jujutsu (jj) Support section covering setup, the jj spr command set, divergences for edit/sync, the multi-head error message, and the auto-revset behavior. - docs/jj-mode-design.md: full design rationale including alternatives considered (spr-tip bookmark, 3-option prompt), the stderr bug fix, testing strategy, and what's deferred. Files: +4500/-110 across 35 files.
1 parent 0767a45 commit 49dfab9

29 files changed

Lines changed: 4400 additions & 121 deletions

.github/workflows/ci.yml

Lines changed: 40 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,11 @@ name: golang build
22

33
on: [push, pull_request, workflow_dispatch]
44

5+
env:
6+
# Pin the jj version used by integration tests. Bump intentionally;
7+
# jj's revset/template syntax can change between minor versions.
8+
JJ_VERSION: 0.40.0
9+
510
jobs:
611

712
build:
@@ -15,22 +20,52 @@ jobs:
1520
with:
1621
go-version: 1.21
1722

23+
- name: Install jj
24+
run: |
25+
set -euo pipefail
26+
curl -fsSL \
27+
"https://github.com/jj-vcs/jj/releases/download/v${JJ_VERSION}/jj-v${JJ_VERSION}-x86_64-unknown-linux-musl.tar.gz" \
28+
| tar -xz -C /usr/local/bin/ jj
29+
jj --version
30+
1831
- name: Build
1932
working-directory: ./
2033
run: go build -v ./...
2134

22-
- name: Test
35+
- name: Test (unit)
2336
working-directory: ./
24-
run: go test -race -coverprofile=./coverage.txt -covermode=atomic ./...
37+
run: go test -race -coverprofile=cov-unit.out -covermode=atomic ./...
38+
39+
- name: Test (integration)
40+
working-directory: ./
41+
run: |
42+
# Configure a default git identity so jj's colocated git backend
43+
# has something to use (the per-repo overrides in jjtest also set
44+
# this, but a global default avoids any first-call edge cases).
45+
git config --global user.name "spr-ci"
46+
git config --global user.email "spr-ci@example.com"
47+
go test -race -tags=integration -coverprofile=cov-integration.out -covermode=atomic ./...
48+
49+
- name: Merge coverage and report per-file attribution
50+
working-directory: ./
51+
run: go run ./scripts/coverage-merge cov-unit.out cov-integration.out cov-merged.out
52+
53+
- name: Upload coverage artifacts
54+
uses: actions/upload-artifact@v4
55+
with:
56+
name: coverage
57+
path: |
58+
cov-unit.out
59+
cov-integration.out
60+
cov-merged.out
2561
2662
- name: Check goreleaser file
2763
uses: goreleaser/goreleaser-action@v6.2.1
2864
with:
2965
version: latest
3066
args: check
3167

32-
#- name: Upload coverage
68+
#- name: Upload coverage to codecov
3369
# uses: codecov/codecov-action@v2
3470
# with:
35-
# files: ./coverage.txt
36-
71+
# files: ./cov-merged.out

Makefile

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,30 @@
1+
.PHONY: bin test test-unit test-integration test-all coverage-html
12

23
bin:
34
goreleaser build --snapshot --clean --single-target
45

6+
# Default test target: unit tests only.
7+
test: test-unit
8+
9+
# Unit tests — no integration build tag, no jj binary required.
10+
# -coverpkg=./... so coverage of cross-package calls is counted (e.g.
11+
# spr_test.go calls into vcs.JjOps, which we want reflected in vcs/jj_ops.go
12+
# coverage).
13+
test-unit:
14+
go test -race -coverpkg=./... -coverprofile=cov-unit.out -covermode=atomic ./...
15+
16+
# Integration tests — runs against a real `jj` binary. Set
17+
# SPR_SKIP_JJ_INTEGRATION=1 to skip cleanly when jj is unavailable.
18+
test-integration:
19+
go test -race -tags=integration -coverpkg=./... -coverprofile=cov-integration.out -covermode=atomic ./...
20+
21+
# Both suites, then merge profiles and print per-file attribution.
22+
test-all: test-unit test-integration
23+
@go run ./scripts/coverage-merge cov-unit.out cov-integration.out cov-merged.out
24+
25+
# Generate HTML coverage reports for both profiles plus the merged view.
26+
coverage-html: test-all
27+
go tool cover -html=cov-unit.out -o cov-unit.html
28+
go tool cover -html=cov-integration.out -o cov-integration.html
29+
go tool cover -html=cov-merged.out -o cov-merged.html
30+
@echo "Open cov-unit.html, cov-integration.html, cov-merged.html"

cmd/amend/main.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"github.com/ejoffe/spr/git/realgit"
1111
"github.com/ejoffe/spr/github/githubclient"
1212
"github.com/ejoffe/spr/spr"
13+
"github.com/ejoffe/spr/vcs"
1314
"github.com/jessevdk/go-flags"
1415
"github.com/rs/zerolog"
1516
"github.com/rs/zerolog/log"
@@ -62,7 +63,8 @@ func main() {
6263
client := githubclient.NewGitHubClient(ctx, cfg)
6364
gitcmd = realgit.NewGitCmd(cfg)
6465

65-
sd := spr.NewStackedPR(cfg, client, gitcmd)
66+
vcsOps := vcs.NewVCSOperations(cfg, gitcmd)
67+
sd := spr.NewStackedPR(cfg, client, gitcmd, vcsOps)
6668
sd.AmendCommit(ctx)
6769

6870
if opts.Update {

cmd/spr/main.go

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"context"
66
"fmt"
77
"os"
8+
"os/exec"
89
"strings"
910

1011
"github.com/ejoffe/rake"
@@ -13,6 +14,7 @@ import (
1314
"github.com/ejoffe/spr/git/realgit"
1415
"github.com/ejoffe/spr/github/githubclient"
1516
"github.com/ejoffe/spr/spr"
17+
"github.com/ejoffe/spr/vcs"
1618
"github.com/rs/zerolog"
1719
"github.com/rs/zerolog/log"
1820

@@ -95,9 +97,21 @@ func main() {
9597
}
9698
gitcmd = realgit.NewGitCmd(cfg)
9799

100+
// Check for --no-jj flag or SPR_NOJJ env var before creating VCS operations.
101+
// This must happen before app.Run() since vcsOps is created here.
102+
for _, arg := range os.Args[1:] {
103+
if arg == "--no-jj" {
104+
cfg.User.NoJJ = true
105+
}
106+
}
107+
if os.Getenv("SPR_NOJJ") == "true" {
108+
cfg.User.NoJJ = true
109+
}
110+
98111
ctx := context.Background()
99112
client := githubclient.NewGitHubClient(ctx, cfg)
100-
stackedpr := spr.NewStackedPR(cfg, client, gitcmd)
113+
vcsOps := vcs.NewVCSOperations(cfg, gitcmd)
114+
stackedpr := spr.NewStackedPR(cfg, client, gitcmd, vcsOps)
101115

102116
detailFlag := &cli.BoolFlag{
103117
Name: "detail",
@@ -160,6 +174,12 @@ VERSION: fork of {{.Version}}
160174
Value: false,
161175
Usage: "Show runtime debug info",
162176
},
177+
&cli.BoolFlag{
178+
Name: "no-jj",
179+
Value: false,
180+
Usage: "Disable jj (Jujutsu) mode even in jj-colocated repos",
181+
EnvVars: []string{"SPR_NOJJ"},
182+
},
163183
},
164184
Before: func(c *cli.Context) error {
165185
if c.IsSet("debug") {
@@ -346,6 +366,25 @@ VERSION: fork of {{.Version}}
346366
return nil
347367
},
348368
},
369+
{
370+
Name: "jj-setup",
371+
Usage: "Register 'jj spr' alias for use in Jujutsu repos",
372+
Action: func(c *cli.Context) error {
373+
cmd := exec.Command("jj", "config", "set", "--user",
374+
"aliases.spr", `["util", "exec", "--", "git-spr"]`)
375+
cmd.Stdout = os.Stdout
376+
cmd.Stderr = os.Stderr
377+
err := cmd.Run()
378+
if err != nil {
379+
return cli.Exit(fmt.Sprintf("Failed to set jj alias: %s", err), 1)
380+
}
381+
fmt.Println("jj alias registered. You can now use:")
382+
fmt.Println(" jj spr update")
383+
fmt.Println(" jj spr status")
384+
fmt.Println(" jj spr merge")
385+
return nil
386+
},
387+
},
349388
{
350389
Name: "version",
351390
Usage: "Show version info",

config/config.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ type RepoConfig struct {
4141
ForceFetchTags bool `default:"false" yaml:"forceFetchTags"`
4242

4343
ShowPrTitlesInStack bool `default:"false" yaml:"showPrTitlesInStack"`
44+
ShowStackNumberInTitle bool `default:"false" yaml:"showStackNumberInTitle"`
4445
BranchPushIndividually bool `default:"false" yaml:"branchPushIndividually"`
4546
}
4647

@@ -55,6 +56,7 @@ type UserConfig struct {
5556
PreserveTitleAndBody bool `default:"false" yaml:"preserveTitleAndBody"`
5657
NoRebase bool `default:"false" yaml:"noRebase"`
5758
NoFetch bool `default:"false" yaml:"noFetch"`
59+
NoJJ bool `default:"false" yaml:"noJJ"`
5860
DeleteMergedBranches bool `default:"false" yaml:"deleteMergedBranches"`
5961
ShortPRLink bool `default:"false" yaml:"shortPRLink"`
6062
ShowCommitID bool `default:"false" yaml:"showCommitID"`

0 commit comments

Comments
 (0)