Jj compat progressive#513
Open
jucor wants to merge 12 commits into
Open
Conversation
Split history-rewriting VCS operations out of spr/spr.go into a new
vcs package behind a VCSOperations interface. Today this is wholly
git-backed via GitOps (a no-behavior-change extraction of the existing
git rebase / amend / edit logic). The interface is shaped so alternate
backends (e.g. jj/Jujutsu) can implement it without changing spr.go
callers.
Behavior preserved:
- EditCommitDone's specific 'Rebase conflict detected. Resolve
conflicts and run "<cmd> edit --done" again.' recovery hint is
preserved by returning a sentinel vcs.ErrRebaseConflict from
EditFinish and detecting it with errors.Is in spr.go.
Interface design:
- GetLocalCommitStack() ([]git.Commit, error) — no args (receiver
already holds cfg + gitcmd), error return so a future jj
implementation can surface immutable-commit or revset errors
cleanly instead of panicking.
- NewStackedPR(cfg, github, gitcmd, vcsOps) — required vcsOps arg
rather than a variadic shim, so all call sites pass an explicit
backend.
- vcs.NewVCSOperations factory returns GitOps unconditionally for
now; extension points are documented.
Files:
- spr/spr.go: callers route through sd.vcsOps.X instead of in-lining
git commands. EditCommit / EditCommitDone / EditCommitAbort,
SyncStack, FetchAndRebase, AmendInto, GetLocalCommitStack all
delegate.
- vcs/interface.go: VCSOperations + NewVCSOperations factory + the
ErrRebaseConflict sentinel (in git_ops.go).
- vcs/git_ops.go: GitOps — extracts the existing git logic.
- vcs/git_ops_test.go: unit tests (mockgit).
- cmd/spr/main.go, cmd/amend/main.go: construct VCSOperations via
the factory and pass to NewStackedPR.
- spr/spr_test.go: stubVcsOps for tests; existing fixtures updated
minimally to feed the interface.
- git/mockgit/mockgit.go: ExpectFetchTags helper for GitOps tests.
All existing tests pass under `go test -race ./...`.
golang.org/x/tools v0.21.x (transitive via fezzik) has a constant-overflow build error on Go 1.26. Bumping the toolchain directive resolves it so the GraphQL regeneration step in github/githubclient runs cleanly. The actual codegen change (added ClosedOrphanPullRequests query) lives in a follow-up commit. No source-code changes; go.mod + go.sum only.
go vet on Go 1.25 flags non-constant format strings in
fmt.Sprintf-based wrappers. The Mock.expect helper accepted variadic
args but every existing caller already passed a fully-formatted
string, so the variadic was dead code. Signature change:
-func (m *Mock) expect(cmd string, args ...interface{}) *Mock
+func (m *Mock) expect(cmd string) *Mock
No call sites change because all callers pre-formatted with fmt.Sprintf.
Removes the vet diagnostic with no behavior change.
Pure plumbing for forthcoming jj/Jujutsu support — no callers yet.
- git/commit.go: ChangeID field on git.Commit. Populated only when
the VCS backend produces one (jj does; git doesn't). Stable across
rewrites, unlike CommitHash.
- vcs/jj_cmd.go: JjCmd / JjInterface — exec.Command-based wrapper
for jj invocations. Separates stdout/stderr cleanly so parsers
don't ingest jj's informational stderr text.
- vcs/jj_parse.go: parser for `jj log` template output, including
handling of multi-line descriptions, empty commits, conflict
markers, and the commit-id trailer.
- vcs/jj_cmd_test.go, vcs/jj_parse_test.go: unit coverage for both.
Nothing in this commit consumes any of these — the VCSOperations
factory still returns GitOps unconditionally. JjOps wiring lands in
the next commit.
Adds the jj (Jujutsu) backend behind the VCSOperations interface and
makes spr's user-facing commands jj-aware in a single coherent unit,
so jj users never observe a half-baked state when only part of this
work has shipped.
VCS layer:
- vcs/jj_ops.go: JjOps implementation (jj git fetch / jj rebase /
jj squash / jj edit / jj bookmark set / jj git push). Preserves
change IDs across operations. GetLocalCommitStack returns errors
(no panics) so the spr layer can surface immutable-commit failures
cleanly. FetchAndRebase uses `-d trunk()` rather than
`-d <branch>@<remote>`, tracking any user customization of jj's
trunk() revset and staying symmetric with the
`trunk()..(@:: | ::@)` discovery revset.
- vcs/jj_ops_test.go: unit tests via mockjj. The immutable-commit
and log-failure paths assert error returns (no recover()).
- vcs/mockjj/mockjj.go: expectation-based jj mock, mirrors mockgit.
- vcs/detect.go + tests: IsJJColocated checks for .jj/ + .git/.
- vcs/interface.go: NewVCSOperations returns JjOps when colocated
and !NoJJ; falls back to GitOps otherwise.
Config + CLI:
- config/config.go: NoJJ user-config field (default false).
- cmd/spr/main.go: --no-jj flag + SPR_NOJJ env var; new jj-setup
subcommand to register the 'jj spr' alias.
spr layer (the jj-mode UX reshape):
- spr/spr.go: EditCommit announces 'jj edit <change-id>' and calls
EditStart, then exits — no session in jj mode.
- EditCommitDone / EditCommitAbort: echo-only in jj mode. jj has no
edit sessions; output points users at the native jj equivalents
(jj new / jj undo).
- SyncStack: in jj mode, runs 'jj git fetch' and stops. The git-mode
cherry-pick logic doesn't apply to jj's change-id alignment model.
Tests:
- spr/spr_test.go: jj-mode unit tests for each of the four entry
points, using stubVcsOps with commandName='jj spr'.
Integration tests against a real jj binary land in the next commit
(needs the jjtest harness).
Build-tagged integration suite (//go:build integration) that exercises
the jj backend end-to-end against a real jj binary. CI runs both this
and the unit suite; locally use `go test -tags=integration`.
- vcs/jjtest/jjtest.go: test harness — colocated repo factory, bare
'origin' remote, helpers to add/edit/fork commits, op-log
snapshot/diff for asserting exactly which jj operations a code
path invoked. Doc comment in vcs/jjtest/doc.go.
- vcs/jj_integration_test.go: VCS-layer coverage for
GetLocalCommitStack (auto-revset across @-position variations,
WIP truncation, divergent siblings, commit-id trailer auto-add),
CheckStackCompleteness (multi-head detection), FetchAndRebase,
AmendInto, PushBranches (including conflict-refusal pin), and a
smoke test.
- spr/jj_integration_test.go: spr-layer tests that exercise the
backend through StackedPR — status from mid-stack (auto-revset),
refusal-on-fork variants for update/amend/check, the immutable-
commit error path through JjOps.EditStart, plus the four jj-mode
behaviors introduced in the previous commit (Edit_MovesAtAndAnnounces,
Sync_RunsJjGitFetch, EditDone_EchoOnly_NoOps, EditAbort_EchoOnly_NoOps).
TestMain in both files fails fast if jj is missing; set
SPR_SKIP_JJ_INTEGRATION=1 to skip cleanly when jj is unavailable.
Build/test infrastructure to run the jj-backed integration suite from
the previous commit alongside the existing unit suite, both locally
and in CI.
- Makefile: `make test` (unit only, no jj needed), `make test-integration`
(real jj, build tag), `make test-all` runs both then merges
coverage. Per-package coverage attribution via the merge script.
- .github/workflows/ci.yml: install pinned JJ_VERSION (v0.40.0),
run unit + integration suites, upload coverage artifacts.
- scripts/coverage-merge/main.go: tiny tool that merges Go coverage
profiles per-file (max-coverage strategy) so unit + integration
runs combine cleanly. Stdlib only, no extra deps.
- git/helpers_test.go: pure-Go parseLocalCommitStack tests added
(previously this code was only exercised indirectly via mockgit).
JJ_VERSION is pinned — bump intentionally when verifying against new
jj releases.
When a stacked PR is squash-merged on GitHub, the resulting commit on
trunk absorbs the cumulative diff of all PRs at-or-below the merge
target. Local commits whose patches are subsumed by that squash should
drop out on the next fetch+rebase — git mode does this automatically
via patch-id detection in plain `git rebase`. jj mode did not: vanilla
`jj rebase` left them as [EMPTY] stubs cluttering the stack.
Passing --skip-emptied makes jj match git's self-healing behavior for
the clean form of the cascade (no local drift). It does NOT cover:
- User-edit drift (orphan commit amended locally before next update)
- Structural overlap (multiple PRs touching the same file with
cumulative diffs that don't bit-match individual slices)
Both cases still produce conflicts on rebase; the next step (orphan-
abandon in FetchAndRebase) closes them.
Adds a characterization test suite:
- vcs/gittest/ — bare remote + local clone harness, mirrors jjtest
- vcs/gittest/cascade_integration_test.go — git mode self-heals (asserted)
- vcs/jj_cascade_integration_test.go — 6 sub-tests covering vanilla
vs --skip-emptied across clean, drift, and structural-overlap variants
- TestJjOps_FetchAndRebase_SelfHealsCleanCascade — spr-API regression
guard confirming the flag is wired up
mockjj/mockjj.go ExpectRebase / ExpectRebaseAndFail updated to match
the new command form.
commit-id:f009356f
Phase 2a of the cascade-orphan fix: introduce the VCS-level plumbing
that abandons local commits whose PRs have been orphaned by an upstream
squash, before rebase has a chance to see them. Without this, --skip-
emptied alone resolves the clean case (orphan whose content is wholly
absorbed by trunk) but the polis production form (structural overlap on
shared files, or local drift on amended orphans) still produces
conflicts.
Interface changes:
- VCSOperations.FetchAndRebase now takes (cfg, orphanChangeIDs).
Callers pass nil for backwards compatibility; Phase 2c will populate.
- VCSOperations.AbandonChangeIDs([]string) — new hook.
JjOps:
- FetchAndRebase: fetch → AbandonChangeIDs(orphans) → rebase
--skip-emptied. Abandon errors short-circuit before rebase, so
rebasing with a still-orphan commit never happens.
- AbandonChangeIDs: jj abandon <id> per entry; empty input is no-op.
GitOps:
- FetchAndRebase: signature change; orphans ignored. Git's rebase
handles the clean case via patch-id; drift/overlap cases have no
git-side equivalent fix today.
- AbandonChangeIDs: no-op for interface compat.
Mocks: mockjj.ExpectAbandon + ExpectAbandonAndFail.
Tests:
- vcs/jj_ops_test.go: 5 unit tests covering AbandonChangeIDs (empty,
multiple ordered, error short-circuit) and FetchAndRebase (with
orphans, with abandon-fail, with NoRebase).
- vcs/git_ops_test.go: AbandonChangeIDs no-op + FetchAndRebase
ignores orphans (regression guard).
- vcs/jj_cascade_integration_test.go: 2 new spr-API regression
guards — OrphanPrune_ResolvesDrift and OrphanPrune_ResolvesStructur-
alOverlap — verifying the previously-stuck scenarios from the
characterization suite now resolve to a clean stack.
spr.fetchAndGetGitHubInfo passes nil orphans; populated in Phase 2c.
commit-id:4222bc0d
Phase 2b of the cascade-orphan fix: expose GitHub's view of orphan PRs
(state=CLOSED-not-merged whose head ref matches the spr branch prefix)
so the spr update flow can identify which local change IDs to abandon
before rebase.
GraphQL:
- New ClosedOrphanPullRequests query in queries.graphql, mirroring
PullRequests' shape but with states:[CLOSED]. GitHub's enum has
CLOSED (closed-not-merged) as distinct from MERGED, so this filter
alone is sufficient — no client-side dedup needed.
- gen/genclient/operations.go regenerated via fezzik (`go generate
./github/githubclient/`).
GitHubInterface:
- GetClosedOrphanPRs(ctx) []*PullRequest returns orphan candidates
with .Commit.CommitID populated from the HeadRefName, so callers
don't need to re-parse trailers. Nil on transport error; empty
slice on "no orphans found".
githubclient.client:
- GetClosedOrphanPRs delegates the per-node filter to
filterClosedOrphans (pure helper, no transport) so the matching
logic can be unit-tested without mocking the genclient interface
(14 methods, too many to stub for one test).
Mockclient:
- MockClient.GetClosedOrphanPRs returns the package-level
MockClientClosedOrphans slice. Intentionally NOT expectation-
verified — it's a pure read of GitHub PR state that
fetchAndGetGitHubInfo calls on every `spr update`. Verifying
would require updating dozens of existing fixtures with
`ExpectGetClosedOrphanPRs()` they don't otherwise care about.
Tests:
- client_test.go: TestFilterClosedOrphans — 4 sub-tests covering
nil input, no-orphans, mixed match/skip, custom branch prefix.
The Go-toolchain bump (required by x/tools for the codegen step) and
the unrelated mockgit format-string fix moved to dedicated earlier
commits to keep this one focused on the orphan-PR feature.
Bridge the GitHub-side orphan signal (closed-not-merged PRs whose
HeadRefName matches the spr branch prefix) to the VCS-side abandon
plumbing, so jj users get cascade-orphan recovery as default behavior
on every spr update.
This is *mostly* parity with git mode: `git rebase`'s patch-id
detection silently drops absorbed commits on every spr update on the
same code path. Our jj implementation extends the recovery further —
it also catches the 'structural overlap' case (multiple PRs touching
the same file with cumulative additions where each slice doesn't
bit-match the cumulative squash) which git rebase's patch-id drop
does NOT handle silently. This extension is safe to ship default-on
because jj has `jj op log` + `jj op restore`, which together with
the user-visible notice (below) gives users a one-command undo if
abandon was wrong. Git has no introspection equivalent.
Flow inside identifyOrphanChangeIDs:
1. Short-circuit if cfg.User.NoRebase (FetchAndRebase will skip
abandon too — silent skip avoids a misleading notice)
2. github.GetClosedOrphanPRs(ctx)
3. Index orphans by spr commit-id (the trailer value), keeping PR
pointer so the user notice can name PR number + subject
4. vcsOps.GetLocalCommitStack — intersect with orphan set
5. For each match: collect ChangeID for abandon, plus PR + Subject
for the user notice
6. Print a transparent notice naming each abandoned commit and a
`jj op restore` recovery hint
fetchAndGetGitHubInfo: calls identifyOrphanChangeIDs first, then
passes the result into FetchAndRebase. JjOps abandons each ID before
rebase; GitOps ignores (no-op per its interface contract).
Mockclient: GetClosedOrphanPRs intentionally NOT expectation-
verified — it's a pure read of GitHub PR state that fetchAndGetGitHubInfo
calls on every `spr update`. Verifying would require adding
ExpectGetClosedOrphanPRs() to 17 existing test fixtures that don't
care. Tests that DO care set mockclient.MockClientClosedOrphans to
the canned response.
Tests (spr/spr_test.go::TestIdentifyOrphanChangeIDs):
- NoRebase_skips_detection_silently (regression guard for the
short-circuit)
- no_orphans_returns_nil_and_silent
- orphan_matches_local_commit_returns_change_id_and_notice (asserts
user notice contains PR number references + subjects + the
`jj op restore` recovery hint)
- orphan_with_no_local_match_is_dropped_and_silent
- local_commit_without_change_id_is_skipped_silently (git-mode
safety: no jj change ID = no abandon attempt)
Single user-facing announcement of jj support and the cascade-orphan
recovery mechanism, placed at the end of the stack so it lands only
after the full feature is in place. Users following intermediate PRs
see no announcement about jj until everything works.
readme.md:
- 'Jujutsu (jj) Support' section under Usage Guide. Covers setup
(jj-setup alias), what works identically vs what differs (edit,
sync, non-linear stacks), the noJJ opt-out config / --no-jj flag
/ SPR_NOJJ env var, and the auto-revset behavior (work from any
@ position).
- 'Merging mid-stack and cascade-orphan recovery' subsection
explaining the squash-merge mechanism, what spr does about
orphans automatically, parity with git mode.
docs/jj-mode-design.md:
- Twelve sections covering the guiding principle (lean on jj,
don't mimic git), auto-revset for stack discovery, multi-head
ambiguity, the jj-mode edit/sync semantics, WIP truncation,
conflict delegation, cascade-orphan recovery (with worked
example output), VCS interface changes, the stdout/stderr
separation bug fix, testing strategy, out-of-scope items, and
a glossary.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.