Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 43 additions & 9 deletions internal/git/git.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
Expand All @@ -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.
Expand Down
43 changes: 43 additions & 0 deletions internal/git/repo_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
21 changes: 21 additions & 0 deletions internal/validate/attribution_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
38 changes: 38 additions & 0 deletions plans/attribution-stale-base.md
Original file line number Diff line number Diff line change
@@ -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/<base>`. 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`.
Loading