Skip to content

Commit 7cb1544

Browse files
authored
Support modifying stacks across worktrees (#521)
* Support modifying stacks across worktrees * Fix Windows worktree test failures * Disambiguate the fold recovery revision on Windows * Restore checkout to the compiled fold receiver * increment skill file version
1 parent cbe6380 commit 7cb1544

20 files changed

Lines changed: 2696 additions & 680 deletions

File tree

‎.github/copilot-instructions.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,6 @@ No Makefile, no code generation, no external linter config. Standard Go toolchai
3939
- Recovery must match stack identity, execute in the recorded worktree, and retain journals on partial failures. Native Git markers stay per-worktree.
4040
- Finish paused operations before switching preview stages or versions. Reject origin-only and unknown rebase execution modes before routing recovery; use the matching layer3 build in the recorded origin for origin-only journals.
4141
- Mutation locks coordinate gh-stack only, not Git commands/editors. Keep affected worktrees quiescent during rewrites. Pass the snapshot SHA (or prior `Context.Touched` SHA) to `Context.Start` before ref mutations; never claim an external commit as this operation's work during continuation.
42-
- Core modify rejects distributed stack branches before TUI/apply; foreign trunk ownership alone is allowed. Do not enable distributed modify until its dependent layer is implemented.
42+
- Distributed modify preflights action/cascade targets and executes in recorded owners; only the origin may switch for unoccupied branches. Preserve dropped/folded source worktrees and refs. Before native continuation, preflight other worktrees, not remaining branches in the intentionally busy pending worktree.
4343

4444
For full architecture details, see [AGENTS.md](../AGENTS.md) in the repository root.

‎.github/workflows/test.yml‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,4 +26,5 @@ jobs:
2626
- name: Vet
2727
run: go vet ./...
2828
- name: Test
29-
run: go test -race -count=1 ./...
29+
# The real-Git worktree cases can exceed Go's 10-minute package limit on Windows.
30+
run: go test -race -count=1 -timeout=20m ./...

‎AGENTS.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -126,8 +126,8 @@ if errors.As(err, &exitErr) { ... }
126126
- **Migration:** Consolidate only nonconflicting legacy catalogs and preserve originals. Stop on conflicting definitions; finish legacy recovery in its original worktree before migrating. Do not mix old and new writers.
127127
- **Recovery:** gh-stack journals live in the common directory and record origin/owner identities, original refs, and progress. Native Git markers remain per-worktree. Continue/abort must use recorded scoped executors, match stack identity (not catalog array position), and retain state on any partial restore or save failure.
128128
- **External changes:** Mutation locks coordinate gh-stack, not arbitrary Git commands or editors. Keep affected worktrees quiescent during rewrites, except for requested conflict resolution while paused. Call `Context.Start(branch, expectedSHA)` before ref mutations: use the original snapshot SHA, or the last `Context.Touched` SHA for a branch already changed by this operation. Do not adopt a freshly read tip as this operation's baseline during continuation.
129-
- **Separate Git directories:** Native topology may report the administration directory as the main worktree path for `--separate-git-dir` repositories. A known origin remains usable, but a foreign main-owner root may be undiscoverable. Never infer a working directory from an administration path, emit it as a successful navigation target, or add a private registry/config mutation to guess ownership.
130-
- **Core modify boundary:** Plain modify permits unoccupied branches and branches owned by its origin worktree, but rejects distributed stack membership before the TUI/apply. Trunk ownership alone does not block it. Full distributed modify is a separate layer.
129+
- **Separate Git directories:** Main-worktree invocation and existing absolute/relative `core.worktree` backlinks, including the main `config.worktree`, are supported. The discovery caveat is only linked invocation without a main-worktree backlink: fail actionably if that owner is required, but allow unaffected worktrees to proceed. Never infer a working directory from an administration path, emit it as a successful navigation target, or add a private registry/config mutation to guess ownership.
130+
- **Distributed modify:** Preflight the staged actions and surviving cascade branches before mutation. Run renames and history rewrites in recorded owners, using the origin only for unoccupied branches. Persist execution progress, rename aliases, created refs, and last-written heads. Before native continuation, preflight other target worktrees; the pending worktree is intentionally busy and its later branches are checked after continuation. Never switch a foreign owner to another branch, reset external commits, delete a preserved source branch/worktree, or clear a partially restored journal.
131131
- **Journal transition:** Complete paused operations before switching preview stages or versions. Unmarked rebase/sync journals with a worktree context use distributed recovery. Reject `executionMode: "origin-only"` before context-based routing or mutation; preserve its bytes and require the matching layer3 build in the recorded origin. Unknown nonempty modes also fail closed. Empty-mode journals without a context retain the legacy original-catalog route.
132132

133133
## CI workflows (`.github/workflows/`)

‎README.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,9 +78,9 @@ Mutation locks coordinate **gh-stack processes only**, not arbitrary Git command
7878

7979
Navigation to a branch checked out elsewhere reports its path and fails without switching. Add `--print-path` to `up`, `down`, `top`, `bottom`, `trunk`, or an explicit-target `checkout` to get the owning path instead. Unoccupied targets are checked out here before printing this worktree's path; successful stdout contains only the absolute path and a newline. See the [worktree workflow](docs/src/content/docs/guides/workflows.md#working-across-git-worktrees) for a shell wrapper that checks errors before changing directories.
8080

81-
**Temporary core limitation:** `modify` works inside a linked worktree only when all stack branches are unoccupied or checked out in that same worktree. Distributed modify is rejected before opening the TUI or applying changes. A trunk checked out elsewhere is allowed because modify only reads it.
81+
`modify` supports stacks distributed across worktrees. It preflights the owners needed by the staged actions and cascade, renames in each branch's owner, and cherry-picks or rebases in the receiving owner's worktree. Unoccupied branches use the initiating worktree; other worktrees are never switched to different branches. Dropped/folded branches and their worktrees remain intact. If the origin's nearest surviving branch is owned elsewhere, modify keeps the preserved original branch checked out and reports the survivor's path. Trunk is only read.
8282

83-
With `git init --separate-git-dir`, Git may report the administration directory instead of the main working directory in its worktree list. Shared storage and operations from a known main or linked worktree still work, but discovering that main worktree's owner path from another checkout can be unavailable. Do not treat an administration-directory path as a checkout directory; use the actual main worktree when its location cannot be discovered.
83+
Repositories created with `git init --separate-git-dir` support main-worktree invocation and main-owner discovery through an existing absolute or relative `core.worktree` backlink, including one stored in the main `config.worktree`. The discovery limitation is only linked-worktree invocation without a main-worktree backlink: an operation requiring that main owner fails with actionable guidance, while unaffected worktrees continue. An administration directory is never treated as a checkout destination.
8484

8585
## Commands
8686

‎cmd/modify.go‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,10 @@ Operations available:
3535
• Rename branches
3636
3737
All changes are staged in the TUI and applied together when you press Ctrl+S.
38+
Branches may be checked out in different worktrees. Changes run in their clean
39+
owning worktrees; unoccupied branches use the initiating worktree. Foreign
40+
worktrees are never switched to different branches. Changes are never autostashed,
41+
and no worktrees are created or removed.
3842
If your changes affect branches with pull requests, run 'gh stack submit'
3943
afterward to push changes, update PRs, and recreate the stack on GitHub.`,
4044
Example: ` # Open the interactive TUI to restructure the stack
@@ -168,11 +172,11 @@ func runModify(cfg *config.Config) error {
168172
if state == nil || state.Worktrees == nil {
169173
return fmt.Errorf("modify conflict has no recorded worktree; recovery state was retained")
170174
}
171-
ops, err := state.Worktrees.OriginOps()
175+
ops, path, err := modify.ConflictOps(state)
172176
if err != nil {
173177
return err
174178
}
175-
printConflictDetailsAt(cfg, ops, state.Worktrees.Origin.Path, conflict.Branch, "gh stack modify --continue")
179+
printConflictDetailsAt(cfg, ops, path, conflict.Branch, "gh stack modify --continue")
176180
cfg.Printf("")
177181

178182
cfg.Printf("Or restore the stack to its pre-modify state with `%s`",

‎cmd/modify_test.go‎

Lines changed: 193 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
"testing"
1313
"time"
1414

15+
tea "github.com/charmbracelet/bubbletea"
1516
"github.com/github/gh-stack/internal/config"
1617
"github.com/github/gh-stack/internal/git"
1718
"github.com/github/gh-stack/internal/github"
@@ -993,6 +994,12 @@ func TestCheckModifyPreconditions_Worktrees(t *testing.T) {
993994
}, nil
994995
},
995996
}
997+
mock.ForWorktreeFn = func(path string) (git.Ops, error) {
998+
if worktree.SamePath(path, foreign) {
999+
return &git.MockOps{RootDirFn: func() (string, error) { return foreign, nil }}, nil
1000+
}
1001+
return mock, nil
1002+
}
9961003
restore := git.SetOps(mock)
9971004
defer restore()
9981005
cfg, _, errR := config.NewTestConfig()
@@ -1009,14 +1016,9 @@ func TestCheckModifyPreconditions_Worktrees(t *testing.T) {
10091016
cfg.Err.Close()
10101017
output, readErr := io.ReadAll(errR)
10111018
require.NoError(t, readErr)
1012-
if ownerBranch == "b2" {
1013-
require.Error(t, err)
1014-
assert.Contains(t, string(output), "distributed modify is not supported yet")
1015-
assert.Contains(t, string(output), foreign)
1016-
assert.Zero(t, prQueries.Load(), "distributed guard must run before PR refresh or TUI")
1017-
} else {
1018-
require.NoError(t, err)
1019-
}
1019+
require.NoError(t, err)
1020+
assert.NotContains(t, string(output), "distributed modify is not supported")
1021+
assert.Positive(t, prQueries.Load(), "action-specific owner checks happen after the TUI produces its plan")
10201022
assert.False(t, modify.StateExists(dir))
10211023
})
10221024
}
@@ -1060,12 +1062,18 @@ func TestRunModifyRecovery_UsesRecordedOrigin(t *testing.T) {
10601062
require.NoError(t, modify.SaveState(common, state))
10611063
inProgress, continued, aborted := true, false, false
10621064
sha := "original"
1065+
revParse := func(ref string) (string, error) {
1066+
if ref == "B" {
1067+
return "source", nil
1068+
}
1069+
return sha, nil
1070+
}
10631071
originOps := &git.MockOps{
10641072
GitDirFn: func() (string, error) { return originDir, nil },
10651073
CommonDirFn: func() (string, error) { return common, nil },
10661074
RootDirFn: func() (string, error) { return origin, nil },
10671075
CurrentBranchFn: func() (string, error) { return "A", nil },
1068-
RevParseFn: func(string) (string, error) { return sha, nil },
1076+
RevParseFn: revParse,
10691077
IsRebaseInProgressFn: func() (bool, error) { return inProgress && tc.conflictType == "rebase", nil },
10701078
RebaseContinueFn: func(git.RebaseOpts) error {
10711079
require.Equal(t, "rebase", tc.conflictType)
@@ -1095,7 +1103,7 @@ func TestRunModifyRecovery_UsesRecordedOrigin(t *testing.T) {
10951103
CommonDirFn: func() (string, error) { return common, nil },
10961104
RootDirFn: func() (string, error) { return caller, nil },
10971105
CurrentBranchFn: func() (string, error) { return "observer", nil },
1098-
RevParseFn: func(string) (string, error) { return sha, nil },
1106+
RevParseFn: revParse,
10991107
CheckoutBranchFn: func(string) error {
11001108
callerSensitiveCalls++
11011109
return nil
@@ -1168,6 +1176,49 @@ func TestModifyStateIOFailures(t *testing.T) {
11681176
require.Error(t, modify.CheckStateGuard(dir))
11691177
}
11701178

1179+
func TestModifyApply_DoesNotReportAdministrationDirectoryAsOwner(t *testing.T) {
1180+
dir, origin := t.TempDir(), t.TempDir()
1181+
s := stack.Stack{Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{{Branch: "A"}, {Branch: "B"}}}
1182+
writeStackFile(t, dir, s)
1183+
mock := &git.MockOps{
1184+
GitDirFn: func() (string, error) { return filepath.Join(dir, "worktrees", "origin"), nil },
1185+
CommonDirFn: func() (string, error) { return dir, nil },
1186+
RootDirFn: func() (string, error) { return origin, nil },
1187+
CurrentBranchFn: func() (string, error) { return "B", nil },
1188+
BranchExistsFn: func(string) (bool, error) { return true, nil },
1189+
RevParseFn: func(ref string) (string, error) { return "sha-" + ref, nil },
1190+
WorktreesFn: func() ([]git.Worktree, error) {
1191+
return []git.Worktree{{Path: dir, Branch: "A"}, {Path: origin, Branch: "B"}}, nil
1192+
},
1193+
}
1194+
mock.ForWorktreeFn = func(path string) (git.Ops, error) {
1195+
if worktree.SamePath(path, dir) {
1196+
return &git.MockOps{
1197+
GitDirFn: func() (string, error) { return dir, nil },
1198+
CommonDirFn: func() (string, error) { return dir, nil },
1199+
RootDirFn: func() (string, error) { return "", assert.AnError },
1200+
}, nil
1201+
}
1202+
return mock, nil
1203+
}
1204+
restore := git.SetOps(mock)
1205+
defer restore()
1206+
cfg, _, _ := config.NewTestConfig()
1207+
defer cfg.Out.Close()
1208+
defer cfg.Err.Close()
1209+
sf, err := stack.Load(dir)
1210+
require.NoError(t, err)
1211+
nodes := []modifyview.ModifyBranchNode{
1212+
{BranchNode: stackview.BranchNode{Ref: s.Branches[1]}, OriginalPosition: 1},
1213+
{BranchNode: stackview.BranchNode{Ref: s.Branches[0]}, OriginalPosition: 0},
1214+
}
1215+
_, _, err = modify.ApplyPlan(cfg, dir, &sf.Stacks[0], sf, nodes, "B", func(*stack.Stack) {})
1216+
require.Error(t, err)
1217+
assert.Contains(t, err.Error(), "working-tree root")
1218+
assert.NotContains(t, err.Error(), "checked out in worktree "+dir)
1219+
assert.False(t, modify.StateExists(dir))
1220+
}
1221+
11711222
func TestRunModifyContinue_LegacyPrivateJournalKeepsOriginalCatalog(t *testing.T) {
11721223
common, origin := t.TempDir(), t.TempDir()
11731224
private := filepath.Join(common, "worktrees", "legacy")
@@ -1241,3 +1292,135 @@ func TestRunModifyContinue_LegacyPrivateJournalKeepsOriginalCatalog(t *testing.T
12411292
require.NoError(t, err)
12421293
assert.Equal(t, []string{"A", "B", "C"}, privateCatalog.Stacks[0].BranchNames())
12431294
}
1295+
1296+
func TestRunModifyContinue_UsesForeignPendingOwner(t *testing.T) {
1297+
common, origin, target, caller := t.TempDir(), t.TempDir(), t.TempDir(), t.TempDir()
1298+
refs := map[string]string{"main": "sha-main", "A": "sha-A", "C": "sha-C"}
1299+
s := stack.Stack{Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{{Branch: "A"}, {Branch: "C"}}}
1300+
writeStackFile(t, common, s)
1301+
metadata, err := json.Marshal(s)
1302+
require.NoError(t, err)
1303+
state := &modify.StateFile{
1304+
SchemaVersion: 1, Phase: modify.PhaseConflict, ConflictType: "rebase", ConflictBranch: "A",
1305+
OriginalBranch: "C", RemainingBranches: []string{"C"}, OriginalRefs: map[string]string{"C": "sha-A"},
1306+
Snapshot: modify.Snapshot{
1307+
StackMetadata: metadata,
1308+
Branches: []modify.BranchSnapshot{{Name: "A", TipSHA: "sha-A"}, {Name: "C", TipSHA: "sha-C"}},
1309+
},
1310+
Worktrees: &worktree.Context{
1311+
Origin: worktree.Location{Path: origin},
1312+
Owners: map[string]*worktree.Location{"A": {Path: target}},
1313+
Pending: "A",
1314+
PendingBefore: "sha-A",
1315+
},
1316+
}
1317+
state.RecordStack(&s)
1318+
require.NoError(t, modify.SaveState(common, state))
1319+
scoped := func(path, name string) *git.MockOps {
1320+
return &git.MockOps{
1321+
RootDirFn: func() (string, error) { return path, nil },
1322+
CommonDirFn: func() (string, error) { return common, nil },
1323+
GitDirFn: func() (string, error) { return filepath.Join(common, "worktrees", name), nil },
1324+
CurrentBranchFn: func() (string, error) { return name, nil },
1325+
RevParseFn: func(ref string) (string, error) { return refs[ref], nil },
1326+
IsAncestorFn: func(string, string) (bool, error) { return true, nil },
1327+
MergeBaseFn: func(string, string) (string, error) { return "sha-A", nil },
1328+
RebaseContinueFn: func(git.RebaseOpts) error {
1329+
t.Fatal("native continuation must run only in the pending owner's worktree")
1330+
return nil
1331+
},
1332+
}
1333+
}
1334+
originOps, targetOps, callerOps := scoped(origin, "C"), scoped(target, "A"), scoped(caller, "observer")
1335+
inProgress, continued := true, false
1336+
targetOps.IsRebaseInProgressFn = func() (bool, error) { return inProgress, nil }
1337+
targetOps.RebaseContinueFn = func(git.RebaseOpts) error { inProgress, continued = false, true; return nil }
1338+
callerOps.WorktreesFn = func() ([]git.Worktree, error) {
1339+
return []git.Worktree{{Path: origin, Branch: "C"}, {Path: target, Branch: "A"}, {Path: caller, Branch: "observer"}}, nil
1340+
}
1341+
callerOps.ForWorktreeFn = func(path string) (git.Ops, error) {
1342+
if worktree.SamePath(path, target) {
1343+
return targetOps, nil
1344+
}
1345+
if worktree.SamePath(path, origin) {
1346+
return originOps, nil
1347+
}
1348+
return callerOps, nil
1349+
}
1350+
restore := git.SetOps(callerOps)
1351+
defer restore()
1352+
nativeOps, path, err := modify.ConflictOps(state)
1353+
require.NoError(t, err)
1354+
assert.Same(t, targetOps, nativeOps)
1355+
assert.Equal(t, target, path)
1356+
cfg, _, _ := config.NewTestConfig()
1357+
defer cfg.Out.Close()
1358+
defer cfg.Err.Close()
1359+
require.NoError(t, runModifyContinue(cfg))
1360+
assert.True(t, continued)
1361+
assert.False(t, modify.StateExists(common))
1362+
assert.Nil(t, cfg.StackMutation)
1363+
}
1364+
1365+
func TestModifyTUI_RejectsMixedReorderFold(t *testing.T) {
1366+
for _, scenario := range []struct {
1367+
name string
1368+
keys []rune
1369+
order []string
1370+
kind modifyview.ActionType
1371+
}{
1372+
{"move B below A then fold up", []rune{'J', 'u'}, []string{"C", "A", "B"}, modifyview.ActionMove},
1373+
{"fold B up then move", []rune{'u', 'J'}, []string{"C", "B", "A"}, modifyview.ActionFoldUp},
1374+
} {
1375+
t.Run(scenario.name, func(t *testing.T) {
1376+
nodes := []modifyview.ModifyBranchNode{
1377+
{BranchNode: stackview.BranchNode{Ref: stack.BranchRef{Branch: "C"}}, OriginalPosition: 0},
1378+
{BranchNode: stackview.BranchNode{Ref: stack.BranchRef{Branch: "B"}, IsCurrent: true}, OriginalPosition: 1},
1379+
{BranchNode: stackview.BranchNode{Ref: stack.BranchRef{Branch: "A"}}, OriginalPosition: 2},
1380+
}
1381+
model := modifyview.New(nodes, stack.BranchRef{Branch: "main"}, "test")
1382+
for _, key := range scenario.keys {
1383+
updated, _ := model.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{key}})
1384+
var ok bool
1385+
model, ok = updated.(modifyview.Model)
1386+
require.True(t, ok)
1387+
}
1388+
var order []string
1389+
for _, node := range model.Nodes() {
1390+
order = append(order, node.Ref.Branch)
1391+
if scenario.kind == modifyview.ActionMove {
1392+
assert.Nil(t, node.PendingAction, "fold must be rejected after reordering")
1393+
assert.False(t, node.Removed)
1394+
}
1395+
}
1396+
1397+
assert.Equal(t, scenario.order, order)
1398+
require.Len(t, model.StagedActions(), 1, "only the first operation may be staged")
1399+
assert.Equal(t, scenario.kind, model.StagedActions()[0].Type)
1400+
})
1401+
}
1402+
}
1403+
1404+
func TestModifyTUI_DropThenFoldUpSkipsDroppedNeighbor(t *testing.T) {
1405+
nodes := []modifyview.ModifyBranchNode{
1406+
{BranchNode: stackview.BranchNode{Ref: stack.BranchRef{Branch: "C"}}, OriginalPosition: 0},
1407+
{BranchNode: stackview.BranchNode{Ref: stack.BranchRef{Branch: "B"}, IsCurrent: true}, OriginalPosition: 1},
1408+
{BranchNode: stackview.BranchNode{Ref: stack.BranchRef{Branch: "A"}}, OriginalPosition: 2},
1409+
}
1410+
model := modifyview.New(nodes, stack.BranchRef{Branch: "main"}, "test")
1411+
for _, key := range []rune{'x', 'j', 'u'} {
1412+
updated, _ := model.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{key}})
1413+
var ok bool
1414+
model, ok = updated.(modifyview.Model)
1415+
require.True(t, ok)
1416+
}
1417+
actions := model.StagedActions()
1418+
require.Len(t, actions, 2)
1419+
assert.Equal(t, modifyview.ActionDrop, actions[0].Type)
1420+
assert.Equal(t, "B", actions[0].BranchName)
1421+
assert.Equal(t, modifyview.ActionFoldUp, actions[1].Type)
1422+
assert.Equal(t, "A", actions[1].BranchName)
1423+
assert.Equal(t, "C", actions[1].FoldTarget)
1424+
assert.True(t, model.Nodes()[1].Removed)
1425+
assert.True(t, model.Nodes()[2].Removed)
1426+
}

0 commit comments

Comments
 (0)