Skip to content
Open
Show file tree
Hide file tree
Changes from 2 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
6 changes: 3 additions & 3 deletions internal/planner/benchmark_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ func BenchmarkSampledCheckpointCandidates(b *testing.B) {
}
}

func BenchmarkReachesCommit(b *testing.B) {
func BenchmarkCheckAncestry(b *testing.B) {
repo, err := git.Init(memory.NewStorage(), nil)
if err != nil {
b.Fatalf("init repo: %v", err)
Expand All @@ -123,11 +123,11 @@ func BenchmarkReachesCommit(b *testing.B) {

b.ResetTimer()
for range b.N {
ok, err := ReachesCommit(repo.Storer, tip, root)
result, err := CheckAncestry(repo.Storer, tip, root)
if err != nil {
b.Fatal(err)
}
if !ok {
if result != AncestryReachable {
b.Fatal("expected tip to reach root")
}
}
Expand Down
71 changes: 60 additions & 11 deletions internal/planner/planner.go
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,7 @@ func PlanRef(store storer.EncodedObjectStorer, want DesiredRef, targetHash plumb
return plan, nil
}

isFF, err := ReachesCommit(store, want.SourceHash, targetHash)
ancestry, err := CheckAncestry(store, want.SourceHash, targetHash)
if err != nil {
if errors.Is(err, ErrAncestryDepthExceeded) {
// Can't prove fast-forward within depth limit — block with explanation.
Expand All @@ -344,7 +344,7 @@ func PlanRef(store storer.EncodedObjectStorer, want DesiredRef, targetHash plumb
}
return plan, fmt.Errorf("check fast-forward for %s: %w", want.TargetRef, err)
}
if isFF {
if ancestry == AncestryReachable {
plan.Action = ActionUpdate
plan.Reason = ShortHash(targetHash) + " -> " + ShortHash(want.SourceHash)
return plan, nil
Expand All @@ -357,6 +357,15 @@ func PlanRef(store storer.EncodedObjectStorer, want DesiredRef, targetHash plumb
}

plan.Action = ActionBlock
if ancestry == AncestryIndeterminate {
// The target already has the commits where the two histories meet, so
// the fetch pruned them from the planning store and a fast-forward
// can't be confirmed locally. Don't fail the sync or claim divergence
// we haven't proven — block with an actionable message.
plan.Reason = "cannot verify fast-forward for " + want.TargetRef.String() +
" locally (the target already has the relevant commits); use --force-with-lease if this is a valid fast-forward"
return plan, nil
}
plan.Reason = ShortHash(targetHash) + " is not an ancestor of " + ShortHash(want.SourceHash)
return plan, nil
}
Expand Down Expand Up @@ -400,27 +409,61 @@ func PlanReplicationRef(want DesiredRef, targetHash plumbing.Hash, existsOnTarge
// on real repos — even the Linux kernel has ~1.3M commits.
const MaxAncestryDepth = 2_000_000

// ErrAncestryDepthExceeded is returned when ReachesCommit exceeds MaxAncestryDepth.
// ErrAncestryDepthExceeded is returned when the ancestry walk exceeds MaxAncestryDepth.
var ErrAncestryDepthExceeded = errors.New("ancestry check exceeded depth limit")

// ReachesCommit checks whether the commit at startHash has targetHash as an
// AncestryResult is the outcome of a fast-forward ancestry check against a
// store that was populated by a fetch advertising the target's refs as haves.
type AncestryResult int

const (
// AncestryReachable means targetHash is provably an ancestor of startHash:
// the update is a fast-forward.
AncestryReachable AncestryResult = iota

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that we return this by default (e.g. return 0, ErrAncestryDepthExceeded), ideally we would:

Suggested change
AncestryReachable AncestryResult = iota
AncestryInvalid AncestryResult = iota
AncestryReachable

That way, callers ignoring error (e.g. v, _ := f()), get AncestryInvalid instead of AncestryReachable.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you take a look?

// AncestryUnreachable means startHash's entire ancestry was walked without
// reaching targetHash and without hitting any object the target already
// has — a genuine non-fast-forward (the two histories have diverged).
AncestryUnreachable
// AncestryIndeterminate means the walk reached the frontier of objects the
// target already has (absent from this store because the fetch pruned
// everything reachable from a target ref) before the question was settled.
// The deciding commits live beyond that frontier, so a fast-forward can be
// neither confirmed nor ruled out from this store alone.
AncestryIndeterminate
)

// CheckAncestry reports whether the commit at startHash has targetHash as an
// ancestor, bounded to MaxAncestryDepth commits to prevent degenerate walks.
func ReachesCommit(store storer.EncodedObjectStorer, startHash, targetHash plumbing.Hash) (bool, error) {
//
// The store is the one BuildPlans runs against: a fetch populated it using the
// target's refs as haves, so every object reachable from any target ref is
// pruned. startHash and the commits between it and the target are normally
// present (they are the new work being synced), but when another target ref
// already points into that range the frontier commits are missing. A missing
// object therefore means "the target already has this" rather than an error —
// the walk records that it could not cross the frontier and returns
// AncestryIndeterminate instead of misreporting a non-fast-forward or failing
// the whole sync on a missing start commit.
func CheckAncestry(store storer.EncodedObjectStorer, startHash, targetHash plumbing.Hash) (AncestryResult, error) {
if startHash == targetHash {
return true, nil
return AncestryReachable, nil
}

start, err := object.GetCommit(store, startHash)
if err != nil {
return false, fmt.Errorf("load source commit %s: %w", startHash, err)
if errors.Is(err, plumbing.ErrObjectNotFound) {
return AncestryIndeterminate, nil
}
return 0, fmt.Errorf("load source commit %s: %w", startHash, err)
}

seen := map[plumbing.Hash]struct{}{}
stack := []*object.Commit{start}
hitFrontier := false

for len(stack) > 0 {
if len(seen) >= MaxAncestryDepth {
return false, ErrAncestryDepthExceeded
return 0, ErrAncestryDepthExceeded
}
current := stack[len(stack)-1]
stack = stack[:len(stack)-1]
Expand All @@ -431,22 +474,28 @@ func ReachesCommit(store storer.EncodedObjectStorer, startHash, targetHash plumb

for _, parentHash := range current.ParentHashes {
if parentHash == targetHash {
return true, nil
return AncestryReachable, nil
}
if _, ok := seen[parentHash]; ok {
continue
}
parent, err := object.GetCommit(store, parentHash)
if err != nil {
if errors.Is(err, plumbing.ErrObjectNotFound) {
// Pruned ancestor: the target already has it, so the
// walk cannot cross this point. The answer may lie beyond.
hitFrontier = true
continue
}
return false, fmt.Errorf("load parent commit %s: %w", parentHash, err)
return 0, fmt.Errorf("load parent commit %s: %w", parentHash, err)
}
stack = append(stack, parent)
}
}
return false, nil
if hitFrontier {
return AncestryIndeterminate, nil
}
return AncestryUnreachable, nil
}

// ObjectsToPush computes the set of objects that need to be sent to the target.
Expand Down
98 changes: 98 additions & 0 deletions internal/planner/planner_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package planner
import (
"fmt"
"slices"
"strings"
"testing"
"time"

Expand Down Expand Up @@ -76,6 +77,103 @@ func TestPlanRefFastForwardAndBlock(t *testing.T) {
}
}

// A fully-populated store must still distinguish a real non-fast-forward
// (divergence) from the pruned-frontier case below.
func TestCheckAncestryUnreachableOnDivergence(t *testing.T) {
repo, err := git.Init(memory.NewStorage(), nil)
if err != nil {
t.Fatalf("init repo: %v", err)
}
root := seedCommit(t, repo, nil)
next := seedCommit(t, repo, []plumbing.Hash{root})
side := seedCommit(t, repo, []plumbing.Hash{root})

result, err := CheckAncestry(repo.Storer, side, next)
if err != nil {
t.Fatalf("CheckAncestry: %v", err)
}
if result != AncestryUnreachable {
t.Fatalf("expected AncestryUnreachable for divergent history, got %v", result)
}
}

// When the source tip itself is absent — it lives behind the have frontier
// because the target already has it under another ref — the check must report
// indeterminate, not fail (which previously aborted the whole sync with a
// "load source commit" error).
func TestCheckAncestryIndeterminateOnMissingStart(t *testing.T) {
repo, err := git.Init(memory.NewStorage(), nil)
if err != nil {
t.Fatalf("init repo: %v", err)
}
missingStart := plumbing.NewHash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")
target := plumbing.NewHash("bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb")

result, err := CheckAncestry(repo.Storer, missingStart, target)
if err != nil {
t.Fatalf("CheckAncestry should not error on a pruned start commit: %v", err)
}
if result != AncestryIndeterminate {
t.Fatalf("expected AncestryIndeterminate for missing start, got %v", result)
}
}

// When the walk reaches a parent that was pruned (the target already has it),
// the answer lies beyond the frontier: indeterminate, not a false "diverged".
func TestCheckAncestryIndeterminateOnPrunedAncestor(t *testing.T) {
repo, err := git.Init(memory.NewStorage(), nil)
if err != nil {
t.Fatalf("init repo: %v", err)
}
prunedParent := plumbing.NewHash("cccccccccccccccccccccccccccccccccccccccc")
tip := seedCommit(t, repo, []plumbing.Hash{prunedParent})
target := plumbing.NewHash("dddddddddddddddddddddddddddddddddddddddd")

result, err := CheckAncestry(repo.Storer, tip, target)
if err != nil {
t.Fatalf("CheckAncestry: %v", err)
}
if result != AncestryIndeterminate {
t.Fatalf("expected AncestryIndeterminate when an ancestor is pruned, got %v", result)
}
}

// PlanRef must turn the indeterminate case into a clean, actionable block
// rather than a hard error, and --force must still let it through.
func TestPlanRefIndeterminateBlocksAndForceOverrides(t *testing.T) {
repo, err := git.Init(memory.NewStorage(), nil)
if err != nil {
t.Fatalf("init repo: %v", err)
}
missingSource := plumbing.NewHash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")
target := plumbing.NewHash("bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb")
want := DesiredRef{
Kind: RefKindBranch, Label: "main",
SourceRef: plumbing.NewBranchReferenceName("main"),
TargetRef: plumbing.NewBranchReferenceName("main"),
SourceHash: missingSource,
}

plan, err := PlanRef(repo.Storer, want, target, false)
if err != nil {
t.Fatalf("PlanRef should not error on indeterminate ancestry: %v", err)
}
if plan.Action != ActionBlock {
t.Fatalf("expected ActionBlock, got %s", plan.Action)
}
if !strings.Contains(plan.Reason, "cannot verify fast-forward") {
t.Fatalf("expected actionable indeterminate reason, got %q", plan.Reason)
}

forced, err := PlanRef(repo.Storer, want, target, true)
if err != nil {
t.Fatalf("PlanRef (force): %v", err)
}
if forced.Action != ActionUpdate {
t.Fatalf("expected ActionUpdate under force, got %s", forced.Action)
}
}

func TestPlanReplicationRefOverwritesDivergence(t *testing.T) {
target := plumbing.NewHash("1111111111111111111111111111111111111111")
source := plumbing.NewHash("2222222222222222222222222222222222222222")
Expand Down
2 changes: 1 addition & 1 deletion internal/syncer/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ func TestRun_IntegrationSkipsLocalFetchOnRelayOnlySync(t *testing.T) {

// TestRun_IntegrationKeepsLocalFetchWhenAncestryNeeded ensures the fetch is
// still performed for a fast-forward update where BuildPlans calls
// ReachesCommit on the local store. Skipping it would crash the planner.
// CheckAncestry on the local store. Skipping it would crash the planner.
func TestRun_IntegrationKeepsLocalFetchWhenAncestryNeeded(t *testing.T) {
sourceRepo, sourceFS := newSourceRepo(t)
makeCommits(t, sourceRepo, sourceFS, 2)
Expand Down
Loading