diff --git a/app/src/lib/kept-reason.test.ts b/app/src/lib/kept-reason.test.ts new file mode 100644 index 000000000..d2d005b08 --- /dev/null +++ b/app/src/lib/kept-reason.test.ts @@ -0,0 +1,29 @@ +// node --experimental-strip-types --test src/lib/kept-reason.test.ts (pnpm test) +import assert from "node:assert/strict"; +import { test } from "node:test"; + +import { keptReason } from "./kept-reason.ts"; + +const box = (why: string) => `git worktree remove: '/home/acme/code/shop-fix' ${why}, use --force to delete it`; + +test("work git or the box would lose keeps the worktree, and says what it is", () => { + assert.match(keptReason(box("contains modified or untracked files"))!, /uncommitted changes/); + assert.match(keptReason(box("contains modified or untracked files in a submodule"))!, /uncommitted changes/); + assert.match(keptReason(box("contains submodule commits or stashes that no remote has"))!, /commits or stashes no remote has/); +}); + +test("a check that couldn't run still offers Remove anyway, without calling it uncommitted changes", () => { + for (const why of ["could not be checked for changes", "has submodules that could not be checked for unpushed work"]) { + const said = keptReason(box(why)); + assert.ok(said, why); + assert.doesNotMatch(said!, /uncommitted/); + } +}); + +test("other failures aren't a reason to force", () => { + assert.equal(keptReason("git worktree remove: fatal: 'shop-fix' is not a working tree"), null); + assert.equal(keptReason("not connected"), null); + // Words that only sound like unsaved work. + assert.equal(keptReason("git worktree remove: fatal: '/home/acme/code/shop-fix' contains a .git file that is not a gitlink"), null); + assert.equal(keptReason("git worktree remove: error: unable to unlink old 'src/modified.ts': Permission denied"), null); +}); diff --git a/app/src/lib/kept-reason.ts b/app/src/lib/kept-reason.ts new file mode 100644 index 000000000..f42905eb9 --- /dev/null +++ b/app/src/lib/kept-reason.ts @@ -0,0 +1,10 @@ +// keptReason reads a box's refusal to remove a worktree unforced: why it +// was kept, in words for the "Kept" toast that offers Remove anyway, or +// null when the error is something else that forcing wouldn't fix. +export function keptReason(msg: string): string | null { + if (/submodule commits/i.test(msg)) return "A submodule in it has commits or stashes no remote has, so it was left as it is."; + if (/could not be checked/i.test(msg)) return "Shipyard couldn't check it for work that removing it would lose, so it was left as it is."; + // git's own words for work it would lose, which the box's refusals use too. + if (/modified or untracked files/i.test(msg)) return "It has uncommitted changes, so git left it as it is."; + return null; +} diff --git a/app/src/lib/remove-worktree.ts b/app/src/lib/remove-worktree.ts index c81622336..b196e66e2 100644 --- a/app/src/lib/remove-worktree.ts +++ b/app/src/lib/remove-worktree.ts @@ -2,6 +2,7 @@ import { toastError } from "@/components/error-note"; import { toastManager } from "@/components/ui/toast"; import type { Client } from "@/lib/api"; import { errorMessage } from "@/lib/format"; +import { keptReason } from "@/lib/kept-reason"; import { unpinWorktree } from "@/lib/pinned-worktrees"; import { clearRemoving, markRemoving, markScript, pruneRemovals, type RemovalKind, removalOf, removeResult, useRemovals } from "@/lib/removing"; import { scheduleRefresh, useStore } from "@/lib/store"; @@ -71,12 +72,12 @@ async function drain(box: string) { await removeWorktreeOnBox(client, box, location, wt, "remove", { force }); } catch (err) { clearRemoving(box, wt.path); - // git's own words for work it would lose. - if (!force && /modified|untracked|uncommitted|contains/i.test(errorMessage(err))) { + const why = force ? null : keptReason(errorMessage(err)); + if (why) { toastManager.add({ type: "warning", title: `Kept ${wt.name}`, - description: "It has uncommitted changes, so git left it as it is.", + description: why, actionProps: { children: "Remove anyway", onClick: () => queueRemoval(box, location, wt, { force: true }) }, }); } else toastError(err, { title: `Couldn't remove ${wt.name}`, box }); diff --git a/docs/changelog.mdx b/docs/changelog.mdx index 9469864cc..82106b0a6 100644 --- a/docs/changelog.mdx +++ b/docs/changelog.mdx @@ -11,6 +11,14 @@ again. ## Unreleased +- **Worktrees with submodules can be removed.** git refuses to remove any + worktree with a submodule checked out, even a clean one, so removing or + archiving one failed. Shipyard now checks the worktree and its submodules + itself and removes it when nothing would be lost. If a submodule has + changes, stashes, or commits that aren't pushed anywhere (a commit only a + local tag points at counts), the worktree is kept and you can still choose + **Remove anyway**. + - **Pin worktrees to the top of the sidebar.** Right-click a worktree, or open its ⋯ menu, and choose **Pin to top**. Pinned worktrees sit in their own list above your projects, in the order you pinned them, whether or not diff --git a/internal/box/locations.go b/internal/box/locations.go index c9367126a..ec985aa96 100644 --- a/internal/box/locations.go +++ b/internal/box/locations.go @@ -417,7 +417,17 @@ func (l *Locations) RemoveWorktree(ctx context.Context, location, name string, f if force { args = append(args, "--force") } - if out, err := git(ctx, args...); err != nil { + out, err := git(ctx, args...) + // git refuses any worktree with a submodule checked out, however + // clean, and only --force gets past it. Look for the work git won't, + // and force only when there is none to lose. + if err != nil && !force && strings.Contains(string(out), "containing submodules") { + if why := unsavedWork(ctx, w.Path); why != "" { + return httpError{status: http.StatusConflict, msg: fmt.Sprintf("git worktree remove: '%s' %s, use --force to delete it", w.Path, why)} + } + out, err = git(ctx, append(args, "--force")...) + } + if err != nil { return fmt.Errorf("git worktree remove: %s", strings.TrimSpace(string(out))) } // A worktree made again under the same name starts without a title. @@ -549,6 +559,106 @@ func dirtyWorktree(dir string) bool { return err == nil && strings.TrimSpace(string(out)) != "" } +// unsavedWork says what removing a worktree with submodules would lose, or +// "" for nothing: changes in it or its submodules, or submodule commits or +// stashes no remote has (any local ref's: a branch's, a tag's, the stash), +// since a worktree keeps its submodules' repositories inside its own git +// folder. A ref's name on history a remote has is not kept: whether a +// remote has the tag itself would take asking it, on every removal. +// Untracked files count whatever status.showUntrackedFiles says, and when +// git can't tell, that counts as something to lose. +func unsavedWork(ctx context.Context, dir string) string { + out, err := git(ctx, "-C", dir, "status", "--porcelain", "--untracked-files=normal", "--ignore-submodules=none") + if err != nil { + return "could not be checked for changes" + } + if strings.TrimSpace(string(out)) != "" { + return "contains modified or untracked files" + } + // Each checked-out submodule is asked itself too, so its own config + // (status.showUntrackedFiles, submodule..ignore for the ones + // inside it) can't hide a change from the status above. + out, err = git(ctx, "-C", dir, "submodule", "foreach", "--quiet", "--recursive", + "git status --porcelain --untracked-files=normal --ignore-submodules=none") + if err != nil { + return "has submodules that could not be checked for changes" + } + if strings.TrimSpace(string(out)) != "" { + return "contains modified or untracked files in a submodule" + } + // Every submodule repository the removal deletes, against every local + // ref it has: those in the worktree's git folder, checked out or not (a + // deinit leaves one behind), and those a checked-out submodule keeps in + // its own .git (one made with git clone), with the ones under that .git + // in turn. + gitDir, err := git(ctx, "-C", dir, "rev-parse", "--absolute-git-dir") + if err != nil { + return "has submodules that could not be checked for unpushed work" + } + repos, err := moduleRepos(filepath.Join(strings.TrimSpace(string(gitDir)), "modules")) + if err != nil { + return "has submodules that could not be checked for unpushed work" + } + out, err = git(ctx, "-C", dir, "submodule", "foreach", "--quiet", "--recursive", "git rev-parse --absolute-git-dir") + if err != nil { + return "has submodules that could not be checked for unpushed work" + } + for _, repo := range strings.Split(strings.TrimSpace(string(out)), "\n") { + if repo == "" { + continue + } + nested, err := moduleRepos(filepath.Join(repo, "modules")) + if err != nil { + return "has submodules that could not be checked for unpushed work" + } + for _, r := range append([]string{repo}, nested...) { + if !slices.Contains(repos, r) { + repos = append(repos, r) + } + } + } + for _, repo := range repos { + out, err := git(ctx, "--git-dir="+repo, "rev-list", "-1", "--all", "--not", "--remotes") + if err != nil { + return "has submodules that could not be checked for unpushed work" + } + if strings.TrimSpace(string(out)) != "" { + return "contains submodule commits or stashes that no remote has" + } + } + return "" +} + +// moduleRepos lists the submodule repositories git keeps under a modules +// folder, nested submodules' included. A name may hold slashes, so any +// folder that isn't a repository is looked into. +func moduleRepos(dir string) ([]string, error) { + entries, err := os.ReadDir(dir) + if os.IsNotExist(err) { + return nil, nil + } + if err != nil { + return nil, err + } + var repos []string + for _, e := range entries { + if !e.IsDir() { + continue + } + p := filepath.Join(dir, e.Name()) + if _, err := os.Stat(filepath.Join(p, "HEAD")); err == nil { + repos = append(repos, p) + p = filepath.Join(p, "modules") + } + nested, err := moduleRepos(p) + if err != nil { + return nil, err + } + repos = append(repos, nested...) + } + return repos, nil +} + func expandHome(path string) string { if path == "~" || strings.HasPrefix(path, "~/") { if home, err := os.UserHomeDir(); err == nil { diff --git a/internal/box/locations_test.go b/internal/box/locations_test.go index 5898deecb..9488844d7 100644 --- a/internal/box/locations_test.go +++ b/internal/box/locations_test.go @@ -215,6 +215,201 @@ func TestAWorktreeSomeoneLockedIsLeftAlone(t *testing.T) { } } +// submoduleGit runs git in dir for a test, local submodule clones allowed. +func submoduleGit(t *testing.T) func(dir string, args ...string) { + return func(dir string, args ...string) { + t.Helper() + cmd := exec.Command("git", append([]string{"-c", "protocol.file.allow=always"}, args...)...) + cmd.Dir = dir + cmd.Env = append(os.Environ(), "GIT_AUTHOR_NAME=t", "GIT_AUTHOR_EMAIL=t@example.com", "GIT_COMMITTER_NAME=t", "GIT_COMMITTER_EMAIL=t@example.com") + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %s", args, out) + } + } +} + +func TestAWorktreeWithSubmodulesIsRemovedWhenNothingWouldBeLost(t *testing.T) { + repo := gitRepo(t) + ctx := context.Background() + run := submoduleGit(t) + lib := filepath.Join(filepath.Dir(repo), "lib") + os.MkdirAll(lib, 0o755) + run(lib, "init", "-q", "-b", "main") + run(lib, "commit", "-q", "--allow-empty", "-m", "lib") + run(repo, "submodule", "add", "-q", lib, "lib") + run(repo, "commit", "-q", "-m", "add lib") + + l := NewLocations(filepath.Join(t.TempDir(), "locations.json")) + if _, err := l.Add(ctx, "cal", repo); err != nil { + t.Fatal(err) + } + wt, err := l.CreateWorktree(ctx, "cal", "vendor", "vendor", "main") + if err != nil { + t.Fatal(err) + } + run(wt.Path, "submodule", "update", "-q", "--init") + sub := filepath.Join(wt.Path, "lib") + + kept := func(what, want string) { + t.Helper() + err := l.RemoveWorktree(ctx, "cal", "vendor", false) + if err == nil || !strings.Contains(err.Error(), want) || statusFor(err) != 409 { + t.Fatalf("%s: err = %v (status %d), want a 409 saying %q", what, err, statusFor(err), want) + } + if _, err := os.Stat(wt.Path); err != nil { + t.Fatalf("%s: the worktree went: %v", what, err) + } + } + os.WriteFile(filepath.Join(sub, "notes"), []byte("wip"), 0o644) + kept("a file in the submodule", "modified or untracked") + // Config that hides untracked files from status doesn't hide them here. + run(wt.Path, "config", "status.showUntrackedFiles", "no") + run(sub, "config", "status.showUntrackedFiles", "no") + kept("a file in the submodule, hidden from status", "untracked files in a submodule") + os.WriteFile(filepath.Join(wt.Path, "todo"), []byte("wip"), 0o644) + os.Remove(filepath.Join(sub, "notes")) + kept("a file in the worktree, hidden from status", "modified or untracked") + os.Remove(filepath.Join(wt.Path, "todo")) + + os.WriteFile(filepath.Join(sub, "notes"), []byte("wip"), 0o644) + run(sub, "stash", "push", "-q", "-u") + kept("a stash in the submodule", "stashes that no remote has") + run(sub, "stash", "drop", "-q") + + run(sub, "commit", "-q", "--allow-empty", "-m", "local only") + kept("a submodule commit, uncommitted in the worktree", "modified or untracked") + // Committed in the worktree, git status is clean, but the submodule's + // commit is only in the worktree's own copy of its history. + run(wt.Path, "commit", "-q", "-am", "bump lib") + kept("a submodule commit no remote has", "submodule commits or stashes that no remote has") + run(sub, "push", "-q", "origin", "HEAD:refs/heads/shared") + + // A commit only a local tag holds, with the submodule back where the + // worktree has it: clean, but the tag's commit would go. + run(sub, "commit", "-q", "--allow-empty", "-m", "tagged") + run(sub, "-c", "tag.gpgSign=false", "tag", "keep") + run(sub, "checkout", "-q", "HEAD~1") + kept("a commit only a submodule tag holds", "submodule commits or stashes") + run(sub, "tag", "-d", "keep") + + // A deinit takes the submodule's checkout, not its repository: its + // branches stay in the worktree's git folder, and so go with it. + run(sub, "commit", "-q", "--allow-empty", "-m", "branched") + run(sub, "branch", "keep") + run(sub, "checkout", "-q", "HEAD~1") + run(wt.Path, "submodule", "deinit", "-q", "-f", "lib") + kept("a branch in a deinitialized submodule", "submodule commits or stashes") + out, err := exec.Command("git", "-C", wt.Path, "rev-parse", "--absolute-git-dir").Output() + if err != nil { + t.Fatal(err) + } + run(wt.Path, "--git-dir="+filepath.Join(strings.TrimSpace(string(out)), "modules", "lib"), "branch", "-D", "-q", "keep") + + // A submodule cloned by hand keeps its repository in its own .git, + // inside the worktree's folder, and so goes with it too. + run(wt.Path, "clone", "-q", lib, "lib") + run(sub, "checkout", "-q", "origin/shared") + run(sub, "commit", "-q", "--allow-empty", "-m", "embedded") + run(wt.Path, "commit", "-q", "-am", "bump lib again") + kept("a commit in an embedded submodule repository", "submodule commits or stashes") + run(sub, "push", "-q", "origin", "HEAD:refs/heads/embedded") + + if err := l.RemoveWorktree(ctx, "cal", "vendor", false); err != nil { + t.Fatalf("a clean worktree with a submodule: %v", err) + } + if _, err := os.Stat(wt.Path); !os.IsNotExist(err) { + t.Fatalf("worktree folder still there: %v", err) + } +} + +func TestADeinitializedSubmoduleInsideAClonedOneIsChecked(t *testing.T) { + repo := gitRepo(t) + ctx := context.Background() + run := submoduleGit(t) + // cal has lib, and lib has deep. + deep := filepath.Join(filepath.Dir(repo), "deep") + lib := filepath.Join(filepath.Dir(repo), "lib") + os.MkdirAll(deep, 0o755) + os.MkdirAll(lib, 0o755) + run(deep, "init", "-q", "-b", "main") + run(deep, "commit", "-q", "--allow-empty", "-m", "deep") + run(lib, "init", "-q", "-b", "main") + run(lib, "submodule", "add", "-q", deep, "deep") + run(lib, "commit", "-q", "-m", "add deep") + run(repo, "submodule", "add", "-q", lib, "lib") + run(repo, "commit", "-q", "-m", "add lib") + + l := NewLocations(filepath.Join(t.TempDir(), "locations.json")) + if _, err := l.Add(ctx, "cal", repo); err != nil { + t.Fatal(err) + } + wt, err := l.CreateWorktree(ctx, "cal", "vendor", "vendor", "main") + if err != nil { + t.Fatal(err) + } + // lib cloned by hand: deep's repository lives in lib's own .git. A + // branch only deep has, then deep deinitialized. + sub := filepath.Join(wt.Path, "lib") + run(wt.Path, "clone", "-q", lib, "lib") + run(sub, "submodule", "update", "-q", "--init") + run(filepath.Join(sub, "deep"), "commit", "-q", "--allow-empty", "-m", "only here") + run(filepath.Join(sub, "deep"), "branch", "keep") + run(filepath.Join(sub, "deep"), "checkout", "-q", "HEAD~1") + run(sub, "submodule", "deinit", "-q", "-f", "deep") + + err = l.RemoveWorktree(ctx, "cal", "vendor", false) + if err == nil || !strings.Contains(err.Error(), "submodule commits or stashes") || statusFor(err) != 409 { + t.Fatalf("err = %v (status %d), want a 409 about submodule commits", err, statusFor(err)) + } + run(sub, "--git-dir="+filepath.Join(sub, ".git", "modules", "deep"), "branch", "-D", "-q", "keep") + if err := l.RemoveWorktree(ctx, "cal", "vendor", false); err != nil { + t.Fatalf("with nothing left to lose: %v", err) + } +} + +func TestANestedSubmoduleMovedBehindAnIgnoreIsChecked(t *testing.T) { + repo := gitRepo(t) + ctx := context.Background() + run := submoduleGit(t) + // cal has lib, and lib has deep at deep's second commit. + deep := filepath.Join(filepath.Dir(repo), "deep") + lib := filepath.Join(filepath.Dir(repo), "lib") + os.MkdirAll(deep, 0o755) + os.MkdirAll(lib, 0o755) + run(deep, "init", "-q", "-b", "main") + run(deep, "commit", "-q", "--allow-empty", "-m", "deep") + run(deep, "commit", "-q", "--allow-empty", "-m", "deep 2") + run(lib, "init", "-q", "-b", "main") + run(lib, "submodule", "add", "-q", deep, "deep") + run(lib, "commit", "-q", "-m", "add deep") + run(repo, "submodule", "add", "-q", lib, "lib") + run(repo, "commit", "-q", "-m", "add lib") + + l := NewLocations(filepath.Join(t.TempDir(), "locations.json")) + if _, err := l.Add(ctx, "cal", repo); err != nil { + t.Fatal(err) + } + wt, err := l.CreateWorktree(ctx, "cal", "vendor", "vendor", "main") + if err != nil { + t.Fatal(err) + } + run(wt.Path, "submodule", "update", "-q", "--init", "--recursive") + sub := filepath.Join(wt.Path, "lib") + // lib ignores deep, and deep moves to a commit its remote has: only + // asking lib itself, ignores overridden, shows the move. + run(sub, "config", "submodule.deep.ignore", "all") + run(filepath.Join(sub, "deep"), "checkout", "-q", "HEAD~1") + + err = l.RemoveWorktree(ctx, "cal", "vendor", false) + if err == nil || !strings.Contains(err.Error(), "files in a submodule") || statusFor(err) != 409 { + t.Fatalf("err = %v (status %d), want a 409 about changes in a submodule", err, statusFor(err)) + } + run(filepath.Join(sub, "deep"), "checkout", "-q", "main") + if err := l.RemoveWorktree(ctx, "cal", "vendor", false); err != nil { + t.Fatalf("with deep back where lib has it: %v", err) + } +} + func TestAFailedAddLeavesNothingBehind(t *testing.T) { repo := gitRepo(t) ctx := context.Background()