From 07a35c80214026f9d398651a18bff57d0ec7d02e Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Sun, 25 Oct 2026 17:10:49 -0700 Subject: [PATCH] merge: Preserve native stacks behind trunk An internally aligned native stack can merge after trunk advances. Preparing its trunk-based bottom instead rebases that branch and forces every member above it to restack and resubmit before merging. Keep the preflight head for a member originally based on trunk, matching individual merge preparation. Continue preparing other members, including range bottoms whose bases were merged by earlier queue items. Allow the same case in ShamHub by separating the current target head from the bottom change's merge base. Apply the stack's delta from that merge base onto the current target so merge and squash operations preserve independent trunk changes. Keep the target head as the publication parent and CAS expectation. --- internal/forge/shamhub/merge_range.go | 25 ++++++-- internal/forge/shamhub/merge_range_test.go | 47 ++++++++++++++ internal/handler/merge/merge_range_test.go | 48 +++++++------- internal/handler/merge/workspace.go | 12 ++-- internal/handler/merge/workspace_test.go | 56 +++++++++++++++++ ...tack_merge_native_range_advanced_trunk.txt | 63 +++++++++++++++++++ 6 files changed, 215 insertions(+), 36 deletions(-) create mode 100644 internal/handler/merge/workspace_test.go create mode 100644 testdata/script/downstack_merge_native_range_advanced_trunk.txt diff --git a/internal/forge/shamhub/merge_range.go b/internal/forge/shamhub/merge_range.go index 06f6984df..4286ace1f 100644 --- a/internal/forge/shamhub/merge_range.go +++ b/internal/forge/shamhub/merge_range.go @@ -149,6 +149,10 @@ type preparedMergeRange struct { // rootBaseHash is the target branch value used by the publishing CAS. rootBaseHash string + // rootMergeBaseHash is the fork point of the bottom change and its target. + // It can precede rootBaseHash when the target advanced independently. + rootMergeBaseHash string + // changes retains the validated server records in bottom-to-top order. changes []preparedMergeRangeChange } @@ -322,7 +326,19 @@ func (sh *ShamHub) prepareMergeRange( expected.Number, baseHash, prepared.changes[i-1].headHash, ) } - if err := sh.gitCmd( + if i == 0 { + // The target may have advanced since the bottom change was submitted. + // Apply the stack's delta from its fork point onto the current target; + // using the current target as the merge base would discard its new work. + mergeBase, err := sh.gitCmd( + ctx, owner, repo, "merge-base", baseHash, headHash, + ).OutputChomp() + if err != nil { + return preparedMergeRange{}, fmt.Errorf("find change %d merge base: %w", expected.Number, err) + } + prepared.rootBaseHash = baseHash + prepared.rootMergeBaseHash = mergeBase + } else if err := sh.gitCmd( ctx, owner, repo, @@ -337,9 +353,6 @@ func (sh *ShamHub) prepareMergeRange( ) } - if i == 0 { - prepared.rootBaseHash = baseHash - } prepared.changes[i] = preparedMergeRangeChange{ index: changeIndex, change: change, @@ -365,7 +378,7 @@ func (sh *ShamHub) buildMergeRangeCommit( repo, "merge-tree", "--write-tree", - "--merge-base="+prepared.rootBaseHash, + "--merge-base="+prepared.rootMergeBaseHash, prepared.rootBaseHash, top.headHash, ).OutputChomp() @@ -391,7 +404,7 @@ func (sh *ShamHub) buildMergeRangeCommit( // Squashing rewrites ancestry: parent follows the new commits, while // base follows the original stack heads. Pinning base applies only // this PR's delta onto parent, including reversions of lower changes. - base := prepared.rootBaseHash + base := prepared.rootMergeBaseHash parent := prepared.rootBaseHash for _, change := range prepared.changes { tree, err := sh.gitCmd( diff --git a/internal/forge/shamhub/merge_range_test.go b/internal/forge/shamhub/merge_range_test.go index 093186ac0..67b398da3 100644 --- a/internal/forge/shamhub/merge_range_test.go +++ b/internal/forge/shamhub/merge_range_test.go @@ -74,6 +74,53 @@ func TestStackRepository_MergeRange_squash(t *testing.T) { ) } +func TestStackRepository_MergeRange_advancedTrunk(t *testing.T) { + for _, tt := range []struct { + name string + method forge.MergeMethod + commits string + }{ + {name: "Merge", method: forge.MergeMethodMerge, commits: "1"}, + {name: "Squash", method: forge.MergeMethodSquash, commits: "2"}, + } { + t.Run(tt.name, func(t *testing.T) { + fixture := newMergeRangeFixture(t) + fixture.request.Method = tt.method + + // Advance the receiving branch without changing either submitted head. + workDir := t.TempDir() + worktree, err := git.Clone(t.Context(), + fixture.sh.RepoURL("alice", "example"), workDir, + git.CloneOptions{Log: silogtest.New(t)}, + ) + require.NoError(t, err) + writeCommitAndPush(t, worktree, workDir, + "trunk.txt", "trunk\n", "Advance trunk", "main") + trunkHash, err := worktree.Head(t.Context()) + require.NoError(t, err) + + operation, err := fixture.plan().Merge(t.Context(), fixture.request) + require.NoError(t, err) + require.NotNil(t, operation) + assertMergeRangeCompleted(t, fixture) + + // Merging the stack must retain work added on both sides of the fork. + for _, name := range []string{"trunk", "bottom", "top"} { + contents, err := fixture.sh.gitCmd(t.Context(), "alice", "example", + "show", "main:"+name+".txt", + ).OutputChomp() + require.NoError(t, err) + assert.Equal(t, name, contents) + } + count, err := fixture.sh.gitCmd(t.Context(), "alice", "example", + "rev-list", "--first-parent", "--count", trunkHash.String()+"..main", + ).OutputChomp() + require.NoError(t, err) + assert.Equal(t, tt.commits, count) + }) + } +} + func TestStackRepository_MergeRange_squashUpstackRevert(t *testing.T) { fixture := newMergeRangeFixture(t) fixture.request.Method = forge.MergeMethodSquash diff --git a/internal/handler/merge/merge_range_test.go b/internal/handler/merge/merge_range_test.go index 5c2a36a56..b8548fc4a 100644 --- a/internal/handler/merge/merge_range_test.go +++ b/internal/handler/merge/merge_range_test.go @@ -83,10 +83,10 @@ func TestMergeScheduler_nativeLinearPathWithUnselectedDivergence(t *testing.T) { mockService := NewMockService(ctrl) mockGit := NewMockGitRepository(ctrl) - for idx, branch := range []string{"feat1", "feat2", "feat3"} { + for idx, branch := range []string{"feat2", "feat3"} { mockService.EXPECT().VerifyRestacked(gomock.Any(), branch).Return(nil) mockGit.EXPECT().PeelToCommit(gomock.Any(), branch). - Return(git.Hash(fmt.Sprintf("head-%d", idx+1)), nil) + Return(git.Hash(fmt.Sprintf("head-%d", idx+2)), nil) } syncHandler := &recordingSyncHandler{} @@ -96,8 +96,10 @@ func TestMergeScheduler_nativeLinearPathWithUnselectedDivergence(t *testing.T) { gitRepo: mockGit, sync: syncHandler, }) + bottom := testPlanEntry("feat1", "main", pr1) + bottom.headHash = "head-1" // captured during preflight err := h.executePlan(t.Context(), testMergePlanWithBases( - testPlanEntry("feat1", "main", pr1), + bottom, testPlanEntry("feat2", "feat1", pr2), testPlanEntry("feat3", "feat2", pr3), ), mergeExecutionOptions{ @@ -199,10 +201,8 @@ func TestMergeScheduler_nativeRangeUnsupportedFallsBack(t *testing.T) { } mockService := NewMockService(ctrl) - mockService.EXPECT().VerifyRestacked(gomock.Any(), "feat1").Return(nil) mockService.EXPECT().VerifyRestacked(gomock.Any(), "feat2").Return(nil).Times(2) mockGit := NewMockGitRepository(ctrl) - mockGit.EXPECT().PeelToCommit(gomock.Any(), "feat1").Return(git.Hash("head-1"), nil) mockGit.EXPECT().PeelToCommit(gomock.Any(), "feat2").Return(git.Hash("head-2"), nil).Times(2) syncHandler := &recordingSyncHandler{operations: operations} @@ -212,8 +212,10 @@ func TestMergeScheduler_nativeRangeUnsupportedFallsBack(t *testing.T) { gitRepo: mockGit, sync: syncHandler, }) + bottom := testPlanEntry("feat1", "main", pr1) + bottom.headHash = "head-1" err := h.executePlan(t.Context(), testMergePlanWithBases( - testPlanEntry("feat1", "main", pr1), + bottom, testPlanEntry("feat2", "feat1", pr2), ), mergeExecutionOptions{MergeTimeout: time.Second}) require.NoError(t, err) @@ -294,16 +296,8 @@ func TestMergeScheduler_nativeFallbackSerializesWorkspaceChanges(t *testing.T) { }) mockService := NewMockService(ctrl) mockGit := NewMockGitRepository(ctrl) - for idx, branch := range []string{"feat1", "feat2"} { - mockService.EXPECT(). - VerifyRestacked(gomock.Any(), branch). - Return(nil). - Times(idx + 1) - mockGit.EXPECT(). - PeelToCommit(gomock.Any(), branch). - Return(git.Hash(fmt.Sprintf("head-%d", idx+1)), nil). - Times(idx + 1) - } + mockService.EXPECT().VerifyRestacked(gomock.Any(), "feat2").Return(nil).Times(2) + mockGit.EXPECT().PeelToCommit(gomock.Any(), "feat2").Return(git.Hash("head-2"), nil).Times(2) h := newTestHandler(t, ctrl, testHandlerOpts{ forgeRepo: rangeRepo, service: mockService, @@ -311,9 +305,11 @@ func TestMergeScheduler_nativeFallbackSerializesWorkspaceChanges(t *testing.T) { sync: syncHandler, }) done := make(chan error, 1) + bottom := testPlanEntry("feat1", "main", pr1) + bottom.headHash = "head-1" go func() { done <- h.executePlan(t.Context(), []*mergeItem{ - testPlanEntry("feat1", "main", pr1), + bottom, testPlanEntry("feat2", "feat1", pr2), testPlanEntry("feat3", "main", pr3), }, mergeExecutionOptions{}) @@ -461,6 +457,7 @@ func TestRangeMergeQueueItem_Run_partialFallbackFailure(t *testing.T) { testPlanEntry("feat2", "feat1", pr2), testPlanEntry("feat3", "feat2", pr3), } + plan[0].headHash = "head-1" // The range covers feat1 and feat2. After native refusal, feat1 merges // individually but feat2 becomes blocked. Only feat2 may be reported as // failed; feat1 remains merged and dependent feat3 must be skipped. @@ -468,16 +465,17 @@ func TestRangeMergeQueueItem_Run_partialFallbackFailure(t *testing.T) { mockService := NewMockService(ctrl) mockGit := NewMockGitRepository(ctrl) for _, member := range []struct { - id fakeChangeID - branch string - head git.Hash - checks int + id fakeChangeID + branch string + head git.Hash + checks int + preparations int }{ - {pr1, "feat1", "head-1", 1}, - {pr2, "feat2", "head-2", 2}, + {pr1, "feat1", "head-1", 1, 0}, + {pr2, "feat2", "head-2", 2, 2}, } { - mockService.EXPECT().VerifyRestacked(gomock.Any(), member.branch).Return(nil).Times(member.checks) - mockGit.EXPECT().PeelToCommit(gomock.Any(), member.branch).Return(member.head, nil).Times(member.checks) + mockService.EXPECT().VerifyRestacked(gomock.Any(), member.branch).Return(nil).Times(member.preparations) + mockGit.EXPECT().PeelToCommit(gomock.Any(), member.branch).Return(member.head, nil).Times(member.preparations) mockForge.EXPECT(). ChangeStatuses(gomock.Any(), []forge.ChangeID{member.id}). Return([]forge.ChangeStatus{{State: forge.ChangeOpen, HeadHash: member.head}}, nil). diff --git a/internal/handler/merge/workspace.go b/internal/handler/merge/workspace.go index 695e84cca..724011a68 100644 --- a/internal/handler/merge/workspace.go +++ b/internal/handler/merge/workspace.go @@ -51,9 +51,9 @@ func (w *mergeWorkspace) PrepareChange(ctx context.Context, item *mergeItem) err return nil } -// PrepareRange aligns every member and captures its resulting head commit. -// Unlike an ordinary item, a range must also verify its bottom member: -// the provider requires the entire range to be restacked before merging it. +// PrepareRange aligns members with their branch bases and collects their heads. +// Like ordinary merging, a range does not rebase its trunk-based bottom merely +// because trunk advanced. Its preflight head remains valid. func (w *mergeWorkspace) PrepareRange(ctx context.Context, items []*mergeItem) ([]forge.ChangeHead, error) { release, err := w.acquire(ctx) if err != nil { @@ -63,8 +63,10 @@ func (w *mergeWorkspace) PrepareRange(ctx context.Context, items []*mergeItem) ( heads := make([]forge.ChangeHead, len(items)) for idx, item := range items { - if err := w.prepare(ctx, item); err != nil { - return nil, fmt.Errorf("prepare %q: %w", item.branch, err) + if item.base != w.Trunk { + if err := w.prepare(ctx, item); err != nil { + return nil, fmt.Errorf("prepare %q: %w", item.branch, err) + } } heads[idx] = forge.ChangeHead{Change: item.changeID, HeadHash: item.headHash} } diff --git a/internal/handler/merge/workspace_test.go b/internal/handler/merge/workspace_test.go new file mode 100644 index 000000000..8cf06b94c --- /dev/null +++ b/internal/handler/merge/workspace_test.go @@ -0,0 +1,56 @@ +package merge + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + + "go.abhg.dev/gs/internal/forge" + "go.abhg.dev/gs/internal/git" + "go.abhg.dev/gs/internal/handler/restack" + "go.abhg.dev/gs/internal/handler/submit" + "go.abhg.dev/gs/internal/spice" +) + +func TestMergeWorkspace_PrepareRange_bottomAfterBaseMerge(t *testing.T) { + ctrl := gomock.NewController(t) + service := NewMockService(ctrl) + // The range starts above an earlier queue item that has now merged. + // Its bottom still needs preparation onto trunk despite being first in range. + service.EXPECT().VerifyRestacked(gomock.Any(), "bottom"). + Return(&spice.BranchNeedsRestackError{Base: "main"}) + restackHandler := NewMockRestackHandler(ctrl) + restackHandler.EXPECT().RestackBranch(gomock.Any(), &restack.BranchRequest{ + Branch: "bottom", + }).Return(nil) + submitHandler := NewMockSubmitHandler(ctrl) + submitHandler.EXPECT().Submit(gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, req *submit.Request) error { + assert.Equal(t, "bottom", req.Branch) + return nil + }) + gitRepo := NewMockGitRepository(ctrl) + gitRepo.EXPECT().PeelToCommit(gomock.Any(), "bottom"). + Return(git.Hash("restacked-bottom"), nil) + + workspace := &mergeWorkspace{ + Repository: gitRepo, + Service: service, + Restack: restackHandler, + Submit: submitHandler, + Sync: new(recordingSyncHandler), + Progress: new(recordingMergeProgress), + Trunk: "main", + gate: make(chan struct{}, 1), + } + item := testPlanEntry("bottom", "merged", "pr-bottom") + item.headHash = "preflight-bottom" + heads, err := workspace.PrepareRange(t.Context(), []*mergeItem{item}) + require.NoError(t, err) + assert.Equal(t, []forge.ChangeHead{{ + Change: fakeChangeID("pr-bottom"), HeadHash: "restacked-bottom", + }}, heads) +} diff --git a/testdata/script/downstack_merge_native_range_advanced_trunk.txt b/testdata/script/downstack_merge_native_range_advanced_trunk.txt new file mode 100644 index 000000000..c9033366c --- /dev/null +++ b/testdata/script/downstack_merge_native_range_advanced_trunk.txt @@ -0,0 +1,63 @@ +# An aligned native stack can merge without rebasing onto a newer trunk. + +[!git:2.40.0] skip # native stack merges require merge-tree --merge-base + +as 'Test ' +at '2026-10-26T00:03:25Z' + +cd repo +git init +git config spice.experiment.merge true +git config spice.forge.shamhub.stacks on +git commit --allow-empty -m 'Initial commit' + +shamhub-setup +shamhub new origin alice/example.git +shamhub register alice +git push origin main +env SHAMHUB_USERNAME=alice +gs auth login + +git add bottom.txt +gs bc bottom -m 'Add bottom' +git add middle.txt +gs bc middle -m 'Add middle' +git add top.txt +gs bc top -m 'Add top' +gs downstack submit --fill + +# Trunk advances independently; the members still align with each other. +gs branch checkout main +git add trunk.txt +git commit -m 'Advance trunk' +git push origin main +git tag before-merge +gs branch checkout top + +gs downstack merge --no-prompt +stderr 'All 3 change.s. merged.' +! stderr 'restacked' +! stderr 'Updated #' + +# One atomic merge preserves both trunk's new work and the stack's work. +git rev-list --first-parent --count before-merge..main +stdout '^1$' +git log -1 --format=%s main +stdout '^Merge changes #1 through #3$' +git show main:trunk.txt +stdout '^trunk$' +git show main:bottom.txt +stdout '^bottom$' +git show main:middle.txt +stdout '^middle$' +git show main:top.txt +stdout '^top$' + +-- repo/bottom.txt -- +bottom +-- repo/middle.txt -- +middle +-- repo/top.txt -- +top +-- repo/trunk.txt -- +trunk