From 843fd11bcd57b7b3490db66cc5b485c303c1da0e Mon Sep 17 00:00:00 2001 From: Dominic Black Date: Tue, 13 Oct 2026 12:02:53 +0100 Subject: [PATCH] Bring uncommitted changes along when sync moves you off a merged branch If you were sat on a branch whose PR had merged and had any uncommitted change at all, sync kept the branch and told you to commit or stash first. That was far stricter than it needed to be: an edit to a file that's the same on the branch and on trunk (a local .mcp.json tweak, say) moves across a git switch fine, and the workaround was just running git co main yourself and syncing again. So sync no longer checks whether the tree is dirty up front. It switches to trunk the way git switch does, so changes git can carry (untracked files included) come along, and you're told they did. The branch is only kept when git refuses because the switch would overwrite something, and the notice now names those files; when untracked ones are in the way it points at git stash --include-untracked, since a plain stash leaves them behind. Any other failure stops the sync like it always did. Switch and the detached switch return the same OverwriteError the fast forward already used, so its message no longer says fast forward. --- README.md | 4 ++ docs/architecture.md | 4 +- pkg/app/sync_cleanup.go | 73 ++++++++++++++++++++++++++++-------- pkg/app/sync_cleanup_test.go | 68 ++++++++++++++++++++++++++++++++- pkg/git/branch.go | 6 +++ pkg/git/refs.go | 15 ++++++-- pkg/git/worktree_test.go | 31 +++++++++++++++ 7 files changed, 181 insertions(+), 20 deletions(-) diff --git a/README.md b/README.md index d45a57d..cd90435 100644 --- a/README.md +++ b/README.md @@ -339,6 +339,10 @@ lock on the branch itself (`.git/refs/heads/main.lock`, which can happen whether checked out) is reported straight away with its path rather than waited on, because by then git may already have updated the checkout. It never deletes a lock itself. +If the branch being deleted is the one you're on, sync switches you to its trunk first, and like +`git switch` your uncommitted changes come along. It only keeps the branch when git says the +switch would overwrite them, and then names the files. + Flags: - `-d` / `--delete-all` delete merged or closed branches without asking - `-f` / `--force` like `-d`, and also reset a diverged trunk to the remote diff --git a/docs/architecture.md b/docs/architecture.md index 3dd3c84..c18fe53 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -270,7 +270,9 @@ A branch recorded in two worktrees' metadata files is read from the first; once 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 trunk is checked out in another worktree leaves HEAD detached at the trunk tip rather than -failing. Consent for deletion is `stack.sync.prune` (`always` by default), `-d`, or `-f`; +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 +the branch (as `dirty`, files named); any other failure stops the sync as before. Consent for deletion is `stack.sync.prune` (`always` by default), `-d`, or `-f`; with no terminal and `ask`, branches are kept with a notice. Nothing is pushed; `submit` does that. diff --git a/pkg/app/sync_cleanup.go b/pkg/app/sync_cleanup.go index 6bff0c9..de88148 100644 --- a/pkg/app/sync_cleanup.go +++ b/pkg/app/sync_cleanup.go @@ -87,27 +87,18 @@ func (a *App) deleteApproved(ctx context.Context, st *syncState, approved []cand switch lb.Worktree { case "": case st.repo.TopLevel: - if st.isDirty(ctx, a.d.Git, lb.Worktree) { - res.Kept = append(res.Kept, KeptBranch{Name: c.name, Reason: KeptDirty}) - res.notice("%s is checked out here with uncommitted changes, so it was kept; commit or stash them and sync again", c.name) - continue - } trunk, ok := st.local[c.trunk] if !ok { res.Kept = append(res.Kept, KeptBranch{Name: c.name, Reason: KeptNoTrunk}) res.notice("%s is checked out here and its trunk %s has no local branch, so it was kept", c.name, c.trunk) continue } - if trunk.Worktree != "" && trunk.Worktree != st.repo.TopLevel { - if err := a.d.Git.SwitchDetached(ctx, st.repo, trunk.Head); err != nil { - return err - } - res.notice("%s was checked out here; HEAD is now detached at %s (%s) because %s is checked out in %s", c.name, c.trunk, short(trunk.Head), c.trunk, shortPath(trunk.Worktree)) - } else { - if err := a.d.Git.Switch(ctx, st.repo, c.trunk); err != nil { - return err - } - st.local[c.trunk] = withWorktree(trunk, st.repo.TopLevel) + moved, err := a.moveOffDeleted(ctx, st, c, trunk, res) + if err != nil { + return err + } + if !moved { + continue } default: res.Kept = append(res.Kept, KeptBranch{Name: c.name, Reason: KeptOtherWorktree}) @@ -124,6 +115,58 @@ func (a *App) deleteApproved(ctx context.Context, st *syncState, approved []cand return nil } +// moveOffDeleted switches the current checkout from a branch about to be +// deleted to its trunk (or to the trunk's tip, detached, when the trunk is +// checked out in another worktree). Uncommitted changes, untracked files +// included, come along the way git switch carries them; only when git +// refuses because they'd be overwritten is the branch kept and false +// returned. Any other failure is returned as is. +func (a *App) moveOffDeleted(ctx context.Context, st *syncState, c candidate, trunk git.Branch, res *SyncResult) (bool, error) { + dirty := st.isDirty(ctx, a.d.Git, st.repo.TopLevel) + if !dirty { + dirty, _ = a.d.Git.HasUntrackedFiles(ctx, st.repo) + } + detach := trunk.Worktree != "" && trunk.Worktree != st.repo.TopLevel + var err error + if detach { + err = a.d.Git.SwitchDetached(ctx, st.repo, trunk.Head) + } else { + err = a.d.Git.Switch(ctx, st.repo, c.trunk) + } + if oe, ok := errors.AsType[*git.OverwriteError](err); ok { + var what []string + if len(oe.Changed) > 0 { + what = append(what, "your changes to "+joinNames(oe.Changed)) + } + step := "commit or stash them and sync again" + if len(oe.Untracked) > 0 { + what = append(what, "untracked "+joinNames(oe.Untracked)) + step = "commit them, move them aside or stash them with git stash --include-untracked, then sync again" + } + res.Kept = append(res.Kept, KeptBranch{Name: c.name, Reason: KeptDirty}) + res.notice("%s is checked out here and switching to %s would overwrite %s, so it was kept; %s", + c.name, c.trunk, strings.Join(what, " and "), step) + return false, nil + } + if err != nil { + return false, err + } + carried := "" + if dirty { + carried = "; your uncommitted changes came along" + } + if detach { + res.notice("%s was checked out here; HEAD is now detached at %s (%s) because %s is checked out in %s%s", + c.name, c.trunk, short(trunk.Head), c.trunk, shortPath(trunk.Worktree), carried) + } else { + st.local[c.trunk] = withWorktree(trunk, st.repo.TopLevel) + if dirty { + res.notice("%s was checked out here, so you are now on %s%s", c.name, c.trunk, carried) + } + } + return true, nil +} + // short abbreviates a commit id for messages. func short(sha string) string { if len(sha) > 7 { diff --git a/pkg/app/sync_cleanup_test.go b/pkg/app/sync_cleanup_test.go index e43f7a8..33c44f7 100644 --- a/pkg/app/sync_cleanup_test.go +++ b/pkg/app/sync_cleanup_test.go @@ -336,10 +336,70 @@ func TestSyncMovesCurrentWorktreeOffDeletedBranch(t *testing.T) { } } -func TestSyncKeepsDeletedBranchWhenCurrentTreeIsDirty(t *testing.T) { +func TestSyncCarriesUncommittedChangesToTrunk(t *testing.T) { f := newSyncFixture(t) gittest.Run(t, f.dir, "switch", "-q", "a") f.mergeOnRemote(t, "a", 0) + // a.txt is the same on a and on the merged main, so git can carry the edit over. + gittest.WriteFile(t, f.dir, "a.txt", "edited") + res, err := f.sync(t, app.SyncOptions{NoRestack: true}) + if err != nil { + t.Fatal(err) + } + if _, ok := deleted(res, "a"); !ok || branchExists(t, f.dir, "a") { + t.Fatalf("a should be deleted: deleted = %+v kept = %+v", res.Deleted, res.Kept) + } + if cur := gittest.Run(t, f.dir, "branch", "--show-current"); cur != "main" { + t.Errorf("on %s, want main", cur) + } + if got := gittest.Run(t, f.dir, "status", "--porcelain"); got != "M a.txt" { + t.Errorf("status = %q; the edit should have come along", got) + } + if !strings.Contains(strings.Join(res.Notices, "\n"), "uncommitted changes came along") { + t.Errorf("notices = %v", res.Notices) + } +} + +func TestSyncSaysWhenOnlyUntrackedFilesCameAlong(t *testing.T) { + f := newSyncFixture(t) + gittest.Run(t, f.dir, "switch", "-q", "a") + f.mergeOnRemote(t, "a", 0) + gittest.WriteFile(t, f.dir, "notes.txt", "mine") + res, err := f.sync(t, app.SyncOptions{NoRestack: true}) + if err != nil { + t.Fatal(err) + } + if _, ok := deleted(res, "a"); !ok { + t.Fatalf("a should be deleted: kept = %+v", res.Kept) + } + if !strings.Contains(strings.Join(res.Notices, "\n"), "uncommitted changes came along") { + t.Errorf("notices = %v", res.Notices) + } +} + +func TestSyncKeepsDeletedBranchWhenSwitchWouldOverwriteUntrackedFiles(t *testing.T) { + f := newSyncFixture(t) + gittest.Run(t, f.dir, "switch", "-q", "a") + f.mergeOnRemote(t, "a", 0) + f.advanceRemote(t, "main", "new.txt") // main gains a file a doesn't have + gittest.WriteFile(t, f.dir, "new.txt", "mine") + res, err := f.sync(t, app.SyncOptions{NoRestack: true}) + if err != nil { + t.Fatal(err) + } + if k, ok := kept(res, "a"); !ok || k.Reason != app.KeptDirty { + t.Errorf("kept = %+v", res.Kept) + } + if n := strings.Join(res.Notices, "\n"); !strings.Contains(n, "untracked new.txt") || !strings.Contains(n, "--include-untracked") { + t.Errorf("notices = %v", res.Notices) + } +} + +func TestSyncKeepsDeletedBranchWhenSwitchWouldOverwriteChanges(t *testing.T) { + f := newSyncFixture(t) + gittest.Run(t, f.dir, "switch", "-q", "a") + f.mergeOnRemote(t, "a", 0) + f.advanceRemote(t, "main", "a.txt") // now a.txt differs between a and main gittest.WriteFile(t, f.dir, "a.txt", "edited") res, err := f.sync(t, app.SyncOptions{NoRestack: true}) if err != nil { @@ -348,6 +408,12 @@ func TestSyncKeepsDeletedBranchWhenCurrentTreeIsDirty(t *testing.T) { if k, ok := kept(res, "a"); !ok || k.Reason != app.KeptDirty || !branchExists(t, f.dir, "a") { t.Errorf("kept = %+v", res.Kept) } + if cur := gittest.Run(t, f.dir, "branch", "--show-current"); cur != "a" { + t.Errorf("on %s, want a", cur) + } + if !strings.Contains(strings.Join(res.Notices, "\n"), "would overwrite your changes to a.txt") { + t.Errorf("notices = %v", res.Notices) + } } func TestSyncDropsGoneBranchesFromMetadata(t *testing.T) { diff --git a/pkg/git/branch.go b/pkg/git/branch.go index 1766ede..60ed213 100644 --- a/pkg/git/branch.go +++ b/pkg/git/branch.go @@ -72,6 +72,9 @@ func (c *Client) Switch(ctx context.Context, repo Repo, name string) error { _, err := c.gitIn(ctx, repo, "switch", "--no-guess", name) if err != nil { if ee, ok := errors.AsType[*exec.ExitError](err); ok { + if oe := overwriteError(ee); oe != nil { + return oe + } return fmt.Errorf("switch to %q: %s", name, ee.Result.Err()) } return err @@ -124,6 +127,9 @@ func (c *Client) SwitchDetached(ctx context.Context, repo Repo, rev string) erro _, err := c.gitIn(ctx, repo, "switch", "--detach", rev) if err != nil { if ee, ok := errors.AsType[*exec.ExitError](err); ok { + if oe := overwriteError(ee); oe != nil { + return oe + } return fmt.Errorf("switch --detach %q: %s", rev, ee.Result.Err()) } return err diff --git a/pkg/git/refs.go b/pkg/git/refs.go index d74225f..2cf4f69 100644 --- a/pkg/git/refs.go +++ b/pkg/git/refs.go @@ -103,8 +103,8 @@ func (c *Client) MergeFF(ctx context.Context, repo Repo, rev string) error { _, err := c.gitIn(ctx, repo, "merge", "--ff-only", "--quiet", rev) if err != nil { if ee, ok := errors.AsType[*exec.ExitError](err); ok && LockPath(err) == "" { - if changed, untracked := parseOverwrite(ee.Result.Err()); len(changed)+len(untracked) > 0 { - return &OverwriteError{Changed: changed, Untracked: untracked} + if oe := overwriteError(ee); oe != nil { + return oe } return fmt.Errorf("fast forward: %s", firstLine(ee.Result.Err())) } @@ -197,7 +197,16 @@ func (e *OverwriteError) Error() string { if n := len(e.Untracked); n > 0 { parts = append(parts, fmt.Sprintf("%d untracked", n)) } - return "fast forward would overwrite " + strings.Join(parts, " and ") + " files" + return "moving the checkout would overwrite " + strings.Join(parts, " and ") + " files" +} + +// overwriteError is the *OverwriteError in a git failure, or nil when git +// failed for some other reason. +func overwriteError(ee *exec.ExitError) *OverwriteError { + if changed, untracked := parseOverwrite(ee.Result.Err()); len(changed)+len(untracked) > 0 { + return &OverwriteError{Changed: changed, Untracked: untracked} + } + return nil } // parseOverwrite reads the file lists out of git's "would be overwritten" diff --git a/pkg/git/worktree_test.go b/pkg/git/worktree_test.go index 2f6ce52..d5fbbc4 100644 --- a/pkg/git/worktree_test.go +++ b/pkg/git/worktree_test.go @@ -2,9 +2,11 @@ package git_test import ( "context" + "errors" "path/filepath" "testing" + "github.com/DomBlack/git-stack/pkg/git" "github.com/DomBlack/git-stack/pkg/git/gittest" ) @@ -39,3 +41,32 @@ func TestWorktrees(t *testing.T) { t.Errorf("detached worktree = %+v", wts[2]) } } + +func TestSwitchReportsFilesInTheWay(t *testing.T) { + gittest.Isolate(t) + dir := gittest.InitRepo(t) + c := newClient() + ctx := context.Background() + repo, err := c.Discover(ctx, dir) + if err != nil { + t.Fatal(err) + } + gittest.Run(t, dir, "switch", "-q", "-c", "side") + gittest.Commit(t, dir, "f.txt", "side", "side changes f") + gittest.WriteFile(t, dir, "f.txt", "edited") + gittest.WriteFile(t, dir, "other.txt", "carried") + + err = c.Switch(ctx, repo, "main") + oe, ok := errors.AsType[*git.OverwriteError](err) + if !ok || len(oe.Changed) != 1 || oe.Changed[0] != "f.txt" { + t.Fatalf("err = %v; want an OverwriteError naming f.txt", err) + } + + gittest.Run(t, dir, "checkout", "-q", "--", "f.txt") + if err := c.Switch(ctx, repo, "main"); err != nil { + t.Fatalf("a change git can carry over should not stop the switch: %v", err) + } + if got := gittest.Run(t, dir, "status", "--porcelain"); got != "?? other.txt" { + t.Errorf("status = %q", got) + } +}