diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ee3477..347e811 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,10 +21,13 @@ can afford. staged — the change is already committed — so a gate pointed at the staged scope skips, reports success, and measures nothing. - It refuses a checkout with uncommitted work in it. A range scope names bytes of - `HEAD` while the sandbox is written from the index, and those are the same tree - only while nothing is modified or staged; scoping against one and mutating the - other is the defect already measured at seven of eight verdicts moving. + It refuses a checkout with staged changes in it. A range scope names bytes of + `HEAD` while the sandbox is written from the index, and those agree exactly + while the index agrees with HEAD; scoping against one and mutating the other is + the defect already measured at seven of eight verdicts moving. A worktree-only + modification never reaches the sandbox and is allowed — written the other way + round first, it refused ditto's own CI in fifty-one seconds, because the Devbox + install step modifies a tracked lockfile. There is no default base, and there will not be one: on a CI checkout the useful base is the last release, on a branch it is the trunk, and a base diff --git a/changed.go b/changed.go index f86a299..b87f139 100644 --- a/changed.go +++ b/changed.go @@ -47,12 +47,16 @@ func PlanChanged(directory, baseRef string, excludePrefixes []string) (StagedPla // RunChanged mutates exactly what a committed change justifies. // -// It refuses a checkout with uncommitted work in it, and that refusal is the -// whole safety of the thing. The sandbox is written from the INDEX and a range -// scope names bytes of HEAD; those are the same tree only while nothing is -// modified or staged. Scoping against one tree and mutating another is the -// defect already measured on a fixture built for it — seven of eight verdicts -// moved — and it is silent, which is why this stops rather than warns. +// It refuses a checkout whose index has moved away from HEAD, and that refusal +// is the whole safety of the thing. The sandbox is written from the INDEX and a +// range scope names bytes of HEAD; those agree exactly while the index agrees +// with HEAD. Scoping against one tree and mutating another is the defect already +// measured on a fixture built for it — seven of eight verdicts moved — and it is +// silent, which is why this stops rather than warns. +// +// A worktree modification is not that. It is never written into the sandbox, so +// it cannot move a verdict, and refusing it only stops runs that would have been +// correct. // // Everything below the scope is the staged path unchanged: the same sandbox, the // same `.ditto.json` for what git does not carry, the same notice when the diff @@ -72,7 +76,7 @@ func RunChanged(directory, baseRef string, excludePrefixes []string, options ... return fmt.Errorf("reading the repository: %w", err) } - if err := repository.RequireClean(); err != nil { + if err := repository.RequireNothingStaged(); err != nil { return fmt.Errorf("checking the checkout: %w", err) } diff --git a/changed_scope_test.go b/changed_scope_test.go index 0673890..4749ebd 100644 --- a/changed_scope_test.go +++ b/changed_scope_test.go @@ -98,24 +98,27 @@ func TestPlanChangedRefusesSomewhereThatIsNotARepository(t *testing.T) { } } -// RunChanged refuses a dirty checkout, and that refusal is the whole safety of -// reusing the index-backed sandbox: a range scope names bytes of HEAD, and those -// are the same bytes only while nothing is modified or staged. -func TestRunChangedRefusesADirtyCheckout(t *testing.T) { - dir := dittotesting.GitRepository(t) +// RunChanged refuses a STAGED change, and that refusal is the whole safety of +// reusing the index-backed sandbox: the sandbox is written from the index while +// a range scope names bytes of HEAD, so the two agree while the index does. +// +// A worktree-only modification is a different thing and is deliberately allowed; +// it never reaches the sandbox, so it cannot move a verdict. That case is pinned +// in internal/staged rather than here, because allowing it here would run a real +// release. +func TestRunChangedRefusesAStagedChange(t *testing.T) { + dir := dittotesting.GitRepositoryWithAChange(t) - dittotesting.WriteFile(t, dir, "added.go", "package fixture\n\nfunc Added(a, b int) bool { return a > b }\n") - dittotesting.Git(t, dir, "add", "-A") - dittotesting.Git(t, dir, "commit", "-m", "add") dittotesting.WriteFile(t, dir, "kept.go", "package fixture\n\nfunc Kept() int { return 2 }\n") + dittotesting.Git(t, dir, "add", "kept.go") err := ditto.RunChanged(dir, "base", nil) if err == nil { - t.Fatal("a dirty checkout was accepted") + t.Fatal("a staged change was accepted") } if !strings.Contains(err.Error(), "kept.go") { - t.Fatalf("the refusal does not name what is dirty: %v", err) + t.Fatalf("the refusal does not name what is staged: %v", err) } } diff --git a/docs/backlog.md b/docs/backlog.md index f625197..1ee79b3 100644 --- a/docs/backlog.md +++ b/docs/backlog.md @@ -585,10 +585,14 @@ The staged gate could not be that gate, which is why this stayed open after staged, so it would skip and report a green that measured nothing. A range scope names bytes of HEAD while the sandbox is written from the index, -so `RunChanged` refuses a checkout with uncommitted work in it. Those two trees -are the same one only while nothing is modified or staged, and scoping against -one while mutating the other is the defect already measured at seven of eight -verdicts moving. +so `RunChanged` refuses a checkout whose INDEX has moved away from HEAD. Scoping +against one tree while mutating the other is the defect already measured at seven +of eight verdicts moving. + +Written as a clean-worktree check first, and CI refuted that in fifty-one +seconds: the Devbox install step modifies a tracked lockfile with nothing to do +with any change, and the gate refused to run at all. A worktree modification is +never written into the sandbox, so it cannot move a verdict. `make test.mutation` is unchanged and still reachable by hand. The repository-sized question is worth asking on purpose; it was being asked on every diff --git a/internal/staged/changed.go b/internal/staged/changed.go index 689478b..2f32320 100644 --- a/internal/staged/changed.go +++ b/internal/staged/changed.go @@ -68,28 +68,35 @@ func (r *Repository) ChangedScopeOf(baseRef string, files []string) (Scope, erro // else's commits. func rangeOf(baseRef string) string { return baseRef + "...HEAD" } -// RequireClean refuses a checkout with uncommitted work in it. +// RequireNothingStaged refuses a checkout whose index has moved away from HEAD. // -// This is what makes the range scope safe to run in the existing sandbox. That -// sandbox is written from the INDEX, and a range scope names bytes of HEAD; the -// two are the same tree only while nothing is modified or staged. Scoping -// against one tree and mutating another is the defect already measured on a -// fixture built for it — seven of eight verdicts moved — and it is silent, which -// is why this refuses rather than warns. +// This is what makes the range scope safe to run in the existing sandbox, and +// the precise condition matters. That sandbox is `git checkout-index --all`, so +// it holds the INDEX; a range scope names bytes of HEAD. The two agree exactly +// while the index agrees with HEAD, which is a narrower thing than a clean +// worktree. +// +// It was written as a clean-worktree check first, and CI refuted that in +// fifty-one seconds: the Devbox install step modifies `devbox.lock`, a tracked +// file with nothing to do with any change, and the gate refused to run at all. A +// worktree modification is never written into the sandbox, so it cannot move a +// verdict; a STAGED one is, and would mean scoping against one tree while +// mutating another -- the defect already measured at seven of eight verdicts +// moving, and silent. // // The staged path needs no such check: it derives its scope from the index and // mutates the index, and RejectPartial covers the one file that could disagree. -func (r *Repository) RequireClean() error { - output, err := r.git("status", "--porcelain") +func (r *Repository) RequireNothingStaged() error { + output, err := r.git("diff", "--cached", "--name-only") if err != nil { - return fmt.Errorf("checking whether the checkout is clean: %w", err) + return fmt.Errorf("checking whether anything is staged: %w", err) } - dirty := strings.TrimSpace(string(output)) - if dirty == "" { + staged := strings.TrimSpace(string(output)) + if staged == "" { return nil } return fmt.Errorf( //nolint:err113 // the listing is the message - "ditto: a range scope measures committed bytes, and this checkout has uncommitted work:\n%s", dirty) + "ditto: a range scope measures committed bytes, and this checkout has staged changes:\n%s", staged) } diff --git a/internal/staged/changed_internal_test.go b/internal/staged/changed_internal_test.go index 34c2bb8..51122ec 100644 --- a/internal/staged/changed_internal_test.go +++ b/internal/staged/changed_internal_test.go @@ -90,36 +90,42 @@ func TestChangedScopeOfDerivesByteRanges(t *testing.T) { } } -// TestRequireCleanRefusesADirtyCheckout is what makes reusing the index-backed -// sandbox honest. +// TestRequireNothingStagedGuardsTheRightTree is what makes reusing the +// index-backed sandbox honest, and the precise condition is the point. // -// A range scope names bytes of HEAD, and the sandbox is written from the INDEX. -// Those are the same bytes only while the checkout is clean, and a scope that -// names one tree while the mutants run in another is the defect measured at -// seven of eight verdicts moving. Refusing is cheap; being wrong is not. -func TestRequireCleanRefusesADirtyCheckout(t *testing.T) { +// The sandbox is `git checkout-index --all`, so it holds the INDEX; a range +// scope names bytes of HEAD. Those agree while the index agrees with HEAD, which +// is narrower than a clean worktree — and the difference is not academic. Written +// as a clean-worktree check, this refused ditto's own CI in fifty-one seconds, +// because the Devbox install step modifies a tracked lockfile that has nothing +// to do with any change. +func TestRequireNothingStagedGuardsTheRightTree(t *testing.T) { t.Parallel() - repository, _ := scripted(map[string]string{"status --porcelain": " M internal/thing/thing.go\n"}) + t.Run("refuses a staged change, because the sandbox would carry it", func(t *testing.T) { + t.Parallel() - err := repository.RequireClean() - if err == nil { - t.Fatal("a dirty checkout was accepted") - } + repository, _ := scripted(map[string]string{"diff --cached --name-only": "internal/thing/thing.go\n"}) - if !strings.Contains(err.Error(), "thing.go") { - t.Fatalf("the refusal does not name what is dirty: %v", err) - } -} + err := repository.RequireNothingStaged() + if err == nil { + t.Fatal("a staged change was accepted") + } -func TestRequireCleanAcceptsACleanCheckout(t *testing.T) { - t.Parallel() + if !strings.Contains(err.Error(), "thing.go") { + t.Fatalf("the refusal does not name what is staged: %v", err) + } + }) - repository, _ := scripted(map[string]string{"status --porcelain": ""}) + t.Run("allows a worktree modification, because the sandbox never sees it", func(t *testing.T) { + t.Parallel() - if err := repository.RequireClean(); err != nil { - t.Fatalf("a clean checkout was refused: %v", err) - } + repository, _ := scripted(map[string]string{"diff --cached --name-only": ""}) + + if err := repository.RequireNothingStaged(); err != nil { + t.Fatalf("a worktree-only modification was refused: %v", err) + } + }) } // TestChangedScopeFailsOpenRatherThanGuessing covers both fail-open branches. diff --git a/readme.md b/readme.md index 0713116..cf650f5 100644 --- a/readme.md +++ b/readme.md @@ -142,10 +142,11 @@ else's commits into the bill. ditto changed --since "$(git describe --tags --abbrev=0 HEAD^)" --threshold 0.8 ``` -It refuses a checkout with uncommitted work in it, and that refusal is the point -rather than fussiness: a range scope names bytes of `HEAD` while the sandbox is -written from the index, and those are the same tree only while nothing is -modified or staged. +It refuses a checkout with **staged** changes in it, and that refusal is the +point rather than fussiness: a range scope names bytes of `HEAD` while the +sandbox is written from the index, and those agree exactly while the index +agrees with HEAD. A worktree-only modification is never written into the sandbox +and is allowed. There is no default base. On a CI checkout the useful one is the last release, on a branch it is the trunk, and a base guessed wrong is either a bill nobody asked