Skip to content

sync: retarget upstack PRs on forge after deletion#1150

Merged
abhinav merged 4 commits into
mainfrom
sync-survivingancestor-loop
May 23, 2026
Merged

sync: retarget upstack PRs on forge after deletion#1150
abhinav merged 4 commits into
mainfrom
sync-survivingancestor-loop

Conversation

@ed-irl

@ed-irl ed-irl commented May 12, 2026

Copy link
Copy Markdown
Collaborator

When gs repo sync deletes branches whose PRs were merged,
upstack PRs on the forge are left targeting deleted branches.
This causes confusion on platforms like GitHub, where the PR
base no longer matches the local stack state.

This change retargets surviving upstack PRs on the forge to
their nearest surviving ancestor (or trunk if none exists)
after sync deletes merged branches. Cyclic base chains fall
back to trunk to avoid infinite loops.

Covered by unit tests for candidate collection and ancestor
resolution, plus test scripts exercising single-merge and
deep-stack scenarios end-to-end.

Regarding this question on #1073:

So just to clarify/confirm the behavior:
git-spice already retargets upstack branches when their bases are deleted, but it does so entirely locally.
The focus of this change is to specifically also update the forge?
The next submit for those branches would also update the base branch in the forge.
I would appreciate some more context on what issue we're running into here.
(Context on my hesitance: repo sync is currently read-only for the forge. This would add a write, so I want to make sure we have a good reason to.)

The issue encountered is branch corruption when adding the gs bm command - which I will post later today on this stack. Prior to the merge command, the forge itself drove merge events and the sync/submit cycle pulled merge changes and drove repointing based on merges that happened elsewhere. When I added the gs bm command the flow was flipped, and the stack would become corrupted if you merged anything besides the bottom branch. I'll post a ref to that PR when I post it. It took me a minute to remember what had been happening and I needed claude to help me figure it out again 😂 . It provided this somewhat more verbose explanation:

Before gs branch merge, gs read forge merge state and reconciled local — that's the read-only pattern you're describing, and repo sync was fine being one-way (forge → local). The next submit falling back to fix the forge base was fine because nothing else gs did was authoritative over forge merges.

gs branch merge (in a separate PR in this stack) breaks that assumption. It writes merges to the forge and then reacts to those merges by mutating local state (deletes branches, reparents upstack). That makes the forge↔local invariant load-bearing inside gs bm, not just at "next submit" time. Concretely, a stack of A → B → C where A was squash-merged externally and you then run gs bm C will silently bring A's pre-squash commits back into main via B's merge, because B's remote branch still contains them and gs has no way to know B should be rebased first.

PR #1073 is the other half of that contract: when repo sync rebases B locally, also retarget B's forge PR base so the forge and local agree on what B's parent is. Without it, gs is making local decisions (rebases, reparents) that the forge isn't told about, and the next forge-driven operation (review, or another gs bm) sees an inconsistent world.

[skip changelog]: bug fix only

@ed-irl

ed-irl commented May 12, 2026

Copy link
Copy Markdown
Collaborator Author

@ed-irl ed-irl requested a review from abhinav May 12, 2026 11:53
@ed-irl ed-irl added the skip changelog PRs that don't need a changelog. label May 12, 2026
@ed-irl ed-irl changed the base branch from main to submit-stale-base-check May 12, 2026 13:48
@ed-irl ed-irl force-pushed the sync-survivingancestor-loop branch from a40644a to 979d0cd Compare May 12, 2026 13:48
@ed-irl ed-irl force-pushed the submit-stale-base-check branch from 42f7928 to 0694ab5 Compare May 12, 2026 14:10
Base automatically changed from submit-stale-base-check to main May 17, 2026 21:40
@abhinav abhinav force-pushed the sync-survivingancestor-loop branch from 979d0cd to 3d9ac92 Compare May 17, 2026 21:43
@ed-irl ed-irl closed this May 18, 2026
@ed-irl ed-irl deleted the sync-survivingancestor-loop branch May 18, 2026 11:24
@ed-irl ed-irl restored the sync-survivingancestor-loop branch May 18, 2026 11:28
@ed-irl

ed-irl commented May 18, 2026

Copy link
Copy Markdown
Collaborator Author

Attempted to rename via gh rename api - but wound up closing. Attempting a push-then-repoint-then-delete workflow instead.

ed-irl and others added 4 commits May 23, 2026 11:47
Add cycle detection to survivingAncestor so that if deleted branches
form a cycle in their base references, the function falls back to
trunk instead of looping forever.
`repo sync` and branch deletion both need to choose the next base
when one or more downstack branches are being removed from consideration.
Keeping that traversal in the branch graph gives both paths the same
answer while leaving each caller responsible for its own removal policy.

Sync now loads a branch graph for finished-branch detection and remote
retargeting.
The delete handler uses the same graph operation when moving local
upstack branches around deleted bases.
`repo sync` should still retarget surviving upstack PRs when a
finished downstack branch cannot be deleted from the local checkout.
The local branch may remain because another worktree has it checked out,
but the forge stack should no longer target a branch that was already
merged there.

The script keeps the local stack check separate from the forge assertion:
`gs ls` verifies the starting stack shape,
and the final ShamHub JSON verifies that the surviving PR targets trunk.
@abhinav abhinav force-pushed the sync-survivingancestor-loop branch from e4b5f2e to ddbdacb Compare May 23, 2026 18:50
@abhinav abhinav enabled auto-merge (squash) May 23, 2026 18:55
@abhinav abhinav merged commit 9276c36 into main May 23, 2026
22 checks passed
@abhinav abhinav deleted the sync-survivingancestor-loop branch May 23, 2026 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip changelog PRs that don't need a changelog.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants