diff --git a/AGENTS.md b/AGENTS.md index d4edaa0..5809688 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` 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 `/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. ## 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/docs/architecture.md b/docs/architecture.md index d0c9fbd..b8345f8 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -117,6 +117,11 @@ no git config keys. The shape is roughly; 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. - `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. diff --git a/pkg/backend/ghstack/backend.go b/pkg/backend/ghstack/backend.go index 7e635e5..ec3f886 100644 --- a/pkg/backend/ghstack/backend.go +++ b/pkg/backend/ghstack/backend.go @@ -158,31 +158,11 @@ func (b *Backend) Load(_ context.Context, repo git.Repo) (*stack.Graph, error) { // record). It must not remove or reorder existing stacks; a stack left with // no branches is removed. func (b *Backend) Update(ctx context.Context, repo git.Repo, fn func(*stack.Graph) error) 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) - } - } - // The current git dir is always locked, even with no file yet: Update - // may create its file for a new stack and two writers must not race it. - if !slices.Contains(dirs, repo.GitDir) { - dirs = append(dirs, repo.GitDir) - } - slices.Sort(dirs) - var unlocks []func() - defer func() { - for _, u := range unlocks { - u() - } - }() - for _, dir := range dirs { - unlock, err := lock(ctx, filepath.Join(dir, LockName), lockWait) - if err != nil { - return err - } - unlocks = append(unlocks, unlock) + unlock, err := lockAll(ctx, repo) + if err != nil { + return err } + defer unlock() r, err := b.read(repo) if err != nil { return err @@ -220,6 +200,92 @@ func (b *Backend) Update(ctx context.Context, repo git.Repo, fn func(*stack.Grap return nil } +// 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) + } + slices.Sort(dirs) + var unlocks []func() + unlockAll := func() { + for _, u := range slices.Backward(unlocks) { + u() + } + } + for _, dir := range dirs { + unlock, err := lock(ctx, filepath.Join(dir, LockName), lockWait) + if err != nil { + unlockAll() + return nil, err + } + unlocks = append(unlocks, unlock) + } + return unlockAll, 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 + } + 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 + } + return writeFile(r.files[from.file].src.gitDir, src) +} + // 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 @@ -344,11 +410,17 @@ func (b *Backend) writeBack(r *reading, fi int, root *node, pool map[string]*nod 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 } - path := filepath.Join(r.files[fi].src.gitDir, FileName) + path := filepath.Join(gitDir, FileName) tmp, err := os.CreateTemp(filepath.Dir(path), FileName+".*") if err != nil { return err diff --git a/pkg/backend/ghstack/submitter.go b/pkg/backend/ghstack/submitter.go index d654ab6..bb396aa 100644 --- a/pkg/backend/ghstack/submitter.go +++ b/pkg/backend/ghstack/submitter.go @@ -17,11 +17,15 @@ 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. +// 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. 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") diff --git a/pkg/backend/ghstack/tracker.go b/pkg/backend/ghstack/tracker.go index 72e5c5a..ae2256d 100644 --- a/pkg/backend/ghstack/tracker.go +++ b/pkg/backend/ghstack/tracker.go @@ -3,6 +3,7 @@ package ghstack import ( "context" "errors" + "slices" "strings" "github.com/DomBlack/git-stack/pkg/git" @@ -20,11 +21,16 @@ 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). +// 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. 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 && @@ -38,3 +44,24 @@ 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 fa47703..3fb13fb 100644 --- a/pkg/backend/ghstack/update_test.go +++ b/pkg/backend/ghstack/update_test.go @@ -472,3 +472,63 @@ func TestUpdateMoveAcrossFilesKeepsBranchRecords(t *testing.T) { t.Errorf("worktree file should only hold w2 now:\n%s", wtData) } } + +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) + } + + // 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 { + t.Fatal(err) + } + data, err := os.ReadFile(filepath.Join(wtRepo.GitDir, FileName)) + 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) + } + } + 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) + } + g, err := b.Load(ctx, wtRepo) + if err != nil { + t.Fatal(err) + } + if s, _, ok := g.StackOf("b"); !ok || s.ID != "S1" || s.Bottom() != "a" || s.Worktree != wtRepo.TopLevel { + t.Errorf("stack of b = %+v", s) + } + + // 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 after, _ := os.ReadFile(filepath.Join(wtRepo.GitDir, FileName)); string(after) != string(before) { + t.Errorf("a second adopt changed the file:\n%s", after) + } + + // 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) + } +}