Address follow-up review on --all-refs

13f69e3→main· Soph·2mo ago·4 files·+84 added/-2 removed

1. bootstrap.go's batched-mode phase log said "pushing tags" even when the tail batch contained only RefKindOther refs. tailPhaseLabel picks "pushing tags", "pushing other refs", or "pushing tags and other refs" based on the actual contents.

2. The runReplicate gate change (relayPlans → pushPlans) affects all replicate flows, not just AllRefs. Add a non-AllRefs delete-only replicate test that pre-fix would have silently no-op'd on prune, pinning the broader behavior.

3. bootstrapWithInputs now recounts Pushed by walking the rewritten plan slice (mirroring finalizeCounts) instead of subtracting warned from bResult.Pushed. The arithmetic relied on bResult.Pushed having already counted rejected refs, which was true but implicit. The new loop makes the relationship with applyRejections explicit and resists future drift.

4. Comment on allRefsFlag noting it's not idempotent — calling it twice on one command would stack PreRunE hooks. Not a current bug; a one-line guard for future readers.

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

Sessions

5e3f6446f73cView transcript

Changes

4

• cmd/git-sync

  • Mflags.go+3

• internal

  • strategy/bootstrap

    • Mbootstrap.go+23/-1

  • syncer

    • Mintegration_test.go+48
    • Msyncer.go+10/-1

54 unmodified lines

55
56
57
58
59
60
61
62
63

54 unmodified lines

