Skip to content

Commit 2d07a1c

Browse files
authored
Support modifying partially merged stacks (#575)
* Handle partial unstacking after stack modification * Match recreated stacks without unrelated merged history * Refresh released PRs before retrying modify submission
1 parent 83dbac2 commit 2d07a1c

4 files changed

Lines changed: 323 additions & 115 deletions

File tree

‎cmd/checkout.go‎

Lines changed: 3 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -538,24 +538,10 @@ func findLocalStackForRemotePRs(sf *stack.StackFile, prs []*github.PullRequest)
538538
return nil
539539
}
540540

541-
// stackCompositionMatches checks if a local stack's PR numbers match
542-
// the remote stack's PR numbers in the same order.
541+
// stackCompositionMatches ignores merged history that belongs to a previous stack.
543542
func stackCompositionMatches(localStack *stack.Stack, remotePRNumbers []int) bool {
544-
var localPRNumbers []int
545-
for _, b := range localStack.Branches {
546-
if b.PullRequest != nil {
547-
localPRNumbers = append(localPRNumbers, b.PullRequest.Number)
548-
}
549-
}
550-
if len(localPRNumbers) != len(remotePRNumbers) {
551-
return false
552-
}
553-
for i := range localPRNumbers {
554-
if localPRNumbers[i] != remotePRNumbers[i] {
555-
return false
556-
}
557-
}
558-
return true
543+
localPRNumbers := stackPRNumbers(localStack, &github.RemoteStack{PullRequests: remotePRNumbers})
544+
return slicesEqual(localPRNumbers, remotePRNumbers)
559545
}
560546

561547
// handleCompositionConflict prompts the user to resolve a mismatch between

