Address force-clarification review findings · Entire

Address force-clarification review findings

583fcf1→main·

Soph·2mo ago·8 files·+116 added/-8 removed

Three issues from review on top of the force-clarification branch:

  1. BestEffort silently defeated --force-with-lease. The OnRejection callback installed under BestEffort stored every per-ref ng status for downgrade to a warning, including lease-mismatch statuses. sync --all-refs --force-with-lease could then exit successfully after a concurrent target update, contradicting the lease.

Export gitproto.IsLeaseFailure and add a syncer.leaseFailureError pass after finalizeCounts. Lease-class rejections (stale info / fetch first / non-fast-forward / does not match) now escalate to a fatal error even with BestEffort on; non-lease rejections continue to downgrade to warnings.

  1. Public and unstable APIs accepted invalid force combinations and only rejected them deep in newSession (after auth resolution). Add SyncPolicy.Validate, invoke it from gitsync.SyncRequest.Validate and gitsync.PlanRequest.Validate, and from unstable.Client.Sync, .Plan, .Replicate. The syncer.go check stays as defense-in-depth for callers reaching syncer.Config directly (tests).

  2. docs/usage.md described replicate as "fast-forward-only by design"; in fact replicate's contract is source-authoritative overwrite — divergent branches and tags are retargeted unconditionally, which is why force flags are unnecessary rather than disallowed by a gate. Rewrite the sentence.

Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com

Sessions

eaa899c96ddfView transcript

Changes

8

152 unmodified lines

153
154
155
156
157
158
159
160
161
13 unmodified lines

175
176
177
178
179
180
181
182
183

152 unmodified lines

if err := validateOperationMode(r.Policy.Mode); err != nil {
        return err
    }
    if err := r.Policy.Validate(); err != nil {
        return err
    }
    if _, err := validation.NormalizeProtocolMode(string(r.Policy.Protocol)); err != nil {
        return fmt.Errorf("normalize protocol: %w", err)
    }
13 unmodified lines

if err := validateOperationMode(r.Policy.Mode); err != nil {
        return err
    }
    if err := r.Policy.Validate(); err != nil {
        return err
    }
    if _, err := validation.NormalizeProtocolMode(string(r.Policy.Protocol)); err != nil {
        return fmt.Errorf("normalize protocol: %w", err)
    }

Mclient.go+6

58 unmodified lines

59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78

58 unmodified lines

}).Validate(); err == nil {
        t.Fatalf("expected duplicate mapping validation error")
    }
    if err := (SyncRequest{
        Source: Endpoint{URL: "https://source.example/repo.git"},
        Target: Endpoint{URL: "https://target.example/repo.git"},
        Policy: SyncPolicy{ForceWithLease: true, ForceBlind: true},
    }).Validate(); err == nil {
        t.Fatalf("expected force-with-lease + force-blind to be rejected at the request edge")
    }
    if err := (SyncRequest{
        Source: Endpoint{URL: "https://source.example/repo.git"},
        Target: Endpoint{URL: "https://target.example/repo.git"},
        Policy: SyncPolicy{Mode: ModeReplicate, ForceWithLease: true},
    }).Validate(); err == nil {
        t.Fatalf("expected replicate + force to be rejected at the request edge")
    }
}

func TestClientReturnsAuthProviderErrors(t *testing.T) {

Mclient_test.go+14

245 unmodified lines

246
247
248
249
250
249
250
251
252
253
254
255

245 unmodified lines

races for users who opt into non-fast-forward updates.

`bootstrap` and `replicate` do not accept force flags. Bootstrap seeds an empty target where every ref is a create; replicate is fast-forward-only by design.
empty target where every ref is a create. Replicate's contract is source-authoritative overwrite: divergent branches and tags are retargeted against the source unconditionally, so there is no fast-forward gate for a force flag to opt out of.

The pre-0.5 `--force` flag is removed. Its semantics were lease-protected (it never sent a zero expected-old), so the closest direct replacement is

Mdocs/usage.md+4/-2

115 unmodified lines

116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
1 unmodified line

137
138
139
126
127
128
129
130
140
141
142
132
143
144
145
146

115 unmodified lines

"does not match",
}

// IsLeaseFailure reports whether a receive-pack ng reason indicates the
// captured target tip no longer matched at push time. Callers that downgrade
// per-ref rejections to warnings (BestEffort) must still treat these as fatal
// to preserve --force-with-lease semantics.
func IsLeaseFailure(status string) bool {
    lowered := strings.ToLower(status)
    for _, marker := range leaseFailureMarkers {
        if strings.Contains(lowered, marker) {
            return true
        }
    }
    return false
}

