From ec1ee9f52f4563ac9d67a453b2e146663e595967 Mon Sep 17 00:00:00 2001 From: prode Date: Thu, 24 Sep 2026 20:22:37 -0300 Subject: [PATCH] fix(git): count a commit already on origin/ as the base's, not the branch's Commits measured "not on base" against refs/heads/ alone. A local main that was never pulled after pull requests merged on the forge made every commit it lagged behind count as work of a branch cut from origin/main, so the attribution validator blocked `git push` on a commit merged weeks earlier. Commits now excludes every existing copy of the base (`git log HEAD --not refs/heads/ refs/remotes/origin/`). Changed, a working-tree diff that takes one ref, diffs against the remote when the local base is merely behind it. Standing on the base branch still compares against the remote only. Plan: plans/attribution-stale-base.md --- internal/git/git.go | 52 ++++++++++++++++++++++----- internal/git/repo_test.go | 43 ++++++++++++++++++++++ internal/validate/attribution_test.go | 21 +++++++++++ plans/attribution-stale-base.md | 38 ++++++++++++++++++++ 4 files changed, 145 insertions(+), 9 deletions(-) create mode 100644 plans/attribution-stale-base.md diff --git a/internal/git/git.go b/internal/git/git.go index 2413e1d..5b3674e 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -372,6 +372,12 @@ const ( // history, and a caller that reported on it would report the same thing on every // branch in the repository forever. // +// "On base" means on either copy of it. A local `main` nobody has pulled in weeks +// is the ordinary shape of a repository whose pull requests merge on the forge, +// and a branch cut from `origin/main` would otherwise claim every commit the local +// copy has not caught up with — somebody else's merged work, reported as this +// branch's, on every push. +// // Absence is a normal answer here as it is everywhere else in this package: no // git, no repository, an unborn HEAD, a shallow clone with no base ref, or a // branch that is exactly its base all return no commits and no error. A caller @@ -384,12 +390,13 @@ func Commits(dir, base string) ([]Commit, error) { if base == "" { base = Base(dir) } - against := compareRef(dir, base) - if against == "" { + against := baseRefs(dir, base) + if len(against) == 0 { return nil, nil } format := strings.Join([]string{"%H", "%h", "%s", "%B"}, fieldSep) + recordSep - out, err := run(Bin, dir, "log", "--no-merges", "--format="+format, against+"..HEAD") + args := append([]string{"log", "--no-merges", "--format=" + format, "HEAD", "--not"}, against...) + out, err := run(Bin, dir, args...) if err != nil { // An unborn HEAD, a range git cannot resolve, a repository mid-rebase. // Nothing to report, and nothing worth failing a validation run over. @@ -398,24 +405,51 @@ func Commits(dir, base string) ([]Commit, error) { return parseLog(out), nil } -// compareRef is what "not on base" is measured against, or "" when no ref can -// answer. +// baseRefs is every ref "not on base" is measured against, or none when no ref +// can answer: the local base and the remote's copy, whichever of them exist. // // Standing on the base branch is the case worth spelling out: refs/heads/main // compared against itself is empty, so a check run there would silently pass on // every commit in the repository. The remote's copy is the honest comparison — // what is here and not yet pushed — and a base branch with no remote at all // leaves genuinely nothing to compare against. -func compareRef(dir, base string) string { +func baseRefs(dir, base string) []string { + remote := "refs/remotes/origin/" + base branch, err := CurrentBranch(dir) if err == nil && branch == base { - remote := "refs/remotes/origin/" + base if verify(dir, remote) { - return remote + return []string{remote} + } + return nil + } + var refs []string + for _, r := range []string{"refs/heads/" + base, remote} { + if verify(dir, r) { + refs = append(refs, r) } + } + return refs +} + +// compareRef is the one ref a diff is taken against, or "" when no ref can +// answer: the fresher copy of the base, which is the remote's whenever the local +// branch is merely behind it. A diff cannot exclude two refs the way a log can, +// so a local base that has diverged from its remote keeps the local answer. +func compareRef(dir, base string) string { + refs := baseRefs(dir, base) + switch { + case len(refs) == 0: return "" + case len(refs) == 2 && isAncestor(dir, refs[0], refs[1]): + return refs[1] } - return ref(dir, base) + return refs[0] +} + +// isAncestor reports whether ancestor is reachable from descendant. +func isAncestor(dir, ancestor, descendant string) bool { + _, err := run(Bin, dir, "merge-base", "--is-ancestor", ancestor, descendant) + return err == nil } // verify reports whether a ref resolves. diff --git a/internal/git/repo_test.go b/internal/git/repo_test.go index a3be3e6..2d2c99f 100644 --- a/internal/git/repo_test.go +++ b/internal/git/repo_test.go @@ -272,6 +272,49 @@ func TestCommitsOnTheBaseBranchCompareAgainstTheRemote(t *testing.T) { } } +// staleBase builds the shape a forge-merged workflow leaves behind: origin/main +// carries a commit the local main never pulled, and feat/x is cut from +// origin/main with one commit of its own on top. +func staleBase(t *testing.T) string { + t.Helper() + remote := t.TempDir() + dir := repo(t) + git(t, remote, "init", "--bare", "-b", DefaultBase) + commit(t, dir, "a.txt", "one\n", "feat: one") + git(t, dir, "remote", "add", "origin", remote) + git(t, dir, "push", "-u", "origin", DefaultBase) + commit(t, dir, "merged.txt", "theirs\n", "feat: merged on the forge") + git(t, dir, "push", "origin", DefaultBase) + git(t, dir, "switch", "-c", "feat/x") + git(t, dir, "branch", "-f", DefaultBase, "HEAD~1") + commit(t, dir, "mine.txt", "mine\n", "feat: mine") + return dir +} + +// A local base behind its remote is not this branch's work: a commit already on +// origin/main is somebody else's history, whatever the local copy says. +func TestCommitsIgnoreWhatOnlyTheRemoteBaseHas(t *testing.T) { + dir := staleBase(t) + got, err := Commits(dir, DefaultBase) + if err != nil { + t.Fatalf("Commits: %v", err) + } + if len(got) != 1 || got[0].Subject != "feat: mine" { + t.Errorf("commits = %+v, want only the one this branch added", got) + } +} + +func TestChangedDiffsAgainstTheFresherBase(t *testing.T) { + dir := staleBase(t) + got, err := Changed(dir, DefaultBase) + if err != nil { + t.Fatalf("Changed: %v", err) + } + if len(got) != 1 || got[0].Path != "mine.txt" { + t.Errorf("changes = %+v, want only mine.txt", got) + } +} + func TestDirtyAndCommentChar(t *testing.T) { dir := repo(t) commit(t, dir, "a.txt", "one\n", "feat: one") diff --git a/internal/validate/attribution_test.go b/internal/validate/attribution_test.go index 2a9777b..3e44c1f 100644 --- a/internal/validate/attribution_test.go +++ b/internal/validate/attribution_test.go @@ -93,6 +93,27 @@ func TestAttributionIgnoresWhatIsAlreadyOnTheBase(t *testing.T) { } } +// A local main nobody pulled is the common shape once pull requests merge on the +// forge. A signed commit that already reached origin/main through somebody else's +// merge is not this branch's to fix, and blocking the push over it blocks every +// branch cut from the remote until the local copy is pulled. +func TestAttributionIgnoresWhatIsAlreadyOnTheRemoteBase(t *testing.T) { + root := repo(t) + remote := t.TempDir() + gitIn(t, remote, "init", "--bare", "--initial-branch=main") + gitIn(t, root, "remote", "add", "origin", remote) + gitIn(t, root, "push", "-u", "origin", "main") + commit(t, root, signed) + gitIn(t, root, "push", "origin", "main") + gitIn(t, root, "switch", "-c", "fix/thing") + gitIn(t, root, "branch", "-f", "main", "HEAD~1") + commit(t, root, "fix: something clean") + + if got := runValidator(t, Attribution, root); len(got) != 0 { + t.Errorf("findings from a commit already on origin/main: %v", got) + } +} + // Standing on the base with no remote leaves nothing to compare against, which is // silence rather than a failure — and the common shape of a fresh local repo. func TestAttributionIsSilentOnTheBaseWithNoRemote(t *testing.T) { diff --git a/plans/attribution-stale-base.md b/plans/attribution-stale-base.md new file mode 100644 index 0000000..f841f99 --- /dev/null +++ b/plans/attribution-stale-base.md @@ -0,0 +1,38 @@ +--- +autonomy: auto +ci: no-wait +--- + +# Attribution stale base + +The commit range scc checks is measured against the local base branch only, so a +local `main` behind `origin/main` pulls already-merged commits into "this branch". + +## Why + +`git.Commits` resolves the base with `ref()`, which prefers `refs/heads/`. A +branch cut from a fresh `origin/main` while local `main` is twenty commits behind +reports every one of those twenty as its own work — the attribution validator then +blocks `git push` on a commit somebody else merged weeks ago. Done means a commit +reachable from either copy of the base is never in the range. + +## Paths + +- `internal/git/git.go` +- `internal/git/repo_test.go` +- `internal/validate/attribution_test.go` + +## Out of scope + +- `Look`'s ahead/behind counts, which answer a different question about a named branch. + +## Tasks + +- [x] 1.1 (Unit) Exclude commits reachable from the local or the remote base in `Commits`, and diff `Changed` against the fresher of the two +- [x] 1.2 (Unit) Cover a stale local base in the git and attribution tests + _Depends 1.1_ + +## Done when + +- `go test ./internal/git/ ./internal/validate/ ./internal/cli/` passes, including a branch cut from `origin/main` while local `main` is behind. +- `scc validate` reports no finding for a commit that is already on `origin/main`.