‎cmd/checkout_test.go‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1341,6 +1341,28 @@ func TestStackCompositionMatches(t *testing.T) {
13411341
remote: []int{10, 11},
13421342
matches: true,
13431343
},
1344+
{
1345+
name: "merged history from a previous stack",
1346+
local: &stack.Stack{
1347+
Branches: []stack.BranchRef{
1348+
{Branch: "a", PullRequest: &stack.PullRequestRef{Number: 10, Merged: true}},
1349+
{Branch: "b", PullRequest: &stack.PullRequestRef{Number: 11}},
1350+
},
1351+
},
1352+
remote: []int{11},
1353+
matches: true,
1354+
},
1355+
{
1356+
name: "merged member of the current stack",
1357+
local: &stack.Stack{
1358+
Branches: []stack.BranchRef{
1359+
{Branch: "a", PullRequest: &stack.PullRequestRef{Number: 10, Merged: true}},
1360+
{Branch: "b", PullRequest: &stack.PullRequestRef{Number: 11}},
1361+
},
1362+
},
1363+
remote: []int{10, 11},
1364+
matches: true,
1365+
},
13441366
{
13451367
name: "different order",
13461368
local: &stack.Stack{

‎cmd/submit.go‎

Lines changed: 111 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package cmd
33
import (
44
"errors"
55
"fmt"
6+
"slices"
67
"strconv"
78
"strings"
89

@@ -147,17 +148,6 @@ func runSubmit(cfg *config.Config, opts *submitOptions) error {
147148
// Sync PR state to detect merged/queued PRs before pushing.
148149
prDetails := syncStackPRs(cfg, s)
149150

150-
// If the active branches now sit on top of a fully-merged base, they can no
151-
// longer extend the existing remote stack. Fork them into a fresh stack
152-
// rooted at the trunk and continue the submit with that new stack.
153-
if stacksAvailable {
154-
s, err = maybeForkFromMergedBase(cfg, client, sf, s, gitDir)
155-
if err != nil {
156-
cfg.Errorf("%s", err)
157-
return ErrSilent
158-
}
159-
}
160-
161151
// Resolve remote for pushing
162152
remote, err := pickRemote(cfg, currentBranch, opts.remote)
163153
if err != nil {
@@ -198,6 +188,20 @@ func runSubmit(cfg *config.Config, opts *submitOptions) error {
198188
}
199189
}
200190

191+
// A modify retry may still have the old catalog ID. Refresh detached PRs
192+
// after clearing it so merged or queued PRs are not pushed again.
193+
if pendingModify {
194+
prDetails = syncStackPRs(cfg, s)
195+
} else if stacksAvailable {
196+
s, err = maybeForkFromMergedBase(cfg, client, sf, s, gitDir)
197+
if err != nil {
198+
cfg.Errorf("%s", err)
199+
return ErrSilent
200+
}
201+
}
202+
activeBranches = activeBranchNames(s)
203+
queued = s.QueuedBranches()
204+
201205
// Best-effort fetch to update tracking refs (helps --force-with-lease
202206
// in shallow clones). Silently ignored if branches don't exist on the
203207
// remote yet.
@@ -283,7 +287,7 @@ func runSubmit(cfg *config.Config, opts *submitOptions) error {
283287
return stackSaveError(cfg, err)
284288
}
285289
if pendingModify {
286-
if !allPRsSubmitted || (!stackSynced && len(s.Branches) != 1) {
290+
if !allPRsSubmitted || (!stackSynced && len(activeBranches) > 1) {
287291
cfg.Errorf("modify is not fully submitted; recovery state was retained")
288292
cfg.Printf("Finish submitting all remaining PRs, then retry `%s`", cfg.ColorCyan("gh stack submit"))
289293
return ErrSilent
@@ -565,7 +569,11 @@ func maybeForkFromMergedBase(cfg *config.Config, client github.ClientOps, sf *st
565569
return s, nil
566570
}
567571

568-
remotePRs := remoteStackPRs(client, s.ID)
572+
remote, err := remoteStackByID(client, s.ID)
573+
if err != nil || remote == nil {
574+
return s, nil
575+
}
576+
remotePRs := remote.PRNumbers()
569577
if len(remotePRs) == 0 {
570578
return s, nil
571579
}
@@ -651,19 +659,17 @@ func maybeForkFromMergedBase(cfg *config.Config, client github.ClientOps, sf *st
651659
return &sf.Stacks[len(sf.Stacks)-1], nil
652660
}
653661

654-
// remoteStackPRs returns the PR numbers that are officially part of the remote
655-
// stack identified by stackID, or nil if it can't be determined.
656-
func remoteStackPRs(client github.ClientOps, stackID string) []int {
662+
func remoteStackByID(client github.ClientOps, stackID string) (*github.RemoteStack, error) {
657663
stacks, err := client.ListStacks()
658664
if err != nil {
659-
return nil
665+
return nil, err
660666
}
661-
for _, rs := range stacks {
662-
if strconv.Itoa(rs.ID) == stackID {
663-
return rs.PRNumbers()
667+
for i := range stacks {
668+
if strconv.Itoa(stacks[i].ID) == stackID {
669+
return &stacks[i], nil
664670
}
665671
}
666-
return nil
672+
return nil, nil
667673
}
668674

669675
// mergedPRNumbers returns the set of PR numbers whose local branch is marked
@@ -679,9 +685,36 @@ func mergedPRNumbers(s *stack.Stack) map[int]bool {
679685
return merged
680686
}
681687

688+
// prepareStackRecreation recognizes a merged-only remote remnant and records
689+
// fresh merge state without removing any local branches.
690+
func prepareStackRecreation(s *stack.Stack, remaining *github.RemoteStack) bool {
691+
if remaining == nil || len(remaining.PRNumbers()) == 0 {
692+
return false
693+
}
694+
merged := mergedPRNumbers(s)
695+
for _, pr := range remaining.PRDetails {
696+
if pr.IsMerged() {
697+
merged[pr.Number] = true
698+
}
699+
}
700+
for _, number := range remaining.PRNumbers() {
701+
if !merged[number] {
702+
return false
703+
}
704+
}
705+
for i := range s.Branches {
706+
b := &s.Branches[i]
707+
if b.PullRequest != nil && merged[b.PullRequest.Number] {
708+
b.PullRequest.Merged = true
709+
b.Queued = false
710+
}
711+
}
712+
return true
713+
}
714+
682715
// handlePendingModify handles the stack recreation after a modify operation.
683-
// It deletes the old remote stack and clears s.ID so syncStack creates a new
684-
// one. The bool identifies a matching pending modification. Its journal is
716+
// It unstacks the old PRs and clears s.ID so syncStack creates a replacement.
717+
// The bool identifies a matching pending modification. Its journal is
685718
// cleared only after the caller verifies all required submission outcomes.
686719
func handlePendingModify(cfg *config.Config, client github.ClientOps, s *stack.Stack, gitDir string) (bool, error) {
687720
state, err := modify.LoadState(gitDir)
@@ -719,17 +752,19 @@ func handlePendingModify(cfg *config.Config, client github.ClientOps, s *stack.S
719752
}
720753
}
721754

722-
// Delete the old remote stack
755+
// Release the old stack's unmerged PRs before submitting the replacement.
723756
if state.PriorRemoteStackID != "" {
724-
number, found, lookupErr := stackNumberByID(client, state.PriorRemoteStackID)
757+
remote, lookupErr := remoteStackByID(client, state.PriorRemoteStackID)
725758
if lookupErr != nil {
726759
cfg.Warningf("Failed to look up existing stack: %v", lookupErr)
727760
cfg.Printf("Run `%s` again to retry", cfg.ColorCyan("gh stack submit"))
728761
return true, lookupErr
729762
}
730-
if !found {
763+
if remote == nil {
731764
cfg.Printf("Previous stack already deleted on GitHub")
732-
} else if _, dissolved, err := client.Unstack(number); err != nil {
765+
} else if prepareStackRecreation(s, remote) {
766+
cfg.Infof("Only merged pull requests remain in the previous stack on GitHub")
767+
} else if remaining, dissolved, err := client.Unstack(remote.Number); err != nil {
733768
var httpErr *api.HTTPError
734769
if errors.As(err, &httpErr) && httpErr.StatusCode == 404 {
735770
cfg.Printf("Previous stack already deleted on GitHub")
@@ -738,11 +773,14 @@ func handlePendingModify(cfg *config.Config, client github.ClientOps, s *stack.S
738773
cfg.Printf("Run `%s` again to retry", cfg.ColorCyan("gh stack submit"))
739774
return true, err
740775
}
741-
} else if !dissolved {
742-
cfg.Errorf("the previous stack still has pull requests queued for merge or with auto-merge enabled; it cannot be recreated yet")
776+
} else if !dissolved && !prepareStackRecreation(s, remaining) {
777+
cfg.Errorf("Some pull requests still belong to the previous stack on GitHub; it cannot be recreated yet")
743778
return true, ErrConflict
744-
} else {
779+
} else if dissolved {
745780
cfg.Successf("Cleared existing stack on GitHub")
781+
} else {
782+
cfg.Successf("Unstacked pull requests from the previous stack on GitHub")
783+
cfg.Infof("Merged pull requests remain in the previous stack on GitHub")
746784
}
747785
}
748786
// Record branch identity before the replacement receives a different ID.
@@ -778,41 +816,52 @@ func clearPendingModifyState(cfg *config.Config, s *stack.Stack, gitDir string)
778816
return nil
779817
}
780818

781-
// syncStack creates or updates a stack on GitHub from the active PRs.
782-
// If the stack already exists (s.ID is set), it calls the PUT endpoint with
783-
// the full list of PRs to keep the remote stack in sync. If no stack exists
784-
// yet, it calls POST to create one.
819+
// syncStack creates or updates a stack on GitHub. Existing stacks keep their
820+
// merged members; new stacks exclude merged history from previous stacks.
785821
// This is a best-effort operation: failures are reported as warnings but do
786822
// not cause the submit command to fail (the PRs are already created).
787823
//
788824
// It returns true when the remote stack object reflects the local stack
789825
// (created, updated, or already in sync) and false otherwise (fewer than two
790826
// PRs, an unresolved divergence, stacked PRs unavailable, or an API failure).
791827
func syncStack(cfg *config.Config, client github.ClientOps, s *stack.Stack) bool {
792-
// Collect PR numbers in stack order (bottom to top), including merged PRs.
793-
// The API expects the full list — omitting merged PRs causes a
794-
// "Stack contents have changed" rejection.
795-
var prNumbers []int
828+
prCount := 0
796829
for _, b := range s.Branches {
797830
if b.PullRequest != nil {
798-
prNumbers = append(prNumbers, b.PullRequest.Number)
831+
prCount++
799832
}
800833
}
801834

802835
// The API requires at least 2 PRs to form a stack.
803-
if len(prNumbers) < 2 {
836+
if prCount < 2 {
804837
return false
805838
}
806839

807840
if s.ID != "" {
808-
return updateStack(cfg, client, s, prNumbers)
841+
return updateStack(cfg, client, s)
809842
}
810843

811844
// No locally tracked stack ID. The stack may already exist on GitHub
812845
// (created from the web UI or another clone) without being recorded
813846
// locally. Adopt it instead of blindly creating a new one, which the API
814847
// rejects because the PRs are already part of a stack.
815-
return reconcileUntrackedStack(cfg, client, s, prNumbers)
848+
return reconcileUntrackedStack(cfg, client, s, stackPRNumbers(s, nil))
849+
}
850+
851+
// stackPRNumbers includes merged PRs only when they belong to the remote stack
852+
// being updated, rather than to a previous stack retained after unstacking.
853+
func stackPRNumbers(s *stack.Stack, remote *github.RemoteStack) []int {
854+
var members []int
855+
if remote != nil {
856+
members = remote.PRNumbers()
857+
}
858+
var numbers []int
859+
for _, b := range s.Branches {
860+
if b.PullRequest != nil && (!b.IsMerged() || slices.Contains(members, b.PullRequest.Number)) {
861+
numbers = append(numbers, b.PullRequest.Number)
862+
}
863+
}
864+
return numbers
816865
}
817866

818867
// reconcileUntrackedStack reconciles a locally untracked stack (s.ID == "")
@@ -828,7 +877,7 @@ func reconcileUntrackedStack(cfg *config.Config, client github.ClientOps, s *sta
828877
if err != nil {
829878
// Couldn't inspect remote state — fall back to the create path, which
830879
// reports its own errors (handleCreate422 covers "already stacked").
831-
return createNewStack(cfg, client, s, prNumbers)
880+
return createNewStack(cfg, client, s)
832881
}
833882

834883
matched, err := findMatchingStack(stacks, prNumbers)
@@ -844,9 +893,10 @@ func reconcileUntrackedStack(cfg *config.Config, client github.ClientOps, s *sta
844893

845894
if matched == nil {
846895
// No existing stack contains any of our PRs — create a new one.
847-
return createNewStack(cfg, client, s, prNumbers)
896+
return createNewStack(cfg, client, s)
848897
}
849898

899+
prNumbers = stackPRNumbers(s, matched)
850900
// A remote stack already contains some of our PRs. Refuse to silently drop
851901
// any PRs it holds that we aren't tracking locally; let the user reconcile.
852902
if dropped := prsMissingFrom(matched.PRNumbers(), prNumbers); len(dropped) > 0 {
@@ -869,7 +919,7 @@ func reconcileUntrackedStack(cfg *config.Config, client github.ClientOps, s *sta
869919
}
870920

871921
cfg.Infof("Found the stack on GitHub — updating it to match your local stack")
872-
return updateStack(cfg, client, s, prNumbers)
922+
return updateStack(cfg, client, s)
873923
}
874924

875925
// prsMissingFrom returns the numbers in remote that do not appear in local,
@@ -895,13 +945,13 @@ func prsMissingFrom(remote, local []int) []int {
895945
// leaves the remote stack untouched. If the remote stack is gone (404) it
896946
// clears the local ID and re-creates it. Returns true when the remote stack
897947
// reflects the local stack (updated, already in sync, or recreated).
898-
func updateStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, prNumbers []int) bool {
948+
func updateStack(cfg *config.Config, client github.ClientOps, s *stack.Stack) bool {
899949
number, err := ensureStackNumber(client, s)
900950
if err != nil || number == 0 {
901951
// Can't resolve the remote stack — treat as missing and (re)create.
902952
s.ID = ""
903953
s.Number = 0
904-
return createNewStack(cfg, client, s, prNumbers)
954+
return createNewStack(cfg, client, s)
905955
}
906956

907957
remote, err := client.GetStack(number)
@@ -910,13 +960,20 @@ func updateStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, pr
910960
if errors.As(err, &httpErr) && httpErr.StatusCode == 404 {
911961
s.ID = ""
912962
s.Number = 0
913-
return createNewStack(cfg, client, s, prNumbers)
963+
return createNewStack(cfg, client, s)
914964
}
915965
cfg.Warningf("Failed to read stack on GitHub: %v", err)
916966
return false
917967
}
918968

969+
if prepareStackRecreation(s, remote) && len(s.ActiveBranches()) > 0 {
970+
s.ID = ""
971+
s.Number = 0
972+
return createNewStack(cfg, client, s)
973+
}
974+
919975
current := remote.PRNumbers()
976+
prNumbers := stackPRNumbers(s, remote)
920977
if slicesEqual(current, prNumbers) {
921978
s.ID = strconv.Itoa(remote.ID)
922979
s.Number = remote.Number
@@ -947,7 +1004,7 @@ func updateStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, pr
9471004
// immediately try to re-create it.
9481005
s.ID = ""
9491006
s.Number = 0
950-
return createNewStack(cfg, client, s, prNumbers)
1007+
return createNewStack(cfg, client, s)
9511008
case 422:
9521009
// A merged branch whose ref has been deleted upstream breaks the
9531010
// stack's base→head chain, so the update is rejected. This is
@@ -977,7 +1034,11 @@ func updateStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, pr
9771034
// createNewStack calls the POST endpoint to create a new stack, handling the
9781035
// three types of 422 errors the API may return.
9791036
// Returns true when the stack was created or is confirmed already in sync.
980-
func createNewStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, prNumbers []int) bool {
1037+
func createNewStack(cfg *config.Config, client github.ClientOps, s *stack.Stack) bool {
1038+
prNumbers := stackPRNumbers(s, nil)
1039+
if len(prNumbers) < 2 {
1040+
return false
1041+
}
9811042
rs, err := client.CreateStack(prNumbers)
9821043
if err == nil {
9831044
s.ID = strconv.Itoa(rs.ID)

0 commit comments

Comments
 (0)