diff --git a/cmd/entire/cli/integration_test/checkpoint_sync_remote_test.go b/cmd/entire/cli/integration_test/checkpoint_sync_remote_test.go index 6a64d96519..77d9a036b8 100644 --- a/cmd/entire/cli/integration_test/checkpoint_sync_remote_test.go +++ b/cmd/entire/cli/integration_test/checkpoint_sync_remote_test.go @@ -304,6 +304,78 @@ func TestCheckpointSyncRemote_TrackedPushCapturesElection(t *testing.T) { }) } +func TestCheckpointSyncRemote_IncludedForkCapturesElection(t *testing.T) { + t.Parallel() + ForEachBackend(t, func(t *testing.T, backend string) { + env := NewFeatureBranchEnv(t) + env.CheckpointStore = backend + + bareOrigin := env.SetupBareRemote() + bareFork := env.SetupNamedBareRemote(forkRemote) + testutil.RunGit(t, env.RepoDir, "config", "--local", "--unset-all", "remote.fork.url") + includedConfigPath := filepath.Join(t.TempDir(), "remotes.cfg") + if err := os.WriteFile(includedConfigPath, + []byte(fmt.Sprintf("[remote %q]\n\turl = %s\n", forkRemote, bareFork)), 0o600); err != nil { + t.Fatal(err) + } + testutil.RunGit(t, env.RepoDir, "config", "--local", "include.path", includedConfigPath) + env.setGitConfigBaseline() + + checkpointID := createCheckpointedCommit(t, env, "Add included fork", "fork.go", "package fork", "Add included fork") + env.RunPrePush(forkRemote) + + if !env.CheckpointExistsOnRemote(bareFork, checkpointID) { + t.Errorf("checkpoint %s should land on the included fork remote", checkpointID) + } + if env.CheckpointsPresentOnRemote(bareOrigin) { + t.Error("origin must not receive checkpoints from a push to the included fork") + } + if got := capturedSyncRemotesOnDisk(t, env); len(got) != 1 || got[0] != forkRemote { + t.Errorf("included fork push should capture the election, got %v", got) + } + }) +} + +// TestCheckpointSyncRemote_PushURLOnlyRemoteCannotCaptureElection verifies +// that capture and election use the same fetch-URL eligibility rule. Git treats +// a pushurl-only entry as a configured push target, but checkpoint sync cannot +// read or reconcile from it, so a push there must carry no checkpoint data and +// must not make the destination sticky. +func TestCheckpointSyncRemote_PushURLOnlyRemoteCannotCaptureElection(t *testing.T) { + t.Parallel() + ForEachBackend(t, func(t *testing.T, backend string) { + env := NewFeatureBranchEnv(t) + env.CheckpointStore = backend + + bareOrigin := env.SetupBareRemote() + barePushOnly := env.SetupNamedBareRemote("pushonly") + testutil.RunGit(t, env.RepoDir, "config", "remote.pushonly.pushurl", barePushOnly) + testutil.RunGit(t, env.RepoDir, "config", "--unset-all", "remote.pushonly.url") + env.setGitConfigBaseline() + + checkpointID := createCheckpointedCommit(t, env, "Add capture guard", "guard.go", "package guard", "Add capture guard") + + env.RunPrePush("pushonly") + if env.CheckpointsPresentOnRemote(barePushOnly) { + t.Error("a pushurl-only remote must not receive checkpoint data") + } + if got := capturedSyncRemotesOnDisk(t, env); got != nil { + t.Errorf("a pushurl-only remote must not capture the election, got %v", got) + } + if env.usingGitRefs() && queuedCheckpointRefCount(t, env) == 0 { + t.Error("push queue should be preserved after the ineligible push") + } + + env.RunPrePush("origin") + if !env.CheckpointExistsOnRemote(bareOrigin, checkpointID) { + t.Errorf("checkpoint %s should remain available to the elected origin", checkpointID) + } + if env.usingGitRefs() && queuedCheckpointRefCount(t, env) != 0 { + t.Error("push queue should drain after pushing to origin") + } + }) +} + // TestCheckpointSyncRemote_DeferredPushDoesNotCaptureElection covers the // gate-to-delivery gap on the most likely first push there is: the user adds a // brand-new empty fork and pushes their branch to it for the first time. That diff --git a/cmd/entire/cli/status.go b/cmd/entire/cli/status.go index 4f6b3bed6b..b6bde95155 100644 --- a/cmd/entire/cli/status.go +++ b/cmd/entire/cli/status.go @@ -440,8 +440,8 @@ func computeCheckpointSyncInfo(ctx context.Context, s *EntireSettings) checkpoin elected, err := strategy.ResolveCheckpointSyncRemote(ctx) if err != nil { - // Fail-closed: checkpoint_push_remote names a remote that does not - // exist. The pre-push gate is silently skipping checkpoint sync, so + // Fail-closed: election could not confirm a usable checkpoint sync + // remote. The pre-push gate is silently skipping checkpoint sync, so // status is the user's signal. // Accepted divergence: if a structured checkpoint_remote is also // configured, the gate's dedicated exemption may still sync checkpoint diff --git a/cmd/entire/cli/status_test.go b/cmd/entire/cli/status_test.go index 644708b81b..dfaf61ff31 100644 --- a/cmd/entire/cli/status_test.go +++ b/cmd/entire/cli/status_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/json" + "fmt" "os" "path/filepath" "runtime" @@ -2928,24 +2929,48 @@ func TestRunStatus_CheckpointSyncDestination_CapturedAnnotated(t *testing.T) { func TestRunStatus_CheckpointSyncFailClosed(t *testing.T) { testutil.IsolateGitConfigEnv(t) - setupTestRepo(t) - writeSettings(t, `{"enabled": true, "strategy_options": {"checkpoint_push_remote": "gone"}}`) - testutil.AddRemote(t, ".", "origin", "https://example.com/origin.git") - var stdout bytes.Buffer - if err := runStatus(context.Background(), &stdout, false, false); err != nil { - t.Fatalf("runStatus() error = %v", err) - } + for _, tt := range []struct { + name string + remote string + pushurlOnly bool + emptyFetchURL bool + wantReason string + }{ + {name: "missing remote", remote: "gone"}, + {name: "pushurl-only remote", remote: "pushonly", pushurlOnly: true, wantReason: "fetch URL"}, + {name: "empty fetch URL", remote: "empty", emptyFetchURL: true, wantReason: "fetch URL"}, + } { + t.Run(tt.name, func(t *testing.T) { + setupTestRepo(t) + writeSettings(t, fmt.Sprintf(`{"enabled": true, "strategy_options": {"checkpoint_push_remote": %q}}`, tt.remote)) + if tt.pushurlOnly { + testutil.RunGit(t, ".", "config", "remote."+tt.remote+".pushurl", "https://example.com/pushonly.git") + } + if tt.emptyFetchURL { + testutil.RunGit(t, ".", "config", "remote."+tt.remote+".url", "") + } + testutil.AddRemote(t, ".", "origin", "https://example.com/origin.git") - out := stdout.String() - if !strings.Contains(out, "Checkpoints NOT syncing:") { - t.Errorf("expected fail-closed warning line, got:\n%s", out) - } - if !strings.Contains(out, `"gone"`) { - t.Errorf("fail-closed line should name the misconfigured remote, got:\n%s", out) - } - if strings.Contains(out, "Checkpoints sync to:") { - t.Errorf("fail-closed status must not also print a destination line, got:\n%s", out) + var stdout bytes.Buffer + if err := runStatus(context.Background(), &stdout, false, false); err != nil { + t.Fatalf("runStatus() error = %v", err) + } + + out := stdout.String() + if !strings.Contains(out, "Checkpoints NOT syncing:") { + t.Errorf("expected fail-closed warning line, got:\n%s", out) + } + if !strings.Contains(out, fmt.Sprintf("%q", tt.remote)) { + t.Errorf("fail-closed line should name remote %q, got:\n%s", tt.remote, out) + } + if tt.wantReason != "" && !strings.Contains(out, tt.wantReason) { + t.Errorf("fail-closed line should contain %q, got:\n%s", tt.wantReason, out) + } + if strings.Contains(out, "Checkpoints sync to:") { + t.Errorf("fail-closed status must not also print a destination line, got:\n%s", out) + } + }) } } diff --git a/cmd/entire/cli/strategy/checkpoint_sync_capture.go b/cmd/entire/cli/strategy/checkpoint_sync_capture.go index 63b8359e47..fff70a70d4 100644 --- a/cmd/entire/cli/strategy/checkpoint_sync_capture.go +++ b/cmd/entire/cli/strategy/checkpoint_sync_capture.go @@ -102,7 +102,7 @@ func saveCapturedSyncRemote(ctx context.Context, name string) error { // the write idempotent — "first capture sticks" is decided when the state lands, // not when it was proposed. func pendingCaptureCheckpointSyncRemote(ctx context.Context, pushRemote string) bool { - if !isConfiguredRemote(ctx, pushRemote) { + if !isCheckpointSyncRemoteEligible(ctx, pushRemote) { return false } root, err := capturedSyncRemotesRoot(ctx) diff --git a/cmd/entire/cli/strategy/checkpoint_sync_remote.go b/cmd/entire/cli/strategy/checkpoint_sync_remote.go index 0457c9c000..7f80bfbc24 100644 --- a/cmd/entire/cli/strategy/checkpoint_sync_remote.go +++ b/cmd/entire/cli/strategy/checkpoint_sync_remote.go @@ -42,12 +42,12 @@ type CheckpointSyncRemote struct { // ResolveCheckpointSyncRemote elects the one configured git remote that // checkpoint data syncs to. Pure local lookup — no network. Precedence: -// checkpoint_push_remote setting (fail-closed if the named remote does not -// exist), then the captured election (evidence-elected by a past push that -// agreed with the branch's declared push destination; fail-soft if that -// remote is gone), then "origin", then the sole remote, then the first remote -// in .git/config order. It knows nothing about the checkpoint_remote URL -// feature; callers exempt that case themselves. +// checkpoint_push_remote setting (fail-closed if the named remote has no +// fetch URL), then the captured election (evidence-elected by a past push that +// agreed with the branch's declared push destination; fail-soft if that remote +// is no longer fetchable), then "origin", then the sole remote, then the first +// remote in local config traversal order, including included files. It knows +// nothing about the checkpoint_remote URL feature; callers exempt that case. // // Deliberately NOT keyed on the branch's tracking config alone // (branch..pushRemote / remote.pushDefault / branch..remote). @@ -79,35 +79,40 @@ func ResolveCheckpointSyncRemote(ctx context.Context) (CheckpointSyncRemote, err if err != nil { return CheckpointSyncRemote{}, fmt.Errorf("cannot read settings to resolve the checkpoint sync remote: %w", err) } + // Every tier elects from the same fetchable set. `git remote get-url` + // accepts a pushurl-only entry even though reads and reconciliation cannot. + fetchRemotes, err := configuredRemotesInConfigOrderResult(ctx) + if err != nil { + return CheckpointSyncRemote{}, fmt.Errorf("cannot read git remotes to resolve the checkpoint sync remote: %w", err) + } if name := s.GetCheckpointPushRemote(); name != "" { - if !isConfiguredRemote(ctx, name) { + if !slices.Contains(fetchRemotes, name) { return CheckpointSyncRemote{}, fmt.Errorf( - "checkpoint_push_remote %q is not a configured git remote; checkpoint sync disabled until fixed", name) + "checkpoint_push_remote %q has no configured fetch URL; checkpoint sync disabled until fixed", name) } return CheckpointSyncRemote{Name: name, Source: SyncRemoteSourceConfig}, nil } // Captured tier: fail-soft, unlike the explicit setting above — capture - // is automatic state, so a captured remote that was since renamed or - // removed falls through to the default tiers instead of disabling sync. + // is automatic state, so a captured remote that is no longer fetchable + // falls through to the default tiers instead of disabling sync. for _, name := range loadCapturedSyncRemotes(ctx) { - if isConfiguredRemote(ctx, name) { + if slices.Contains(fetchRemotes, name) { return CheckpointSyncRemote{Name: name, Source: SyncRemoteSourceObserved}, nil } - logging.Debug(ctx, "captured checkpoint sync remote is not configured; falling through", + logging.Debug(ctx, "captured checkpoint sync remote has no configured fetch URL; falling through", slog.String("remote", name)) } - remotes := configuredRemotesInConfigOrder(ctx) switch { - case len(remotes) == 0: + case len(fetchRemotes) == 0: return CheckpointSyncRemote{}, nil - case slices.Contains(remotes, "origin"): + case slices.Contains(fetchRemotes, "origin"): return CheckpointSyncRemote{Name: "origin", Source: SyncRemoteSourceDefault}, nil - case len(remotes) == 1: - return CheckpointSyncRemote{Name: remotes[0], Source: SyncRemoteSourceSole}, nil + case len(fetchRemotes) == 1: + return CheckpointSyncRemote{Name: fetchRemotes[0], Source: SyncRemoteSourceSole}, nil default: - return CheckpointSyncRemote{Name: remotes[0], Source: SyncRemoteSourceFirst}, nil + return CheckpointSyncRemote{Name: fetchRemotes[0], Source: SyncRemoteSourceFirst}, nil } } @@ -147,17 +152,17 @@ func checkpointSyncAllowedForRemote(ctx context.Context, pushRemote, pendingCapt // until the elected remote happens to be pushed. // // Stays quiet unless every condition holds: the push target is a configured -// remote (checkpoint_push_remote takes a remote name, so a raw-URL push has -// no actionable suggestion), the election succeeded AND was automatic (an -// explicit checkpoint_push_remote is a decision already made, and the -// fail-closed misconfigured case logs a warning through the gate itself), and -// checkpoints are actually waiting. Fully local — no network. +// remote with a fetch URL (checkpoint_push_remote must name one), the election +// succeeded AND was automatic (an explicit checkpoint_push_remote is a +// decision already made, and the fail-closed misconfigured case logs a warning +// through the gate itself), and checkpoints are actually waiting. Fully local +// — no network. // // The hint names .entire/settings.local.json: a remote name is a per-clone // fact, and committing it to the tracked settings.json would fail-close // checkpoint sync for every teammate whose clone lacks that remote name. func hintGatedCheckpointSync(ctx context.Context, pushRemote string) { - if !isConfiguredRemote(ctx, pushRemote) { + if !isCheckpointSyncRemoteEligible(ctx, pushRemote) { return } syncRemote, err := ResolveCheckpointSyncRemote(ctx) @@ -211,24 +216,34 @@ func hintGatedCheckpointSync(ctx context.Context, pushRemote string) { slog.String("push_remote", pushRemote)) } -// configuredRemotesInConfigOrder lists remote names in .git/config section -// order (approximates "first remote added"; `git remote` output is -// alphabetical and unsuitable). Remotes configured with only pushurl are -// deliberately invisible (spec Unit 1). Errors yield an empty list. +// configuredRemotesInConfigOrder lists remote names in local config traversal +// order, including included files (approximates "first remote added"; `git +// remote` output is alphabetical and unsuitable). Remotes configured with only +// pushurl are deliberately invisible. Errors yield an empty list. func configuredRemotesInConfigOrder(ctx context.Context) []string { return cachedRemotesInConfigOrder(ctx, readRemotesInConfigOrder) } -// readRemotesInConfigOrder lists remote names, distinguishing "this repo has no -// remotes" from "the read failed". Both used to collapse to nil, which was -// harmless while every caller re-ran the command — but the per-invocation cache -// would memoize a failure's nil as a legitimately empty list and then skip +func configuredRemotesInConfigOrderResult(ctx context.Context) ([]string, error) { + return cachedRemotesInConfigOrderResult(ctx, readRemotesInConfigOrder) +} + +// isCheckpointSyncRemoteEligible keeps capture and hinting on the same +// fetch-URL eligibility rule as election. +func isCheckpointSyncRemoteEligible(ctx context.Context, name string) bool { + return slices.Contains(configuredRemotesInConfigOrder(ctx), name) +} + +// readRemotesInConfigOrder lists remote names from local config and its includes, +// distinguishing "this repo has no remotes" from "the read failed". Both used to +// collapse to nil, which was harmless while every caller re-ran the command. +// The per-invocation cache would memoize a failure's nil as an empty list and skip // checkpoint sync for the rest of the process. `git config --get-regexp` exits 1 // for no match, so that exit code alone is the empty answer; anything else (a // fork failure under load, a cancelled context, a locked config) is an error the // cache must not keep. func readRemotesInConfigOrder(ctx context.Context) ([]string, error) { - cmd := exec.CommandContext(ctx, "git", "config", "--local", "--get-regexp", `^remote\..*\.url$`) + cmd := exec.CommandContext(ctx, "git", "config", "--local", "--includes", "--get-regexp", `^remote\..*\.url$`) if worktreeRoot, ok := settings.WorktreeRoot(ctx); ok { cmd.Dir = worktreeRoot } @@ -245,8 +260,8 @@ func readRemotesInConfigOrder(ctx context.Context) ([]string, error) { for _, line := range strings.Split(strings.TrimSpace(string(out)), "\n") { // line: "remote..url "; may contain dots, so trim // the fixed prefix and the ".url " suffix instead of splitting. - key, _, ok := strings.Cut(line, " ") - if !ok { + key, value, ok := strings.Cut(line, " ") + if !ok || strings.TrimSpace(value) == "" { continue } name := strings.TrimSuffix(strings.TrimPrefix(key, "remote."), ".url") diff --git a/cmd/entire/cli/strategy/checkpoint_sync_remote_test.go b/cmd/entire/cli/strategy/checkpoint_sync_remote_test.go index 239d324a95..3e9d49c9b3 100644 --- a/cmd/entire/cli/strategy/checkpoint_sync_remote_test.go +++ b/cmd/entire/cli/strategy/checkpoint_sync_remote_test.go @@ -11,6 +11,7 @@ import ( "github.com/entireio/cli/cmd/entire/cli/osroot" "github.com/entireio/cli/cmd/entire/cli/paths" + "github.com/entireio/cli/cmd/entire/cli/settings" "github.com/entireio/cli/cmd/entire/cli/testutil" "github.com/stretchr/testify/assert" @@ -51,24 +52,43 @@ func TestResolveCheckpointSyncRemote_ConfigSetting(t *testing.T) { } // Not parallel: uses t.Chdir() -func TestResolveCheckpointSyncRemote_ConfigSettingMissingRemote_FailsClosed(t *testing.T) { +func TestResolveCheckpointSyncRemote_ConfigSettingInvalidRemote_FailsClosed(t *testing.T) { testutil.IsolateGitConfigEnv(t) ctx := context.Background() - tmpDir := t.TempDir() - testutil.InitRepo(t, tmpDir) - testutil.WriteFile(t, tmpDir, "f.txt", "init") - testutil.GitAdd(t, tmpDir, "f.txt") - testutil.GitCommit(t, tmpDir, "init") - testutil.AddRemote(t, tmpDir, "origin", "https://example.com/origin.git") - testutil.WriteCheckpointPushRemoteSetting(t, tmpDir, "gone") - - t.Chdir(tmpDir) + for _, tt := range []struct { + name string + remote string + pushURL string + emptyFetchURL bool + }{ + {name: "missing remote", remote: "gone"}, + {name: "pushurl-only remote", remote: "pushonly", pushURL: "https://example.com/pushonly.git"}, + {name: "empty fetch URL", remote: "empty", emptyFetchURL: true}, + } { + t.Run(tt.name, func(t *testing.T) { + tmpDir := t.TempDir() + testutil.InitRepo(t, tmpDir) + testutil.WriteFile(t, tmpDir, "f.txt", "init") + testutil.GitAdd(t, tmpDir, "f.txt") + testutil.GitCommit(t, tmpDir, "init") + if tt.pushURL != "" { + testutil.RunGit(t, tmpDir, "config", "remote."+tt.remote+".pushurl", tt.pushURL) + } + if tt.emptyFetchURL { + testutil.RunGit(t, tmpDir, "config", "remote."+tt.remote+".url", "") + } + testutil.AddRemote(t, tmpDir, "origin", "https://example.com/origin.git") + testutil.WriteCheckpointPushRemoteSetting(t, tmpDir, tt.remote) + t.Chdir(tmpDir) - got, err := ResolveCheckpointSyncRemote(ctx) - require.Error(t, err) - assert.Contains(t, err.Error(), "gone") - assert.Empty(t, got.Name) + got, err := ResolveCheckpointSyncRemote(ctx) + require.Error(t, err) + assert.Contains(t, err.Error(), tt.remote) + assert.Contains(t, err.Error(), "fetch URL") + assert.Empty(t, got.Name) + }) + } } // Not parallel: uses t.Chdir() @@ -159,6 +179,33 @@ func TestResolveCheckpointSyncRemote_SettingsLoadErrorFailsClosed(t *testing.T) assert.Empty(t, got.Name) } +// Not parallel: uses t.Chdir() +func TestResolveCheckpointSyncRemote_GitConfigReadErrorFailsClosed(t *testing.T) { + testutil.IsolateGitConfigEnv(t) + tmpDir := t.TempDir() + testutil.InitRepo(t, tmpDir) + testutil.WriteFile(t, tmpDir, "f.txt", "init") + testutil.GitAdd(t, tmpDir, "f.txt") + testutil.GitCommit(t, tmpDir, "init") + testutil.AddRemote(t, tmpDir, "origin", "https://example.com/origin.git") + t.Chdir(tmpDir) + + ctx := settings.WithWorktreeRoot(context.Background(), tmpDir) + t.Setenv("PATH", t.TempDir()) + + got, err := ResolveCheckpointSyncRemote(ctx) + require.ErrorContains(t, err, "executable file not found") + assert.Empty(t, got.Name) +} + +func TestReadRemotesInConfigOrder_CanceledContext(t *testing.T) { + ctx, cancel := context.WithCancel(settings.WithWorktreeRoot(context.Background(), t.TempDir())) + cancel() + + _, err := readRemotesInConfigOrder(ctx) + require.ErrorIs(t, err, context.Canceled) +} + // Not parallel: uses t.Chdir() func TestResolveCheckpointSyncRemote_NoRemotes(t *testing.T) { testutil.IsolateGitConfigEnv(t) @@ -204,6 +251,34 @@ func TestResolveCheckpointSyncRemote_PushurlOnlyRemoteIsInvisible(t *testing.T) assert.Equal(t, CheckpointSyncRemote{Name: "first-real", Source: SyncRemoteSourceFirst}, got) } +// Not parallel: uses t.Chdir(). +func TestCheckpointSyncRemote_IncludedRemote(t *testing.T) { + testutil.IsolateGitConfigEnv(t) + ctx := context.Background() + dir := newCaptureTestRepo(t) + testutil.RunGit(t, dir, "remote", "remove", "fork") + testutil.WriteFile(t, dir, "remotes.cfg", "[remote \"fork\"]\n\turl = https://example.com/fork.git\n") + setGitConfig(t, dir, "include.path", filepath.Join(dir, "remotes.cfg")) + setGitConfig(t, dir, "remote.pushDefault", "fork") + require.Equal(t, "https://example.com/fork.git", strings.TrimSpace(testutil.RunGit(t, dir, "remote", "get-url", "fork"))) + t.Chdir(dir) + + got, err := ResolveCheckpointSyncRemote(ctx) + require.NoError(t, err) + assert.Equal(t, CheckpointSyncRemote{Name: "origin", Source: SyncRemoteSourceDefault}, got) + require.True(t, pendingCaptureCheckpointSyncRemote(ctx, "fork"), "the included remote is a valid capture target") + captureOnSuccessfulPush(ctx, "fork") + assert.Equal(t, []string{"fork"}, loadCapturedSyncRemotes(ctx)) + got, err = ResolveCheckpointSyncRemote(ctx) + require.NoError(t, err) + assert.Equal(t, CheckpointSyncRemote{Name: "fork", Source: SyncRemoteSourceObserved}, got) + + testutil.WriteCheckpointPushRemoteSetting(t, dir, "fork") + got, err = ResolveCheckpointSyncRemote(ctx) + require.NoError(t, err) + assert.Equal(t, CheckpointSyncRemote{Name: "fork", Source: SyncRemoteSourceConfig}, got) +} + // Not parallel: uses t.Chdir() // Regression guard for the tracking tier that was removed before merge: the // branch's tracking config must NOT decide the election. @@ -512,6 +587,18 @@ func TestHintGatedCheckpointSync(t *testing.T) { assert.Empty(t, buf.String(), "checkpoint_push_remote takes a remote name; a URL push has no actionable hint") }) + t.Run("pushurl-only remote stays silent", func(t *testing.T) { + dir := initHintRepo(t, false) + testutil.RunGit(t, dir, "config", "remote.pushonly.pushurl", "https://example.com/pushonly.git") + setGitConfig(t, dir, "remote.pushDefault", "pushonly") + t.Chdir(dir) + buf := captureStderrWriter(t) + + hintGatedCheckpointSync(ctx, "pushonly") + + assert.Empty(t, buf.String(), "the hint must not recommend a remote that checkpoint sync cannot read from") + }) + t.Run("failed election stays silent", func(t *testing.T) { dir := initHintRepo(t, true) testutil.WriteCheckpointPushRemoteSetting(t, dir, "gone") @@ -569,6 +656,17 @@ func TestResolveCheckpointSyncRemote_CapturedTier(t *testing.T) { assert.Equal(t, CheckpointSyncRemote{Name: "origin", Source: SyncRemoteSourceDefault}, got) }) + t.Run("captured pushurl-only remote falls through to origin", func(t *testing.T) { + dir := newCaptureTestRepo(t) + testutil.RunGit(t, dir, "config", "remote.pushonly.pushurl", "https://example.com/pushonly.git") + t.Chdir(dir) + require.NoError(t, saveCapturedSyncRemote(ctx, "pushonly")) + + got, err := ResolveCheckpointSyncRemote(ctx) + require.NoError(t, err) + assert.Equal(t, CheckpointSyncRemote{Name: "origin", Source: SyncRemoteSourceDefault}, got) + }) + t.Run("corrupt capture state falls through to origin", func(t *testing.T) { dir := newCaptureTestRepo(t) t.Chdir(dir) @@ -712,6 +810,17 @@ func TestCaptureCheckpointSyncRemote(t *testing.T) { assert.Equal(t, []string{"fork"}, loadCapturedSyncRemotes(ctx)) }) + t.Run("pushurl-only remote cannot capture", func(t *testing.T) { + dir := newCaptureTestRepo(t) + testutil.RunGit(t, dir, "config", "remote.pushonly.pushurl", "https://example.com/pushonly.git") + setGitConfig(t, dir, "remote.pushDefault", "pushonly") + t.Chdir(dir) + + assert.False(t, pendingCaptureCheckpointSyncRemote(ctx, "pushonly"), + "capture must use the same fetch-URL eligibility rule as election") + assert.Empty(t, loadCapturedSyncRemotes(ctx)) + }) + t.Run("raw URL push never captures", func(t *testing.T) { dir := newCaptureTestRepo(t) setGitConfig(t, dir, "branch."+currentBranchName(t, dir)+".remote", "fork") diff --git a/cmd/entire/cli/strategy/git_remote_cache.go b/cmd/entire/cli/strategy/git_remote_cache.go index bdf727a7ae..8f36fed7dd 100644 --- a/cmd/entire/cli/strategy/git_remote_cache.go +++ b/cmd/entire/cli/strategy/git_remote_cache.go @@ -10,7 +10,7 @@ import ( // The checkpoint sync remote is elected from scratch on every call — a // documented tradeoff (see CheckpointReadRemotes), and every election shells out -// to git to answer two questions about .git/config: which remotes exist, and +// to git to answer two questions about local Git config: which remotes exist, and // does remote X exist. Those answers are identical for every election within one // command against one repository, and there are several: `entire checkpoint list` // resolves the chain four times (metadata-disconnection warning, the branch @@ -31,7 +31,7 @@ import ( type gitRemoteCacheKey struct{} -// gitRemoteCache memoizes .git/config remote reads, partitioned by repository. +// gitRemoteCache memoizes local Git config remote reads, partitioned by repository. // // The partition is load-bearing, not tidiness: `entire dispatch --repos a,b` // walks several repositories in ONE process, scoping each one's election with @@ -69,10 +69,10 @@ type remoteSnapshot struct { orderedSet bool // member holds isConfiguredRemote answers per remote name. Kept separate // from ordered because the two ask git different questions: ordered lists - // only remotes carrying a url key in local config, while isConfiguredRemote - // runs `git remote get-url`, which also sees pushurl-only remotes and - // inherited scopes. Deriving one from the other would change election - // semantics, so each is memoized against its own git call. + // only remotes carrying a url key in local config or its includes, while + // isConfiguredRemote runs `git remote get-url`, which also sees pushurl-only + // remotes. Deriving one from the other would change election semantics, so + // each is memoized against its own call. member map[string]bool } @@ -176,28 +176,31 @@ func (c *gitRemoteCache) snapshotFor(ctx context.Context) *remoteSnapshot { // list would turn one transient git hiccup into a process-long "this repo has no // remotes", and the election answers that by silently skipping checkpoint sync. func cachedRemotesInConfigOrder(ctx context.Context, read func(context.Context) ([]string, error)) []string { + names, _ := cachedRemotesInConfigOrderResult(ctx, read) //nolint:errcheck // historical best-effort contract + return names +} + +func cachedRemotesInConfigOrderResult(ctx context.Context, read func(context.Context) ([]string, error)) ([]string, error) { c := cacheFromContext(ctx) if c == nil { - names, _ := read(ctx) //nolint:errcheck // uncached path keeps the historical best-effort contract - return names + return read(ctx) } snap := c.snapshotFor(ctx) if snap == nil { - names, _ := read(ctx) //nolint:errcheck // unidentifiable repo: same best-effort contract - return names + return read(ctx) } snap.mu.Lock() defer snap.mu.Unlock() if snap.orderedSet { - return snap.ordered + return snap.ordered, nil } names, err := read(ctx) if err != nil { // Transient: answer this call, leave the slot unset so the next one retries. - return nil + return nil, err } snap.ordered, snap.orderedSet = names, true - return snap.ordered + return snap.ordered, nil } // cachedIsConfiguredRemote returns the memoized answer for name in this call's