diff --git a/docs/architecture.md b/docs/architecture.md index 069abc0..3dd3c84 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -188,7 +188,11 @@ rather than a confusing git error. **`create` from trunk runs `gh stack init`, from the top runs `gh stack add`, and we always commit natively.** This sidesteps both `add` quirks above. From the middle of a stack you get a `not_at_top` error with next steps; from an untracked branch you get told -the branch isn't in a stack. +the branch isn't in a stack. The branch is created, checked out and registered before the +commit, so when the commit fails (a signing key that isn't unlocked, a hook saying no) create +undoes all of that, forgetting the branch through `Update`, switching back and deleting it, +and the staged changes are still staged; the error says to run it again. If the undo itself +fails the error says what was left behind and that `modify -c` commits on it. **`modify` amends natively, then runs the native restack upstack.** Conflicts come back as a `conflict` error listing the files and the `git stack continue` / `git stack abort` diff --git a/pkg/app/create.go b/pkg/app/create.go index 887e0ff..5bf5ca0 100644 --- a/pkg/app/create.go +++ b/pkg/app/create.go @@ -4,7 +4,9 @@ import ( "context" "errors" "fmt" + "slices" "strings" + "time" "github.com/DomBlack/git-stack/pkg/ai" "github.com/DomBlack/git-stack/pkg/git" @@ -36,6 +38,10 @@ type CreateResult struct { // recentSubjectsCount is how many trunk subjects feed the AI prompt. const recentSubjectsCount = 10 +// undoTimeout bounds the undo of a failed create, which runs even when the +// command's own context was cancelled. +const undoTimeout = 30 * time.Second + // Create stages, creates a branch on top of the current one, commits and // registers the branch with the backend. func (a *App) Create(ctx context.Context, repo git.Repo, o CreateOptions) (CreateResult, error) { @@ -96,13 +102,65 @@ func (a *App) Create(ctx context.Context, repo git.Repo, o CreateOptions) (Creat } sha, err := a.d.Git.Commit(ctx, repo, git.CommitOptions{Message: message, NoVerify: o.NoVerify}) if err != nil { - return res, commitError(err, "pass -m (the branch was created; commit with git commit)") + return CreateResult{}, a.undoCreate(ctx, repo, name, current, commitError(err, "pass -m ")) } info := a.commitInfo(ctx, repo, sha) res.Commit = &info return res, nil } +// undoCreate makes a failed create all or nothing: the branch was already +// created, checked out and registered before the commit failed (a locked +// signing key, a hook saying no), so forget it, go back to the parent and +// delete it. The staged changes survive, both branches point at the same +// commit. The error says to run create again, or, when undoing failed too, +// what was left behind and how to commit on it instead. +func (a *App) undoCreate(ctx context.Context, repo git.Repo, name, parent string, cause error) error { + err := asStackError(cause) + // The commit may have failed because ctx was cancelled; the undo still + // has to run, so give it its own bounded context. + ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), undoTimeout) + defer cancel() + // Commit can fail after git has actually committed (reading HEAD back + // under a cancelled context, say). Then the branch holds the commit + // and must stay: deleting it would orphan the work. + if tip, ok, terr := a.d.Git.Tip(ctx, repo, name); terr == nil && ok { + if parentTip, _, perr := a.d.Git.Tip(ctx, repo, parent); perr == nil && tip != parentTip { + return err.WithSteps(fmt.Sprintf("%s has a commit after all, so it was kept and is tracked; check `git stack log`", name)) + } + } + undo := func() error { + if err := a.d.Meta.Update(ctx, repo, func(g *stack.Graph) error { + for i := range g.Stacks { + g.Stacks[i].Branches = slices.DeleteFunc(g.Stacks[i].Branches, func(b stack.Branch) bool { return b.Name == name }) + } + return nil + }); err != nil { + return fmt.Errorf("forget %s: %w", name, err) + } + if err := a.d.Git.Switch(ctx, repo, parent); err != nil { + return err + } + return a.d.Git.DeleteBranch(ctx, repo, name) + } + if undoErr := undo(); undoErr != nil { + a.d.Log.Debug("undo create", "branch", name, "err", undoErr) + return err.WithSteps( + fmt.Sprintf("%s was left behind with no commit of its own (undoing it failed: %v)", name, undoErr), + "check `git stack log`; if it is still tracked, commit on it with `git stack modify -c -m `, otherwise delete it") + } + return err.WithSteps("nothing was created; run `git stack create` again once that is sorted") +} + +// asStackError returns err as a *stack.Error, wrapping anything else as +// KindUnknown so next steps can be attached. +func asStackError(err error) *stack.Error { + if se, ok := errors.AsType[*stack.Error](err); ok { + return se + } + return stack.New(stack.KindUnknown, err.Error()).WithCause(err) +} + // isDefaultBranch reports whether name is the repository's default branch // while no stack has declared a trunk yet (the first stack starts here). func (a *App) isDefaultBranch(ctx context.Context, repo git.Repo, graph *stack.Graph, name string) bool { diff --git a/pkg/app/modify.go b/pkg/app/modify.go index 68e825b..5c6b942 100644 --- a/pkg/app/modify.go +++ b/pkg/app/modify.go @@ -104,7 +104,11 @@ func (a *App) Modify(ctx context.Context, repo git.Repo, o ModifyOptions) (Modif } sha, err := a.d.Git.Commit(ctx, repo, co) if err != nil { - return ModifyResult{}, commitError(err, "pass -m or --no-edit") + err = commitError(err, "pass -m or --no-edit") + if errors.Is(err, &stack.Error{Kind: stack.KindSigningFailed}) { + err = asStackError(err).WithSteps("then run `git stack modify` again; the branch is untouched") + } + return ModifyResult{}, err } res.Commit = a.commitInfo(ctx, repo, sha) diff --git a/pkg/app/mutate_test.go b/pkg/app/mutate_test.go index 1be866f..5bf135f 100644 --- a/pkg/app/mutate_test.go +++ b/pkg/app/mutate_test.go @@ -3,9 +3,12 @@ package app_test import ( "context" "errors" + "os" + "path/filepath" "slices" "strings" "testing" + "time" "github.com/DomBlack/git-stack/pkg/ai" "github.com/DomBlack/git-stack/pkg/app" @@ -21,11 +24,16 @@ import ( type fakeBackend struct { git *git.Client graph *stack.Graph + // updateErr makes Update fail, for the paths that undo a change. + updateErr error } func (f *fakeBackend) Load(context.Context, git.Repo) (*stack.Graph, error) { return f.graph, nil } func (f *fakeBackend) Update(_ context.Context, _ git.Repo, fn func(*stack.Graph) error) error { + if f.updateErr != nil { + return f.updateErr + } if err := fn(f.graph); err != nil { return err } @@ -383,3 +391,117 @@ func TestCreateExplainsSigningFailure(t *testing.T) { }) } } + +// rejectCommits installs a pre-commit hook that fails every commit. +func rejectCommits(t *testing.T, dir string) { + t.Helper() + hooks := filepath.Join(dir, ".git", "hooks") + if err := os.MkdirAll(hooks, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(hooks, "pre-commit"), []byte("#!/bin/sh\necho nope >&2\nexit 1\n"), 0o755); err != nil { + t.Fatal(err) + } +} + +func TestCreateUndoesTheBranchWhenTheCommitFails(t *testing.T) { + ctx := context.Background() + + t.Run("on top of a stack", func(t *testing.T) { + a, fb, repo, dir, _ := mutFixture(t) + rejectCommits(t, dir) + gittest.WriteFile(t, dir, "b.txt", "b") + gittest.Run(t, dir, "add", "b.txt") + + _, err := a.Create(ctx, repo, app.CreateOptions{Name: "feat/b", Message: []string{"Add b"}}) + se, ok := errors.AsType[*stack.Error](err) + if !ok || !strings.Contains(se.Detail+se.Msg, "nope") { + t.Fatalf("err = %v, want the hook's output", err) + } + if !slices.ContainsFunc(se.NextSteps, func(s string) bool { return strings.Contains(s, "git stack create") }) { + t.Errorf("next steps %q should say to run create again", se.NextSteps) + } + if cur := gittest.Run(t, dir, "branch", "--show-current"); cur != "a" { + t.Errorf("on %s, want back on a", cur) + } + if exists, _ := fb.git.BranchExists(ctx, repo, "feat/b"); exists { + t.Error("feat/b should have been deleted") + } + if names := fb.graph.Stacks[0].Names(); !slices.Equal(names, []string{"a"}) { + t.Errorf("stack = %v, want feat/b forgotten", names) + } + if out := gittest.Run(t, dir, "diff", "--cached", "--name-only"); out != "b.txt" { + t.Errorf("staged = %q, want b.txt still staged", out) + } + }) + + t.Run("new stack from trunk", func(t *testing.T) { + a, fb, repo, dir, _ := mutFixture(t) + gittest.Run(t, dir, "switch", "-q", "main") + fb.graph = stack.NewGraph(nil) + rejectCommits(t, dir) + gittest.WriteFile(t, dir, "b.txt", "b") + gittest.Run(t, dir, "add", "b.txt") + + if _, err := a.Create(ctx, repo, app.CreateOptions{Name: "feat/b", Message: []string{"Add b"}}); err == nil { + t.Fatal("expected the commit to fail") + } + if cur := gittest.Run(t, dir, "branch", "--show-current"); cur != "main" { + t.Errorf("on %s, want back on main", cur) + } + if len(fb.graph.Stacks) != 0 { + t.Errorf("stacks = %+v, want the empty stack gone", fb.graph.Stacks) + } + }) + + t.Run("undo fails", func(t *testing.T) { + a, fb, repo, dir, _ := mutFixture(t) + rejectCommits(t, dir) + fb.updateErr = errors.New("metadata locked") + gittest.WriteFile(t, dir, "b.txt", "b") + gittest.Run(t, dir, "add", "b.txt") + + _, err := a.Create(ctx, repo, app.CreateOptions{Name: "feat/b", Message: []string{"Add b"}}) + se, ok := errors.AsType[*stack.Error](err) + if !ok { + t.Fatalf("err = %v, want *stack.Error", err) + } + if steps := strings.Join(se.NextSteps, "\n"); !strings.Contains(steps, "feat/b was left behind") || !strings.Contains(steps, "git stack modify") { + t.Errorf("next steps %q should say feat/b is left behind and how to commit on it", se.NextSteps) + } + if cur := gittest.Run(t, dir, "branch", "--show-current"); cur != "feat/b" { + t.Errorf("on %s, want still on feat/b", cur) + } + }) +} + +func TestCreateUndoesTheBranchWhenTheCommitIsCancelled(t *testing.T) { + a, fb, repo, dir, _ := mutFixture(t) + // A hook that outlives the deadline: the commit fails with the + // context's error, and the undo must still run on a context of its own. + hooks := filepath.Join(dir, ".git", "hooks") + if err := os.MkdirAll(hooks, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(hooks, "pre-commit"), []byte("#!/bin/sh\nsleep 5\n"), 0o755); err != nil { + t.Fatal(err) + } + gittest.WriteFile(t, dir, "b.txt", "b") + gittest.Run(t, dir, "add", "b.txt") + + ctx, cancel := context.WithTimeout(context.Background(), 300*time.Millisecond) + defer cancel() + _, err := a.Create(ctx, repo, app.CreateOptions{Name: "feat/b", Message: []string{"Add b"}}) + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("err = %v, want the deadline", err) + } + if cur := gittest.Run(t, dir, "branch", "--show-current"); cur != "a" { + t.Errorf("on %s, want back on a", cur) + } + if exists, _ := fb.git.BranchExists(context.Background(), repo, "feat/b"); exists { + t.Error("feat/b should have been deleted") + } + if names := fb.graph.Stacks[0].Names(); !slices.Equal(names, []string{"a"}) { + t.Errorf("stack = %v, want feat/b forgotten", names) + } +} diff --git a/pkg/app/staging.go b/pkg/app/staging.go index 33f37ea..5b7bd3d 100644 --- a/pkg/app/staging.go +++ b/pkg/app/staging.go @@ -139,6 +139,6 @@ func signingError(se *git.SigningError) error { e = stack.New(stack.KindSigningFailed, "commit signing failed: gpg could not sign the commit (its agent may need a terminal to ask for the passphrase)"). WithSteps("check that gpg can sign without a prompt: echo test | gpg --batch --clearsign") } - return e.WithSteps("or turn signing off for this repository: git config commit.gpgsign false", "then run the command again"). + return e.WithSteps("or turn signing off for this repository: git config commit.gpgsign false"). WithDetail(se.Detail).WithCause(se) } diff --git a/pkg/mcp/tools.go b/pkg/mcp/tools.go index 3748b06..10f2088 100644 --- a/pkg/mcp/tools.go +++ b/pkg/mcp/tools.go @@ -435,7 +435,7 @@ func (s *Server) registerTools() { mcp.AddTool(s.mcp, &mcp.Tool{ Name: "stack_create", Title: "Create a stacked branch", - Description: "Create a new branch on top of the current branch, commit staged changes with the given message, and register it in the stack. From the trunk this starts a new stack. Fails with not_at_top when the current branch is not the top of its stack.", + Description: "Create a new branch on top of the current branch, commit staged changes with the given message, and register it in the stack. From the trunk this starts a new stack. Fails with not_at_top when the current branch is not the top of its stack. If the commit fails (signing_failed, a hook) the branch is removed again and the changes stay staged, so fix the cause and call it again; should removing it fail too, next_steps says what was left behind and how to commit on it.", Annotations: &mcp.ToolAnnotations{DestructiveHint: boolp(false), OpenWorldHint: boolp(false)}, }, s.create) mcp.AddTool(s.mcp, &mcp.Tool{