// annotateLeaseFailure wraps a lease-failure CommandStatusErr with a retry/
// override hint. Other receive-pack errors pass through unchanged.
func annotateLeaseFailure(err error) error {
1 unmodified line

if !errors.As(err, &cs) {
        return err
    }
    status := strings.ToLower(cs.Status)
    for _, marker := range leaseFailureMarkers {
        if strings.Contains(status, marker) {
            return fmt.Errorf("%w (target ref %s moved or differs from session start; rerun, or use --force-blind to overwrite)", err, cs.ReferenceName)
        }
    }
    if !IsLeaseFailure(cs.Status) {
        return err
    }
    return err
    return fmt.Errorf("%w (target ref %s moved or differs from session start; rerun, or use --force-blind to overwrite)", err, cs.ReferenceName)
}

// sendReceivePack encodes and POSTs a receive-pack request, then decodes the report.

Minternal/gitproto/push.go+17/-6

425 unmodified lines

426
427
428
429
430
431
432
433
434
435
436
437
438
439
440
441
442
443
444
445
446
447
448
449
450
451
452
369 unmodified lines

822
823
824
825
826
827
828
829
830
88 unmodified lines

919
920
921
922
923
924
925
926
927

425 unmodified lines

return pushed, deleted
}// leaseFailureError surfaces receive-pack lease misses as a fatal error even
// when BestEffort would otherwise downgrade them. Without this, a sync with
// both --force-with-lease and --all-refs (which implies BestEffort) would
// silently treat a concurrent target update as a warning, defeating the lease.
func (s *syncSession) leaseFailureError() error {
    if len(s.rejections) == 0 {
        return nil
    }
    var refs []string
    for name, status := range s.rejections {
        if gitproto.IsLeaseFailure(status) {
            refs = append(refs, name.String())
        }
    }
    if len(refs) == 0 {
        return nil
    }
    sort.Strings(refs)
    return fmt.Errorf("lease failure on %d ref(s) (%s) — target moved during sync; rerun, or use --force-blind to overwrite", len(refs), strings.Join(refs, ", "))
}
// applyRejections downgrades plans whose ref was rejected by the target to
// ActionWarn and returns the count.
func (s *syncSession) applyRejections(plans []BranchPlan) int {
369 unmodified lines

}

s.finalizeCounts(pushPlans, &result)
    if err := s.leaseFailureError(); err != nil {
        return result, err
    }
    result.Stats = stats.snapshot()
    result.Measurement = measurementDone()
    return result, nil
88 unmodified lines

}

s.finalizeCounts(pushPlans, &result)
    if err := s.leaseFailureError(); err != nil {
        return result, err
    }
    result.Stats = s.stats.snapshot()
    result.Measurement = s.measurementDone()
    return result, nil

Minternal/syncer/syncer.go+27

34 unmodified lines

35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66

34 unmodified lines

}
}

func TestLeaseFailureErrorEscalatesPastBestEffort(t *testing.T) {
    main := plumbing.NewBranchReferenceName("main")
    pull := plumbing.ReferenceName("refs/pull/1/head")

// Non-lease rejection alone: BestEffort handles it as a warning, no fatal.
    s := &syncSession{rejections: map[plumbing.ReferenceName]string{
        pull: "deny updating a hidden ref",
    }}
    if err := s.leaseFailureError(); err != nil {
        t.Fatalf("expected nil for non-lease rejection, got %v", err)
    }

// Lease rejection: must escalate, naming the affected ref and the override flag.
    s.rejections[main] = "stale info, exp 1234, got abcd"
    err := s.leaseFailureError()
    if err == nil {
        t.Fatal("expected lease failure to escalate past BestEffort")
    }
    if !strings.Contains(err.Error(), main.String()) {
        t.Errorf("expected affected ref in error, got %q", err)
    }
    if !strings.Contains(err.Error(), "--force-blind") {
        t.Errorf("expected migration hint in error, got %q", err)
    }
}

func TestApplyRejectionsEmptyMapIsNoOp(t *testing.T) {
    plans := []BranchPlan{{TargetRef: plumbing.NewBranchReferenceName("main"), Action: ActionUpdate}}
    s := &syncSession{}

Minternal/syncer/syncer_test.go+26

1 unmodified line

2
3
4
5
6
7
8
9
103 unmodified lines

113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129

1 unmodified line

import (
    "context"
    "errors"

"entire.io/entire/git-sync/internalbridge"
)

103 unmodified lines

Protocol       ProtocolMode  `json:"protocol"`
}

// Validate enforces SyncPolicy invariants at the request edge.
func (p SyncPolicy) Validate() error {
    if p.ForceWithLease && p.ForceBlind {
        return errors.New("ForceWithLease and ForceBlind are mutually exclusive")
    }
    if p.Mode == ModeReplicate && (p.ForceWithLease || p.ForceBlind) {
        return errors.New("replicate does not support force flags; use sync instead")
    }
    return nil
}

// ProbeRequest inspects source refs and optional target capabilities.
type ProbeRequest struct {
    Source             Endpoint     `json:"source"`