// allRefsFlag registers --all-refs with the supplied usage string and 
// bundles its implications. Each pointer in implies is set to true when 
// --all-refs is set, via a PreRunE hook that fires after flag parsing.
//
// Not idempotent: calling twice on the same command stacks two PreRunE 
// hooks on the same flag pointer. Call once per command.
func allRefsFlag(cmd *cobra.Command, usage string, allRefs *bool, implies ...*bool) {
    cmd.Flags().BoolVar(allRefs, "all-refs", false, usage)
    if len(implies) == 0 {

Mcmd/git-sync/flags.go+3

621 unmodified lines

622
623
624
625
625
626
627
628
27 unmodified lines

656
657
658
659
660
661
662
663
664
665
666
667
668
669
670
671
672
673
674
675
676
677
678
679
680
681
682
683

621 unmodified lines

if len(tailPlans) > 0 {
        p.log("bootstrap batch pushing tail refs after branch batches", "tail_count", len(tailPlans))
        if p.OnPhase != nil {
            p.OnPhase("pushing tags")
            p.OnPhase(tailPhaseLabel(tailPlans))
        }
        tailTargetRefs := planner.CopyRefHashMap(p.TargetRefs)
        for _, batch := range batches {
27 unmodified lines

return result, nil
}

// tailPhaseLabel picks a phase label that doesn't lie when --all-refs runs
// without any tags in scope.
func tailPhaseLabel(plans []planner.BranchPlan) string {
    hasTag, hasOther := false, false
    for _, plan := range plans {
        switch plan.Kind {
        case planner.RefKindTag:
            hasTag = true
        case planner.RefKindOther:
            hasOther = true
        }
    }
    switch {
    case hasTag && hasOther:
        return "pushing tags and other refs"
    case hasOther:
        return "pushing other refs"
    default:
        return "pushing tags"
    }
}

// --- Checkpoint planning ---

func planBatches(ctx context.Context, p Params, desired []planner.DesiredRef) ([]plannedBatch, error) {

Minternal/strategy/bootstrap/bootstrap.go+23/-1

3049 unmodified lines

3050
3051
3052
3053
3054
3055
3056
3057
3058
3059
3060
3061
3062
3063
3064
3065
3066
3067
3068
3069
3070
3071
3072
3073
3074
3075
3076
3077
3078
3079
3080
3081
3082
3083
3084
3085
3086
3087
3088
3089
3090
3091
3092
3093
3094
3095
3096
3097
3098
3099
3100
3101
3102
3103

3049 unmodified lines

assertHeadsMatch(t, sourceRepo, targetRepo, testBranch)
    // Pure-prune replicate runs (no source-side updates) must actually delete
    // the orphaned ref. The runReplicate gate previously required at least one
    // relay plan, so delete-only scenarios silently no-op'd; this pins the
    // broader gate (any push plan triggers executeReplicate) for the non-
    // AllRefs branch case too.

func TestRun_IntegrationReplicatePruneDeleteOnlyRunsExecutor(t *testing.T) {
    sourceRepo, sourceFS := newSourceRepo(t)
    makeCommits(t, sourceRepo, sourceFS, 1)

targetRepo, err := git.Init(memory.NewStorage())
    if err != nil {
        t.Fatalf("init target repo: %v", err)
    }
    if err := copyRefsAndObjects(sourceRepo.Storer, targetRepo.Storer, []plumbing.ReferenceName{plumbing.NewBranchReferenceName(testBranch)}); err != nil {
        t.Fatalf("copy target baseline: %v", err)
    }
    staleHead, err := sourceRepo.Reference(plumbing.NewBranchReferenceName(testBranch), true)
    if err != nil {
        t.Fatalf("resolve source head: %v", err)
    }
    orphanRef := plumbing.NewBranchReferenceName("stale-branch")
    if err := targetRepo.Storer.SetReference(plumbing.NewHashReference(orphanRef, staleHead.Hash())); err != nil {
        t.Fatalf("set orphan branch: %v", err)
    }

sourceServer := newSmartHTTPRepoServerV2(t, sourceRepo)
    targetServer := newSmartHTTPRepoServer(t, targetRepo)
    defer sourceServer.Close()
    defer targetServer.Close()

result, err := Run(context.Background(), Config{
        Source:       Endpoint{URL: sourceServer.RepoURL()},
        Target:       Endpoint{URL: targetServer.RepoURL()},
        ProtocolMode: protocolModeAuto,
        Mode:         modeReplicate,
        Prune:        true,
    })
    if err != nil {
        t.Fatalf("delete-only replicate --prune failed: %v", err)
    }
    if result.Deleted != 1 {
        t.Fatalf("expected Deleted=1, got %+v", result)
    }
    if _, err := targetRepo.Reference(orphanRef, true); !errors.Is(err, plumbing.ErrReferenceNotFound) {
        t.Fatalf("expected orphan branch to be pruned, got err=%v", err)
    }
}

// Replicate's bootstrap shortcut must not fire when --prune --all-refs has
// stale other-kind refs to delete on target; otherwise replicate would
// claim "target matches source" while leaving orphaned refs/notes/* behind.

Minternal/syncer/integration_test.go+48

999 unmodified lines

1000
1001
1002
1003
1004
1005
1006
1007
1008
1009
1010
1011
1012
1004
1013
1014
1015
1016

999 unmodified lines

}
    plans := bResult.Plans
    warned := s.applyRejections(plans)
    // Recount Pushed from the rewritten plan slice (mirrors finalizeCounts)
    // rather than subtracting from bResult.Pushed, so the relationship with
    // applyRejections is explicit.
    pushed := 0
    for _, plan := range plans {
        if plan.Action == ActionCreate || plan.Action == ActionUpdate {
            pushed++
        }
    }
    return Result{
        Plans: plans, Pushed: bResult.Pushed - warned, Warned: warned, OperationMode: s.cfg.Mode,
        Plans: plans, Pushed: pushed, Warned: warned, OperationMode: s.cfg.Mode,
        Relay: bResult.Relay, RelayMode: bResult.RelayMode, RelayReason: bResult.RelayReason,
        Batching: bResult.Batching, BatchCount: bResult.BatchCount,
        PlannedBatchCount: bResult.PlannedBatchCount, TempRefs: bResult.TempRefs,

Minternal/syncer/syncer.go+10/-1