diff --git a/docs/changes/unreleased/1099-no-git-command-without-a-directory.md b/docs/changes/unreleased/1099-no-git-command-without-a-directory.md new file mode 100644 index 0000000000..d6bdf92d81 --- /dev/null +++ b/docs/changes/unreleased/1099-no-git-command-without-a-directory.md @@ -0,0 +1,14 @@ +--- +kind: fixed +title: No git command runs without a directory, and the checkout guard reads a linked worktree's own share +pr: 1099 +surface: [engine] +invalidates: + - "`worktreeDirtIn` and `UnsavedEditsNote` built `git -C ` themselves, and `git -C \"\"` is a no-op, so an empty directory ran the command where the process stood. Both go through `gitWith` now (`gitContext` is the same call under a caller's context), where the refusal of an empty directory and the pinned environment live once." + - "`taskTree.releaseLanded` removed a worktree and deleted its branch with no empty-root guard and without the root lock. It refuses a tree with no root or directory and takes the root lock, as `releaseKept` does." + - "The session package's checkout guard read branches and worktree registrations for the whole shared clone, so from a linked worktree a sibling's task/* churn was reported as the run's damage, and its live-codeaf probe walked `/proc`, which does not exist on darwin. From a linked worktree the branch reading is skipped and the worktree reading keeps only registrations under the checkout; the probe reads the process table." +--- + +A command with no directory runs wherever the process happens to be. Every +spelling of a git call in this package is a wrapper of one function that +refuses that, and the two that had gone round it are wrappers of it now. diff --git a/internal/session/groundladder.go b/internal/session/groundladder.go index cf16c1a929..c3cb24dc4f 100644 --- a/internal/session/groundladder.go +++ b/internal/session/groundladder.go @@ -1147,6 +1147,19 @@ func (t taskTree) carryBranchHomeLocked() (string, error) { // session's own and goes with the work it carried; what furrow is told is that // the record may be forgotten ([taskTree.dropUniverse]). func (t taskTree) releaseLanded() { + // THE SAME DOOR [taskTree.releaseKept] closes: a tree whose ground was never + // made has nowhere a worktree could be registered and no repository to ask, + // and a command with no directory runs in the process's own directory — so + // the removal below would take a working copy out of somebody else's + // checkout and the branch deletion would name a branch in it. + if strings.TrimSpace(t.root) == "" || strings.TrimSpace(t.dir) == "" { + return + } + // THE ROOT LOCK IS THE CALLER'S. Every road that reaches here is a landing + // that already holds it ([taskTree.carryBranchHome]'s span, task_run.go), + // the same way [taskTree.releaseKeptLocked] is reached, and the lock is not + // re-entrant: taking it again here waited on itself for the whole of a + // suite's timeout. if t.ownRepository() { _ = os.RemoveAll(t.dir) } else if _, err := git(t.root, "worktree", "remove", t.dir); err != nil { diff --git a/internal/session/hermetic_checkout_test.go b/internal/session/hermetic_checkout_test.go index 3388179be8..b3f10ca26e 100644 --- a/internal/session/hermetic_checkout_test.go +++ b/internal/session/hermetic_checkout_test.go @@ -4,7 +4,9 @@ import ( "fmt" "os" "os/exec" + "path/filepath" "sort" + "strconv" "strings" "testing" ) @@ -66,6 +68,15 @@ import ( // do not care what anything is called and still catch the commit and the files, // which is the shape that actually cost somebody an afternoon. // +// AND A LINKED WORKTREE OF A SHARED CLONE READS LESS STILL. Branches belong to +// the clone's common directory, not to the working copy they were read from, so +// from a linked worktree every sibling's task/* churn — a branch cut for their +// own run, a branch deleted when their run landed — would look exactly like +// this run's damage, and the branch reading is skipped outright there. The +// worktree reading narrows instead of skipping: registrations under this root +// can still be this suite's, and a sibling's registration elsewhere in the +// clone cannot. +// // It cannot see a content change to a file that was already dirty before the // run — hashing the whole checkout each run costs more than it is worth — and // the incident's shape, new commits, new files, new branches and new worktrees, @@ -73,9 +84,12 @@ import ( // checkout is what the tree looked like at a moment: the commit it stood on, // the paths git considered dirty or unknown, the branches it had, and the -// worktrees registered against it. +// worktrees registered against it. When the tree is a linked worktree of a +// shared clone, branches is nil — the reading is skipped, not empty — and the +// worktrees are only the ones registered under the tree itself. type checkout struct { root string + linked bool head string dirty map[string]bool branches map[string]bool @@ -99,12 +113,38 @@ func watchTheCheckout() checkout { if watched.head, err = gitLine(root, "rev-parse", "HEAD"); err != nil { return checkout{} } + watched.linked = isLinkedWorktree(root) + if !watched.linked { + watched.branches = branchNames(root) + } watched.dirty = porcelain(root) - watched.branches = branchNames(root) - watched.worktrees = worktreePaths(root) + watched.worktrees = worktreePaths(root, watched.linked) return watched } +// isLinkedWorktree says whether the working copy at root is one worktree among +// several of one shared clone, which `rev-parse` answers by naming two +// different directories: the repository this copy carries its state in, and +// the common one every sibling of the same clone points at. A plain checkout +// answers the same directory twice. +func isLinkedWorktree(root string) bool { + gitDir, err := gitLine(root, "rev-parse", "--git-dir") + if err != nil || gitDir == "" { + return false + } + commonDir, err := gitLine(root, "rev-parse", "--git-common-dir") + if err != nil || commonDir == "" { + return false + } + if !filepath.IsAbs(gitDir) { // a plain checkout answers relative to root + gitDir = filepath.Join(root, gitDir) + } + if !filepath.IsAbs(commonDir) { + commonDir = filepath.Join(root, commonDir) + } + return filepath.Clean(gitDir) != filepath.Clean(commonDir) +} + // moved says how the checkout differs from what it was, and "" when it does // not. A watch that was never taken never reports. func (c checkout) moved() string { @@ -127,33 +167,40 @@ func (c checkout) moved() string { if moved := differing(c.dirty, porcelain(c.root)); len(moved) > 0 { said = append(said, "these paths are not what they were before the run:\n\t"+strings.Join(moved, "\n\t")) } - // A BRANCH IS A WRITE THAT MOVES NOTHING. `task/do-the-thing-` cut in - // the person's repository leaves HEAD exactly where it was and leaves the - // porcelain empty, and it is still the suite committing into a tree it does - // not own — the merge that would have moved HEAD is the only part that did - // not happen yet. - // - // AND A LIVE codeaf CREATES THE SAME BRANCHES. When another codeaf is using - // this checkout and TMPDIR is outside it, branch and worktree churn is - // ambient the way journal growth is (hermetic_test.go) — report the head - // and the porcelain, which a neighbour's ordinary work does not move, and - // leave the shared refs alone. The #578 TempDir-inside-checkout case still - // reports everything, because that is when the suite itself can mint them. - ignoreAmbientRefs := liveCodeafUsingCheckout(c.root) && !tempDirInsideCheckout(c.root) - if !ignoreAmbientRefs { - if moved := differing(c.branches, branchNames(c.root)); len(moved) > 0 { - said = append(said, "these branches are not what they were before the run:\n\t"+strings.Join(moved, "\n\t")) - } - // AND A WORKTREE IS A WRITE GIT IS TOLD TO IGNORE. The ground ladder puts a - // task's tree under `.codeaf/`, which .gitignore covers, so the porcelain - // above stays silent about a whole second checkout sitting in the person's - // tree. The registration is not ignorable: git keeps it, so it is asked for. - if moved := differing(c.worktrees, worktreePaths(c.root)); len(moved) > 0 { + // BRANCHES AND WORKTREES BELONG TO THE WHOLE SHARED CLONE the checkout was + // read from, and a LINKED worktree reads even less than the ambient case: + // every sibling of the same clone churns the same task/* refs this suite + // could not have written, so from a linked worktree the branch reading is + // skipped outright and the worktree reading keeps only the registrations + // under the checkout itself. The head and the porcelain above still watch + // everything, whatever the checkout is. + if c.linked { + if moved := differing(c.worktrees, worktreePaths(c.root, true)); len(moved) > 0 { said = append(said, "these worktrees are not what they were before the run:\n\t"+strings.Join(moved, "\n\t")) } - } else if moved := differing(c.branches, branchNames(c.root)); len(moved) > 0 || - len(differing(c.worktrees, worktreePaths(c.root))) > 0 { - fmt.Fprintf(os.Stderr, "checkout guard: another codeaf is using %s; ignoring task branch/worktree churn (head and porcelain still watched)\n", c.root) + } else { + // AND A LIVE codeaf CREATES THE SAME BRANCHES. When another codeaf is using + // this checkout and TMPDIR is outside it, branch and worktree churn is + // ambient the way journal growth is (hermetic_test.go) — report the head + // and the porcelain, which a neighbour's ordinary work does not move, and + // leave the shared refs alone. The #578 TempDir-inside-checkout case still + // reports everything, because that is when the suite itself can mint them. + ignoreAmbientRefs := liveCodeafUsingCheckout(c.root) && !tempDirInsideCheckout(c.root) + if !ignoreAmbientRefs { + if moved := differing(c.branches, branchNames(c.root)); len(moved) > 0 { + said = append(said, "these branches are not what they were before the run:\n\t"+strings.Join(moved, "\n\t")) + } + // AND A WORKTREE IS A WRITE GIT IS TOLD TO IGNORE. The ground ladder puts a + // task's tree under `.codeaf/`, which .gitignore covers, so the porcelain + // above stays silent about a whole second checkout sitting in the person's + // tree. The registration is not ignorable: git keeps it, so it is asked for. + if moved := differing(c.worktrees, worktreePaths(c.root, false)); len(moved) > 0 { + said = append(said, "these worktrees are not what they were before the run:\n\t"+strings.Join(moved, "\n\t")) + } + } else if moved := differing(c.branches, branchNames(c.root)); len(moved) > 0 || + len(differing(c.worktrees, worktreePaths(c.root, false))) > 0 { + fmt.Fprintf(os.Stderr, "checkout guard: another codeaf is using %s; ignoring task branch/worktree churn (head and porcelain still watched)\n", c.root) + } } if len(said) == 0 { return "" @@ -167,7 +214,7 @@ func (c checkout) moved() string { "`rev-parse --show-toplevel` answers with this checkout (task_run.go's repositoryRoot, " + "#578). Which one it was, this guard cannot tell you; if the run had GOTMPDIR or TMPDIR " + "pointing inside a checkout, suspect the second. Undo the above before pushing.\n" + - "(a second checkout running this same suite beside you cannot cause this; a test that names no directory can, and so can a temporary directory that is not outside your tree. A live codeaf using this checkout is ignored for branch/worktree churn when TMPDIR is outside the tree.)" + "(a second checkout running this same suite beside you cannot cause this; a test that names no directory can, and so can a temporary directory that is not outside your tree. A live codeaf using this checkout is ignored for branch/worktree churn when TMPDIR is outside the tree. When this checkout is a linked worktree of a shared clone, branch churn is not reported at all and the worktree reading covers only registrations under the checkout, because both belong to the clone the siblings share — a leak that only cut a task/ branch from inside a linked worktree slips past here, and the head and the porcelain above are what catch it.)" } // differing names every line that is in one listing and not the other, marked @@ -249,9 +296,12 @@ func harnessBranch(name string) bool { return strings.HasPrefix(name, harnessBranchPrefix) } -// worktreePaths is the set of worktree directories registered against root that -// THIS SUITE could have registered — including one it hid under an ignored -// folder, which is the whole reason this reading exists. +// worktreePaths is the set of worktree directories registered against root +// that THIS SUITE could have registered — including one it hid under an ignored +// folder, which is the whole reason this reading exists. For a linked worktree +// it is asked with underRoot, because the listing answers for the whole shared +// clone and only the registrations under the checkout itself can be the +// suite's. // // ONLY THE PATHS ARE KEPT. The plain listing carries each worktree's head // beside its path, so keeping the whole line would report a head move a second @@ -259,12 +309,22 @@ func harnessBranch(name string) bool { // question. The porcelain form is asked for so the path and the branch can be // read separately, and an unreadable tree is an empty set as everywhere else // here. -func worktreePaths(root string) map[string]bool { +func worktreePaths(root string, underRoot bool) map[string]bool { out, err := gitLines(root, "worktree", "list", "--porcelain") if err != nil { return map[string]bool{} } - return harnessWorktrees(root, out) + found := harnessWorktrees(root, out) + if !underRoot { + return found + } + under := strings.TrimSuffix(root, "/") + "/" + for path := range found { + if !strings.HasPrefix(path, under) { + delete(found, path) + } + } + return found } // harnessWorktrees reads `git worktree list --porcelain` — one record per @@ -440,58 +500,104 @@ func TestTheCheckoutGuardReportsOnlyWhatThisRunCouldHaveWritten(t *testing.T) { } } +// TestTheCheckoutGuardDoesNotReportASiblingWorktreesBranch is the shared-clone +// case, run for real: a linked worktree stands beside a sibling in one clone, +// the sibling cuts and takes away task/ branches and registers a worktree of +// its own while "the run" is watched, and none of it may read as this run's +// damage — branches belong to the clone's common directory, and the sibling's +// registrations stand outside the watched tree. The second half proves the +// churn is real: watched from the main checkout, where the refs are this +// checkout's own, the same branch deletion is reported. +func TestTheCheckoutGuardDoesNotReportASiblingWorktreesBranch(t *testing.T) { + shared := newTestRepo(t) + linked := filepath.Join(shared, "linked") + sibling := filepath.Join(shared, "sibling") + mustGit(t, shared, "worktree", "add", "-b", "task/this-run-0a1b2c", linked) + mustGit(t, shared, "worktree", "add", "-b", "task/sibling-run-9f8e7d", sibling) + // A branch the sibling will take away during the run, seen only through + // the clone's shared refs. + mustGit(t, shared, "branch", "task/sibling-gone-554433") + + t.Chdir(linked) + watched := watchTheCheckout() + if !watched.linked { + t.Fatal("the linked worktree was not read as linked, so this test is not standing where it means to") + } + // THE SAME REFS READ FROM THE MAIN CHECKOUT, before the churn, are this + // checkout's own — that watch is the contrast the narrowing has to hold + // against. + head, _ := gitLine(shared, "rev-parse", "HEAD") + watchedMain := checkout{root: shared, head: head, + dirty: porcelain(shared), branches: branchNames(shared), worktrees: worktreePaths(shared, false)} + + mustGit(t, sibling, "branch", "task/sibling-cuts-778899") + mustGit(t, sibling, "branch", "-D", "task/sibling-gone-554433") + mustGit(t, shared, "worktree", "add", "-b", "task/sibling-tree-112233", filepath.Join(shared, "sibling-tree")) + + if said := watched.moved(); said != "" { + t.Fatalf("a sibling worktree's churn was reported as this run's damage:\n%s", said) + } + + // AND THE SAME CHURN IS REPORTED FROM THE MAIN CHECKOUT — the narrowing + // above is tied to the linked worktree, not to the branch having been + // someone else's. + if said := watchedMain.moved(); !strings.Contains(said, "gone: task/sibling-gone-554433") { + t.Fatalf("from the main checkout the sibling's deleted branch was not reported:\n%s", said) + } +} + // liveCodeafUsingCheckout reports whether another codeaf process looks like it // is working in this repository right now — a chat, a tick, a hosted session — // whose task/ branches and `.codeaf/` worktrees would otherwise look exactly // like the suite's own leak. // -// IT IS BEST-EFFORT AND FAILS OPEN TO "NO". A machine without /proc, a binary +// THE QUESTION IS ASKED OF THE PROCESS TABLE, which exists wherever this +// program does: any other process whose command line names a worktree of the +// repository root belongs to is taken for a live codeaf. The worktrees come +// from `git worktree list --porcelain` — the checkout itself, and every tree +// registered against the same clone. A /proc walk was tried first and +// abandoned: it exists on Linux only, so on darwin the answer was always +// "no", and the ambient reading never engaged on the machine this suite is +// written on. +// +// IT IS BEST-EFFORT AND FAILS OPEN TO "NO". A machine without ps, a binary // renamed something else, or a sandbox that hides other processes simply keeps // the stricter reading; a false negative is a red that a person can re-run with // codeaf closed, and a false positive would hide a real suite write. func liveCodeafUsingCheckout(root string) bool { - under := strings.TrimSuffix(root, "/") + "/" - entries, err := os.ReadDir("/proc") + lines, err := gitLines(root, "worktree", "list", "--porcelain") if err != nil { return false } - self := os.Getpid() - for _, entry := range entries { - if !entry.IsDir() { - continue - } - name := entry.Name() - if len(name) == 0 || name[0] < '0' || name[0] > '9' { - continue - } - var pid int - for i := 0; i < len(name); i++ { - if name[i] < '0' || name[i] > '9' { - pid = -1 - break - } - pid = pid*10 + int(name[i]-'0') + places := map[string]bool{root: true} + for _, line := range lines { + if rest, ok := strings.CutPrefix(line, "worktree "); ok { + places[strings.TrimSpace(rest)] = true } - if pid <= 0 || pid == self { + } + out, err := exec.Command("ps", "-axo", "pid=,command=").Output() + if err != nil { + return false + } + self := os.Getpid() + for _, line := range strings.Split(string(out), "\n") { + pid, command, found := strings.Cut(strings.TrimSpace(line), " ") + if !found { continue } - cmdline, err := os.ReadFile("/proc/" + name + "/cmdline") - if err != nil { + // The suite's own test binary stands IN a worktree but does not name one + // on its command line, so it never reads as a neighbour here. + own, err := strconv.Atoi(strings.TrimSpace(pid)) + if err != nil || own <= 0 || own == self { continue } - joined := strings.ReplaceAll(string(cmdline), "\x00", " ") - if !strings.Contains(joined, "codeaf") { + if !strings.Contains(command, "codeaf") { continue } - // The suite's own test binary and helpers name codeaf too (`go test`, - // `codeaf-session-test-home`). A live chat is the binary itself, or a - // tick/host child, with the checkout in its cwd or its arguments. - cwd, err := os.Readlink("/proc/" + name + "/cwd") - if err == nil && (cwd == root || strings.HasPrefix(cwd, under)) { - return true - } - if strings.Contains(joined, root) { - return true + for place := range places { + if strings.Contains(command, place) { + return true + } } } return false diff --git a/internal/session/task_release_landed_test.go b/internal/session/task_release_landed_test.go new file mode 100644 index 0000000000..4a815758b6 --- /dev/null +++ b/internal/session/task_release_landed_test.go @@ -0,0 +1,69 @@ +package session + +import ( + "os" + "strings" + "testing" +) + +// A TREE WITH NO GROUND IS LEFT ALONE BY ITS RELEASE. +// +// [taskTree.releaseKept] refuses a tree with an empty root or directory, for +// the reason gitWith does: a command with no directory runs wherever the +// process happens to be, and a removal and a branch deletion pointed there +// take apart the checkout the process is standing in. [taskTree.releaseLanded] +// is the mirror image of releaseKept and was missing both the refusal and the +// root lock; these tests hold the refusal in place. + +// TestReleaseLandedLeavesAnEmptyTreeAlone proves the empty case never runs git +// at all: the tree is released while the process stands inside a real +// repository, and neither that repository's worktrees nor its branches move. +func TestReleaseLandedLeavesAnEmptyTreeAlone(t *testing.T) { + repo := newTestRepo(t) + for _, tree := range []taskTree{ + {}, // nothing was ever grounded + {dir: repo}, // a directory with no root + {root: repo}, // a root with no directory + {root: " ", dir: " ", branch: "task/x"}, // whitespace is emptiness too + } { + before := gitOut(t, repo, "worktree", "list", "--porcelain") + branches := gitOut(t, repo, "branch", "--list", "--format=%(refname:short)") + + t.Chdir(repo) + tree.releaseLanded() + + if after := gitOut(t, repo, "worktree", "list", "--porcelain"); after != before { + t.Errorf("releasing %+v changed the registered worktrees:\n%s", tree, after) + } + if after := gitOut(t, repo, "branch", "--list", "--format=%(refname:short)"); after != branches { + t.Errorf("releasing %+v changed the branches:\n%s", tree, after) + } + if _, err := os.Stat(repo); err != nil { + t.Fatalf("releasing %+v disturbed the repository: %v", tree, err) + } + } +} + +// TestReleaseLandedRemovesItsWorktreeAndBranch is the ordinary case beside it: +// a released landing still unregisters its worktree and deletes its branch, +// so the guard above cannot be read as the release having gone quiet. +func TestReleaseLandedRemovesItsWorktreeAndBranch(t *testing.T) { + repo := newTestRepo(t) + place := Place{Dir: t.TempDir(), Workspace: repo} + tree, err := prepareTaskTree(place, repo, "abcd1234abcd9876", 3, "paint the fence") + if err != nil { + t.Fatalf("prepareTaskTree: %v", err) + } + if !strings.Contains(gitOut(t, repo, "worktree", "list"), tree.dir) { + t.Fatalf("git does not know the worktree %s", tree.dir) + } + + tree.releaseLanded() + + if list := gitOut(t, repo, "worktree", "list"); strings.Contains(list, tree.dir) { + t.Errorf("the worktree is still registered:\n%s", list) + } + if branchIsThere(repo, tree.branch) { + t.Errorf("the branch %s is still there", tree.branch) + } +} diff --git a/internal/session/task_run.go b/internal/session/task_run.go index f3ef6300ef..a9b75fcd6d 100644 --- a/internal/session/task_run.go +++ b/internal/session/task_run.go @@ -6596,15 +6596,15 @@ func worktreeDirt(dir string) string { return worktreeDirtIn(context.Background( // worktreeDirtIn is [worktreeDirt] under a context, for a reading somebody may // have to stop ([treeWatch.close]). func worktreeDirtIn(ctx context.Context, dir string) string { - args := []string{"-C", dir, "--no-optional-locks", "status", "--porcelain", "--untracked-files=all", "--", "."} + args := []string{"--no-optional-locks", "status", "--porcelain", "--untracked-files=all", "--", "."} for _, dropping := range taskDroppingNames() { args = append(args, ":(exclude)"+dropping) } - out, err := exec.CommandContext(ctx, "git", args...).Output() + out, err := gitContext(ctx, dir, nil, args...) if err != nil { return "" } - sum := sha256.Sum256(out) + sum := sha256.Sum256([]byte(out)) return string(sum[:8]) } @@ -8959,13 +8959,22 @@ func git(dir string, args ...string) (string, error) { return gitWith(dir, nil, args...) } -// gitWith is [git] with something extra in its environment, and it exists for -// exactly one caller: the ground ladder stages a parent's tree into AN INDEX OF -// ITS OWN so that handing out a child never moves the parent's index -// ([sealGroundWork]). GIT_INDEX_FILE is the only way to say that to git, and a +// gitWith is [gitContext] with [context.Background] in place of a caller's own +// context, kept as its own name because nearly every call here is that. The +// environment it takes exists for exactly one caller: the ground ladder stages +// a parent's tree into AN INDEX OF ITS OWN so that handing out a child never +// moves the parent's index ([sealGroundWork]). GIT_INDEX_FILE is the only way to say that to git, and a // second copy of the pager and editor settings beside it would be the drift the // one-source-of-truth law forbids. func gitWith(dir string, environment []string, args ...string) (string, error) { + return gitContext(context.Background(), dir, environment, args...) +} + +// gitContext is [gitWith] under a context, for a command somebody may have to +// stop before git answers ([worktreeDirtIn]'s watch). The refusal of an empty +// directory and the pinned environment live here, so there is one copy of +// each; every other spelling of a git call in this package is a wrapper of it. +func gitContext(ctx context.Context, dir string, environment []string, args ...string) (string, error) { // A COMMAND WITH NO DIRECTORY RUNS WHEREVER THE PROCESS HAPPENS TO BE. // // exec.Cmd reads an empty Dir as "the calling process's working directory", @@ -8984,7 +8993,7 @@ func gitWith(dir string, environment []string, args ...string) (string, error) { if strings.TrimSpace(dir) == "" { return "", errors.New("git: no directory to run in") } - command := exec.Command("git", args...) + command := exec.CommandContext(ctx, "git", args...) command.Dir = dir // A pager or an editor in the middle of a merge would hang a node forever on // a terminal it does not have. diff --git a/internal/session/taskgitdir_test.go b/internal/session/taskgitdir_test.go index 3ed374555a..296421cbcf 100644 --- a/internal/session/taskgitdir_test.go +++ b/internal/session/taskgitdir_test.go @@ -1,6 +1,7 @@ package session import ( + "context" "os" "path/filepath" "strings" @@ -17,6 +18,48 @@ import ( // wrote "task: Rewrite", "task: Measure" and "task: Paint" onto the branch of // the worktree it was launched from. Every call site names a directory, so one // that answers "" is a defect, and it fails here rather than somewhere else. +// TestStatusReadsWithNoDirectoryNeverRuns is the same proof for the two call +// sites that built `git -C ` themselves instead of asking [gitWith]: +// [worktreeDirtIn]'s dirty fingerprint and [UnsavedEditsNote]'s status reading. +// `git -C ""` is a no-op — git runs where the process is standing — so a direct +// exec.Command with an empty directory slipped past the refusal in gitWith. +// The repository the process stands in holds a TRACKED file with an unsaved +// edit, because [UnsavedEditsNote] reads with --untracked-files=no and an +// untracked file alone would leave it silent whether or not git ran. +func TestStatusReadsWithNoDirectoryNeverRuns(t *testing.T) { + repo := t.TempDir() + for _, args := range [][]string{{"init", "-q", "-b", "main"}, {"config", "user.name", "codeaf"}} { + if _, err := git(repo, args...); err != nil { + t.Skipf("no usable git here: %v", err) + } + } + writeFile(t, filepath.Join(repo, "note.md"), "work\n") + mustGit(t, repo, "add", "-A", "--", ".") + mustGit(t, repo, "-c", "user.name=t", "-c", "user.email=t@t", "commit", "-q", "-m", "the note") + writeFile(t, filepath.Join(repo, "note.md"), "more work\n") + + // Both readings see the edit when they are told where to look; that is what + // makes their silence below a refusal rather than an empty status. + if got := UnsavedEditsNote(repo); got != unsavedEditsWord { + t.Fatalf("UnsavedEditsNote(repo) = %q, so the edit is not visible to a status read", got) + } + if got := worktreeDirtIn(context.Background(), repo); got == "" { + t.Fatal("worktreeDirtIn(repo) answered \"\", so the edit is not visible to a status read") + } + + t.Chdir(repo) + + if got := worktreeDirtIn(context.Background(), ""); got != "" { + t.Errorf("worktreeDirtIn with no directory returned %q, so a git command ran", got) + } + if got := worktreeDirt(""); got != "" { + t.Errorf("worktreeDirt with no directory returned %q, so a git command ran", got) + } + if got := UnsavedEditsNote(""); got != "" { + t.Errorf("UnsavedEditsNote with no directory returned %q, so a git command ran", got) + } +} + func TestAGitCommandWithNoDirectoryNeverRuns(t *testing.T) { // The proof that it never RAN, rather than merely never committed: this is // a real repository holding a real change, and a `git add` that reached it diff --git a/internal/session/taskpreflight.go b/internal/session/taskpreflight.go index 54a1098c74..0a500ea356 100644 --- a/internal/session/taskpreflight.go +++ b/internal/session/taskpreflight.go @@ -43,7 +43,6 @@ package session // an overlap worth a line ([hotFiles]). import ( - "os/exec" "path" "path/filepath" "strconv" @@ -153,8 +152,8 @@ func UnsavedEditsNote(root string) string { if strings.TrimSpace(root) == "" { return "" } - out, err := exec.Command("git", "-C", root, "status", "--porcelain", "--untracked-files=no").Output() - if err != nil || strings.TrimSpace(string(out)) == "" { + out, err := git(root, "status", "--porcelain", "--untracked-files=no") + if err != nil || strings.TrimSpace(out) == "" { return "" } return unsavedEditsWord