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) + } +}