diff --git a/AGENTS.md b/AGENTS.md index 5809688..e1720a7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -39,7 +39,7 @@ git-stack is a CLI (`git stack `) and stdio MCP server for GitHub's native - Read and write git config via `git config`, never by editing files as text. - Never edit `~/.claude.json` or `~/.codex/config.toml` directly; use `claude mcp` / `codex mcp`. - `install` must never clobber existing user aliases or config without confirmation, and `--uninstall` removes only what we recorded as managed. -- Only `pkg/backend/ghstack` writes gh-stack's `/gh-stack` file, through its `Update` (or, before running `gh stack` on a stack kept in another worktree's file, `adopt`, which moves that record into the current worktree's) under `gh-stack.lock`, editing the JSON in place; nothing else writes it, and `gh stack` commands do every other mutation. +- Only `pkg/backend/ghstack` writes gh-stack's shared `/gh-stack` file, through its `Update` (which also moves in any per-worktree file left from gh-stack 0.2, see `Migrate`) under `gh-stack.lock`, editing the JSON in place; nothing else writes it, and `gh stack` commands do every other mutation. ## Testing - Tests must never touch the real user environment. Use `gittest.Isolate(t)` (sets `HOME`, `XDG_CONFIG_HOME`, `GIT_CONFIG_GLOBAL`, `GIT_CONFIG_NOSYSTEM`) in any test that runs git or installers. diff --git a/README.md b/README.md index 2c3f8df..1afa18b 100644 --- a/README.md +++ b/README.md @@ -120,7 +120,8 @@ You'll need: - git 2.40+ - `gh` (the [GitHub CLI](https://cli.github.com/)) logged in via `gh auth login`, plus the stack - extension; `gh extension install github/gh-stack` + extension, v0.3.0 or later; `gh extension install github/gh-stack` (or `gh extension upgrade + stack`) Then grab the archive for your system from the [releases page](https://github.com/DomBlack/git-stack/releases). Builds are for Linux and macOS @@ -628,7 +629,7 @@ come back. If a branch was deleted by mistake, the output says which commit it w Nothing gets pushed; that's what `git ss` is for. If your local trunk has diverged from the remote, it's only reset with `-f` or a yes at the prompt. -Stacks are per worktree (that's how gh stack stores them), but sync covers all of them. A branch +Every worktree shares one set of stacks, and sync covers all of them. A branch checked out in another worktree is fast forwarded there too, and like `git pull` any uncommitted changes it doesn't touch come along. If a change or an untracked file is in the way, the branch is left where it was and sync ends on `⚠ Synced, but main was not updated`, naming the files diff --git a/docs/architecture.md b/docs/architecture.md index 7d46ee3..0b2629c 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -101,9 +101,11 @@ All of this came from reading the gh-stack source (pinned at commit `2bd699a`, a its `schema.json` sits in `pkg/backend/ghstack/testdata`). Several of these findings overturned assumptions in the original brief, so they're worth keeping. -**Storage.** One JSON file at `/gh-stack` (`schemaVersion: 1`), plus -`gh-stack.lock`, `gh-stack-rebase-state` and `gh-stack-modify-state` next to it. No refs, -no git config keys. The shape is roughly; +**Storage.** One JSON file at `/gh-stack` (`schemaVersion: 1`), shared by every +worktree of the repository, plus `gh-stack.lock` (held while the file is written), +`gh-stack-operation.lock` (held for the whole of a gh stack command that changes stacks), +`gh-stack-rebase-state` and `gh-stack-modify-state` next to it. No refs, no git config keys. +The shape is roughly; ```json { "schemaVersion": 1, "repository": "github.com:owner/name", @@ -116,12 +118,21 @@ no git config keys. The shape is roughly; - The parent relationship is array order (bottom to top). There is no parent field, so stacks are strictly linear. - Trunk is per stack and several stacks can share one. A branch is in at most one stack. -- `` is per worktree, so a linked worktree doesn't see the main checkout's stacks. - git-stack reads every worktree's file, but `gh stack submit` and `gh stack add` only find - a stack in the file of the worktree they run in. So before either, the backend moves the - current branch's stack record, untouched, into this worktree's file when it lives in another - (a stack started in the main checkout and then checked out in a worktree, say), and moves - it back the same way when the branch is next submitted from somewhere else. +- Up to gh-stack 0.2 the file lived in each worktree's own ``, and gh stack only read + the one in the worktree it ran in. 0.3 (gh-stack PR #528, not in its release notes) moved + to the one shared file. The first 0.3 command moves each worktree's file into it, keeping + `.pre-worktree-migration` backups and a `gh-stack-migration` journal while it works, + and from then on refuses for good if a backup it made differs from the file it would + move. Older git-stack versions kept writing per-worktree files after gh stack had moved them, + which is exactly how that happens. So git-stack needs gh-stack 0.3 or later (preflight + checks), reads and writes only the shared file, and moves any per-worktree file in itself + before its next write or `gh stack` call (`Migrate`): the same merge as gh stack's, under + both of gh stack's locks, but a backup name that's taken goes to `.pre-worktree-migration.1` + (and on) instead of stopping. Where two files hold a stack with the same branch the shared + file wins, then the current worktree's; the losing record survives in its backup. While gh + stack's own journal exists, or a worktree whose file would move has an interrupted gh stack + rebase or modify, git-stack leaves the files alone and says why. `Load` reads per-worktree + files too, without writing, so stacks look the same before and after the move. - `gh stack view --json` exists but only covers the current stack, calls the GitHub API, and writes the stack file as a side effect. Useless for completion or for listing every stack. @@ -314,9 +325,7 @@ holds a change (by patch id, ignoring what came from trunk) the PR never saw; a that sync restacked since its last push differs only by commit id and is deleted as usual. Patch ids are `--verbatim` rather than `--stable`, so a whitespace only amend counts as a change and keeps the branch. -A branch recorded in two worktrees' metadata files is read from the first; once it is -deleted the stale record in the other file shows up as gone on the next sync and is -dropped then, so it heals over two syncs. A candidate checked out in the current worktree whose +A candidate checked out in the current worktree whose trunk is checked out in another worktree leaves HEAD detached at the trunk tip rather than failing. Moving the current checkout off a branch being deleted is a plain `git switch`, so uncommitted changes git can carry come along; only git's "would be overwritten" refusal keeps @@ -419,9 +428,7 @@ stack. All three go through one reshape step in `pkg/app`. It snapshots the stac to change, rewrites them in a single `Update`, then hands the branches whose parent changed to the native restack, so nothing is checked out (init stops after the `Update` and just says which branches need a restack). `Update` grew for this: its callback may now append stacks (written to -the current worktree's own file, which is created with `schemaVersion: 1` if there isn't one -yet; gh stack only reads the file in the git dir it runs in, so a stack put in the main -checkout's file from a linked worktree was invisible to `gh stack submit` there) and clear or set a stack's id and number, though it +the shared file, which is created with `schemaVersion: 1` if there isn't one yet) and clear or set a stack's id and number, though it still can't drop or reorder stacks. In that same `Update` every branch whose parent changed gets its recorded base fixed to where its own commits start (the recorded base if it's still in the branch's history, else the merge base with its old parent), because the restack replays diff --git a/docs/mcp.md b/docs/mcp.md index 50200da..e01bf0a 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -68,7 +68,9 @@ Tool errors (`isError: true`) carry a JSON object in the text content: Codes: `not_repo`, `not_in_stack`, `not_at_top`, `conflict`, `partial`, `rebase_active`, `locked`, `stacks_unavailable`, `auth_required`, `not_installed`, `unsupported`, `invalid_args`, `interaction_required`, `api_failure`, `disambiguate`, `modify_recovery`, `checks_failing`, -`checks_pending`, `signing_failed`, `unknown`. +`checks_pending`, `signing_failed`, `state_migration`, `unknown`. +`state_migration` means gh stack's stack file is in the layout gh-stack 0.2 used (one file per +worktree) and can't be moved into the shared file yet; `next_steps` says what is in the way. `signing_failed` comes from `stack_create` and `stack_modify` when git could not sign the commit, and from `stack_restack`, `stack_modify` and `stack_continue` when the git rebase a conflict needs could not sign a replayed commit; most often an SSH signing key whose passphrase nobody diff --git a/pkg/app/reshape.go b/pkg/app/reshape.go index 9d362cd..6c680a0 100644 --- a/pkg/app/reshape.go +++ b/pkg/app/reshape.go @@ -28,9 +28,7 @@ type reshapeSnapshot struct { } type snapshotStack struct { - Trunk string `json:"trunk"` - // Worktree is the working tree whose metadata held the stack. - Worktree string `json:"worktree,omitempty"` + Trunk string `json:"trunk"` ID string `json:"id,omitempty"` Number int `json:"number,omitempty"` Branches []snapshotBranch `json:"branches"` @@ -55,7 +53,7 @@ func snapshotOf(g *stack.Graph, names []string) *reshapeSnapshot { continue } seen[i] = true - ss := snapshotStack{Trunk: s.Trunk, Worktree: s.Worktree, ID: s.ID, Number: s.Number} + ss := snapshotStack{Trunk: s.Trunk, ID: s.ID, Number: s.Number} for _, b := range s.Branches { ss.Branches = append(ss.Branches, snapshotBranch{Name: b.Name, Base: b.Base}) } @@ -181,7 +179,7 @@ func (a *App) restoreReshape(ctx context.Context, repo git.Repo, snap *reshapeSn continue // every branch of it is gone; nothing to put back } s := &g.Stacks[homes[k]] - s.Trunk, s.Worktree, s.ID, s.Number = ss.Trunk, ss.Worktree, ss.ID, ss.Number + s.Trunk, s.ID, s.Number = ss.Trunk, ss.ID, ss.Number if ss.ID != "" { ids = append(ids, ss.ID) } diff --git a/pkg/app/reshape_internal_test.go b/pkg/app/reshape_internal_test.go index 3b1cb1b..6890dd4 100644 --- a/pkg/app/reshape_internal_test.go +++ b/pkg/app/reshape_internal_test.go @@ -143,7 +143,7 @@ func trunksAndNames(g *stack.Graph) []string { func TestRestoreReshapePutsAStackBackOnItsTrunk(t *testing.T) { g := stack.NewGraph([]stack.Stack{ - {Trunk: "main", Worktree: "/w", ID: "S1", Number: 1, Branches: []stack.Branch{{Name: "a", Base: "m"}, {Name: "b", Base: "ta"}}}, + {Trunk: "main", ID: "S1", Number: 1, Branches: []stack.Branch{{Name: "a", Base: "m"}, {Name: "b", Base: "ta"}}}, }) snap := snapshotOf(g, []string{"a"}) snap.Created = []string{"a"} @@ -159,7 +159,7 @@ func TestRestoreReshapePutsAStackBackOnItsTrunk(t *testing.T) { if want := []string{"main:a,b"}; !slices.Equal(trunksAndNames(got), want) { t.Fatalf("stacks = %v, want %v", trunksAndNames(got), want) } - if s := got.Stacks[0]; s.Worktree != "/w" || s.ID != "S1" || s.Branches[0].Base != "m" || s.Branches[0].Head != "na" { + if s := got.Stacks[0]; s.ID != "S1" || s.Branches[0].Base != "m" || s.Branches[0].Head != "na" { t.Errorf("stack = %+v", s) } } diff --git a/pkg/app/restack_state.go b/pkg/app/restack_state.go index 1ca29a6..dd9a20c 100644 --- a/pkg/app/restack_state.go +++ b/pkg/app/restack_state.go @@ -140,6 +140,15 @@ func clearRestackState(repo git.Repo) error { // rebase recorded for this worktree, which a user who upgraded mid // conflict may still have. func ghRebaseStateExists(repo git.Repo) bool { - _, err := os.Stat(filepath.Join(repo.GitDir, "gh-stack-rebase-state")) - return err == nil + // gh stack 0.3 keeps it with its catalog in the common dir; older + // versions kept it in the worktree's own git dir. + for _, dir := range []string{repo.CommonDir, repo.GitDir} { + if dir == "" { + continue + } + if _, err := os.Stat(filepath.Join(dir, "gh-stack-rebase-state")); err == nil { + return true + } + } + return false } diff --git a/pkg/app/sync_cleanup_test.go b/pkg/app/sync_cleanup_test.go index e490416..33a6340 100644 --- a/pkg/app/sync_cleanup_test.go +++ b/pkg/app/sync_cleanup_test.go @@ -226,8 +226,8 @@ func TestSyncKeepsEmptyBranchAtTrunkTip(t *testing.T) { f := newSyncFixture(t) gittest.Run(t, f.dir, "branch", "empty", "main") f.deps.Meta = memMeta{stack.NewGraph([]stack.Stack{ - {Trunk: "main", Branches: []stack.Branch{{Name: "a"}, {Name: "b"}}, Worktree: f.repo.TopLevel}, - {Trunk: "main", Branches: []stack.Branch{{Name: "empty"}}, Worktree: f.repo.TopLevel}, + {Trunk: "main", Branches: []stack.Branch{{Name: "a"}, {Name: "b"}}}, + {Trunk: "main", Branches: []stack.Branch{{Name: "empty"}}}, })} res, err := f.sync(t, app.SyncOptions{NoRestack: true}) if err != nil || len(res.Deleted) != 0 || !branchExists(t, f.dir, "empty") { @@ -243,7 +243,7 @@ func TestSyncDeletesBranchFastForwardedIntoTrunk(t *testing.T) { gittest.Run(t, f.dir, "push", "-q", "origin", "main") gittest.Run(t, f.dir, "switch", "-q", "b") f.deps.Meta = memMeta{stack.NewGraph([]stack.Stack{ - {Trunk: "main", Branches: []stack.Branch{{Name: "a", Base: base}, {Name: "b", Base: f.rev(t, "a")}}, Worktree: f.repo.TopLevel}, + {Trunk: "main", Branches: []stack.Branch{{Name: "a", Base: base}, {Name: "b", Base: f.rev(t, "a")}}}, })} res, err := f.sync(t, app.SyncOptions{NoRestack: true}) if err != nil { @@ -262,8 +262,8 @@ func TestSyncKeepsEmptyBranchWhoseBaseIsItsTip(t *testing.T) { tip := f.rev(t, "main") gittest.Run(t, f.dir, "branch", "empty", "main") f.deps.Meta = memMeta{stack.NewGraph([]stack.Stack{ - {Trunk: "main", Branches: []stack.Branch{{Name: "a"}, {Name: "b"}}, Worktree: f.repo.TopLevel}, - {Trunk: "main", Branches: []stack.Branch{{Name: "empty", Base: tip}}, Worktree: f.repo.TopLevel}, + {Trunk: "main", Branches: []stack.Branch{{Name: "a"}, {Name: "b"}}}, + {Trunk: "main", Branches: []stack.Branch{{Name: "empty", Base: tip}}}, })} res, err := f.sync(t, app.SyncOptions{NoRestack: true}) if err != nil || len(res.Deleted) != 0 || !branchExists(t, f.dir, "empty") { @@ -280,7 +280,7 @@ func TestSyncRestackKeepsOlderBaseOfBranchOnItsParent(t *testing.T) { gittest.Run(t, f.dir, "switch", "-q", "b") f.deps.Config.SyncPrune = config.SyncPruneNever // cleanup declines, so restack sees the branch f.deps.Meta = memMeta{stack.NewGraph([]stack.Stack{ - {Trunk: "main", Branches: []stack.Branch{{Name: "a", Base: base}, {Name: "b", Base: f.rev(t, "a")}}, Worktree: f.repo.TopLevel}, + {Trunk: "main", Branches: []stack.Branch{{Name: "a", Base: base}, {Name: "b", Base: f.rev(t, "a")}}}, })} if _, err := f.sync(t, app.SyncOptions{}); err != nil { t.Fatal(err) @@ -506,8 +506,8 @@ func TestSyncDropsGoneBranchesFromMetadata(t *testing.T) { f := newSyncFixture(t) f.deps.Config.SyncPrune = config.SyncPruneNever f.deps.Meta = memMeta{stack.NewGraph([]stack.Stack{ - {Trunk: "main", Branches: []stack.Branch{{Name: "a"}, {Name: "b"}}, Worktree: f.repo.TopLevel}, - {Trunk: "main", Branches: []stack.Branch{{Name: "gone", PR: &stack.PRRef{Number: 3, Merged: true}}}, Worktree: f.repo.TopLevel}, + {Trunk: "main", Branches: []stack.Branch{{Name: "a"}, {Name: "b"}}}, + {Trunk: "main", Branches: []stack.Branch{{Name: "gone", PR: &stack.PRRef{Number: 3, Merged: true}}}}, })} if _, err := f.sync(t, app.SyncOptions{NoRestack: true}); err != nil { t.Fatal(err) diff --git a/pkg/app/sync_restack_test.go b/pkg/app/sync_restack_test.go index b0d1388..826a8a2 100644 --- a/pkg/app/sync_restack_test.go +++ b/pkg/app/sync_restack_test.go @@ -136,8 +136,8 @@ func TestSyncConflictStopsOneStackOnly(t *testing.T) { gittest.Commit(t, f.dir, "README.md", "c version", "c clashes") gittest.Run(t, f.dir, "switch", "-q", "main") f.deps.Meta = memMeta{stack.NewGraph([]stack.Stack{ - {Trunk: "main", Branches: []stack.Branch{{Name: "a"}, {Name: "b"}}, Worktree: f.repo.TopLevel}, - {Trunk: "main", Branches: []stack.Branch{{Name: "c"}}, Worktree: f.repo.TopLevel}, + {Trunk: "main", Branches: []stack.Branch{{Name: "a"}, {Name: "b"}}}, + {Trunk: "main", Branches: []stack.Branch{{Name: "c"}}}, })} cBefore := f.rev(t, "c") f.advanceRemote(t, "main", "README.md") // conflicts with c @@ -207,8 +207,8 @@ func TestSyncPlanningErrorFailsOneStack(t *testing.T) { gittest.Commit(t, f.dir, "c.txt", "c", "feat: c") gittest.Run(t, f.dir, "switch", "-q", "main") f.deps.Meta = memMeta{stack.NewGraph([]stack.Stack{ - {Trunk: "main", Branches: []stack.Branch{{Name: "a"}, {Name: "b"}}, Worktree: f.repo.TopLevel}, - {Trunk: "nope", Branches: []stack.Branch{{Name: "c"}}, Worktree: f.repo.TopLevel}, + {Trunk: "main", Branches: []stack.Branch{{Name: "a"}, {Name: "b"}}}, + {Trunk: "nope", Branches: []stack.Branch{{Name: "c"}}}, })} f.advanceRemote(t, "main", "r.txt") res, err := f.sync(t, app.SyncOptions{}) diff --git a/pkg/app/sync_test.go b/pkg/app/sync_test.go index 8bdedf5..10aa118 100644 --- a/pkg/app/sync_test.go +++ b/pkg/app/sync_test.go @@ -46,7 +46,7 @@ func newSyncFixture(t *testing.T) *syncFixture { if err != nil { t.Fatal(err) } - graph := stack.NewGraph([]stack.Stack{{Trunk: "main", Branches: []stack.Branch{{Name: "a"}, {Name: "b"}}, Worktree: repo.TopLevel}}) + graph := stack.NewGraph([]stack.Stack{{Trunk: "main", Branches: []stack.Branch{{Name: "a"}, {Name: "b"}}}}) fg := &recordingForge{prs: []forge.PullRequest{{Number: 7, Head: "a", Base: "main", State: forge.StateOpen, URL: "u/a"}}} cfg := config.Defaults() cfg.CacheTTL = 0 diff --git a/pkg/backend/ghstack/backend.go b/pkg/backend/ghstack/backend.go index 340c613..70f6bb3 100644 --- a/pkg/backend/ghstack/backend.go +++ b/pkg/backend/ghstack/backend.go @@ -1,15 +1,16 @@ package ghstack import ( + "bytes" "context" "errors" "fmt" "io" "io/fs" + "maps" "os" "path/filepath" "slices" - "strings" "time" "github.com/DomBlack/git-stack/pkg/exec" @@ -46,321 +47,365 @@ func New(r exec.Runner, g *git.Client, opts ...Option) *Backend { // Compile-time port checks. var _ stack.Metadata = (*Backend)(nil) -// LockName is the file gh stack flocks while it edits its metadata. +// LockName is the file gh stack flocks while it edits its catalog. const LockName = "gh-stack.lock" +// OperationLockName is the file gh stack flocks for the whole of a command +// that changes stacks. We take it only while moving per-worktree files into +// the shared catalog, which gh stack does under it too. +const OperationLockName = "gh-stack-operation.lock" + +// migrationJournal is the file gh stack writes while it moves per-worktree +// files into the shared catalog itself, and removes once it is done. While +// it exists the catalog must be left alone for gh stack to finish. +const migrationJournal = "gh-stack-migration" + +// BackupSuffix names the copy of a per-worktree file kept when it is moved +// into the shared catalog, the same name gh stack uses. +const BackupSuffix = ".pre-worktree-migration" + +// recoveryFiles are gh stack's records of an interrupted rebase or modify. +// One left in a worktree's git dir needs that worktree's own file, so its +// file is not moved while one is there. +var recoveryFiles = []string{"gh-stack-rebase-state", "gh-stack-modify-state"} + // lockWait is how long Update waits for the metadata lock. var lockWait = 5 * time.Second -// source is one metadata file: the git dir holding it and the working tree -// it belongs to. -type source struct{ gitDir, worktree string } +// catalogDir is where gh stack keeps its catalog: the common git dir, so +// every worktree of the repository shares one file. +func catalogDir(repo git.Repo) string { + if repo.CommonDir != "" { + return repo.CommonDir + } + return repo.GitDir +} + +// catalogPath is the shared catalog file. +func catalogPath(repo git.Repo) string { return filepath.Join(catalogDir(repo), FileName) } -// sources lists the current worktree's git dir first, then the main -// checkout's, then every linked worktree's. -func sources(repo git.Repo) []source { - out := []source{{repo.GitDir, repo.TopLevel}} - if repo.CommonDir != repo.GitDir { - out = append(out, source{repo.CommonDir, filepath.Dir(repo.CommonDir)}) +// LegacyFiles lists the per-worktree files gh stack 0.2 and older kept in +// each linked worktree's git dir (and git-stack wrote there too), the +// current worktree's first. gh stack 0.3 moved to one shared catalog; these +// are merged into it by the next Update. +func LegacyFiles(repo git.Repo) []string { + var out []string + common := catalogDir(repo) + if repo.GitDir != "" && repo.GitDir != common { + if _, err := os.Stat(filepath.Join(repo.GitDir, FileName)); err == nil { + out = append(out, filepath.Join(repo.GitDir, FileName)) + } } - if entries, err := os.ReadDir(filepath.Join(repo.CommonDir, "worktrees")); err == nil { - for _, e := range entries { - if !e.IsDir() { - continue - } - gitDir := filepath.Join(repo.CommonDir, "worktrees", e.Name()) - if gitDir == repo.GitDir { - continue - } - out = append(out, source{gitDir, worktreePath(gitDir)}) + entries, err := os.ReadDir(filepath.Join(common, "worktrees")) + if err != nil { + return out + } + for _, e := range entries { + if !e.IsDir() { + continue + } + path := filepath.Join(common, "worktrees", e.Name(), FileName) + if slices.Contains(out, path) { + continue + } + if fi, err := os.Stat(path); err == nil && fi.Mode().IsRegular() { + out = append(out, path) } } return out } -// loaded is one metadata file as read. -type loaded struct { - src source - data []byte - f *file +// MigrationPending reports whether gh stack itself is part way through +// moving per-worktree files into the shared catalog. +func MigrationPending(repo git.Repo) bool { + _, err := os.Lstat(filepath.Join(catalogDir(repo), migrationJournal)) + return err == nil } -// recRef locates a stack record: which loaded file and which index in it. -type recRef struct{ file, rec int } - -// reading is every metadata file plus the merged graph and, for each graph -// stack, where its record lives. -type reading struct { - files []loaded +// catalog is the shared catalog with any per-worktree files merged in, as +// one tree whose stack records match graph's stacks one for one. +type catalog struct { + root *node graph *stack.Graph - where []recRef + // legacy are the per-worktree files merged in. + legacy []string } -func (b *Backend) read(repo git.Repo) (*reading, error) { - r := &reading{} - var stacks []stack.Stack +// readCatalog reads the shared catalog, then each per-worktree file, and +// merges them: a stack record joins when none of its branches is in a +// record already taken, so the shared catalog wins over the per-worktree +// files and the current worktree's file over the others. A record left +// out is not lost; the file it was in is kept as a backup when it is moved. +// The members of the shared catalog's root are kept, with the repository +// taken from a per-worktree file when the shared one has none. +func readCatalog(repo git.Repo) (*catalog, error) { + c := &catalog{} + paths := append([]string{catalogPath(repo)}, LegacyFiles(repo)...) + var recs []*node seen := map[string]bool{} - for _, src := range sources(repo) { - data, err := os.ReadFile(filepath.Join(src.gitDir, FileName)) + for i, path := range paths { + data, err := os.ReadFile(path) if err != nil { if errors.Is(err, fs.ErrNotExist) { continue } return nil, err } - f, err := parseFile(data) + if _, err := parseFile(data); err != nil { + return nil, fmt.Errorf("%s: %w", path, err) + } + root, err := parseTree(data) if err != nil { - return nil, fmt.Errorf("%s: %w", filepath.Join(src.gitDir, FileName), err) + return nil, fmt.Errorf("%s: %w", path, err) + } + if i == 0 { + c.root = root + } else { + c.legacy = append(c.legacy, path) + if c.root == nil { + c.root = emptyRoot() + } + if c.root.get("repository") == nil && root.get("repository") != nil { + c.root.set("repository", root.get("repository")) + } + } + stacksNode := root.get("stacks") + if stacksNode == nil || stacksNode.kind != '[' { + continue } - fi := len(r.files) - r.files = append(r.files, loaded{src: src, data: data, f: f}) next: - for ri, s := range f.toGraph().Stacks { - for _, br := range s.Branches { - if seen[br.Name] { + for _, rec := range stacksNode.items { + var names []string + if bn := rec.get("branches"); bn != nil && bn.kind == '[' { + for _, item := range bn.items { + names = append(names, item.str("branch")) + } + } + for _, n := range names { + if seen[n] { continue next } } - for _, br := range s.Branches { - seen[br.Name] = true + for _, n := range names { + seen[n] = true } - s.Worktree = src.worktree - stacks = append(stacks, s) - r.where = append(r.where, recRef{fi, ri}) + recs = append(recs, rec) } } - r.graph = stack.NewGraph(stacks) - return r, nil + if c.root == nil { + c.root = emptyRoot() + } + c.root.set("stacks", arrayNode(recs...)) + if err := c.regraph(); err != nil { + return nil, err + } + return c, nil +} + +// regraph rebuilds graph from root. +func (c *catalog) regraph() error { + data, err := c.root.encode() + if err != nil { + return err + } + f, err := parseFile(data) + if err != nil { + return err + } + c.graph = f.toGraph() + return nil +} + +func emptyRoot() *node { + return objectNode(member{"schemaVersion", intNode(SchemaVersion)}, member{"stacks", arrayNode()}) } -// Load implements stack.Metadata. gh stack keeps its file per worktree -// (/gh-stack), so this reads the current worktree's file first, then -// the main checkout's and every linked worktree's, and merges them so one view -// covers everything checked out on the machine. A missing file means no -// stacks; it never runs gh. A branch that appears in two files keeps its first -// stack. +// Load implements stack.Metadata. It reads gh stack's shared catalog +// (/gh-stack) and merges in any per-worktree file left from gh +// stack 0.2 (see readCatalog), so stacks show up the same before and after +// they are moved. A missing file means no stacks. It never writes and never +// runs gh. func (b *Backend) Load(_ context.Context, repo git.Repo) (*stack.Graph, error) { - r, err := b.read(repo) + c, err := readCatalog(repo) if err != nil { return nil, err } - return r.graph, nil + return c.graph, nil } -// Update implements stack.Metadata. It takes gh stack's lock in every git -// dir that has a metadata file, re-reads them, runs fn on the merged graph -// and writes each file back, editing the JSON in place so members we don't -// model, PR records and ordering survive. A file whose stacks fn didn't -// change is not rewritten. fn may also append stacks (each with a trunk and -// at least one branch; they are written to the current worktree's file, -// created if need be, since that is the only one gh stack reads there) and -// may set or clear a stack's ID and Number (clearing removes them from the -// record). It must not remove or reorder existing stacks; a stack left with -// no branches is removed. +// Update implements stack.Metadata. It takes gh stack's lock, re-reads the +// catalog, runs fn on the graph and writes the catalog back, editing the +// JSON in place so members we don't model, PR records and ordering +// survive. Nothing is written when nothing changed. fn may also append +// stacks (each with a trunk and at least one branch) and may set or clear +// a stack's ID and Number (clearing removes them from the record). It must +// not remove or reorder existing stacks; a stack left with no branches is +// removed. +// +// Any per-worktree file left from gh stack 0.2 is merged into the catalog +// and then moved aside (see Migrate), under gh stack's operation lock too. func (b *Backend) Update(ctx context.Context, repo git.Repo, fn func(*stack.Graph) error) error { - unlock, err := lockAll(ctx, repo) + unlock, err := lockCatalog(ctx, repo) if err != nil { return err } defer unlock() - r, err := b.read(repo) + c, err := readCatalog(repo) + if err != nil { + return err + } + if err := checkMovable(repo, c.legacy); err != nil { + return err + } + before, err := c.root.encode() if err != nil { return err } - count := len(r.graph.Stacks) - if err := fn(r.graph); err != nil { + count := len(c.graph.Stacks) + if err := fn(c.graph); err != nil { return err } - if len(r.graph.Stacks) < count { + if len(c.graph.Stacks) < count { return fmt.Errorf("%s: Update may not remove stacks", FileName) } - added := r.graph.Stacks[count:] - target := -1 - if len(added) > 0 { - for _, s := range added { - if s.Trunk == "" || len(s.Branches) == 0 { - return fmt.Errorf("%s: a new stack needs a trunk and at least one branch", FileName) - } + added := c.graph.Stacks[count:] + for _, s := range added { + if s.Trunk == "" || len(s.Branches) == 0 { + return fmt.Errorf("%s: a new stack needs a trunk and at least one branch", FileName) } - target = r.fileFor(repo) } - roots, pool, err := r.parseAll() + c.apply(added) + after, err := c.root.encode() if err != nil { return err } - for fi := range r.files { - var extra []stack.Stack - if fi == target { - extra = added - } - if err := b.writeBack(r, fi, roots[fi], pool, extra); err != nil { - return err + if !bytes.Equal(before, after) || len(c.legacy) > 0 { + if _, err := os.Stat(catalogPath(repo)); err == nil || len(c.graph.Stacks) > 0 || len(c.legacy) > 0 { + if err := writeBytes(catalogPath(repo), after); err != nil { + return err + } } } - return nil + return retire(c.legacy) } -// lockAll takes gh stack's lock in every git dir that has a metadata file, -// and in the current one even without a file yet, since a new stack may -// create its file there and two writers must not race it. Dirs are locked -// in sorted order so two of us never deadlock. -func lockAll(ctx context.Context, repo git.Repo) (func(), error) { - var dirs []string - for _, src := range sources(repo) { - if _, err := os.Stat(filepath.Join(src.gitDir, FileName)); err == nil { - dirs = append(dirs, src.gitDir) - } - } - if !slices.Contains(dirs, repo.GitDir) { - dirs = append(dirs, repo.GitDir) +// Migrate merges any per-worktree file left from gh stack 0.2 into the +// shared catalog and moves each aside to .pre-worktree-migration +// (numbered when gh stack already left a backup by that name, so a stale +// one never blocks it), and reports the files it moved. gh stack 0.3 does +// the same on its next command but refuses when a backup it made earlier +// differs from the file, which git-stack's own writes to these files used +// to cause. It is a no-op when there is nothing to move. +func (b *Backend) Migrate(ctx context.Context, repo git.Repo) ([]string, error) { + legacy := LegacyFiles(repo) + if len(legacy) == 0 { + return nil, nil + } + if err := b.Update(ctx, repo, func(*stack.Graph) error { return nil }); err != nil { + return nil, err } - slices.Sort(dirs) + return legacy, nil +} + +// lockCatalog takes gh stack's catalog lock, and before it the operation +// lock when per-worktree files are waiting to be moved, the order gh stack +// takes them in. +func lockCatalog(ctx context.Context, repo git.Repo) (func(), error) { + dir := catalogDir(repo) var unlocks []func() - unlockAll := func() { + release := func() { for _, u := range slices.Backward(unlocks) { u() } } - for _, dir := range dirs { - unlock, err := lock(ctx, filepath.Join(dir, LockName), lockWait) + names := []string{LockName} + if len(LegacyFiles(repo)) > 0 { + names = []string{OperationLockName, LockName} + } + for _, name := range names { + unlock, err := lock(ctx, filepath.Join(dir, name), lockWait) if err != nil { - unlockAll() + release() return nil, err } unlocks = append(unlocks, unlock) } - return unlockAll, nil + return release, nil } -// adopt moves the record of the stack holding branch into the current -// worktree's file when it is kept in another worktree's (it was started -// there, or put in the main checkout's file by an older git-stack). gh -// stack only reads the file in the git dir it runs in, so without this it -// says the branch is not in a stack. The record moves as it is, pull -// request records and members we don't model included. A branch in no -// stack is left alone for gh stack to report. -func (b *Backend) adopt(ctx context.Context, repo git.Repo, branch string) error { - if branch == "" { - return nil - } - unlock, err := lockAll(ctx, repo) - if err != nil { - return err +// checkMovable refuses to move per-worktree files while gh stack is part +// way through moving them itself, or while one of their worktrees has an +// interrupted gh stack rebase or modify that still needs its own file. +func checkMovable(repo git.Repo, legacy []string) error { + if MigrationPending(repo) { + return stack.New(stack.KindStateMigration, "gh stack is part way through moving its stacks into one shared file"). + WithSteps("run `gh stack view` to let it finish, then try again", + "or run `git stack doctor` to see what is in the way") } - defer unlock() - r, err := b.read(repo) - if err != nil { - return err - } - gi := slices.IndexFunc(r.graph.Stacks, func(s stack.Stack) bool { return s.Index(branch) >= 0 }) - if gi < 0 { - return nil - } - from := r.where[gi] - if r.files[from.file].src.gitDir == repo.GitDir { - return nil - } - to := r.fileFor(repo) - src, err := parseTree(r.files[from.file].data) - if err != nil { - return err - } - dst, err := parseTree(r.files[to].data) - if err != nil { - return err - } - srcStacks := src.get("stacks") - rec := srcStacks.items[from.rec] - srcStacks.items = slices.Delete(srcStacks.items, from.rec, from.rec+1) - dstStacks := dst.get("stacks") - if dstStacks == nil || dstStacks.kind != '[' { - dstStacks = arrayNode() - dst.set("stacks", dstStacks) - } - dstStacks.items = append(dstStacks.items, rec) - // The new home first: should the second write fail, the stack is in - // both files and the current worktree's copy is the one read first. - if err := writeFile(r.files[to].src.gitDir, dst); err != nil { - return err + for _, path := range legacy { + dir := filepath.Dir(path) + for _, name := range recoveryFiles { + if _, err := os.Lstat(filepath.Join(dir, name)); err == nil { + return stack.Newf(stack.KindStateMigration, "a gh stack rebase or modify was interrupted in the worktree of %s, so its stacks can't be moved into the shared file yet", dir). + WithSteps("in that worktree, run `gh stack rebase --continue` (or `--abort`), or `gh stack modify --continue` (or `--abort`)") + } + } } - return writeFile(r.files[from.file].src.gitDir, src) + return nil } -// parseAll parses every loaded file and collects the node of every branch -// in a record the graph came from, by name. Branch names are unique across -// stacks, so a branch fn moved to another record, or to another file, takes -// its node with it and keeps its pull request record and the members we -// don't model. -func (r *reading) parseAll() ([]*node, map[string]*node, error) { - roots := make([]*node, len(r.files)) - pool := map[string]*node{} - for fi, f := range r.files { - root, err := parseTree(f.data) +// retire moves each per-worktree file aside, once the shared catalog holds +// its stacks: to .pre-worktree-migration, or the first free +// .pre-worktree-migration. when that name is taken by a backup that +// differs. +func retire(legacy []string) error { + for _, path := range legacy { + data, err := os.ReadFile(path) if err != nil { - return nil, nil, err - } - roots[fi] = root - stacksNode := root.get("stacks") - if stacksNode == nil || stacksNode.kind != '[' { - continue + if errors.Is(err, fs.ErrNotExist) { + continue + } + return err } - for ri, rec := range stacksNode.items { - if !slices.Contains(r.where, recRef{fi, ri}) { - continue // a duplicate Load skipped + backup := path + BackupSuffix + for n := 1; ; n++ { + old, err := os.ReadFile(backup) + if errors.Is(err, fs.ErrNotExist) { + break } - branchesNode := rec.get("branches") - if branchesNode == nil || branchesNode.kind != '[' { - continue + if err != nil { + return err } - for _, item := range branchesNode.items { - pool[item.str("branch")] = item + if bytes.Equal(old, data) { + break } + backup = fmt.Sprintf("%s%s.%d", path, BackupSuffix, n) } - } - return roots, pool, nil -} - -// fileFor picks the file new stacks are written to: the current worktree's, -// created when it has none yet. gh stack only reads the file in the git dir -// it runs in, so a stack written anywhere else would be invisible to it from -// here (in the main checkout that is the common dir). -func (r *reading) fileFor(repo git.Repo) int { - for i, f := range r.files { - if f.src.gitDir == repo.GitDir { - return i + if err := os.Rename(path, backup); err != nil { + return err } } - r.files = append(r.files, loaded{ - src: source{repo.GitDir, repo.TopLevel}, - data: []byte("{\n \"schemaVersion\": 1,\n \"stacks\": []\n}\n"), - f: &file{SchemaVersion: SchemaVersion}, - }) - return len(r.files) - 1 + return nil } -// writeBack applies the graph's edits to the fi'th file, parsed as root, -// and rewrites it when anything changed. Each branch is written from its -// node in pool when it has one, wherever it came from. -func (b *Backend) writeBack(r *reading, fi int, root *node, pool map[string]*node, added []stack.Stack) error { - stacksNode := root.get("stacks") - if stacksNode == nil && len(added) > 0 { - stacksNode = arrayNode() - root.set("stacks", stacksNode) - } - if stacksNode == nil || stacksNode.kind != '[' { - return nil +// apply writes the graph's edits into root: each record takes its stack's +// id, number, trunk and branches (each branch from its node wherever it was +// before, so its pull request record and members we don't model travel with +// it), a stack left with no branches goes, and added stacks are appended. +func (c *catalog) apply(added []stack.Stack) { + stacksNode := c.root.get("stacks") + pool := map[string]*node{} + for _, rec := range stacksNode.items { + if bn := rec.get("branches"); bn != nil && bn.kind == '[' { + for _, item := range bn.items { + pool[item.str("branch")] = item + } + } } - changed := false var keep []*node for ri, rec := range stacksNode.items { - gi := slices.Index(r.where, recRef{fi, ri}) - if gi < 0 { - keep = append(keep, rec) // a duplicate Load skipped; leave it be - continue - } - gs := r.graph.Stacks[gi] + gs := c.graph.Stacks[ri] if len(gs.Branches) == 0 { - changed = true continue } // A zero id or number is removed rather than written as "" or 0, @@ -368,59 +413,29 @@ func (b *Backend) writeBack(r *reading, fi int, root *node, pool map[string]*nod switch { case gs.ID == "" && rec.get("id") != nil: rec.del("id") - changed = true case gs.ID != "" && rec.str("id") != gs.ID: rec.set("id", stringNode(gs.ID)) - changed = true } switch { case gs.Number == 0 && rec.get("number") != nil: rec.del("number") - changed = true case gs.Number != 0 && rec.num("number") != gs.Number: rec.set("number", intNode(gs.Number)) - changed = true } if gs.Trunk != "" { if tn := rec.get("trunk"); tn == nil || tn.kind != '{' { rec.set("trunk", objectNode(member{"branch", stringNode(gs.Trunk)})) - changed = true } else if tn.str("branch") != gs.Trunk { tn.set("branch", stringNode(gs.Trunk)) // keeps the recorded head - changed = true } } - branchesNode := rec.get("branches") - if branchesNode == nil || branchesNode.kind != '[' { - branchesNode = arrayNode() - rec.set("branches", branchesNode) - } - kept, edited := branchNodes(gs.Branches, pool) - if edited || !slices.Equal(kept, branchesNode.items) { - changed = true - } - branchesNode.items = kept + rec.set("branches", arrayNode(branchNodes(gs.Branches, pool)...)) keep = append(keep, rec) } for _, s := range added { keep = append(keep, stackNode(s, pool)) - changed = true - } - if !changed { - return nil } stacksNode.items = keep - return writeFile(r.files[fi].src.gitDir, root) -} - -// writeFile replaces the metadata file in gitDir with root, atomically and -// keeping the old file's permissions. -func writeFile(gitDir string, root *node) error { - out, err := root.encode() - if err != nil { - return err - } - return writeBytes(filepath.Join(gitDir, FileName), out) } // writeBytes replaces path with out atomically, keeping the mode of the @@ -455,34 +470,28 @@ func writeBytes(path string, out []byte) error { // branchNodes renders a stack's branches, reusing each branch's node from // pool so its pull request record and unmodelled members travel with it -// (under its old name for a renamed branch), and reports whether any name, -// head or base it wrote differs from the node. -func branchNodes(branches []stack.Branch, pool map[string]*node) ([]*node, bool) { - edited := false +// (under its old name for a renamed branch). +func branchNodes(branches []stack.Branch, pool map[string]*node) []*node { out := make([]*node, 0, len(branches)) for _, gb := range branches { n := pool[gb.Name] if n == nil && gb.RenamedFrom != "" { if n = pool[gb.RenamedFrom]; n != nil { n.set("branch", stringNode(gb.Name)) - edited = true } } if n == nil { n = objectNode(member{"branch", stringNode(gb.Name)}) - edited = true } if gb.Head != "" && n.str("head") != gb.Head { n.set("head", stringNode(gb.Head)) - edited = true } if gb.Base != "" && n.str("base") != gb.Base { n.set("base", stringNode(gb.Base)) - edited = true } out = append(out, n) } - return out, edited + return out } // stackNode is a new stack record: its id and number when it has them (a @@ -498,21 +507,10 @@ func stackNode(s stack.Stack, pool map[string]*node) *node { rec.set("number", intNode(s.Number)) } rec.set("trunk", objectNode(member{"branch", stringNode(s.Trunk)})) - branches, _ := branchNodes(s.Branches, pool) - rec.set("branches", arrayNode(branches...)) + rec.set("branches", arrayNode(branchNodes(s.Branches, pool)...)) return rec } -// worktreePath resolves a linked worktree's working tree from its git dir: -// /worktrees//gitdir holds the path of the worktree's .git file. -func worktreePath(gitDir string) string { - data, err := os.ReadFile(filepath.Join(gitDir, "gitdir")) - if err != nil { - return "" - } - return filepath.Dir(strings.TrimSpace(string(data))) -} - // ghEnv keeps gh-stack non-interactive and its output plain when captured. var ghEnv = []string{ "GH_NO_UPDATE_NOTIFIER=1", @@ -524,8 +522,6 @@ var ghEnv = []string{ // ghUnset removes variables that would re-enable prompts or TUIs. var ghUnset = []string{"GH_FORCE_TTY", "CLICOLOR_FORCE"} -// gh runs `gh stack ` captured, mapping failures to *stack.Error. -// The result carries gh-stack's output for commands that relay it. // gh runs `gh stack ` captured and maps failures to stack.Error. func (b *Backend) gh(ctx context.Context, repo git.Repo, args ...string) error { _, err := b.ghRun(ctx, repo, nil, args...) @@ -534,7 +530,17 @@ func (b *Backend) gh(ctx context.Context, repo git.Repo, args ...string) error { // ghRun is gh with the captured stderr also relayed live to stream when it // is non-nil (the long submit and sync commands pass the WithOutput writer). +// +// gh stack 0.3 moves per-worktree files into its shared catalog itself, but +// stops for good when a backup it made earlier differs from the file. So +// git-stack moves them first (see Migrate), unless gh stack is part way +// through doing it, which it is left to finish. func (b *Backend) ghRun(ctx context.Context, repo git.Repo, stream io.Writer, args ...string) (exec.Result, error) { + if !MigrationPending(repo) { + if _, err := b.Migrate(ctx, repo); err != nil { + return exec.Result{}, err + } + } res, err := b.run.Run(ctx, exec.Cmd{ Name: "gh", Args: append([]string{"stack"}, args...), @@ -549,75 +555,45 @@ func (b *Backend) ghRun(ctx context.Context, repo git.Repo, stream io.Writer, ar return res, nil } -// lockDirs takes gh stack's lock in each of dirs, in sorted order so two of -// us never deadlock. The returned function releases them all. -func lockDirs(ctx context.Context, dirs []string) (func(), error) { - dirs = slices.Clone(dirs) - slices.Sort(dirs) - dirs = slices.Compact(dirs) - var unlocks []func() - release := func() { - for _, u := range slices.Backward(unlocks) { - u() - } - } - for _, dir := range dirs { - unlock, err := lock(ctx, filepath.Join(dir, LockName), lockWait) - if err != nil { - release() - return nil, err - } - unlocks = append(unlocks, unlock) - } - return release, nil -} - -// Snapshot implements stack.Metadata. It reads every metadata file Load -// would (the current worktree's, the main checkout's and every linked -// worktree's) under the lock, recording the ones that don't exist as empty -// so Restore can remove them again. +// Snapshot implements stack.Metadata. It moves any per-worktree file into +// the shared catalog first (see Migrate), so a snapshot is always the one +// file, then reads it under the lock, recording a missing file as empty so +// Restore can remove it again. func (b *Backend) Snapshot(ctx context.Context, repo git.Repo) (stack.Snapshot, error) { - var dirs []string - for _, src := range sources(repo) { - dirs = append(dirs, src.gitDir) + if !MigrationPending(repo) { + if _, err := b.Migrate(ctx, repo); err != nil { + return stack.Snapshot{}, err + } } - unlock, err := lockDirs(ctx, append(slices.Clone(dirs), repo.GitDir)) + unlock, err := lock(ctx, filepath.Join(catalogDir(repo), LockName), lockWait) if err != nil { return stack.Snapshot{}, err } defer unlock() - snap := stack.Snapshot{Files: map[string][]byte{}} - for _, dir := range dirs { - path := filepath.Join(dir, FileName) - data, err := os.ReadFile(path) - if err != nil && !errors.Is(err, fs.ErrNotExist) { - return stack.Snapshot{}, err - } - snap.Files[path] = data + path := catalogPath(repo) + data, err := os.ReadFile(path) + if err != nil && !errors.Is(err, fs.ErrNotExist) { + return stack.Snapshot{}, err } - return snap, nil + return stack.Snapshot{Files: map[string][]byte{path: data}}, nil } // Restore implements stack.Metadata. It writes each file in the snapshot -// back exactly, under the lock, and removes the ones that were empty (did not -// exist) when it was taken. A file whose git dir has gone since (a removed -// worktree) is skipped, and files the snapshot doesn't know about are left -// alone. +// back exactly, under the lock, and removes the ones that were empty (did +// not exist) when it was taken. A snapshot taken by an older git-stack may +// name per-worktree files too; they are put back as they were and moved +// into the shared catalog again by the next command. A file whose git dir +// has gone since (a removed worktree) is skipped. func (b *Backend) Restore(ctx context.Context, repo git.Repo, s stack.Snapshot) error { - var dirs []string - for path := range s.Files { - dir := filepath.Dir(path) - if _, err := os.Stat(dir); err == nil { - dirs = append(dirs, dir) - } - } - unlock, err := lockDirs(ctx, append(slices.Clone(dirs), repo.GitDir)) + unlock, err := lock(ctx, filepath.Join(catalogDir(repo), LockName), lockWait) if err != nil { return err } defer unlock() - for _, dir := range dirs { - path := filepath.Join(dir, FileName) + for _, path := range slices.Sorted(maps.Keys(s.Files)) { + if _, err := os.Stat(filepath.Dir(path)); err != nil { + continue + } data := s.Files[path] if len(data) == 0 { if err := os.Remove(path); err != nil && !errors.Is(err, fs.ErrNotExist) { @@ -625,7 +601,7 @@ func (b *Backend) Restore(ctx context.Context, repo git.Repo, s stack.Snapshot) } continue } - if cur, err := os.ReadFile(path); err == nil && string(cur) == string(data) { + if cur, err := os.ReadFile(path); err == nil && bytes.Equal(cur, data) { continue } if err := writeBytes(path, data); err != nil { diff --git a/pkg/backend/ghstack/backend_test.go b/pkg/backend/ghstack/backend_test.go index a15658e..eb96a2b 100644 --- a/pkg/backend/ghstack/backend_test.go +++ b/pkg/backend/ghstack/backend_test.go @@ -4,8 +4,10 @@ package ghstack import ( "context" + "maps" "os" "path/filepath" + "slices" "syscall" "testing" "time" @@ -47,53 +49,75 @@ func TestSnapshotRestoreCommonFileOnly(t *testing.T) { } } -func TestSnapshotRestoreWorktreeFiles(t *testing.T) { +// Snapshot moves a worktree's own file into the catalog first, so the +// snapshot is the one file and Restore puts that back. +func TestSnapshotMovesWorktreeFilesFirst(t *testing.T) { b, repo, mainPath, wtPath := updateFixture(t) snap, err := b.Snapshot(context.Background(), repo) if err != nil { t.Fatal(err) } - if err := os.WriteFile(mainPath, []byte(`{"schemaVersion":1,"stacks":[]}`), 0o644); err != nil { - t.Fatal(err) + if len(snap.Files) != 1 || snap.Files[mainPath] == nil { + t.Fatalf("snapshot files = %v", slices.Collect(maps.Keys(snap.Files))) + } + if _, err := os.Stat(wtPath); !os.IsNotExist(err) { + t.Errorf("worktree file left behind: %v", err) } - if err := os.Remove(wtPath); err != nil { + merged := string(snap.Files[mainPath]) + if err := os.WriteFile(mainPath, []byte(`{"schemaVersion":1,"stacks":[]}`), 0o644); err != nil { t.Fatal(err) } if err := b.Restore(context.Background(), repo, snap); err != nil { t.Fatal(err) } - if got, _ := os.ReadFile(mainPath); string(got) != mainFile { - t.Errorf("main file differs:\n%s", got) - } - if got, _ := os.ReadFile(wtPath); string(got) != wtFile { - t.Errorf("worktree file differs:\n%s", got) + if got, _ := os.ReadFile(mainPath); string(got) != merged { + t.Errorf("catalog differs:\n%s", got) } } -// A worktree file written after the snapshot is removed by Restore, since -// it didn't exist when the snapshot was taken. +// A catalog written after the snapshot is removed by Restore, since it +// didn't exist when the snapshot was taken; a worktree file named by an +// older git-stack's snapshot is put back as it was. func TestRestoreRemovesFileThatDidNotExist(t *testing.T) { - b, repo, _, wtPath := updateFixture(t) - if err := os.Remove(wtPath); err != nil { + gittest.Isolate(t) + dir := gittest.InitRepo(t) + g := git.New(exec.New()) + repo, err := g.Discover(context.Background(), dir) + if err != nil { t.Fatal(err) } + b := New(exec.New(), g) snap, err := b.Snapshot(context.Background(), repo) if err != nil { t.Fatal(err) } - if err := os.WriteFile(wtPath, []byte(wtFile), 0o644); err != nil { + err = b.Update(context.Background(), repo, func(g *stack.Graph) error { + g.Stacks = append(g.Stacks, stack.Stack{Trunk: "main", Branches: []stack.Branch{{Name: "w"}}}) + return nil + }) + if err != nil { t.Fatal(err) } if err := b.Restore(context.Background(), repo, snap); err != nil { t.Fatal(err) } - if _, err := os.Stat(wtPath); !os.IsNotExist(err) { - t.Errorf("the worktree file should be gone: %v", err) + if _, err := os.Stat(filepath.Join(repo.CommonDir, FileName)); !os.IsNotExist(err) { + t.Errorf("the catalog should be gone: %v", err) } - g, err := b.Load(context.Background(), repo) - if err != nil || g.Tracked("w") { + if g, err := b.Load(context.Background(), repo); err != nil || g.Tracked("w") { t.Errorf("w should not be tracked after the restore: %+v %v", g, err) } + + wt := filepath.Join(t.TempDir(), "wt") + gittest.Run(t, dir, "worktree", "add", "-q", "-b", "x", wt) + wtPath := filepath.Join(repo.CommonDir, "worktrees", "wt", FileName) + old := stack.Snapshot{Files: map[string][]byte{wtPath: []byte(wtFile)}} + if err := b.Restore(context.Background(), repo, old); err != nil { + t.Fatal(err) + } + if got, _ := os.ReadFile(wtPath); string(got) != wtFile { + t.Errorf("worktree file = %s", got) + } } func TestRestoreWaitsForTheLock(t *testing.T) { @@ -120,14 +144,14 @@ func TestRestoreWaitsForTheLock(t *testing.T) { t.Fatalf("Restore finished while the lock was held: %v", err) case <-time.After(200 * time.Millisecond): } - if got, _ := os.ReadFile(mainPath); string(got) == mainFile { + if got, _ := os.ReadFile(mainPath); string(got) == string(snap.Files[mainPath]) { t.Fatal("Restore wrote while the lock was held") } _ = syscall.Flock(int(f.Fd()), syscall.LOCK_UN) if err := <-done; err != nil { t.Fatal(err) } - if got, _ := os.ReadFile(mainPath); string(got) != mainFile { + if got, _ := os.ReadFile(mainPath); string(got) != string(snap.Files[mainPath]) { t.Errorf("main file differs:\n%s", got) } } diff --git a/pkg/backend/ghstack/errors.go b/pkg/backend/ghstack/errors.go index b28a637..38cbc42 100644 --- a/pkg/backend/ghstack/errors.go +++ b/pkg/backend/ghstack/errors.go @@ -25,6 +25,7 @@ const ( var ( reAuth = regexp.MustCompile(`(?i)gh auth login|not logged in|authentication required|HTTP 401`) + reMigration = regexp.MustCompile(`cannot migrate stack catalogs|stack migration is blocked`) reConflictIn = regexp.MustCompile(`(?im)^\s*(?:CONFLICT \([^)]*\):\s*(?:Merge conflict in )?|both modified:\s+|Conflicts?:\s*$\n?\s*)(\S.*?)\s*$`) ) @@ -52,6 +53,11 @@ func mapError(args []string, err error) error { WithSteps("run `gh auth login` (gh-stack needs OAuth, a personal access token is not enough)"). WithDetail(stderr).WithCause(err) } + if reMigration.MatchString(stderr) { + return stack.New(stack.KindStateMigration, "gh stack could not move the stacks kept per worktree into its shared file"). + WithSteps("run `git stack doctor --fix` to move them", "then run the command again"). + WithDetail(stderr).WithCause(err) + } var se *stack.Error switch ee.Result.ExitCode { case exitNotInStack: diff --git a/pkg/backend/ghstack/file.go b/pkg/backend/ghstack/file.go index 8ab61ad..5e1d93f 100644 --- a/pkg/backend/ghstack/file.go +++ b/pkg/backend/ghstack/file.go @@ -1,6 +1,7 @@ // Package ghstack implements the stack ports on top of GitHub's gh-stack -// extension. Stack metadata is read directly from the extension's -// /gh-stack file (documented, schema version 1). The file is written +// extension. Stack metadata is read directly from the extension's catalog, +// /gh-stack (schema version 1), shared by every worktree since +// gh-stack 0.3. The file is written // only by Update, under gh stack's lock; every other mutation still goes // through `gh stack` commands. package ghstack diff --git a/pkg/backend/ghstack/ghstack_test.go b/pkg/backend/ghstack/ghstack_test.go index c0f6269..9e53214 100644 --- a/pkg/backend/ghstack/ghstack_test.go +++ b/pkg/backend/ghstack/ghstack_test.go @@ -6,6 +6,7 @@ import ( "os" "path/filepath" "slices" + "strings" "testing" "github.com/DomBlack/git-stack/pkg/exec" @@ -127,6 +128,15 @@ func TestMapError(t *testing.T) { } } +func TestMapErrorMigration(t *testing.T) { + stderr := "✗ migrating stack state: cannot migrate stack catalogs: existing backup differs from the original catalog; sources: [\"a\" \"b\"]" + err := mapError([]string{"submit"}, &exec.ExitError{Result: exec.Result{ExitCode: 1, Stderr: []byte(stderr)}}) + se, ok := errors.AsType[*stack.Error](err) + if !ok || se.Kind != stack.KindStateMigration || !strings.Contains(strings.Join(se.NextSteps, "\n"), "git stack doctor --fix") { + t.Errorf("err = %#v", err) + } +} + func TestPreflight(t *testing.T) { f := exectest.New() f.On("gh", "extension", "list").Reply("gh pr-review\tagynio/gh-pr-review\tv1.6.2\ngh stack\tgithub/gh-stack\tv0.9.0\n") @@ -142,6 +152,20 @@ func TestPreflight(t *testing.T) { t.Errorf("missing extension: %v", err) } + f = exectest.New() + f.On("gh", "extension", "list").Reply("gh stack\tgithub/gh-stack\tv0.2.1\n") + err = New(f, nil).Preflight(context.Background()) + se, ok = errors.AsType[*stack.Error](err) + if !ok || se.Kind != stack.KindNotInstalled || !slices.Contains(se.NextSteps, "run: gh extension upgrade stack") { + t.Errorf("old extension: %v", err) + } + + f = exectest.New() + f.On("gh", "extension", "list").Reply("gh stack\tgithub/gh-stack\t1a2b3c4\n") + if err := New(f, nil).Preflight(context.Background()); err != nil { + t.Errorf("a local build should pass: %v", err) + } + f = exectest.New() f.On("gh").NotFound() b := New(f, nil) diff --git a/pkg/backend/ghstack/preflight.go b/pkg/backend/ghstack/preflight.go index 50a522a..a575211 100644 --- a/pkg/backend/ghstack/preflight.go +++ b/pkg/backend/ghstack/preflight.go @@ -8,15 +8,22 @@ import ( "github.com/DomBlack/git-stack/pkg/exec" "github.com/DomBlack/git-stack/pkg/stack" + "github.com/DomBlack/git-stack/pkg/version" ) // extensionName is how `gh extension list` reports gh-stack. const extensionName = "github/gh-stack" +// MinVersion is the oldest gh-stack this adapter works with: 0.3.0 keeps +// one catalog for every worktree in the common git dir, which is the only +// layout we read and write. +const MinVersion = "v0.3.0" + var preflightOnce sync.Map // map[*Backend]error -// Preflight verifies gh and the gh-stack extension are installed. The -// result is cached per Backend for the life of the process. +// Preflight verifies gh and the gh-stack extension are installed and that +// gh-stack is new enough. The result is cached per Backend for the life of +// the process. func (b *Backend) Preflight(ctx context.Context) error { if v, ok := preflightOnce.Load(b); ok { err, _ := v.(error) @@ -28,23 +35,51 @@ func (b *Backend) Preflight(ctx context.Context) error { } func (b *Backend) preflight(ctx context.Context) error { + v, err := b.Extension(ctx) + if err != nil { + return err + } + if TooOld(v) { + return stack.Newf(stack.KindNotInstalled, "gh-stack %s is too old; git-stack needs %s or later", v, MinVersion). + WithSteps("run: gh extension upgrade stack", "or run `git stack doctor --fix`") + } + return nil +} + +// TooOld reports whether an installed gh-stack version is older than +// MinVersion. Anything that isn't a release (a local build) is let through. +func TooOld(v string) bool { + return version.IsRelease(v) && version.Compare(v, MinVersion) < 0 +} + +// Extension reports the installed gh-stack's version as `gh extension list` +// shows it ("v0.3.0", or a commit for a local build; "" when it shows none). +// It is a KindNotInstalled error when gh or the extension is missing. +func (b *Backend) Extension(ctx context.Context) (string, error) { res, err := b.run.Run(ctx, exec.Cmd{Name: "gh", Args: []string{"extension", "list"}, Env: ghEnv, Unset: ghUnset}) if err != nil { if errors.Is(err, context.Canceled) { - return err + return "", err } if _, ok := errors.AsType[*exec.NotFoundError](err); ok { - return stack.New(stack.KindNotInstalled, "gh (GitHub CLI) is not installed"). + return "", stack.New(stack.KindNotInstalled, "gh (GitHub CLI) is not installed"). WithSteps("install it: https://cli.github.com", "then: gh auth login && gh extension install github/gh-stack"). WithCause(err) } - return mapError([]string{"extension"}, err) + return "", mapError([]string{"extension"}, err) } for line := range strings.SplitSeq(res.Out(), "\n") { - if strings.Contains(line, extensionName) { - return nil + if !strings.Contains(line, extensionName) { + continue + } + fields := strings.Split(line, "\t") + for i, f := range fields { + if strings.TrimSpace(f) == extensionName && i+1 < len(fields) { + return strings.TrimSpace(fields[i+1]), nil + } } + return "", nil } - return stack.New(stack.KindNotInstalled, "the gh-stack extension is not installed"). + return "", stack.New(stack.KindNotInstalled, "the gh-stack extension is not installed"). WithSteps("run: gh extension install github/gh-stack") } diff --git a/pkg/backend/ghstack/reshape_abort_test.go b/pkg/backend/ghstack/reshape_abort_test.go index 3124654..bb2e888 100644 --- a/pkg/backend/ghstack/reshape_abort_test.go +++ b/pkg/backend/ghstack/reshape_abort_test.go @@ -86,14 +86,13 @@ func TestAbortedMovePutsRecordsBack(t *testing.T) { } type snapStack struct { Trunk string `json:"trunk"` - Worktree string `json:"worktree,omitempty"` ID string `json:"id,omitempty"` Number int `json:"number,omitempty"` Branches []snapBranch `json:"branches"` } var snap []snapStack for _, s := range graph.Stacks { - ss := snapStack{Trunk: s.Trunk, Worktree: s.Worktree, ID: s.ID, Number: s.Number} + ss := snapStack{Trunk: s.Trunk, ID: s.ID, Number: s.Number} for _, sb := range s.Branches { ss.Branches = append(ss.Branches, snapBranch{sb.Name, sb.Base}) } diff --git a/pkg/backend/ghstack/stream_test.go b/pkg/backend/ghstack/stream_test.go index 1d9c946..c95379e 100644 --- a/pkg/backend/ghstack/stream_test.go +++ b/pkg/backend/ghstack/stream_test.go @@ -17,7 +17,7 @@ import ( func TestSubmitStreamOutput(t *testing.T) { newFake := func() *exectest.Fake { f := exectest.New() - f.On("gh", "extension", "list").Reply("gh stack\tgithub/gh-stack\tv0.1.1\n") + f.On("gh", "extension", "list").Reply("gh stack\tgithub/gh-stack\tv0.3.0\n") f.On("gh", "stack", "submit").Do(func(c exec.Cmd) (exec.Result, error) { if c.Stream == nil { t.Error("submit should pass the output writer as Stream") @@ -43,7 +43,7 @@ func TestSubmitStreamOutput(t *testing.T) { // No writer (the MCP server): nothing streamed, output returned. f := exectest.New() - f.On("gh", "extension", "list").Reply("gh stack\tgithub/gh-stack\tv0.1.1\n") + f.On("gh", "extension", "list").Reply("gh stack\tgithub/gh-stack\tv0.3.0\n") f.On("gh", "stack", "submit").Do(func(c exec.Cmd) (exec.Result, error) { if c.Stream != nil { t.Error("no WithOutput means no Stream") diff --git a/pkg/backend/ghstack/submitter.go b/pkg/backend/ghstack/submitter.go index bb396aa..c2be3cf 100644 --- a/pkg/backend/ghstack/submitter.go +++ b/pkg/backend/ghstack/submitter.go @@ -17,15 +17,11 @@ var ( // Submit runs `gh stack submit`. Non-interactive runs use --auto and are // captured; interactive runs hand gh-stack the real terminal so its own -// PR editor can open. The current branch's stack is moved into this -// worktree's file first, as that is the only one gh stack reads here. +// PR editor can open. func (b *Backend) Submit(ctx context.Context, repo git.Repo, o stack.SubmitOptions) (stack.SubmitResult, error) { if err := b.Preflight(ctx); err != nil { return stack.SubmitResult{}, err } - if err := b.adoptCurrent(ctx, repo); err != nil { - return stack.SubmitResult{}, err - } args := []string{"submit"} if o.Publish { args = append(args, "--open") @@ -77,6 +73,11 @@ func (b *Backend) Unstack(ctx context.Context, repo git.Repo, number int) error // runner can do this; the MCP runner returns ErrTTYUnavailable, which is // mapped to an interaction error. func (b *Backend) ghTTY(ctx context.Context, repo git.Repo, args ...string) error { + if !MigrationPending(repo) { + if _, err := b.Migrate(ctx, repo); err != nil { + return err + } + } _, err := b.run.Run(ctx, exec.Cmd{ Name: "gh", Args: append([]string{"stack"}, args...), diff --git a/pkg/backend/ghstack/tracker.go b/pkg/backend/ghstack/tracker.go index ae2256d..72e5c5a 100644 --- a/pkg/backend/ghstack/tracker.go +++ b/pkg/backend/ghstack/tracker.go @@ -3,7 +3,6 @@ package ghstack import ( "context" "errors" - "slices" "strings" "github.com/DomBlack/git-stack/pkg/git" @@ -21,16 +20,11 @@ func (b *Backend) InitStack(ctx context.Context, repo git.Repo, trunk string, br return b.gh(ctx, repo, args...) } -// AddTop runs `gh stack add ` without committing (we commit natively), -// once the current branch's stack is in this worktree's file where gh stack -// will look for it. +// AddTop runs `gh stack add ` without committing (we commit natively). func (b *Backend) AddTop(ctx context.Context, repo git.Repo, name string) error { if err := b.Preflight(ctx); err != nil { return err } - if err := b.adoptCurrent(ctx, repo); err != nil { - return err - } err := b.gh(ctx, repo, "add", name) if err != nil { if se, ok := errors.AsType[*stack.Error](err); ok && se.Kind == stack.KindInvalidArgs && @@ -44,24 +38,3 @@ func (b *Backend) AddTop(ctx context.Context, repo git.Repo, name string) error } return nil } - -// adoptCurrent adopts the stack of the branch checked out here, if any. -// When every stack is already in this worktree's file there is nothing to -// move, so git isn't asked which branch that is. -func (b *Backend) adoptCurrent(ctx context.Context, repo git.Repo) error { - r, err := b.read(repo) - if err != nil { - return err - } - if !slices.ContainsFunc(r.where, func(w recRef) bool { return r.files[w.file].src.gitDir != repo.GitDir }) { - return nil - } - branch, err := b.git.CurrentBranch(ctx, repo) - if errors.Is(err, git.ErrDetached) { - return nil - } - if err != nil { - return err - } - return b.adopt(ctx, repo, branch) -} diff --git a/pkg/backend/ghstack/update_test.go b/pkg/backend/ghstack/update_test.go index 9e6f861..cb5ae31 100644 --- a/pkg/backend/ghstack/update_test.go +++ b/pkg/backend/ghstack/update_test.go @@ -105,8 +105,14 @@ func TestUpdateEditsInPlace(t *testing.T) { if strings.Index(s, `"schemaVersion"`) > strings.Index(s, `"repository"`) || strings.Index(s, `"repository"`) > strings.Index(s, `"future"`) { t.Error("member order must be preserved") } - if after, _ := os.ReadFile(wtPath); string(after) != string(before) { - t.Error("the untouched worktree file must be byte for byte the same") + if !strings.Contains(s, `"head": "hw"`) { + t.Errorf("the worktree file's stack should be merged into the catalog:\n%s", s) + } + if _, err := os.Stat(wtPath); !os.IsNotExist(err) { + t.Errorf("the worktree file should have been moved aside: %v", err) + } + if backup, _ := os.ReadFile(wtPath + BackupSuffix); string(backup) != string(before) { + t.Errorf("the backup must be the worktree file byte for byte:\n%s", backup) } g, err := b.Load(context.Background(), repo) if err != nil || len(g.Stacks) != 2 || g.Stacks[0].Bottom() != "b" || g.Stacks[1].Bottom() != "w" { @@ -114,21 +120,23 @@ func TestUpdateEditsInPlace(t *testing.T) { } } -func TestUpdateWritesEachWorktreeFile(t *testing.T) { - b, repo, _, wtPath := updateFixture(t) +// An edit to a stack kept in a worktree's own file lands in the shared +// catalog, and the worktree's file is moved aside. +func TestUpdateMovesWorktreeFileIntoCatalog(t *testing.T) { + b, repo, mainPath, wtPath := updateFixture(t) err := b.Update(context.Background(), repo, func(g *stack.Graph) error { - for i := range g.Stacks { - if g.Stacks[i].Bottom() == "w" { - g.Stacks[i].Branches[0].Head = "hw2" - } - } + s, i, _ := g.StackOf("w") + s.Branches[i].Head = "hw2" return nil }) if err != nil { t.Fatal(err) } - if got, _ := os.ReadFile(wtPath); !strings.Contains(string(got), `"hw2"`) { - t.Errorf("worktree file not updated: %s", got) + if got, _ := os.ReadFile(mainPath); !strings.Contains(string(got), `"hw2"`) { + t.Errorf("catalog not updated: %s", got) + } + if _, err := os.Stat(wtPath); !os.IsNotExist(err) { + t.Errorf("worktree file left behind: %v", err) } } @@ -165,7 +173,7 @@ func TestUpdateRenamesABranch(t *testing.T) { } func TestUpdateFnErrorWritesNothing(t *testing.T) { - b, repo, mainPath, _ := updateFixture(t) + b, repo, mainPath, wtPath := updateFixture(t) before, _ := os.ReadFile(mainPath) err := b.Update(context.Background(), repo, func(g *stack.Graph) error { g.Stacks[0].Branches = nil @@ -177,6 +185,9 @@ func TestUpdateFnErrorWritesNothing(t *testing.T) { if after, _ := os.ReadFile(mainPath); string(after) != string(before) { t.Error("nothing may be written when fn fails") } + if _, err := os.Stat(wtPath); err != nil { + t.Errorf("the worktree file must stay when fn fails: %v", err) + } } func TestUpdateTimesOutOnHeldLock(t *testing.T) { @@ -198,10 +209,11 @@ func TestUpdateTimesOutOnHeldLock(t *testing.T) { } } -func TestUpdateAppendsStackToCurrentWorktreeFile(t *testing.T) { - b, repo, mainPath, wtPath := updateFixture(t) +// A new stack lands in the shared catalog, whichever worktree it is made +// in: gh stack 0.3 reads only that file. +func TestUpdateAppendsStackToTheCatalog(t *testing.T) { + b, repo, mainPath, _ := updateFixture(t) ctx := context.Background() - // From the main checkout the new stack lands in the main file. err := b.Update(ctx, repo, func(g *stack.Graph) error { g.Stacks = append(g.Stacks, stack.Stack{Trunk: "main", Branches: []stack.Branch{{Name: "n1", Head: "h1", Base: "t1"}, {Name: "n2"}}}) return nil @@ -213,12 +225,9 @@ func TestUpdateAppendsStackToCurrentWorktreeFile(t *testing.T) { s := string(data) for _, want := range []string{`"branch": "n1"`, `"head": "h1"`, `"base": "t1"`, `"branch": "n2"`, `"future"`} { if !strings.Contains(s, want) { - t.Errorf("main file lacks %s:\n%s", want, s) + t.Errorf("catalog lacks %s:\n%s", want, s) } } - if wt, _ := os.ReadFile(wtPath); strings.Contains(string(wt), "n1") { - t.Error("the worktree file got the stack too") - } g, err := b.Load(ctx, repo) if err != nil { t.Fatal(err) @@ -227,28 +236,8 @@ func TestUpdateAppendsStackToCurrentWorktreeFile(t *testing.T) { t.Errorf("stack of n2 = %+v", s) } - // From the linked worktree it lands in that worktree's file. - wtRepo, err := git.New(exec.New()).Discover(ctx, worktreePath(filepath.Dir(wtPath))) - if err != nil { - t.Fatal(err) - } - err = b.Update(ctx, wtRepo, func(g *stack.Graph) error { - g.Stacks = append(g.Stacks, stack.Stack{Trunk: "main", Branches: []stack.Branch{{Name: "w2"}}}) - return nil - }) - if err != nil { - t.Fatal(err) - } - if wt, _ := os.ReadFile(wtPath); !strings.Contains(string(wt), `"branch": "w2"`) { - t.Errorf("worktree file lacks w2:\n%s", wt) - } -} - -func TestUpdateAppendsStackToNewWorktreeFile(t *testing.T) { - b, repo, mainPath, _ := updateFixture(t) - ctx := context.Background() - // A second worktree with no file of its own: gh stack only reads the - // worktree's file there, so the new stack must not go in the main one. + // From a linked worktree it goes to the same file, and none is made + // in the worktree's git dir. dir := filepath.Join(t.TempDir(), "fresh") gittest.Run(t, repo.TopLevel, "worktree", "add", "-q", "-b", "f", dir) wtRepo, err := git.New(exec.New()).Discover(ctx, dir) @@ -262,15 +251,11 @@ func TestUpdateAppendsStackToNewWorktreeFile(t *testing.T) { if err != nil { t.Fatal(err) } - data, err := os.ReadFile(filepath.Join(wtRepo.GitDir, FileName)) - if err != nil { - t.Fatal(err) + if data, _ := os.ReadFile(mainPath); !strings.Contains(string(data), `"branch": "f"`) { + t.Errorf("catalog lacks f:\n%s", data) } - if f, err := parseFile(data); err != nil || len(f.Stacks) != 1 || f.Stacks[0].Branches[0].Branch != "f" { - t.Errorf("worktree file = %s (%v)", data, err) - } - if main, _ := os.ReadFile(mainPath); string(main) != mainFile { - t.Errorf("the main file changed:\n%s", main) + if _, err := os.Stat(filepath.Join(wtRepo.GitDir, FileName)); !os.IsNotExist(err) { + t.Errorf("a file was made in the worktree's git dir: %v", err) } } @@ -410,7 +395,7 @@ func TestUpdateMoveKeepsBranchRecords(t *testing.T) { if err != nil { t.Fatal(err) } - if len(f.Stacks) != 2 || len(f.Stacks[1].Branches) != 2 || f.Stacks[1].Branches[1].Branch != "a" { + if len(f.Stacks) != 3 || len(f.Stacks[1].Branches) != 2 || f.Stacks[1].Branches[1].Branch != "a" { t.Fatalf("file = %s", data) } if pr := f.Stacks[1].Branches[1].PullRequest; pr == nil || pr.Number != 1 || !strings.Contains(string(data), `"extra": "yes"`) { @@ -465,8 +450,8 @@ func TestUpdateRewritesChangedTrunk(t *testing.T) { } } -// A branch moving from the worktree's file into a stack in the main file -// keeps its members, and the worktree file loses it. +// A branch moving from a worktree's file into a stack in the catalog keeps +// its members, and the rest of the worktree's file joins the catalog. func TestUpdateMoveAcrossFilesKeepsBranchRecords(t *testing.T) { b, repo, mainPath, wtPath := updateFixture(t) wt := `{"schemaVersion":1,"stacks":[{"trunk":{"branch":"main"},"branches":[{"branch":"w","head":"hw","pullRequest":{"number":4},"note":"kept"},{"branch":"w2","head":"hw2"}]}]}` @@ -500,67 +485,76 @@ func TestUpdateMoveAcrossFilesKeepsBranchRecords(t *testing.T) { if moved.Branch != "w" || moved.PullRequest == nil || moved.PullRequest.Number != 4 || !strings.Contains(string(data), `"note": "kept"`) { t.Errorf("w lost its records on the way:\n%s", data) } - if wtData, _ := os.ReadFile(wtPath); strings.Contains(string(wtData), `"branch": "w"`) || !strings.Contains(string(wtData), `"w2"`) { - t.Errorf("worktree file should only hold w2 now:\n%s", wtData) + if g, _ := b.Load(ctx, repo); !g.Tracked("w2") { + t.Errorf("w2 should still be tracked:\n%s", data) } -} - -func TestAdoptMovesStackRecordToCurrentWorktree(t *testing.T) { - b, repo, mainPath, _ := updateFixture(t) - ctx := context.Background() - gittest.Run(t, repo.TopLevel, "branch", "b") - dir := filepath.Join(t.TempDir(), "fresh") - gittest.Run(t, repo.TopLevel, "worktree", "add", "-q", dir, "b") - wtRepo, err := b.git.Discover(ctx, dir) - if err != nil { - t.Fatal(err) + if _, err := os.Stat(wtPath); !os.IsNotExist(err) { + t.Errorf("worktree file left behind: %v", err) } +} - // b's stack lives in the main checkout's file; from the worktree where b - // is checked out it moves over whole. - if err := b.adoptCurrent(ctx, wtRepo); err != nil { +// What git-stack and gh stack 0.3 together left behind: gh stack moved the +// worktree's file into the catalog and kept a backup, git-stack wrote the +// worktree's file again, and gh stack then refused for good because the +// backup differs. Migrate merges it anyway and keeps both copies. +func TestMigrateSetsAsideAStaleBackup(t *testing.T) { + b, repo, mainPath, wtPath := updateFixture(t) + stale := `{"schemaVersion":1,"stacks":[]}` + if err := os.WriteFile(wtPath+BackupSuffix, []byte(stale), 0o600); err != nil { t.Fatal(err) } - data, err := os.ReadFile(filepath.Join(wtRepo.GitDir, FileName)) + moved, err := b.Migrate(context.Background(), repo) if err != nil { t.Fatal(err) } - for _, want := range []string{`"id": "S1"`, `"number": 7`, `"extra": "yes"`, `"branch": "b"`} { - if !strings.Contains(string(data), want) { - t.Errorf("worktree file lacks %s:\n%s", want, data) - } + if len(moved) != 1 || moved[0] != wtPath { + t.Errorf("moved = %v", moved) } - main, _ := os.ReadFile(mainPath) - if strings.Contains(string(main), `"S1"`) || !strings.Contains(string(main), `"branch": "done"`) || !strings.Contains(string(main), `"future"`) { - t.Errorf("main file should keep everything but S1:\n%s", main) + if data, _ := os.ReadFile(mainPath); !strings.Contains(string(data), `"branch": "w"`) { + t.Errorf("catalog lacks w:\n%s", data) } - g, err := b.Load(ctx, wtRepo) - if err != nil { - t.Fatal(err) + if got, _ := os.ReadFile(wtPath + BackupSuffix); string(got) != stale { + t.Errorf("the earlier backup was changed: %s", got) } - if s, _, ok := g.StackOf("b"); !ok || s.ID != "S1" || s.Bottom() != "a" || s.Worktree != wtRepo.TopLevel { - t.Errorf("stack of b = %+v", s) + if got, _ := os.ReadFile(wtPath + BackupSuffix + ".1"); string(got) != wtFile { + t.Errorf("the worktree file was not kept as the next backup: %s", got) } - - // Already here: nothing is rewritten. - before, _ := os.ReadFile(filepath.Join(wtRepo.GitDir, FileName)) - if err := b.adoptCurrent(ctx, wtRepo); err != nil { - t.Fatal(err) + if LegacyFiles(repo) != nil { + t.Errorf("legacy files left: %v", LegacyFiles(repo)) } - if after, _ := os.ReadFile(filepath.Join(wtRepo.GitDir, FileName)); string(after) != string(before) { - t.Errorf("a second adopt changed the file:\n%s", after) + // Nothing left to move: a no-op. + if moved, err := b.Migrate(context.Background(), repo); err != nil || moved != nil { + t.Errorf("second Migrate = %v %v", moved, err) } +} - // And back to the main checkout once b is checked out there. - gittest.Run(t, dir, "switch", "-q", "--detach") - gittest.Run(t, repo.TopLevel, "switch", "-q", "b") - if err := b.adoptCurrent(ctx, repo); err != nil { - t.Fatal(err) - } - if main, _ := os.ReadFile(mainPath); !strings.Contains(string(main), `"S1"`) { - t.Errorf("S1 should be back in the main file:\n%s", main) - } - if wt, _ := os.ReadFile(filepath.Join(wtRepo.GitDir, FileName)); strings.Contains(string(wt), `"S1"`) { - t.Errorf("S1 should have left the worktree file:\n%s", wt) +// gh stack's own migration part way through, or an interrupted gh stack +// rebase in the worktree, stops the move with nothing written. +func TestMigrateRefusesWhatGhStackOwns(t *testing.T) { + for _, tc := range []struct { + name string + file func(repo git.Repo, wtPath string) string + }{ + {"journal", func(repo git.Repo, _ string) string { return filepath.Join(repo.CommonDir, migrationJournal) }}, + {"rebase", func(_ git.Repo, wtPath string) string { + return filepath.Join(filepath.Dir(wtPath), "gh-stack-rebase-state") + }}, + } { + t.Run(tc.name, func(t *testing.T) { + b, repo, mainPath, wtPath := updateFixture(t) + if err := os.WriteFile(tc.file(repo, wtPath), []byte("{}"), 0o644); err != nil { + t.Fatal(err) + } + _, err := b.Migrate(context.Background(), repo) + if se, ok := errors.AsType[*stack.Error](err); !ok || se.Kind != stack.KindStateMigration { + t.Fatalf("want KindStateMigration, got %v", err) + } + if data, _ := os.ReadFile(mainPath); string(data) != mainFile { + t.Errorf("catalog changed:\n%s", data) + } + if _, err := os.Stat(wtPath); err != nil { + t.Errorf("worktree file moved: %v", err) + } + }) } } diff --git a/pkg/backend/ghstack/worktrees_test.go b/pkg/backend/ghstack/worktrees_test.go index 377b65f..bc409e3 100644 --- a/pkg/backend/ghstack/worktrees_test.go +++ b/pkg/backend/ghstack/worktrees_test.go @@ -4,19 +4,23 @@ import ( "context" "os" "path/filepath" + "slices" + "strings" "testing" "github.com/DomBlack/git-stack/pkg/exec/exectest" "github.com/DomBlack/git-stack/pkg/git" ) -// TestLoadMergesWorktreeMetadata lays out a .git with a main checkout file -// and two linked worktrees' files and checks Load sees all of them, in the -// current worktree first order, with the worktree recorded on each stack. +// TestLoadMergesWorktreeMetadata lays out a .git with the shared catalog +// and two linked worktrees' own files, as gh stack 0.2 kept them, and +// checks Load sees all of them: the catalog first, then the current +// worktree's file, then the others, a stack sharing a branch with one +// already seen left out. func TestLoadMergesWorktreeMetadata(t *testing.T) { root := t.TempDir() common := filepath.Join(root, "main", ".git") - for _, d := range []string{common, filepath.Join(common, "worktrees", "one"), filepath.Join(common, "worktrees", "two"), filepath.Join(root, "wt-one"), filepath.Join(root, "wt-two")} { + for _, d := range []string{common, filepath.Join(common, "worktrees", "one"), filepath.Join(common, "worktrees", "two")} { if err := os.MkdirAll(d, 0o755); err != nil { t.Fatal(err) } @@ -29,41 +33,28 @@ func TestLoadMergesWorktreeMetadata(t *testing.T) { } write(filepath.Join(common, FileName), `{"schemaVersion":1,"stacks":[{"trunk":{"branch":"main"},"branches":[{"branch":"a"},{"branch":"b"}]}]}`) write(filepath.Join(common, "worktrees", "one", FileName), `{"schemaVersion":1,"stacks":[{"trunk":{"branch":"main"},"branches":[{"branch":"w1"}]}]}`) - write(filepath.Join(common, "worktrees", "one", "gitdir"), filepath.Join(root, "wt-one", ".git")+"\n") - // two repeats branch "a": its stack is dropped, first one wins. - write(filepath.Join(common, "worktrees", "two", FileName), `{"schemaVersion":1,"stacks":[{"trunk":{"branch":"main"},"branches":[{"branch":"a"}]},{"trunk":{"branch":"main"},"branches":[{"branch":"w2"}]}]}`) - write(filepath.Join(common, "worktrees", "two", "gitdir"), filepath.Join(root, "wt-two", ".git")+"\n") + // two repeats branch "a": its stack is dropped, the catalog wins. + write(filepath.Join(common, "worktrees", "two", FileName), `{"schemaVersion":1,"repository":"o/r","stacks":[{"trunk":{"branch":"main"},"branches":[{"branch":"a"}]},{"trunk":{"branch":"main"},"branches":[{"branch":"w2"}]}]}`) b := New(exectest.New(), nil) - - // From the main checkout. - g, err := b.Load(context.Background(), git.Repo{TopLevel: filepath.Join(root, "main"), GitDir: common, CommonDir: common}) - if err != nil { - t.Fatal(err) - } - var got []string - for _, s := range g.Stacks { - got = append(got, s.Bottom()+"@"+filepath.Base(s.Worktree)) - } - want := []string{"a@main", "w1@wt-one", "w2@wt-two"} - if len(got) != len(want) { - t.Fatalf("stacks = %v, want %v", got, want) - } - for i := range want { - if got[i] != want[i] { - t.Errorf("stack %d = %q, want %q", i, got[i], want[i]) + for _, repo := range []git.Repo{ + {TopLevel: filepath.Join(root, "main"), GitDir: common, CommonDir: common}, + {TopLevel: filepath.Join(root, "wt-two"), GitDir: filepath.Join(common, "worktrees", "two"), CommonDir: common}, + } { + g, err := b.Load(context.Background(), repo) + if err != nil { + t.Fatal(err) + } + var got []string + for _, s := range g.Stacks { + got = append(got, strings.Join(s.Names(), ",")) + } + want := []string{"a,b", "w1", "w2"} + if filepath.Base(repo.GitDir) == "two" { + want = []string{"a,b", "w2", "w1"} + } + if !slices.Equal(got, want) { + t.Errorf("from %s: stacks = %v, want %v", repo.TopLevel, got, want) } - } - - // From worktree one: its own stack comes first. - g, err = b.Load(context.Background(), git.Repo{TopLevel: filepath.Join(root, "wt-one"), GitDir: filepath.Join(common, "worktrees", "one"), CommonDir: common}) - if err != nil { - t.Fatal(err) - } - if len(g.Stacks) != 3 || g.Stacks[0].Bottom() != "w1" || g.Stacks[0].Worktree != filepath.Join(root, "wt-one") || g.Stacks[1].Bottom() != "a" { - t.Errorf("from a linked worktree: %+v", g.Stacks) - } - if !g.Tracked("w2") || !g.Tracked("b") { - t.Error("every worktree's branches should be tracked") } } diff --git a/pkg/stack/errors.go b/pkg/stack/errors.go index bfe0ece..3b26486 100644 --- a/pkg/stack/errors.go +++ b/pkg/stack/errors.go @@ -54,6 +54,9 @@ const ( // KindSigningFailed: git could not sign a commit (a locked SSH key, a // gpg agent that can't prompt); nothing was committed. KindSigningFailed + // KindStateMigration: the backend's stack metadata is in an older or + // half converted layout that has to be sorted out first. + KindStateMigration ) var kindCodes = map[Kind]string{ @@ -77,6 +80,7 @@ var kindCodes = map[Kind]string{ KindChecksFailing: "checks_failing", KindChecksPending: "checks_pending", KindSigningFailed: "signing_failed", + KindStateMigration: "state_migration", } // Code is the stable machine-readable name of the kind. diff --git a/pkg/stack/model.go b/pkg/stack/model.go index d7f6462..52dac72 100644 --- a/pkg/stack/model.go +++ b/pkg/stack/model.go @@ -37,9 +37,6 @@ type Stack struct { Trunk string // Branches run bottom (closest to trunk) to top. Branches []Branch - // Worktree is the working tree whose metadata holds this stack (gh stack - // keeps stacks per worktree), or empty when unknown. Sync runs there. - Worktree string } // Index returns the position of name in the stack, or -1. diff --git a/pkg/stack/ports.go b/pkg/stack/ports.go index c45ebf3..83a2986 100644 --- a/pkg/stack/ports.go +++ b/pkg/stack/ports.go @@ -39,9 +39,7 @@ type Metadata interface { // and writes the result back. fn may edit each stack's Branches (remove // entries, change Head and Base, rename one by setting Name and // RenamedFrom) and may also append stacks (each with a - // trunk and at least one branch; they are written to the current - // worktree's file, else the main checkout's, else a new file in the - // common dir) and may set or clear a stack's ID and Number (clearing + // trunk and at least one branch) and may set or clear a stack's ID and Number (clearing // removes them from the record). It must not remove or reorder existing // stacks; a stack left with no branches is removed. When fn returns an // error nothing is written and the error is returned.