Skip to content

fix: retarget PR to master when parent PR has been landed#110

Open
edmondop wants to merge 6 commits intoLucioFranco:mainfrom
edmondop:fix-retarget-to-master
Open

fix: retarget PR to master when parent PR has been landed#110
edmondop wants to merge 6 commits intoLucioFranco:mainfrom
edmondop:fix-retarget-to-master

Conversation

@edmondop
Copy link
Copy Markdown
Contributor

@edmondop edmondop commented Mar 3, 2026

Summary

  • After landing the bottom of a stack and rebasing, jj spr diff now retargets the new bottom PR from its synthetic base branch to master
  • Extract determine_base_branch() function for testability
  • Deletes the stale synthetic base branch after retargeting
  • 6 unit tests covering all base branch determination scenarios

Previously, after landing PR #1 in a stack and rebasing PR #2 onto master, running jj spr diff would keep PR #2 targeting the old synthetic base branch. GitHub would then show the full diff (including already-landed changes) instead of just PR #2's changes.

Test plan

  • cargo test — all tests pass
  • cargo clippy --all-features --all-targets -- -D warnings — clean
  • Land bottom of a stack, rebase, run jj spr diff, verify PR is retargeted to master

edmondop and others added 5 commits March 4, 2026 09:16
* fix: include error source chain in error messages

octocrab's Error::GitHub variant uses snafu without a custom display
attribute, so its Display impl outputs just "GitHub" — the variant name.
The actual API error message lives in the source field but was never
surfaced because From<E> only used format!("{}", error).

Walk std::error::Error::source() chain so wrapped errors include the
underlying message. Also add context to the merge call in land.rs with
PR number and head SHA for better diagnostics.

Before: 🛑 GitHub
After:  🛑 GitHub: <actual reason from GitHub API>

* trigger CI

* style: cargo fmt
* Adding dry run mode

* Improve dry run output formatting

Show richer details (base/head branches, draft status, reviewers)
and suppress commit title output during dry run mode.

* trigger CI

* style: cargo fmt
* Add jj spr cleanup command

List and delete orphan SPR branches on the remote that are no longer
associated with any open pull request. Dry-run by default; pass
--confirm to actually delete.

Extract branch filtering logic into testable functions with unit tests.

* trigger CI

* style: cargo fmt
After the bottom of a stack is landed and the remaining stack is rebased
onto master, jj spr diff now detects that the next PR's commit is directly
on master and retargets the PR from its synthetic base to master. The old
synthetic base branch is deleted as part of cleanup.

Previously, diff would always keep an existing synthetic base even when
the commit was rebased directly onto master, leaving the PR targeting
a stale synthetic branch.

Extract determine_base_branch() for testability.
@edmondop edmondop force-pushed the fix-retarget-to-master branch from 58b498d to 798f349 Compare March 4, 2026 21:56
@jennings
Copy link
Copy Markdown
Collaborator

Nice! I noticed this the other day and could have sworn it was supposed to already do this.

@jennings
Copy link
Copy Markdown
Collaborator

jennings commented Mar 19, 2026

I tried this and the pull request closed before it was able to retarget.

It looks like we're deleting the old branch before retargeting the pull request to main. Am I reading it right?

If so, I think we need to update the PR first, then delete the previous target branch.

This is the output I got:

❯ jj spr diff -r u
57100fd <snip>
  #️⃣   Pull Request #418: https://github.com/<snip>/pull/418
Message (leave empty to abort): rebase
  ⚾  Commit was rebased - updating Pull Request #418
  🎯  Retargeting Pull Request #418 to main
  🛑  GitHub: Validation Failed
      Documentation URL: https://docs.github.com/rest/pulls/pulls#update-a-pull-request
      Errors:
      - {"code":"invalid","field":"base","message":"Cannot change the base branch of a closed pull request.","resource":"PullRequest"}
image

@edmondop
Copy link
Copy Markdown
Contributor Author

Thank you, this is actually something that I had fixed on my fork, and I forgot to update the PR! Let me update the PR

Deleting the synthetic base branch before retargeting the PR to master
causes GitHub to close the PR, since its base branch no longer exists.
Remove the deletion entirely — orphan branches can be cleaned up via
`jj spr cleanup` instead.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@edmondop
Copy link
Copy Markdown
Contributor Author

Should be fixed with 4068da5

@jennings
Copy link
Copy Markdown
Collaborator

I've played with the new version and it worked at least once, but today this happened:

$ jj spr diff

# oops, that should have been cherry picked.

$ jj spr diff --cherry-pick

This is what the PR says:

image

The "deleted the branch main" is alarming. It didn't actually get deleted because it's protected.

If you don't fix it first, I hope to find time to fix it myself because I really like this feature.

@edmondop
Copy link
Copy Markdown
Contributor Author

edmondop commented Mar 30, 2026 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants