From 375b87b6fd29c6db712e487370158f3829836606 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 10 Nov 2026 08:53:16 +0000 Subject: [PATCH 01/10] feat(git): name an empty repository's default branch from protocol v2 unborn HEAD go-git v6.0.0-beta.1 (the latest release) still returns ErrEmptyRemoteRepository from GetRemoteRefs before building RemoteRefs, so Remote.List cannot expose the unborn HEAD. listRemoteRefs now opens the upload-pack session itself and, for an empty repository, asks ls-refs again through go-git's public transport.Commander when the server offers ls-refs=unborn. CheckRepo, the fetch report, the refresh advertisement and the observation carry the name: status.remote.parent is {state: Unborn, branch: X} when the server says X, and has no branch when it does not (v0). A push keeps the name discovery read. Behavior is unchanged: the first commit is still a root on the write branch. Co-Authored-By: Claude Opus 5.5 --- api/v1alpha3/gitbranch_types.go | 5 +- .../bases/configbutler.ai_gitbranches.yaml | 5 +- docs/INDEX.md | 2 +- docs/TODO.md | 19 +- docs/configuration.md | 4 +- .../gittarget-parent-empty-repository.md | 63 ++++--- docs/misconceptions.md | 6 +- internal/git/ado_multiack_test.go | 11 +- internal/git/branch_worker.go | 22 ++- internal/git/git.go | 40 ++--- internal/git/git_operations_test.go | 4 +- internal/git/git_smart_fetch.go | 118 +++++++++++-- internal/git/refresh.go | 14 +- internal/git/remote_observation.go | 7 +- internal/git/types.go | 4 +- internal/git/unborn_head_test.go | 162 ++++++++++++++++++ 16 files changed, 385 insertions(+), 101 deletions(-) create mode 100644 internal/git/unborn_head_test.go diff --git a/api/v1alpha3/gitbranch_types.go b/api/v1alpha3/gitbranch_types.go index ad454b9e..6cfc6339 100644 --- a/api/v1alpha3/gitbranch_types.go +++ b/api/v1alpha3/gitbranch_types.go @@ -186,8 +186,9 @@ type GitBranchParentStatus struct { State GitBranchParentState `json:"state"` // Branch is the parent branch: spec.parentBranch, or the remote's default - // branch when that is omitted. Empty when state is "Unborn", and when the - // remote names no default branch. + // branch when that is omitted. When state is "Unborn" it is the branch the + // empty repository's HEAD names, which only a server speaking Git protocol + // v2 reports. Empty when the remote names no default branch. // +optional Branch string `json:"branch,omitempty"` diff --git a/config/crd/bases/configbutler.ai_gitbranches.yaml b/config/crd/bases/configbutler.ai_gitbranches.yaml index e7e9d345..4a2e558b 100644 --- a/config/crd/bases/configbutler.ai_gitbranches.yaml +++ b/config/crd/bases/configbutler.ai_gitbranches.yaml @@ -391,8 +391,9 @@ spec: branch: description: |- Branch is the parent branch: spec.parentBranch, or the remote's default - branch when that is omitted. Empty when state is "Unborn", and when the - remote names no default branch. + branch when that is omitted. When state is "Unborn" it is the branch the + empty repository's HEAD names, which only a server speaking Git protocol + v2 reports. Empty when the remote names no default branch. type: string commit: description: |- diff --git a/docs/INDEX.md b/docs/INDEX.md index 415a9d60..40a8e5bf 100644 --- a/docs/INDEX.md +++ b/docs/INDEX.md @@ -142,7 +142,7 @@ so you never have to read a proposal to find out it already shipped. | [`gittarget-branch-worker-pending-writes.md`](design/gittarget-branch-worker-pending-writes.md) | **built in #413.** Ordered pending writes, one materializer and one retry schedule; a safe admission boundary for the watch; Git call deadlines; intake pause and resume; publication status. Holds the recovery contract for an outage, replay limits, and the acceptance scenarios. | | [`gittarget-parent-branch.md`](design/gittarget-parent-branch.md) | **merged in #407; the field moved to `GitBranch` in #429.** `GitBranch.spec.parentBranch`: a branch stays on standby until a write needs it, then is created from the parent's tip as checked at publication. The contract as built, the defect it started from, prior art and naming. Open: reporting the parent per branch in status. | | [`gittarget-parent-hardening.md`](design/gittarget-parent-hardening.md) | **done in #407.** Parent changes on a live worker, an empty repository gaining a ref, an unresolved default branch, recovery that cannot strand work, and the refused upload, without adding a pull to the fast path; plus the two recovery fixes from the review at `93120caf`. Records the merge boundary and the next separate PRs. | -| [`gittarget-parent-empty-repository.md`](design/gittarget-parent-empty-repository.md) | **deferred.** An empty repository's default branch: the server reports it over protocol v2, and go-git discarded it at alpha.5. The pinned beta.1 keeps it at the transport layer (`RemoteRefs.Unborn`), so the go-git half may be unblocked (see [TODO](TODO.md)); which branch the first commit may create stays a separate policy decision. Assuming `main` was considered and rejected. | +| [`gittarget-parent-empty-repository.md`](design/gittarget-parent-empty-repository.md) | **evidence built, policy deferred.** An empty repository's default branch: the server reports it over protocol v2, and status now names it for `Unborn`, read through go-git's public `transport.Commander`. Which branch the first commit may create stays a separate policy decision. Assuming `main` was considered and rejected. | | [`gittarget-parent-observation.md`](design/gittarget-parent-observation.md) | **deferred.** Keep the parent visible after the write branch exists: availability first, ancestry only after measuring how often a shallow checkout can answer it. Informational; authorizes no branch mutation. | | [`gittarget-red-status-plan.md`](design/gittarget-red-status-plan.md) | **design, partly built.** The smaller alternative to Git write preflight: make a `GitTarget` turn red and explain why no commit was made, without moving Git checks into the admission webhook and without adding a second state source. Keeps the existing kstatus trio and domain conditions, ranks the most likely operator-facing failures, and has now shipped the first status-transition slice: refusal messages republish when details change, scoped refusals cannot be cleared by unrelated scope success, `BlockMessage()` carries stable issue kind/path/actor detail, and the unsupported-folder e2e covers Git-side break and recovery. | | [`open-asks-priority.md`](design/open-asks-priority.md) | **stale; [`TODO.md`](TODO.md) is the work queue.** Merged three overlapping backlogs (the downstream consumer asks, the API-surface block left unbuilt by the status and configuration-model review, and the config-surface proposal B1–B6) into one ordered queue under four stated tests, and says where we deliberately do **not** do what was asked. B2, B4, F6 and F10 have shipped, and F9 is probably moot since #333 deleted the `ClusterWatchRule` `scope` field. Still the record of one design call: **Option C sibling inference was deleted** rather than given an off-switch, and "what the deletion taught" records what building it found | diff --git a/docs/TODO.md b/docs/TODO.md index 740bec86..23d6089c 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -135,15 +135,16 @@ Their argument for each is in [gitops-api-open-asks.md](future/gitops-api-open-a `GitTarget`, plus the durable queue, plus whether a replica warms its fact index before starting a watch it took over. Redis becomes required in this mode. [ha-gittarget-distribution-plan.md](future/ha-gittarget-distribution-plan.md). -- [ ] **Learn an empty repository's default branch, and simplify around it.** The pinned go-git - (`v6.0.0-beta.1`) now keeps what protocol v2 `ls-refs` reports about an unborn `HEAD` - (`transport.RemoteRefs.Unborn`); at alpha.5 it was discarded, which is why status reports - `unknown` and the code carries `ParentUnborn`, `ErrDefaultBranchUnresolved` and the - `branch_worker_empty_repo_test.go` pins. Check whether `Remote.List` exposes it or only the - transport session does, read it in `refresh.go` and `git_smart_fetch.go`, and remove what it makes - unnecessary. Which branch a first commit may create stays a separate policy decision - ([gittarget-parent-empty-repository.md](design/gittarget-parent-empty-repository.md); its §4 and - the go-git entry in [misconceptions.md](misconceptions.md) need updating with it). +- [ ] **Decide which branch a first commit may create in an empty repository.** Status now names an + empty repository's default branch when a protocol v2 server reports it (`unbornHead` in + `internal/git/git_smart_fetch.go`); behavior is unchanged. The policy choices are in + [gittarget-parent-empty-repository.md](design/gittarget-parent-empty-repository.md) §3. + `ParentUnborn` and `ErrDefaultBranchUnresolved` stay whatever is chosen: the first is a state the + repository is in, the second a repository with commits whose `HEAD` dangles. +- [ ] **Drop `unbornHead` once go-git reports an empty repository's unborn HEAD.** Upstream main + (`d6fa0275`) adds `LsRefsOutput.UnbornHead` but `GetRemoteRefs` still returns + `ErrEmptyRemoteRepository` before building `RemoteRefs`; an upstream PR returning the name there + (or on the error) would let `listRemoteRefs` go back to `Remote.List`. - [ ] **The branch claim is first-proven-wins** (`internal/git/branch_owner.go`). Anyone who can create a `GitProvider` and a `GitBranch` with a credential that passes the push-session proof can claim a branch no older `GitBranch` holds; on Gitea and Forgejo a read-only credential passes it diff --git a/docs/configuration.md b/docs/configuration.md index ddf9ae0e..b97f0ad4 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -1714,7 +1714,9 @@ status: created from, observed together with the branch's absence. `state` is `Found` (the first commit builds on `commit`, the parent's tip), `Missing` (the parent is not on the remote either; the `GitBranch` reports `ParentBranchNotFound` and nothing is written) or `Unborn` (an empty repository, with - no refs at all, and no parent configured; the first commit starts the branch with no history). See + no refs at all, and no parent configured; the first commit starts the branch with no history). For + `Unborn`, `branch` is the branch the empty repository's `HEAD` names, which only a server speaking + Git protocol v2 reports (GitHub does); it is empty when the server does not say. See [starting the write branch from another branch](#starting-the-write-branch-from-another-branch-specparentbranch). `kubectl get gitbranch` shows `commit` in the `Commit` column, and `-o wide` shows diff --git a/docs/design/gittarget-parent-empty-repository.md b/docs/design/gittarget-parent-empty-repository.md index 58aa02df..b2271b50 100644 --- a/docs/design/gittarget-parent-empty-repository.md +++ b/docs/design/gittarget-parent-empty-repository.md @@ -1,6 +1,6 @@ # GitTarget parent branch: an empty repository's default branch -> **design**: deferred; not part of #407. The experiment in §2 is done, nothing else is built. +> **design**: evidence built (§4, 2026-11-10); the policy in §3 is deferred. Not part of #407. > Dated 2026-10-01. > > Related: [`gittarget-parent-branch.md`](gittarget-parent-branch.md), the feature as built; @@ -43,8 +43,9 @@ git -c protocol.version=2 ls-remote --symref https://github.com/ConfigButler/emp answers `unborn HEAD symref-target:refs/heads/trunk`. - Protocol v0 reports nothing. -**go-git asks the question, then throws the answer away.** At v6.0.0-alpha.5, and unchanged on -upstream main at `fc18716c`: +**go-git asks the question, then throws the answer away.** At v6.0.0-alpha.5, still at +v6.0.0-beta.1 and on upstream main at `12b788e4` (line numbers below are alpha.5's; §4 has the +workaround): - It speaks protocol v2 by default, and sends `unborn` when the server advertises `ls-refs=unborn` (`internal/transport/v2.go`, `lsRefsSupportsUnborn`). @@ -123,30 +124,44 @@ explicit bootstrap form is wanted, it gets its own design. ## 4. Getting the go-git change -1. **Upstream first.** Open a go-git issue and a PR with §7: the error type, both paths, and tests - against a v2 `git http-backend` and over `file://`. It is additive and backward-compatible for - every `errors.Is` caller. An upstream PR may prefer to expose the unborn target on the - `Remote.List` result rather than only through the error; settle the shape with the - maintainers. -2. **Our side waits for a go-git release that carries it.** Until then nothing changes. -3. **Fallback, if upstream stalls:** a temporary `replace` to a fork tag, only with the user's - agreement. Do not hand-roll a v2 `ls-refs` client in this repository. +**Built without a go-git change, on v6.0.0-beta.1** (the latest release as of 2026-11-10). beta.1 +still discards the answer: `GetRemoteRefs` returns `ErrEmptyRemoteRepository` for an empty +repository before it builds the `RemoteRefs` whose `Unborn` field would carry it, so `Remote.List` +cannot expose it. Upstream main (`d6fa0275`) adds `LsRefsOutput.UnbornHead` but returns the same +error. What beta.1 does make public is enough: -## 5. Steps once go-git carries it +- `listRemoteRefs` opens the upload-pack session itself (`gitclient.New(...).Handshake`) instead + of calling `Remote.List`, and returns the same refs (peeled ones left out, as `Remote.List` + does by default). +- On `ErrEmptyRemoteRepository`, `unbornHead` asks `ls-refs` again on that session through go-git's + public `transport.Commander`, with `packp.LsRefsArgs{Symrefs, Unborn}`, and only when the server + offers `ls-refs=unborn`. It is not a hand-rolled client: go-git encodes the request and decodes the + reply. A v0/v1 session is not asked, and a reply that carries any hash ref is discarded (the + repository stopped being empty in between). +- Cost: one more upload-pack POST, only for an empty repository over v2. The v0 harness and the + request ledger are unchanged. + +Delete `unbornHead` once a go-git release returns the name for an empty repository; the §7 patch +is still the shape to propose upstream. + +## 5. Steps 1. **Verify the GitHub claim** in §1 on a *throwaway* repository, never on `ConfigButler/empty.git`: - does pushing a non-default branch first make it the default? Record the answer in §1. -2. **Harness.** Add a v2 variant of `startRealGitServer` that keeps `Git-Protocol`. Leave the - ADO/v0 server and the ledger rows untouched. -3. **Discovery.** `CheckRepo`, `listRemoteRefs` and `SmartFetchFrom` read the unborn target with - `errors.As`. `CheckRepo` returns `DefaultBranch{ShortName: X, Unborn: true}`; the observation - carries `ParentBranch=X` for `Unborn`. Update the doc comments that say "empty when Unborn" - (`remote_observation.go`, `api/v1alpha3/gittarget_types.go`, `configuration.md`). -4. **Tests.** On the v2 harness with a bare repository made by `init -b trunk`, status reports - `trunk`; on the v0 harness it reports an empty branch. Behavior is unchanged in both. The - ledger is unchanged: the target rides on the `ls-refs` call discovery already makes. -5. **Live test.** `TestCheckRepo_PublicConnectivityEmpty` expects `main` / `Unborn`. -6. **Then the policy decision in §3**, as its own change with an UPGRADING note. + does pushing a non-default branch first make it the default? Not done; it matters only to §3. +2. **Harness.** Done: `adoSimulator.protocolV2` keeps `Git-Protocol`, off by default, so the + ADO/v0 server and the ledger rows are untouched. +3. **Discovery.** Done: `CheckRepo` returns `DefaultBranch{ShortName: X, Unborn: true}`; + `PullReport.ParentBranch` and the refresh advertisement carry X, and the observation is + `{state: Unborn, branch: X}`. A push cannot see `HEAD`, so its observation keeps the name the + last discovery read. `recordAbsentBranch` now decides `Unborn` by the absent parent commit, not + the absent name. +4. **Tests.** Done (`internal/git/unborn_head_test.go`): a bare repository made by `init -b trunk` + reports `trunk` over v2 and no name over v0; behavior is unchanged in both. Over v2 a dangling + `HEAD` in a repository with commits is named too. +5. **Live test.** Done: `TestCheckRepo_PublicConnectivityEmpty` expects `main` / `Unborn`. +6. **Then the policy decision in §3**, as its own change with an UPGRADING note. Whatever is chosen, + `ParentUnborn` and `ErrDefaultBranchUnresolved` stay: the first is the state an empty repository + is in, the second a repository with commits whose `HEAD` names no branch it carries. ## 6. Out of scope diff --git a/docs/misconceptions.md b/docs/misconceptions.md index ffbb77e1..c9e2fd71 100644 --- a/docs/misconceptions.md +++ b/docs/misconceptions.md @@ -254,8 +254,10 @@ Two habits caused more of these than any single gap in knowledge. They are liste - **A head-of-cycle fetch with `Force: true` cleans a worktree a failed write left dirty.** Only for modified tracked files; new files and directories survive a hard reset. Fixed in #382 (9535d6a8). - **go-git cannot learn an empty repository's default branch.** GitHub advertises it over protocol - v2 (`unborn`), and go-git asks and then discards the reply. Status reports "unknown"; guessing - `main` was rejected + v2 (`unborn`). go-git v6.0.0-beta.1 still discards the reply inside `GetRemoteRefs`, so + `Remote.List` cannot return it, but its public `transport.Commander` can ask `ls-refs` again on + the same session. `unbornHead` in `internal/git/git_smart_fetch.go` does that, and status names the + branch for `Unborn`. Over v0 the server does not say, and nothing is guessed ([gittarget-parent-empty-repository.md](design/gittarget-parent-empty-repository.md)). - **A receive-pack probe proves push access.** On Gitea any valid user with read access gets 200, so it proves only a valid credential (#429). diff --git a/internal/git/ado_multiack_test.go b/internal/git/ado_multiack_test.go index ee65f729..8604d8c7 100644 --- a/internal/git/ado_multiack_test.go +++ b/internal/git/ado_multiack_test.go @@ -63,6 +63,11 @@ type adoSimulator struct { // around, when set, serves each request in place of the backend and decides whether and how // to call it: a test holds a request open, or lets the backend apply it and swallows the reply. around atomic.Pointer[func(w http.ResponseWriter, r *http.Request, backend http.Handler)] + + // protocolV2, when set, keeps the client's Git-Protocol header, so a client that asks for + // protocol v2 gets it, as from GitHub. Off, every request is forced to v0 (see + // startGitHTTPServer), which is what the ADO simulator and the request ledger measure. + protocolV2 atomic.Bool } // gitHTTPBackend locates canonical git's CGI server, skipping the test when it is unavailable. @@ -127,8 +132,10 @@ func startGitHTTPServer(tb testing.TB, projectRoot, repoDir string, enforceMulti } handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // Force protocol v0: see the doc comment above. - r.Header.Del("Git-Protocol") + // Force protocol v0 unless the test opted into v2: see the doc comment above. + if !sim.protocolV2.Load() { + r.Header.Del("Git-Protocol") + } // Count what crosses the wire, in both directions, for every request. The counting // wrappers are installed before anything else touches the request so a rejection is diff --git a/internal/git/branch_worker.go b/internal/git/branch_worker.go index 1b8d25b2..7a807744 100644 --- a/internal/git/branch_worker.go +++ b/internal/git/branch_worker.go @@ -3179,10 +3179,11 @@ func (w *BranchWorker) LastRemoteObservation() (RemoteObservation, bool) { // guess there is how a stale revision reaches status. // // An empty revision is a branch the remote does not carry, observed without a parent to name: an -// empty repository, which leaves the first commit unborn. +// empty repository, which leaves the first commit unborn. Only a push observes it this way, and a +// push advertisement carries no HEAD, so the branch HEAD names is the one the last discovery read. func (w *BranchWorker) recordRemoteObservation(revision string, by ObservationSource) { if revision == "" { - w.recordAbsentBranch("", "", by) + w.recordAbsentBranch(w.lastUnbornHead(), "", by) return } w.publishObservation(RemoteObservation{Commit: revision, At: time.Now(), By: by, Repo: w.repo}) @@ -3200,11 +3201,12 @@ func (w *BranchWorker) recordMissingParent(requested, branch string) { } // recordAbsentBranch records that the write branch is absent, and where the parent it would be -// created from is. An empty parent is an empty repository, which has none. +// created from is. No parent commit is an empty repository, which has no parent; parentBranch is +// then the branch its HEAD names, or empty when the server did not say. func (w *BranchWorker) recordAbsentBranch(parentBranch, parentCommit string, by ObservationSource) { state := ParentFound - if parentBranch == "" { - state, parentCommit = ParentUnborn, "" + if parentCommit == "" { + state = ParentUnborn } w.publishObservation(RemoteObservation{ At: time.Now(), By: by, Repo: w.repo, @@ -3212,6 +3214,16 @@ func (w *BranchWorker) recordAbsentBranch(parentBranch, parentCommit string, by }) } +// lastUnbornHead is the branch an empty repository's HEAD named at the last observation, when that +// observation was of an empty repository. +func (w *BranchWorker) lastUnbornHead() string { + last := w.lastObservation.Load() + if last == nil || last.ParentState != ParentUnborn { + return "" + } + return last.ParentBranch +} + func (w *BranchWorker) publishObservation(observed RemoteObservation) { // Stamped with the repository it is about, so that whoever publishes it can tell whether it // still describes the repository the GitTarget points at. See RemoteObservation.Repo. diff --git a/internal/git/git.go b/internal/git/git.go index 67443f72..bdd05169 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -20,7 +20,6 @@ import ( "github.com/go-git/go-git/v6/plumbing" gitclient "github.com/go-git/go-git/v6/plumbing/client" "github.com/go-git/go-git/v6/plumbing/format/index" - "github.com/go-git/go-git/v6/plumbing/transport" "github.com/go-logr/logr" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "sigs.k8s.io/controller-runtime/pkg/log" @@ -32,11 +31,6 @@ import ( "github.com/ConfigButler/gitops-reverser/internal/types" ) -var ( - ErrRemoteRefNotFound = errors.New("remote ref not found") - ErrRemoteRefNotFoundEmptyRepo = errors.New("remote ref not found (empty repo)") -) - // CheckRepo performs lightweight connectivity checks and gathers repository metadata. func CheckRepo(ctx context.Context, repoURL string, auth []gitclient.Option) (*RepoInfo, error) { logger := log.FromContext(ctx) @@ -44,27 +38,17 @@ func CheckRepo(ctx context.Context, repoURL string, auth []gitclient.Option) (*R // Use a ref listing for a lightweight connectivity check, bounded like every call to a Git // server (network_bound.go): a controller's reconcile has no deadline of its own. - remote := git.NewRemote(nil, &config.RemoteConfig{ - Name: "origin", - URLs: []string{repoURL}, - }) - - callCtx, cancel := boundGitCall(ctx) - defer cancel() - refs, err := remote.ListContext(callCtx, &git.ListOptions{ - ClientOptions: boundToContext(callCtx, auth), - }) - err = boundedCallError(callCtx, err) + refs, unborn, err := listRemoteRefs(ctx, repoURL, auth) if err != nil { - // Check if this is an empty repository error - if errors.Is(err, transport.ErrEmptyRemoteRepository) { - logger.Info("Repository is empty", "url", repoURL) - return &RepoInfo{ - DefaultBranch: nil, // No way to know what the default branch is until it's there - RemoteBranchCount: 0, - }, nil + return nil, err + } + if len(refs) == 0 { + logger.Info("Repository is empty", "url", repoURL, "unbornHead", unborn.Short()) + info := &RepoInfo{} + if unborn != "" { // only a server that reports it says what the default branch is + info.DefaultBranch = &BranchInfo{ShortName: unborn.Short(), Unborn: true} } - return nil, fmt.Errorf("failed to list remote references: %w", err) + return info, nil } repoInfo := &RepoInfo{} @@ -398,7 +382,7 @@ func syncToRemoteFrom( return nil, fmt.Errorf("unexpected fail to read HEAD: %w", err) } - availableBranch, err := SmartFetchFrom(ctx, repo, branch, parent, auth) + availableBranch, unborn, err := smartFetch(ctx, repo, branch, parent, auth) if err != nil { return nil, fmt.Errorf("failed to fetch: %w", err) } @@ -423,7 +407,9 @@ func syncToRemoteFrom( return nil, fmt.Errorf("failed to create root branch: %w", err) } - return createPullReport(branch.Short(), currentHash, plumbing.ZeroHash, false, true), nil + report := createPullReport(branch.Short(), currentHash, plumbing.ZeroHash, false, true) + report.ParentBranch = unborn.Short() + return report, nil } // makeHeadUnborn is called when there are no remote branches to base upon, all is cleared and new commits are created as orphaned branch. diff --git a/internal/git/git_operations_test.go b/internal/git/git_operations_test.go index 283cdcd5..b4b734d3 100644 --- a/internal/git/git_operations_test.go +++ b/internal/git/git_operations_test.go @@ -191,7 +191,8 @@ func TestCheckRepo_PublicConnectivityEmpty(t *testing.T) { skipIfPublicNetworkUnavailable(t, err) require.NoError(t, err) - assert.Empty(t, repoInfo.DefaultBranch) + // GitHub answers protocol v2 ls-refs "unborn": the empty repository's HEAD names main. + assert.Equal(t, &BranchInfo{ShortName: "main", Unborn: true}, repoInfo.DefaultBranch) assert.Equal(t, 0, repoInfo.RemoteBranchCount) localPath := filepath.Join(tempDir, "local") @@ -200,6 +201,7 @@ func TestCheckRepo_PublicConnectivityEmpty(t *testing.T) { require.NotNil(t, pullReport) assert.False(t, pullReport.ExistsOnRemote) assert.True(t, pullReport.HEAD.Unborn) + assert.Equal(t, "main", pullReport.ParentBranch, "the default branch the server reported") // Verify repository was cloned localRepo, err := git.PlainOpen(localPath) diff --git a/internal/git/git_smart_fetch.go b/internal/git/git_smart_fetch.go index 757c5ba1..38687962 100644 --- a/internal/git/git_smart_fetch.go +++ b/internal/git/git_smart_fetch.go @@ -6,11 +6,14 @@ import ( "context" "errors" "fmt" + "slices" "github.com/go-git/go-git/v6" "github.com/go-git/go-git/v6/config" "github.com/go-git/go-git/v6/plumbing" gitclient "github.com/go-git/go-git/v6/plumbing/client" + "github.com/go-git/go-git/v6/plumbing/protocol/capability" + "github.com/go-git/go-git/v6/plumbing/protocol/packp" "github.com/go-git/go-git/v6/plumbing/transport" "sigs.k8s.io/controller-runtime/pkg/log" ) @@ -102,19 +105,36 @@ func SmartFetchFrom( parent string, auth []gitclient.Option, ) (plumbing.ReferenceName, error) { + fetched, _, err := smartFetch(ctx, repo, target, parent, auth) + return fetched, err +} + +// smartFetch is SmartFetchFrom, and for an empty repository also the branch its HEAD names when the +// server reports it (see unbornHead). That second name is empty whenever the first is not. +func smartFetch( + ctx context.Context, + repo *git.Repository, + target plumbing.ReferenceName, + parent string, + auth []gitclient.Option, +) (plumbing.ReferenceName, plumbing.ReferenceName, error) { remoteName := "origin" remote, err := repo.Remote(remoteName) if err != nil { - return "", fmt.Errorf("failed to get remote %s: %w", remoteName, err) + return "", "", fmt.Errorf("failed to get remote %s: %w", remoteName, err) } // 1. Audit: List refs - refs, err := listRemoteRefs(ctx, remote, auth) + urls := remote.Config().URLs + if len(urls) == 0 { + return "", "", fmt.Errorf("remote %s has no URL configured", remoteName) + } + refs, unborn, err := listRemoteRefs(ctx, urls[0], auth) if err != nil { - return "", err + return "", "", err } if len(refs) == 0 && parent == "" { - return "", nil + return "", unborn, nil } // 2. Analyze: Find default branch and check target existence @@ -122,7 +142,7 @@ func SmartFetchFrom( defaultFull, defaultShort, err := fallbackBranch(refs, parent, headFull, headShort, targetExists) if err != nil { - return "", err + return "", "", err } // 3. Plan: Build RefSpecs based on analysis @@ -136,20 +156,20 @@ func SmartFetchFrom( case defaultFull != "": result = plumbing.ReferenceName(defaultFull) default: - return "", nil + return "", "", nil } // 4. Execute: Fetch if len(refSpecs) > 0 { if err := fetchBounded(ctx, repo, remoteName, refSpecs, auth); err != nil { - return "", err + return "", "", err } } // 5. Repair: Fix local symbolic HEAD. It mirrors the remote's HEAD, never a configured parent. repairRemoteSymbolicHead(repo, remoteName, headShort) - return result, nil + return result, "", nil } // fallbackBranch is the branch to fall back on, and to fetch as the safety net: the configured @@ -186,21 +206,91 @@ func configuredParent(refs []*plumbing.Reference, parent string) (string, string } // listRemoteRefs reads the remote's advertisement, bounded by gitCallTimeout: see network_bound.go. +// An empty repository is not an error: it has no refs, and the second result is the branch its HEAD +// names when the server says so (see unbornHead), or empty when it does not. func listRemoteRefs( - ctx context.Context, remote *git.Remote, auth []gitclient.Option, -) ([]*plumbing.Reference, error) { + ctx context.Context, remoteURL string, auth []gitclient.Option, +) ([]*plumbing.Reference, plumbing.ReferenceName, error) { ctx, cancel := boundGitCall(ctx) defer cancel() - refs, err := remote.ListContext(ctx, &git.ListOptions{ClientOptions: boundToContext(ctx, auth)}) + refs, unborn, err := advertise(ctx, remoteURL, boundToContext(ctx, auth)) + if err != nil { + return nil, "", fmt.Errorf("failed to list remote refs: %w", boundedCallError(ctx, err)) + } + return refs, unborn, nil +} + +// advertise is Remote.List with its default options (peeled refs left out), on a session of its +// own so that an empty repository can be asked one more question on it. Remote.List returns +// ErrEmptyRemoteRepository before it builds the transport.RemoteRefs whose Unborn field would +// carry the answer (go-git v6.0.0-beta.1). +func advertise( + ctx context.Context, rawURL string, auth []gitclient.Option, +) ([]*plumbing.Reference, plumbing.ReferenceName, error) { + endpoint, err := transport.ParseURL(rawURL) + if err != nil { + return nil, "", err + } + session, err := gitclient.New(auth...).Handshake(ctx, &transport.Request{ + URL: endpoint, + Command: transport.UploadPackService, + Protocol: config.DefaultProtocolVersion, + }) + if err != nil { + return nil, "", err + } + defer func() { _ = session.Close() }() + + advertised, err := session.GetRemoteRefs(ctx, nil) if errors.Is(err, transport.ErrEmptyRemoteRepository) { - return nil, nil // Valid state, not an error + return nil, unbornHead(ctx, session), nil } if err != nil { - return nil, fmt.Errorf("failed to list remote refs: %w", boundedCallError(ctx, err)) + return nil, "", err + } + var refs []*plumbing.Reference + for _, ref := range advertised.References { + if !ref.Name().IsPeeled() { + refs = append(refs, ref) + } + } + return refs, "", nil +} + +// unbornHead is the branch an empty repository's HEAD names, read from a protocol v2 ls-refs with +// the "unborn" argument (git 2.31 and later; GitHub answers it). It is empty when the session +// speaks v0 or v1, when the server does not offer "unborn", when the call fails, and when the +// repository is no longer empty by the time it answers: the name is evidence, never a guess. +// +// go-git sends the same request inside GetRemoteRefs and then discards the answer for an empty +// repository, so this asks again through its public Commander. Delete it once a go-git release +// reports the unborn HEAD of an empty repository. +func unbornHead(ctx context.Context, session transport.Session) plumbing.ReferenceName { + commander, ok := session.(transport.Commander) + if !ok || !slices.Contains(session.Capabilities().Get(capability.LsRefs), lsRefsUnborn) { + return "" + } + out := &packp.LsRefsOutput{} + if err := commander.Command(ctx, "ls-refs", &packp.LsRefsArgs{Symrefs: true, Unborn: true}, out); err != nil { + log.FromContext(ctx).V(1).Info("Could not read an empty repository's unborn HEAD", "error", err.Error()) + return "" } - return refs, nil + var head plumbing.ReferenceName + for _, ref := range out.References { + switch { + case ref.Type() == plumbing.HashReference: + return "" // it gained a ref since the advertisement: HEAD may well be born now + case ref.Name() == plumbing.HEAD && ref.Target().IsBranch(): + head = ref.Target() + } + } + return head } +// lsRefsUnborn is the ls-refs feature a server offers when it reports an unborn HEAD. Sending the +// argument to a server that does not offer it fails the command. +const lsRefsUnborn = "unborn" + // fetchBounded is SmartFetch's transfer, bounded by gitCallTimeout: see network_bound.go. func fetchBounded( ctx context.Context, repo *git.Repository, remoteName string, refSpecs []config.RefSpec, diff --git a/internal/git/refresh.go b/internal/git/refresh.go index 2de729b6..a0ea1f73 100644 --- a/internal/git/refresh.go +++ b/internal/git/refresh.go @@ -9,7 +9,6 @@ import ( "time" gogit "github.com/go-git/go-git/v6" - "github.com/go-git/go-git/v6/config" "github.com/go-git/go-git/v6/plumbing" gitclient "github.com/go-git/go-git/v6/plumbing/client" @@ -300,7 +299,8 @@ func (w *BranchWorker) recordAdvertisement(advertisement remoteAdvertisement, pa w.recordMissingParent("", advertisement.parentBranch) return true } - w.recordAbsentBranch("", "", ObservedByFetch) // an empty repository: no parent at all + // An empty repository: no parent at all, and the branch its HEAD names when it said. + w.recordAbsentBranch(advertisement.parentBranch, "", ObservedByFetch) return false } w.recordAbsentBranch(advertisement.parentBranch, advertisement.parent.String(), ObservedByFetch) @@ -436,7 +436,8 @@ type remoteAdvertisement struct { // branch, or the remote's default branch when none is configured. Zero when the remote does // not carry it, which includes an empty repository. parent plumbing.Hash - // parentBranch is that parent's short name; empty when the remote's HEAD names no branch. + // parentBranch is that parent's short name; empty when the remote's HEAD names no branch. In an + // empty repository it is the branch HEAD names, when the server reports it: see unbornHead. parentBranch string // empty records an advertisement with no hash refs at all: see advertisesNoRefs. empty bool @@ -445,7 +446,7 @@ type remoteAdvertisement struct { // advertiseRemoteBranch asks the remote where a branch and its parent are, and transfers nothing // else. An empty parentBranch means the branch the remote's HEAD names. // -// It is built on a storage-less remote, the way CheckRepo is, so it needs no clone on disk. Zero +// It reads the advertisement the way CheckRepo does, so it needs no clone on disk. Zero // means the advertisement did not carry the branch, which includes an empty repository — an // observation rather than an error: a branch does not exist without a commit. func advertiseRemoteBranch( @@ -455,14 +456,13 @@ func advertiseRemoteBranch( parentBranch string, auth []gitclient.Option, ) (remoteAdvertisement, error) { - remote := gogit.NewRemote(nil, &config.RemoteConfig{Name: "origin", URLs: []string{remoteURL}}) - refs, err := listRemoteRefs(ctx, remote, auth) + refs, unborn, err := listRemoteRefs(ctx, remoteURL, auth) if err != nil { return remoteAdvertisement{}, err } empty := advertisesNoRefs(refs) hashes := make(map[plumbing.ReferenceName]plumbing.Hash) - var defaultBranch plumbing.ReferenceName + defaultBranch := unborn for _, ref := range refs { switch { case ref.Type() == plumbing.HashReference: diff --git a/internal/git/remote_observation.go b/internal/git/remote_observation.go index d7acc988..1af54e9f 100644 --- a/internal/git/remote_observation.go +++ b/internal/git/remote_observation.go @@ -55,9 +55,10 @@ type RemoteObservation struct { // together with its absence. Empty while the branch exists. ParentState ParentState // ParentBranch and ParentCommit name that parent and where it is. ParentCommit is empty - // unless ParentState is ParentFound; ParentBranch is empty for ParentUnborn. For an omitted - // parent that is ParentMissing, ParentBranch is the branch the remote's HEAD names, or empty - // when it names none. + // unless ParentState is ParentFound. For ParentUnborn, ParentBranch is the branch the empty + // repository's HEAD names, which only a protocol v2 server reports (see unbornHead), and empty + // otherwise. For an omitted parent that is ParentMissing, ParentBranch is the branch the + // remote's HEAD names, or empty when it names none. ParentBranch string ParentCommit string // ParentRequested is spec.parentBranch as the observing operation read it: empty for the diff --git a/internal/git/types.go b/internal/git/types.go index f34b01e5..86a38a99 100644 --- a/internal/git/types.go +++ b/internal/git/types.go @@ -145,7 +145,9 @@ type PullReport struct { HEAD BranchInfo IncomingChanges bool // SHA changed, requiring resource-level reconcile // ParentBranch is the branch checked out instead, when the target branch is absent: the parent - // it would be created from. HEAD.Sha is then the parent's commit. Empty otherwise. + // it would be created from. HEAD.Sha is then the parent's commit. In an empty repository HEAD is + // unborn, HEAD.Sha is empty, and ParentBranch is the branch the remote's HEAD names when the + // server reports it (see unbornHead). Empty otherwise. ParentBranch string } diff --git a/internal/git/unborn_head_test.go b/internal/git/unborn_head_test.go new file mode 100644 index 00000000..ac9d62cc --- /dev/null +++ b/internal/git/unborn_head_test.go @@ -0,0 +1,162 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +// An empty repository's HEAD names a branch that has no commit yet. Protocol v2 servers report it +// (ls-refs "unborn"), v0 servers do not. These pin that the name reaches the observation when the +// server says it, that it is never guessed when the server does not, and that knowing it changes +// nothing about what the worker writes. + +import ( + "context" + "os/exec" + "path/filepath" + "testing" + + "github.com/go-git/go-git/v6/plumbing" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// emptyRepoServedOver serves an empty bare repository whose HEAD names trunk, over v2 or v0. +func emptyRepoServedOver(t *testing.T, v2 bool) *adoSimulator { + t.Helper() + projectRoot := t.TempDir() + repoDir := filepath.Join(projectRoot, "repo.git") + out, err := exec.Command("git", "init", "--bare", "-b", "trunk", repoDir).CombinedOutput() + require.NoError(t, err, "git init: %s", out) + gitIn(t, repoDir, "config", "http.receivepack", "true") + sim := startRealGitServer(t, projectRoot, repoDir) + sim.protocolV2.Store(v2) + return sim +} + +func TestListRemoteRefs_AnEmptyRepositoryNamesItsUnbornHeadOnlyOverV2(t *testing.T) { + for name, tc := range map[string]struct { + v2 bool + want plumbing.ReferenceName + wantPosts int64 + wantReport string + }{ + // GetRemoteRefs' ls-refs, whose answer go-git discards, and ours. + "v2 reports it": {v2: true, want: "refs/heads/trunk", wantPosts: 2, wantReport: "trunk"}, + // The v0 advertisement is the whole answer; nothing more is asked. + "v0 does not": {v2: false, want: "", wantPosts: 0, wantReport: ""}, + } { + t.Run(name, func(t *testing.T) { + sim := emptyRepoServedOver(t, tc.v2) + + refs, unborn, err := listRemoteRefs(context.Background(), sim.RepoURL, nil) + require.NoError(t, err) + assert.Empty(t, refs) + assert.Equal(t, tc.want, unborn) + assert.Equal(t, tc.wantPosts, sim.uploadPackPosts.Load()) + + info, err := CheckRepo(context.Background(), sim.RepoURL, nil) + require.NoError(t, err) + assert.Equal(t, 0, info.RemoteBranchCount) + if tc.want == "" { + assert.Nil(t, info.DefaultBranch, "no guess") + } else { + assert.Equal(t, &BranchInfo{ShortName: tc.want.Short(), Unborn: true}, info.DefaultBranch) + } + + report, err := PrepareBranch( + context.Background(), sim.RepoURL, filepath.Join(t.TempDir(), "clone"), "feature", nil) + require.NoError(t, err) + assert.True(t, report.HEAD.Unborn, "the first commit is still a root on the write branch") + assert.Equal(t, "feature", report.HEAD.ShortName) + assert.Equal(t, tc.wantReport, report.ParentBranch) + }) + } +} + +func TestListRemoteRefs_ARepositoryWithCommitsHasNoUnbornHeadOverV2(t *testing.T) { + projectRoot := t.TempDir() + repoDir := filepath.Join(projectRoot, "repo.git") + createBareRepo(t, repoDir) + main := simulateClientCommitOnDisk(t, repoDir, "main", "README.md", "x\n") + gitIn(t, repoDir, "tag", "-a", "-m", "annotated", "v1", main.String()) + sim := startRealGitServer(t, projectRoot, repoDir) + sim.protocolV2.Store(true) + + refs, unborn, err := listRemoteRefs(context.Background(), sim.RepoURL, nil) + require.NoError(t, err) + assert.Empty(t, unborn) + assert.Equal(t, int64(1), sim.uploadPackPosts.Load(), "one ls-refs, as Remote.List makes") + names := make([]string, 0, len(refs)) + for _, ref := range refs { + names = append(names, ref.Name().String()) + } + assert.ElementsMatch(t, []string{"HEAD", "refs/heads/main", "refs/tags/v1"}, names, "peeled refs left out") +} + +// The worker reports the name with ParentUnborn from every look at the remote: the fetch, a +// refresh, and a push, which cannot see HEAD and keeps what discovery read. +func TestBranchWorker_AnEmptyRepositoryReportsItsUnbornHeadOverV2(t *testing.T) { + f := newRealServerNewBranch(t, "unborn-head-v2", func(repoDir string) { + gitIn(t, repoDir, "symbolic-ref", "HEAD", "refs/heads/trunk") + }) + f.sim.protocolV2.Store(true) + + require.NoError(t, f.worker.ensureRepositoryInitialized(f.worker.ctx)) + o := f.observed() + assert.Equal(t, ParentUnborn, o.ParentState) + assert.Equal(t, "trunk", o.ParentBranch) + assert.Empty(t, o.ParentCommit) + + provider, err := f.worker.getGitProvider(f.worker.ctx) + require.NoError(t, err) + require.NoError(t, f.loop.refreshFromRemote(provider)) + assert.Equal(t, ParentUnborn, f.observed().ParentState, "the refresh agrees") + assert.Equal(t, "trunk", f.observed().ParentBranch) + + noOpWrite(f.loop) + o = f.observed() + assert.Equal(t, ObservedByPush, o.By) + assert.Equal(t, ParentUnborn, o.ParentState) + assert.Equal(t, "trunk", o.ParentBranch, "a push keeps the name discovery read") +} + +// Knowing the name changes no behavior: the first write is still a root commit on the write branch, +// and the branch HEAD names is not created. +func TestBranchWorker_AnEmptyRepositoryStillBootstrapsOverV2(t *testing.T) { + f := newRealServerNewBranch(t, "unborn-bootstrap-v2", func(repoDir string) { + gitIn(t, repoDir, "symbolic-ref", "HEAD", "refs/heads/trunk") + }) + f.sim.protocolV2.Store(true) + require.NoError(t, f.worker.ensureRepositoryInitialized(f.worker.ctx)) + require.Equal(t, "trunk", f.observed().ParentBranch) + + liveWrite(f.loop, "cm1") + tip := f.ref("feature") + require.False(t, tip.IsZero()) + assert.Empty(t, f.parentOf(tip), "a root commit") + assert.True(t, f.ref("trunk").IsZero(), "trunk is not created") +} + +// Over v0 the same repository is Unborn with no name: the server did not say, so nothing is guessed. +func TestBranchWorker_AnEmptyRepositoryNamesNoBranchOverV0(t *testing.T) { + f := newRealServerNewBranch(t, "unborn-head-v0", func(repoDir string) { + gitIn(t, repoDir, "symbolic-ref", "HEAD", "refs/heads/trunk") + }) + + require.NoError(t, f.worker.ensureRepositoryInitialized(f.worker.ctx)) + assert.Equal(t, ParentUnborn, f.observed().ParentState) + assert.Empty(t, f.observed().ParentBranch) +} + +// §4.3 test 1 over v2. A repository that is not empty but whose HEAD dangles is still refused, and +// over v2 the server says what HEAD names, so the status can name it. +func TestBranchWorker_ADanglingRemoteHeadIsNamedOverV2(t *testing.T) { + f := newRealServerNewBranch(t, "dangling-head-v2", func(repoDir string) { + simulateClientCommitOnDisk(t, repoDir, "main", "README.md", "main\n") + gitIn(t, repoDir, "symbolic-ref", "HEAD", "refs/heads/master") + }) + f.sim.protocolV2.Store(true) + + err := f.worker.ensureRepositoryInitialized(f.worker.ctx) + require.ErrorIs(t, err, ErrDefaultBranchUnresolved) + assert.Equal(t, ParentMissing, f.observed().ParentState) + assert.Equal(t, "master", f.observed().ParentBranch) +} From 2752dc719c26bb114ba68725279577ac73e264ba Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 10 Nov 2026 09:15:19 +0000 Subject: [PATCH 02/10] docs(git): decide an empty repository's first commit creates the requested branch With spec.parentBranch omitted, the first commit in an empty repository creates the write branch whatever branch the repository's HEAD names: the user asked for that branch, and the default branch can be changed afterwards. The deferred design page is closed and folded into gittarget-parent-branch.md and configuration.md; the upstream go-git patch shape moves to the TODO entry. An e2e spec on an empty Gitea repository (protocol v2) checks that status names main before the first write, and that the write then creates the requested branch as a root commit without creating main. Co-Authored-By: Claude Opus 5.5 --- docs/INDEX.md | 1 - docs/TODO.md | 11 +- docs/configuration.md | 4 +- docs/design/branch-worker-event-model.md | 4 +- docs/design/gittarget-parent-branch.md | 14 +- .../gittarget-parent-empty-repository.md | 213 ------------------ docs/design/gittarget-parent-hardening.md | 7 +- docs/misconceptions.md | 2 +- test/e2e/new_branch_parent_e2e_test.go | 68 ++++++ 9 files changed, 89 insertions(+), 235 deletions(-) delete mode 100644 docs/design/gittarget-parent-empty-repository.md diff --git a/docs/INDEX.md b/docs/INDEX.md index 40a8e5bf..9bfcbecc 100644 --- a/docs/INDEX.md +++ b/docs/INDEX.md @@ -142,7 +142,6 @@ so you never have to read a proposal to find out it already shipped. | [`gittarget-branch-worker-pending-writes.md`](design/gittarget-branch-worker-pending-writes.md) | **built in #413.** Ordered pending writes, one materializer and one retry schedule; a safe admission boundary for the watch; Git call deadlines; intake pause and resume; publication status. Holds the recovery contract for an outage, replay limits, and the acceptance scenarios. | | [`gittarget-parent-branch.md`](design/gittarget-parent-branch.md) | **merged in #407; the field moved to `GitBranch` in #429.** `GitBranch.spec.parentBranch`: a branch stays on standby until a write needs it, then is created from the parent's tip as checked at publication. The contract as built, the defect it started from, prior art and naming. Open: reporting the parent per branch in status. | | [`gittarget-parent-hardening.md`](design/gittarget-parent-hardening.md) | **done in #407.** Parent changes on a live worker, an empty repository gaining a ref, an unresolved default branch, recovery that cannot strand work, and the refused upload, without adding a pull to the fast path; plus the two recovery fixes from the review at `93120caf`. Records the merge boundary and the next separate PRs. | -| [`gittarget-parent-empty-repository.md`](design/gittarget-parent-empty-repository.md) | **evidence built, policy deferred.** An empty repository's default branch: the server reports it over protocol v2, and status now names it for `Unborn`, read through go-git's public `transport.Commander`. Which branch the first commit may create stays a separate policy decision. Assuming `main` was considered and rejected. | | [`gittarget-parent-observation.md`](design/gittarget-parent-observation.md) | **deferred.** Keep the parent visible after the write branch exists: availability first, ancestry only after measuring how often a shallow checkout can answer it. Informational; authorizes no branch mutation. | | [`gittarget-red-status-plan.md`](design/gittarget-red-status-plan.md) | **design, partly built.** The smaller alternative to Git write preflight: make a `GitTarget` turn red and explain why no commit was made, without moving Git checks into the admission webhook and without adding a second state source. Keeps the existing kstatus trio and domain conditions, ranks the most likely operator-facing failures, and has now shipped the first status-transition slice: refusal messages republish when details change, scoped refusals cannot be cleared by unrelated scope success, `BlockMessage()` carries stable issue kind/path/actor detail, and the unsupported-folder e2e covers Git-side break and recovery. | | [`open-asks-priority.md`](design/open-asks-priority.md) | **stale; [`TODO.md`](TODO.md) is the work queue.** Merged three overlapping backlogs (the downstream consumer asks, the API-surface block left unbuilt by the status and configuration-model review, and the config-surface proposal B1–B6) into one ordered queue under four stated tests, and says where we deliberately do **not** do what was asked. B2, B4, F6 and F10 have shipped, and F9 is probably moot since #333 deleted the `ClusterWatchRule` `scope` field. Still the record of one design call: **Option C sibling inference was deleted** rather than given an off-switch, and "what the deletion taught" records what building it found | diff --git a/docs/TODO.md b/docs/TODO.md index 23d6089c..98a50a92 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -135,16 +135,11 @@ Their argument for each is in [gitops-api-open-asks.md](future/gitops-api-open-a `GitTarget`, plus the durable queue, plus whether a replica warms its fact index before starting a watch it took over. Redis becomes required in this mode. [ha-gittarget-distribution-plan.md](future/ha-gittarget-distribution-plan.md). -- [ ] **Decide which branch a first commit may create in an empty repository.** Status now names an - empty repository's default branch when a protocol v2 server reports it (`unbornHead` in - `internal/git/git_smart_fetch.go`); behavior is unchanged. The policy choices are in - [gittarget-parent-empty-repository.md](design/gittarget-parent-empty-repository.md) §3. - `ParentUnborn` and `ErrDefaultBranchUnresolved` stay whatever is chosen: the first is a state the - repository is in, the second a repository with commits whose `HEAD` dangles. - [ ] **Drop `unbornHead` once go-git reports an empty repository's unborn HEAD.** Upstream main (`d6fa0275`) adds `LsRefsOutput.UnbornHead` but `GetRemoteRefs` still returns - `ErrEmptyRemoteRepository` before building `RemoteRefs`; an upstream PR returning the name there - (or on the error) would let `listRemoteRefs` go back to `Remote.List`. + `ErrEmptyRemoteRepository` before building `RemoteRefs`. An upstream PR returning the name there, + or on the error (an `EmptyRemoteRepositoryError{Unborn}` whose `Is(ErrEmptyRemoteRepository)` is + true, from both `GetRemoteRefs` v2 paths), would let `listRemoteRefs` go back to `Remote.List`. - [ ] **The branch claim is first-proven-wins** (`internal/git/branch_owner.go`). Anyone who can create a `GitProvider` and a `GitBranch` with a credential that passes the push-session proof can claim a branch no older `GitBranch` holds; on Gitea and Forgejo a read-only credential passes it diff --git a/docs/configuration.md b/docs/configuration.md index b97f0ad4..3ec86ef4 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -1551,7 +1551,9 @@ spec: until those are published the `GitBranch` reports `Ready=False` with reason `RecoveringParentBranch`. - An omitted parent is the remote's default branch, as last discovered. In an empty repository (no - refs at all, tags included) the first commit starts a branch with no history. A repository that + refs at all, tags included) the first commit starts the write branch with no history, whatever + branch the repository's `HEAD` names; change the repository's default branch afterwards if you + want another one. A repository that is not empty but whose `HEAD` names no branch it carries also reports `ParentBranchNotFound`, and writes nothing until `HEAD` is fixed or `parentBranch` is set; that never starts an orphan. - A detached `HEAD`, or a server that does not say what `HEAD` points at, resolves the way go-git diff --git a/docs/design/branch-worker-event-model.md b/docs/design/branch-worker-event-model.md index b3855a11..0da80efd 100644 --- a/docs/design/branch-worker-event-model.md +++ b/docs/design/branch-worker-event-model.md @@ -579,8 +579,8 @@ The detailed design records continue to own adjacent policy: the parent after its advertisement. - [Parent observation](gittarget-parent-observation.md): deferred status work that adds no remote connections or publication permission. Incomplete ancestry evidence remains `Unknown`. -- [Empty repositories](gittarget-parent-empty-repository.md): default-branch protocol discovery - and authorization to bootstrap it remain separate decisions. +- [Empty repositories](gittarget-parent-branch.md#implicit-when-omitted-strict-when-set): the + first commit creates the write branch; protocol v2 discovery only names the default branch. - [Target status](gittarget-red-status-plan.md): the writer proves refusals and controllers project them. A sibling scope's success cannot clear another scope's refusal. - [Configuration freshness](gittarget-configuration-freshness.md): watch-plan application and diff --git a/docs/design/gittarget-parent-branch.md b/docs/design/gittarget-parent-branch.md index 374ca647..ead8b63e 100644 --- a/docs/design/gittarget-parent-branch.md +++ b/docs/design/gittarget-parent-branch.md @@ -3,7 +3,6 @@ > **design**: built in #407, with #408's `spec.parentBranch` folded in and the hardening pass > ([`gittarget-parent-hardening.md`](gittarget-parent-hardening.md), done) applied. This page > records the contract as built and the reasoning behind it. Deferred: -> [`gittarget-parent-empty-repository.md`](gittarget-parent-empty-repository.md) and > [`gittarget-parent-observation.md`](gittarget-parent-observation.md). > Related: [`push-notification-and-reconcile-trigger.md`](push-notification-and-reconcile-trigger.md), > [`../definitions.md`](../definitions.md), and `architecture.md` (Ground rules; "A branch that does @@ -57,7 +56,7 @@ check is what protects the first write. | no | yes | compare against the parent; create the write branch from its tip, checked at publication | | no | no, **set** | refuse: `Ready=False`, `Stalled=True`, reason `ParentBranchNotFound`; nothing written, no orphan branch | | no | no, omitted, not empty (`HEAD` resolves to no branch, or only tags) | refuse the same way: `ParentBranchNotFound`, with a message to fix `HEAD` or set the field | -| no | no, omitted, empty repository (no refs at all) | unborn `HEAD`; the first commit is a root commit on the write branch | +| no | no, omitted, empty repository (no refs at all) | unborn `HEAD`; the first commit is a root commit on the write branch, whatever branch the repository's `HEAD` names | - **Recovery from a missing parent** is held open by the branch worker until the held work lands, whatever the parent's observed state. It latches when an attempt fails on the parent and clears @@ -148,9 +147,14 @@ The cost of the name: it can never hold a commit or a tag. ## Implicit when omitted, strict when set Omitted is lenient: the default branch, and a root commit when the repository is empty, because -that is the first-install case. Set is strict: it must exist, or the target refuses. Which branch -an empty repository's first commit may create is the open question in -[`gittarget-parent-empty-repository.md`](gittarget-parent-empty-repository.md). +that is the first-install case. Set is strict: it must exist, or the target refuses. + +In an empty repository the first commit creates the write branch, whatever branch the +repository's `HEAD` names (decided 2026-11-10). The user asked for that branch, and the default +branch can be changed afterwards. Status shows the name `HEAD` gives (`{state: Unborn, branch: +main}`) when the server speaks protocol v2, read by `unbornHead` in `internal/git/git_smart_fetch.go`; +it is never guessed. Considered and dropped: refusing until the default branch exists, and +bootstrapping the default branch alongside the write branch. **Deferred, additive when someone asks:** diff --git a/docs/design/gittarget-parent-empty-repository.md b/docs/design/gittarget-parent-empty-repository.md deleted file mode 100644 index b2271b50..00000000 --- a/docs/design/gittarget-parent-empty-repository.md +++ /dev/null @@ -1,213 +0,0 @@ -# GitTarget parent branch: an empty repository's default branch - -> **design**: evidence built (§4, 2026-11-10); the policy in §3 is deferred. Not part of #407. -> Dated 2026-10-01. -> -> Related: [`gittarget-parent-branch.md`](gittarget-parent-branch.md), the feature as built; -> [`gittarget-parent-hardening.md`](gittarget-parent-hardening.md), which keeps today's -> empty-repository behavior and defines "empty" (§4.2) and `Unborn` (§4.3). - -## 1. The problem - -Take an empty repository whose default branch is `main`, but which has no commits yet. Add a -`GitTarget` with `branch: reverser/edits` and no `parentBranch`. - -Today the first write creates **`reverser/edits` as the repository's first branch**: - -- The commit has no parent, and no `main` exists afterwards. -- What we pushed does not match the branch the repository says is its default. -- On GitHub, the first branch pushed to an empty repository probably becomes the default branch - (unverified, §5). If so, the operator's review branch silently becomes the repository's default - branch. Even if not, the eventual PR `reverser/edits` → `main` cannot be made: the two branches - have no common history. - -This is more than a status detail: for a review-branch workflow it is the wrong branch on the first -commit we make. It is also what the bootstrap contract says today ("only an omitted parent -bootstraps", and the first commit is a root on the write branch), so changing it is a policy and -compatibility decision, not a bug fix. - -`branch: main`, which is what the quickstart uses, is unaffected: the first commit creates `main`, -which is correct. - -## 2. What we know (experiment, 2026-10-01) - -**The server already tells us the default branch.** Over protocol v2, `ls-refs` with the `unborn` -argument reports the target of an unborn HEAD: - -```text -git -c protocol.version=2 ls-remote --symref https://github.com/ConfigButler/empty.git - (GIT_TRACE_PACKET) ls-remote< unborn HEAD symref-target:refs/heads/main -``` - -- Canonical git 2.39 does the same: a bare repository made with `git init --bare -b trunk` - answers `unborn HEAD symref-target:refs/heads/trunk`. -- Protocol v0 reports nothing. - -**go-git asks the question, then throws the answer away.** At v6.0.0-alpha.5, still at -v6.0.0-beta.1 and on upstream main at `12b788e4` (line numbers below are alpha.5's; §4 has the -workaround): - -- It speaks protocol v2 by default, and sends `unborn` when the server advertises - `ls-refs=unborn` (`internal/transport/v2.go`, `lsRefsSupportsUnborn`). -- But when there are no hash refs, `GetRemoteRefs` returns plain `ErrEmptyRemoteRepository` for an - upload-pack session, and the reply is discarded: - - `plumbing/transport/http/handshake.go` ~252; - - `plumbing/transport/pack_stream.go` ~119. -- `RemoteRefs.Unborn` exists, but it is unreachable for an empty repository. -- So our `CheckRepo` has to answer "no way to know" (`internal/git/git.go:59`). - -**Why our live test passes.** `TestCheckRepo_PublicConnectivityEmpty` -(`internal/git/git_operations_test.go:175`, against `ConfigButler/empty.git`) runs, and is not -disabled. It skips only without network. It asserts that `DefaultBranch` is **empty** and that -the local HEAD is unborn on the *write* branch (`cool-test`). It pins today's behavior rather than -the server's knowledge. - -**A small, backward-compatible go-git patch recovers the answer.** It was tested through a -temporary `-modfile` `replace`; nothing in the repository changed. The patch: - -- adds `transport.EmptyRemoteRepositoryError{Unborn plumbing.ReferenceName}`, whose `Is(ErrEmptyRemoteRepository)` - is true; -- returns it from both empty-advertisement paths above. - -Results: - -- `Remote.List` against `ConfigButler/empty.git` returns an error that still matches - `errors.Is(err, ErrEmptyRemoteRepository)`, and `errors.As` yields `Unborn = refs/heads/main`. -- The existing live test still passes unchanged. -- The diff is in §7. - -**Our hermetic harness cannot show this today.** `startGitHTTPServer` -(`internal/git/ado_multiack_test.go` ~125) deliberately strips `Git-Protocol` to force protocol -v0, so that it simulates Azure DevOps. Against it, the patched go-git returns the plain error with -no `Unborn`. - -**Not reported:** any server or path that negotiates v0/v1. Whether Azure DevOps supports v2 is -unknown; the harness assumes v0. SSH to GitHub needs `GIT_PROTOCOL` sent over the session; that is -not verified. - -## 3. Direction - -Two things, in this order: - -1. **Get the evidence.** Recover the server's unborn `HEAD` through go-git (§4). With it, an empty - repository can report what it actually is: `status.remote.parent = {state: Unborn, branch: X}` - when the server said X, and an empty `branch` when it did not. Behavior does not change. -2. **Then decide the policy**, separately, with an explicit compatibility note. The question: when - the server says the default branch is X and the write branch is not X, may the first commit - still create the write branch as the repository's first branch? - - | Option | Effect | - |---|---| - | keep today's bootstrap | the write branch becomes the first branch (as now) | - | refuse until X exists | `Ready=False`, `Stalled=True`, a reason naming X: "push a first commit to 'X', or set `spec.branch: X`"; recovery is the normal standby flow | - | opt-in bootstrap of X | one atomic push: an empty-tree commit on X and the content on the write branch; X must be in `allowedBranches`. This writes to a branch the parent contract says is only read, so it needs its own field | - - Whichever is chosen applies only when the server reported X. Without that evidence, today's - behavior stays. - -### Considered and rejected: assume `main` - -Assuming the default branch is `main` when the server does not say (never `master`) was considered -as a stage that needs no go-git change. It is rejected: - -- It turns working configurations into refusals. A repository whose default is `trunk`, with - `branch: trunk`, bootstraps correctly today and would be refused. -- The default is configurable in practice: `git init` takes `init.defaultBranch`, and GitHub - organizations set the default name for new repositories. A guess is wrong exactly for those - users. -- It would publish a guessed name inside a remote observation. Making the guess visible does not - make it evidence; `status.remote` reports what the remote said. - -Also not introduced here: reading `parentBranch == branch` on an empty repository as "start this -branch with no history". That would reverse a rule of the built contract as a side effect; if an -explicit bootstrap form is wanted, it gets its own design. - -## 4. Getting the go-git change - -**Built without a go-git change, on v6.0.0-beta.1** (the latest release as of 2026-11-10). beta.1 -still discards the answer: `GetRemoteRefs` returns `ErrEmptyRemoteRepository` for an empty -repository before it builds the `RemoteRefs` whose `Unborn` field would carry it, so `Remote.List` -cannot expose it. Upstream main (`d6fa0275`) adds `LsRefsOutput.UnbornHead` but returns the same -error. What beta.1 does make public is enough: - -- `listRemoteRefs` opens the upload-pack session itself (`gitclient.New(...).Handshake`) instead - of calling `Remote.List`, and returns the same refs (peeled ones left out, as `Remote.List` - does by default). -- On `ErrEmptyRemoteRepository`, `unbornHead` asks `ls-refs` again on that session through go-git's - public `transport.Commander`, with `packp.LsRefsArgs{Symrefs, Unborn}`, and only when the server - offers `ls-refs=unborn`. It is not a hand-rolled client: go-git encodes the request and decodes the - reply. A v0/v1 session is not asked, and a reply that carries any hash ref is discarded (the - repository stopped being empty in between). -- Cost: one more upload-pack POST, only for an empty repository over v2. The v0 harness and the - request ledger are unchanged. - -Delete `unbornHead` once a go-git release returns the name for an empty repository; the §7 patch -is still the shape to propose upstream. - -## 5. Steps - -1. **Verify the GitHub claim** in §1 on a *throwaway* repository, never on `ConfigButler/empty.git`: - does pushing a non-default branch first make it the default? Not done; it matters only to §3. -2. **Harness.** Done: `adoSimulator.protocolV2` keeps `Git-Protocol`, off by default, so the - ADO/v0 server and the ledger rows are untouched. -3. **Discovery.** Done: `CheckRepo` returns `DefaultBranch{ShortName: X, Unborn: true}`; - `PullReport.ParentBranch` and the refresh advertisement carry X, and the observation is - `{state: Unborn, branch: X}`. A push cannot see `HEAD`, so its observation keeps the name the - last discovery read. `recordAbsentBranch` now decides `Unborn` by the absent parent commit, not - the absent name. -4. **Tests.** Done (`internal/git/unborn_head_test.go`): a bare repository made by `init -b trunk` - reports `trunk` over v2 and no name over v0; behavior is unchanged in both. Over v2 a dangling - `HEAD` in a repository with commits is named too. -5. **Live test.** Done: `TestCheckRepo_PublicConnectivityEmpty` expects `main` / `Unborn`. -6. **Then the policy decision in §3**, as its own change with an UPGRADING note. Whatever is chosen, - `ParentUnborn` and `ErrDefaultBranchUnresolved` stay: the first is the state an empty repository - is in, the second a repository with commits whose `HEAD` names no branch it carries. - -## 6. Out of scope - -- The policy decision in §3, until the evidence exists. -- Repositories that are non-empty but whose default branch is unresolved. That is hardening §4.3. - -## 7. The go-git patch used in the experiment (against v6.0.0-alpha.5) - -```text ---- a/plumbing/transport/errors.go -+++ b/plumbing/transport/errors.go -@@ import ( - "errors" - -+ "github.com/go-git/go-git/v6/plumbing" -+ - internal "github.com/go-git/go-git/v6/internal/transport" - ) -@@ -+// EmptyRemoteRepositoryError is ErrEmptyRemoteRepository with what the server -+// said about its unborn HEAD (protocol v2 ls-refs "unborn"), when it said it. -+type EmptyRemoteRepositoryError struct { -+ // Unborn is the branch HEAD points at, e.g. refs/heads/main; empty when -+ // the server did not report it (v0/v1, or no ls-refs=unborn support). -+ Unborn plumbing.ReferenceName -+} -+ -+func (e *EmptyRemoteRepositoryError) Error() string { return ErrEmptyRemoteRepository.Error() } -+ -+func (e *EmptyRemoteRepositoryError) Is(target error) bool { return target == ErrEmptyRemoteRepository } -+ -+func NewEmptyRemoteRepositoryError(refs []*plumbing.Reference) error { -+ return &EmptyRemoteRepositoryError{Unborn: NewRemoteRefs(refs).Unborn} -+} ---- a/plumbing/transport/http/handshake.go (smartPackSession.GetRemoteRefs, v2 branch) -- return nil, transport.ErrEmptyRemoteRepository -+ return nil, transport.NewEmptyRemoteRepositoryError(refs) ---- a/plumbing/transport/pack_stream.go (StreamSession.GetRemoteRefs, v2 branch) -- return nil, ErrEmptyRemoteRepository -+ return nil, NewEmptyRemoteRepositoryError(refs) -``` - -To reproduce the experiment: - -1. Copy the module from `/go/pkg/mod/github.com/go-git/go-git/v6@v6.0.0-alpha.5` into a scratch - directory and apply the patch. -2. Copy `go.mod` and `go.sum` to `exp.mod` and `exp.sum`, and append - `replace github.com/go-git/go-git/v6 => /go-git`. -3. Run `go test -modfile=exp.mod ./internal/git -run `. diff --git a/docs/design/gittarget-parent-hardening.md b/docs/design/gittarget-parent-hardening.md index aa4827bb..97959522 100644 --- a/docs/design/gittarget-parent-hardening.md +++ b/docs/design/gittarget-parent-hardening.md @@ -22,8 +22,7 @@ > (empty-repository bootstrap rules, ancestry status) is not part of it. > > Related: [`gittarget-parent-branch.md`](gittarget-parent-branch.md), the feature as built; -> [`gittarget-parent-empty-repository.md`](gittarget-parent-empty-repository.md) and -> [`gittarget-parent-observation.md`](gittarget-parent-observation.md), deferred follow-ups. +> [`gittarget-parent-observation.md`](gittarget-parent-observation.md), a deferred follow-up. ## Merge review at `93120caf` @@ -93,8 +92,8 @@ resolving `Committed`. ### Out of scope for this pass -- [`gittarget-parent-empty-repository.md`](gittarget-parent-empty-repository.md): which branch an - empty repository's first commit may create. Keep today's bootstrap behavior here. +- Which branch an empty repository's first commit may create. Today's bootstrap was kept, and + later decided: see [`gittarget-parent-branch.md`](gittarget-parent-branch.md#implicit-when-omitted-strict-when-set). - [`gittarget-parent-observation.md`](gittarget-parent-observation.md): the parent's state after the write branch exists. diff --git a/docs/misconceptions.md b/docs/misconceptions.md index c9e2fd71..c30d5efb 100644 --- a/docs/misconceptions.md +++ b/docs/misconceptions.md @@ -258,7 +258,7 @@ Two habits caused more of these than any single gap in knowledge. They are liste `Remote.List` cannot return it, but its public `transport.Commander` can ask `ls-refs` again on the same session. `unbornHead` in `internal/git/git_smart_fetch.go` does that, and status names the branch for `Unborn`. Over v0 the server does not say, and nothing is guessed - ([gittarget-parent-empty-repository.md](design/gittarget-parent-empty-repository.md)). + ([gittarget-parent-branch.md](design/gittarget-parent-branch.md#implicit-when-omitted-strict-when-set)). - **A receive-pack probe proves push access.** On Gitea any valid user with read access gets 200, so it proves only a valid credential (#429). - **Gitea's `gpg.error.no_gpg_keys_found` means a GPG problem.** For SSH signatures it means no diff --git a/test/e2e/new_branch_parent_e2e_test.go b/test/e2e/new_branch_parent_e2e_test.go index 9e24fc2d..27fae4d4 100644 --- a/test/e2e/new_branch_parent_e2e_test.go +++ b/test/e2e/new_branch_parent_e2e_test.go @@ -46,6 +46,9 @@ var _ = Describe("New write branch starts from its parent", Label("manager"), fu Context("with spec.parentBranch naming a branch that does not exist yet", Ordered, func() { describeMissingParentRecoverySpec() }) + Context("in an empty repository with spec.parentBranch omitted", Ordered, func() { + describeEmptyRepositoryBootstrapSpec() + }) }) const parentBranchSelectorLabel = "e2e.configbutler.ai/parent-branch" @@ -196,6 +199,71 @@ func describeMissingParentRecoverySpec() { }) } +// describeEmptyRepositoryBootstrapSpec: an empty repository's HEAD names main (Gitea's default). +// Gitea speaks protocol v2, so the GitBranch reports that name before anything is written. The +// first write still creates the requested branch as a root commit, and main is not created: the +// branch asked for is the branch written. +func describeEmptyRepositoryBootstrapSpec() { + const ( + providerName = "empty-repo-provider" + destName = "empty-repo-dest" + ruleName = "empty-repo-rule" + gitPath = "apps/empty-repo" + ) + var ( + testNs string + repo *RepoArtifacts + writeBranch string + ) + + BeforeAll(func() { + testNs = testNamespaceFor("manager-empty-repo") + _, _ = kubectlRun("create", "namespace", testNs) + writeBranch = fmt.Sprintf("reverser-e2e-%d", GinkgoRandomSeed()) + repo = SetupRepo(resolveE2EContext(), testNs, fmt.Sprintf("e2e-empty-repo-%d", GinkgoRandomSeed())) + _, err := kubectlRunInNamespace(testNs, "apply", "-f", repo.SecretsYAML) + Expect(err).NotTo(HaveOccurred(), "failed to apply git secrets to test namespace") + applySOPSAgeKeyToNamespace(testNs) + applyParentSpecProvider(testNs, providerName, repo, writeBranch) + }) + + AfterAll(func() { + cleanupWatchRule(ruleName, testNs) + cleanupGitTarget(destName, testNs) + deleteGitProvider(testNs, providerName) + cleanupNamespace(testNs) + }) + + It("names the unborn default branch, then creates the requested branch as a root commit", func() { + createGitTargetWithParentBranch(destName, testNs, providerName, gitPath, writeBranch, "") + branchName := gitBranchName(providerName, writeBranch) + + By("the GitBranch reports the empty repository and the branch its HEAD names") + Eventually(func(g Gomega) { + g.Expect(gitBranchRemoteField(g, branchName, testNs, "parent.state")).To(Equal("Unborn")) + g.Expect(gitBranchRemoteField(g, branchName, testNs, "parent.branch")).To(Equal("main"), + "Gitea answers protocol v2 ls-refs unborn") + }, 3*time.Minute, 3*time.Second).Should(Succeed()) + + By("the first write creates the requested branch, not main") + applyParentSpecWatchRule(testNs, ruleName, destName) + applyParentConfigMap(testNs, "first-write") + editPath := path.Join(gitPath, testNs, "configmaps", "first-write.yaml") + Eventually(func(g Gomega) { + tip := remoteBranchHeadOf(g, repo.CheckoutDir, writeBranch) + g.Expect(tip).NotTo(BeEmpty(), "the write creates the requested branch") + _, fetchErr := gitRun(repo.CheckoutDir, "fetch", "origin", writeBranch) + g.Expect(fetchErr).NotTo(HaveOccurred()) + roots, revErr := gitRun(repo.CheckoutDir, "rev-list", "--max-parents=0", tip) + g.Expect(revErr).NotTo(HaveOccurred()) + g.Expect(strings.Fields(roots)).To(HaveLen(1), "one history, starting on the requested branch") + _, showErr := gitRun(repo.CheckoutDir, "cat-file", "-e", tip+":"+editPath) + g.Expect(showErr).NotTo(HaveOccurred(), "the write is on the branch") + }, 5*time.Minute, 5*time.Second).Should(Succeed()) + Expect(remoteBranchHeadOf(Default, repo.CheckoutDir, "main")).To(BeEmpty(), "main is not created") + }) +} + func describeNewWriteBranchSpec(parentBranch, parent, slug string) { const ( providerName = "parent-provider" From 935b37d14b4e9762e2ea51d83c3ca0b7a258206b Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 10 Nov 2026 09:20:52 +0000 Subject: [PATCH 03/10] docs(design): analyze an empty repository's first commit per server, and reopen it Gitea and Forgejo make the first branch pushed into an empty repository the default (services/repository/push.go), so today's bootstrap promotes a review branch to default and later no-parent GitBranches build on its unreviewed commits; a plain git server leaves HEAD dangling and refuses them instead. The page compares keeping the bootstrap (A), refusing until the default branch exists (B), seeding it in one atomic two-ref push (C), and seeding only when the provider may write it (D), and recommends B first, then D. The earlier "decided" wording is withdrawn. Co-Authored-By: Claude Opus 5.5 --- docs/INDEX.md | 1 + docs/TODO.md | 5 + docs/design/branch-worker-event-model.md | 4 +- docs/design/gittarget-parent-branch.md | 11 +- .../gittarget-parent-empty-repository.md | 161 ++++++++++++++++++ docs/design/gittarget-parent-hardening.md | 7 +- docs/misconceptions.md | 2 +- 7 files changed, 180 insertions(+), 11 deletions(-) create mode 100644 docs/design/gittarget-parent-empty-repository.md diff --git a/docs/INDEX.md b/docs/INDEX.md index 9bfcbecc..c00f07d4 100644 --- a/docs/INDEX.md +++ b/docs/INDEX.md @@ -142,6 +142,7 @@ so you never have to read a proposal to find out it already shipped. | [`gittarget-branch-worker-pending-writes.md`](design/gittarget-branch-worker-pending-writes.md) | **built in #413.** Ordered pending writes, one materializer and one retry schedule; a safe admission boundary for the watch; Git call deadlines; intake pause and resume; publication status. Holds the recovery contract for an outage, replay limits, and the acceptance scenarios. | | [`gittarget-parent-branch.md`](design/gittarget-parent-branch.md) | **merged in #407; the field moved to `GitBranch` in #429.** `GitBranch.spec.parentBranch`: a branch stays on standby until a write needs it, then is created from the parent's tip as checked at publication. The contract as built, the defect it started from, prior art and naming. Open: reporting the parent per branch in status. | | [`gittarget-parent-hardening.md`](design/gittarget-parent-hardening.md) | **done in #407.** Parent changes on a live worker, an empty repository gaining a ref, an unresolved default branch, recovery that cannot strand work, and the refused upload, without adding a pull to the fast path; plus the two recovery fixes from the review at `93120caf`. Records the merge boundary and the next separate PRs. | +| [`gittarget-parent-empty-repository.md`](design/gittarget-parent-empty-repository.md) | **open.** Which branch an empty repository's first commit may create when it is not the default: today's bootstrap, and on Gitea its promotion of a review branch to default; refuse, seed, or seed when allowed. Recommends refusing first. Status already names the default branch (#480). | | [`gittarget-parent-observation.md`](design/gittarget-parent-observation.md) | **deferred.** Keep the parent visible after the write branch exists: availability first, ancestry only after measuring how often a shallow checkout can answer it. Informational; authorizes no branch mutation. | | [`gittarget-red-status-plan.md`](design/gittarget-red-status-plan.md) | **design, partly built.** The smaller alternative to Git write preflight: make a `GitTarget` turn red and explain why no commit was made, without moving Git checks into the admission webhook and without adding a second state source. Keeps the existing kstatus trio and domain conditions, ranks the most likely operator-facing failures, and has now shipped the first status-transition slice: refusal messages republish when details change, scoped refusals cannot be cleared by unrelated scope success, `BlockMessage()` carries stable issue kind/path/actor detail, and the unsupported-folder e2e covers Git-side break and recovery. | | [`open-asks-priority.md`](design/open-asks-priority.md) | **stale; [`TODO.md`](TODO.md) is the work queue.** Merged three overlapping backlogs (the downstream consumer asks, the API-surface block left unbuilt by the status and configuration-model review, and the config-surface proposal B1–B6) into one ordered queue under four stated tests, and says where we deliberately do **not** do what was asked. B2, B4, F6 and F10 have shipped, and F9 is probably moot since #333 deleted the `ClusterWatchRule` `scope` field. Still the record of one design call: **Option C sibling inference was deleted** rather than given an off-switch, and "what the deletion taught" records what building it found | diff --git a/docs/TODO.md b/docs/TODO.md index 98a50a92..d676f90a 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -135,6 +135,11 @@ Their argument for each is in [gitops-api-open-asks.md](future/gitops-api-open-a `GitTarget`, plus the durable queue, plus whether a replica warms its fact index before starting a watch it took over. Redis becomes required in this mode. [ha-gittarget-distribution-plan.md](future/ha-gittarget-distribution-plan.md). +- [ ] **Decide which branch an empty repository's first commit may create.** Today a review branch + becomes the repository's first branch; on Gitea it then becomes the default, and later branches + build on its unreviewed commits. Recommended: refuse until the default branch exists (B), then + seed it when the provider may write it (C/D). + [gittarget-parent-empty-repository.md](design/gittarget-parent-empty-repository.md). - [ ] **Drop `unbornHead` once go-git reports an empty repository's unborn HEAD.** Upstream main (`d6fa0275`) adds `LsRefsOutput.UnbornHead` but `GetRemoteRefs` still returns `ErrEmptyRemoteRepository` before building `RemoteRefs`. An upstream PR returning the name there, diff --git a/docs/design/branch-worker-event-model.md b/docs/design/branch-worker-event-model.md index 0da80efd..0a0b4925 100644 --- a/docs/design/branch-worker-event-model.md +++ b/docs/design/branch-worker-event-model.md @@ -579,8 +579,8 @@ The detailed design records continue to own adjacent policy: the parent after its advertisement. - [Parent observation](gittarget-parent-observation.md): deferred status work that adds no remote connections or publication permission. Incomplete ancestry evidence remains `Unknown`. -- [Empty repositories](gittarget-parent-branch.md#implicit-when-omitted-strict-when-set): the - first commit creates the write branch; protocol v2 discovery only names the default branch. +- [Empty repositories](gittarget-parent-empty-repository.md): protocol v2 discovery names the + default branch (#480); which branch the first commit may create is open. - [Target status](gittarget-red-status-plan.md): the writer proves refusals and controllers project them. A sibling scope's success cannot clear another scope's refusal. - [Configuration freshness](gittarget-configuration-freshness.md): watch-plan application and diff --git a/docs/design/gittarget-parent-branch.md b/docs/design/gittarget-parent-branch.md index ead8b63e..0e935b70 100644 --- a/docs/design/gittarget-parent-branch.md +++ b/docs/design/gittarget-parent-branch.md @@ -3,6 +3,7 @@ > **design**: built in #407, with #408's `spec.parentBranch` folded in and the hardening pass > ([`gittarget-parent-hardening.md`](gittarget-parent-hardening.md), done) applied. This page > records the contract as built and the reasoning behind it. Deferred: +> [`gittarget-parent-empty-repository.md`](gittarget-parent-empty-repository.md) and > [`gittarget-parent-observation.md`](gittarget-parent-observation.md). > Related: [`push-notification-and-reconcile-trigger.md`](push-notification-and-reconcile-trigger.md), > [`../definitions.md`](../definitions.md), and `architecture.md` (Ground rules; "A branch that does @@ -150,11 +151,11 @@ Omitted is lenient: the default branch, and a root commit when the repository is that is the first-install case. Set is strict: it must exist, or the target refuses. In an empty repository the first commit creates the write branch, whatever branch the -repository's `HEAD` names (decided 2026-11-10). The user asked for that branch, and the default -branch can be changed afterwards. Status shows the name `HEAD` gives (`{state: Unborn, branch: -main}`) when the server speaks protocol v2, read by `unbornHead` in `internal/git/git_smart_fetch.go`; -it is never guessed. Considered and dropped: refusing until the default branch exists, and -bootstrapping the default branch alongside the write branch. +repository's `HEAD` names. Status shows that name (`{state: Unborn, branch: main}`) when the server +speaks protocol v2, read by `unbornHead` in `internal/git/git_smart_fetch.go`; it is never guessed. +Whether a write branch that is not the default should be created there is open: on Gitea it +becomes the repository's default branch, and later branches build on it. See +[`gittarget-parent-empty-repository.md`](gittarget-parent-empty-repository.md). **Deferred, additive when someone asks:** diff --git a/docs/design/gittarget-parent-empty-repository.md b/docs/design/gittarget-parent-empty-repository.md new file mode 100644 index 00000000..2a429c8e --- /dev/null +++ b/docs/design/gittarget-parent-empty-repository.md @@ -0,0 +1,161 @@ +# An empty repository's first commit: behavior analysis + +> **design**: open. Analysis dated 2026-11-10, written after #480 made the evidence available. +> Nothing here is built beyond #480's status field. +> +> Related: [`gittarget-parent-branch.md`](gittarget-parent-branch.md), the parent contract as +> built; [`gittarget-parent-hardening.md`](gittarget-parent-hardening.md), which defines "empty" +> (§4.2) and `Unborn` (§4.3). + +## 1. The situation + +An empty repository has no refs, but its `HEAD` already names a branch: `main` on Gitea, Forgejo +and GitHub by default, or whatever `init.defaultBranch` or an organization setting chose. That +branch is unborn: it has a name and no commit. + +A `GitBranch` writes `spec.branch`, created from `spec.parentBranch`, or from the remote's default +branch when that is omitted. In an empty repository the omitted parent is the unborn branch. Two +cases: + +- **`spec.branch` is the unborn branch** (the quickstart: `branch: main`). The first commit creates + the default branch. Every option below agrees on this, and nothing about it is weird. +- **`spec.branch` is another branch** (a review-branch workflow: `branch: reverser/edits`). This + page is about this case. + +What we know about the unborn branch, since #480: + +| Server | Reports the unborn name | How we know | +|---|---|---| +| GitHub (HTTPS) | yes, `main` | live test `TestCheckRepo_PublicConnectivityEmpty` | +| Gitea, Forgejo (HTTPS) | yes | both forward `Git-Protocol` to git (`routers/web/repo/githttp.go`); the #480 e2e spec checks Gitea | +| canonical `git http-backend` | yes, from git 2.31 | `unborn_head_test.go` | +| any server over protocol v0/v1 | no | `unborn_head_test.go` | +| Azure DevOps | unknown | its v2 support is unverified; the test harness assumes v0 | +| any server over SSH | unknown | needs `GIT_PROTOCOL` passed through the SSH session; unverified | + +## 2. What happens today + +The first write creates `reverser/edits` as a root commit. `main` does not exist afterwards. What +happens next depends on the server, because the server, not us, decides what `HEAD` becomes. + +**Gitea and Forgejo adopt the first pushed branch as the default.** On the first push into an empty +repository, `services/repository/push.go` sets the repository's default branch to the pushed ref +and moves `HEAD` to it (verified in both sources). So after our first push: + +1. The repository's default branch is `reverser/edits`, a review branch nobody reviewed. +2. A second `GitBranch` on the repository, `branch: reverser/other` with no parent, resolves its + parent to the default branch, so its first commit **builds on the first GitBranch's + unreviewed commits**. Its PR into anything carries them. This is the weird case. +3. A PR `reverser/edits` → `main` cannot be opened: `main` does not exist. A human who creates + `main` from the forge's UI (from `reverser/edits`) gets a `main` that already holds the edits, + so the first review never happens. A human who pushes their own first commit to `main` creates + an unrelated history, and the PR needs `--allow-unrelated-histories`. + +**GitHub, GitLab and Azure DevOps reportedly do the same** (the first pushed branch becomes the +default). Unverified; verifying needs a throwaway repository on each. + +**A plain git server keeps `HEAD` at `main`, which now dangles.** The repository is no longer empty +and its default branch resolves to nothing, so the second `GitBranch` with no parent is refused +with `ParentBranchNotFound`: "the remote's default branch (HEAD -> 'main') does not exist". Over v2 +the message names `main` (#480); over v0 it says there is no default branch. Nothing is written +until a human fixes `HEAD` or sets `parentBranch`. Safe, but the human is now repairing a +repository we shaped. + +**Two GitBranches whose first writes race** both create root commits, on two unrelated branches. +On Gitea whichever lands first becomes the default. + +So today's behavior is not wrong for one `GitBranch`, but it makes the repository's history +start on a review branch, and on the common forges it promotes that branch to default. + +## 3. Options + +Each option changes only the case in §1 where `spec.branch` is not the unborn branch and +`spec.parentBranch` is omitted. A set `parentBranch` already behaves like B: it must exist, +or the target refuses and recovers when it appears. + +### A. Keep today's bootstrap + +The write branch is the repository's first branch, with the consequences in §2. No evidence +needed, so it behaves the same on every server. + +### B. Refuse until the default branch exists + +`Ready=False`, `Stalled=True`, reason `ParentBranchNotFound`, with a message that names the branch +the server reported: "the repository is empty and its default branch is 'main': push a first commit +to 'main', or set spec.branch: main". Writes are held, as for a missing configured parent. When +`main` appears, the existing parent probe finds it, and the held writes land on `main`'s tip +(`TestBranchWorker_AnEmptyRepositoryThatGainsABranchIsNotOrphaned` shows that replay works). + +- Same rule as a set `parentBranch`: the omitted parent must exist too, unless it is the write + branch itself. Easy to explain. +- Nothing is written that was not asked for, and the history starts on `main`. +- A human has to create the first commit. On GitHub and Gitea that is the "initialize with a + README" checkbox, so many empty repositories never reach this case. +- **Needs the evidence.** Over v0 we cannot tell whether `spec.branch` is the unborn branch, so we + cannot refuse only the review-branch case. Over v0 B has to fall back to A, or refuse every + empty repository, including the quickstart's `branch: main`. + +### C. The operator seeds the default branch + +One atomic push creates two refs: `main` with an empty root commit ("Initialize repository", +authored as the provider's commit identity), and `reverser/edits` with the content on top of it. + +- `main` stays the default. On Gitea the default branch follows the first ref in the push + (`pushUpdates` handles the updates in order), so `main` goes first. Unverified on the other + forges. +- The first PR, `reverser/edits` → `main`, shows the whole initial snapshot as one reviewable + change. This is the outcome a review workflow wants. +- A second `GitBranch` builds on `main`, the empty commit, not on the first one's edits. +- Racing first writes converge: one seed wins, the other push is refused (it expected `main` to be + absent) and replays onto the winner's `main`. +- **It writes a branch nobody named in `spec.branch`.** That needs authorization: + `main` must be in the provider's `allowedGitBranches`. If it is not, C cannot apply. +- **Needs the evidence**, like B: without the name, there is nothing to seed. +- Cost: today's push sends one ref update (`git_atomic_push.go`); this needs two, with + compare-and-swap on both. The protocol and the `atomic` capability we already request + support that. + +### D. Seed when allowed, otherwise refuse + +C when `allowedGitBranches` covers the unborn branch, B otherwise, and A when the server did not +report the name. No new field: the existing allowlist decides whether we may write `main`. + +- Best outcome where it applies, honest refusal where it does not. +- Implicit: whether we create a commit on `main` depends on an allowlist entry, which a reader of + the `GitBranch` cannot see. Status and an event must say "seeded 'main'" when it happens. + +## 4. How the options compare + +| | A (today) | B (refuse) | C (seed) | D (seed or refuse) | +|---|---|---|---|---| +| History starts on | the review branch | `main`, by a human | `main`, by the operator | `main` | +| Gitea default branch afterwards | the review branch | `main` | `main` | `main` | +| A second no-parent `GitBranch` builds on | the first one's edits (Gitea), or is refused (plain git) | `main` | `main` | `main` | +| First PR into `main` possible | no | yes | yes | yes | +| Writes a branch not in `spec.branch` | no | no | yes | only if allowed | +| Human action needed | later, to repair | before the first write | none | none, or before the first write | +| Without the unborn name (v0) | same | falls back to A, or refuses all | falls back to A | falls back to A | + +## 5. Recommendation + +**D, built as B first.** + +1. **B now**: refuse the review-branch case when the server reported the name, keep A when it did + not. It is small (`fallbackBranch` already refuses a missing configured parent, and the unborn + name reaches it through `smartFetch`). It uses the existing parent probe and replay. It ends + the Gitea stacking. +2. **C behind the allowlist, if the human step in B turns out to be friction**: the two-ref atomic + push is the real work. Both steps keep the quickstart (`branch: main`) unchanged. + +A loses because, on the forges people use, it promotes an unreviewed branch to default and stacks +later branches on it. C alone loses because it writes `main` without permission. B alone is +acceptable but leaves the human step. + +## 6. To verify before building + +- What GitHub, GitLab and Azure DevOps do on the first push into an empty repository: does the + pushed branch become the default? Use a throwaway repository, never `ConfigButler/empty.git`. +- Whether Azure DevOps speaks protocol v2, and whether our SSH transport carries it. Where it does + not, B and C fall back to A. +- For C: that Gitea takes the first ref of an atomic two-ref push as the default branch, and what + the other forges do with it. diff --git a/docs/design/gittarget-parent-hardening.md b/docs/design/gittarget-parent-hardening.md index 97959522..aa4827bb 100644 --- a/docs/design/gittarget-parent-hardening.md +++ b/docs/design/gittarget-parent-hardening.md @@ -22,7 +22,8 @@ > (empty-repository bootstrap rules, ancestry status) is not part of it. > > Related: [`gittarget-parent-branch.md`](gittarget-parent-branch.md), the feature as built; -> [`gittarget-parent-observation.md`](gittarget-parent-observation.md), a deferred follow-up. +> [`gittarget-parent-empty-repository.md`](gittarget-parent-empty-repository.md) and +> [`gittarget-parent-observation.md`](gittarget-parent-observation.md), deferred follow-ups. ## Merge review at `93120caf` @@ -92,8 +93,8 @@ resolving `Committed`. ### Out of scope for this pass -- Which branch an empty repository's first commit may create. Today's bootstrap was kept, and - later decided: see [`gittarget-parent-branch.md`](gittarget-parent-branch.md#implicit-when-omitted-strict-when-set). +- [`gittarget-parent-empty-repository.md`](gittarget-parent-empty-repository.md): which branch an + empty repository's first commit may create. Keep today's bootstrap behavior here. - [`gittarget-parent-observation.md`](gittarget-parent-observation.md): the parent's state after the write branch exists. diff --git a/docs/misconceptions.md b/docs/misconceptions.md index c30d5efb..c9e2fd71 100644 --- a/docs/misconceptions.md +++ b/docs/misconceptions.md @@ -258,7 +258,7 @@ Two habits caused more of these than any single gap in knowledge. They are liste `Remote.List` cannot return it, but its public `transport.Commander` can ask `ls-refs` again on the same session. `unbornHead` in `internal/git/git_smart_fetch.go` does that, and status names the branch for `Unborn`. Over v0 the server does not say, and nothing is guessed - ([gittarget-parent-branch.md](design/gittarget-parent-branch.md#implicit-when-omitted-strict-when-set)). + ([gittarget-parent-empty-repository.md](design/gittarget-parent-empty-repository.md)). - **A receive-pack probe proves push access.** On Gitea any valid user with read access gets 200, so it proves only a valid credential (#429). - **Gitea's `gpg.error.no_gpg_keys_found` means a GPG problem.** For SSH signatures it means no From 13cc0b8aa248d68c5b801f6f2be9d16a01534124 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 10 Nov 2026 09:40:21 +0000 Subject: [PATCH 04/10] feat(git): create an empty repository's default branch before the write branch In an empty repository with spec.parentBranch omitted, a write branch that is not the default branch the server names now starts from it: the default branch is created first with one empty commit ("chore: initialize repository"), authored and signed like the provider's other commits, pushed alone with compare-and-swap on its absence. It needs the existing permission: the GitProvider's spec.allowedGitBranches must allow the GitBranch to write it. There is no setting. Without that permission nothing is written: the GitBranch reports Ready=False, Stalled=True, reason DefaultBranchNotAllowed, its GitTargets GitBranchNotReady, and the writes are held under parent recovery, whose probe finds the parent once the branch exists or is allowed. A default branch created by someone else during our push is started from. A server that does not name its default branch (v0) keeps the old bootstrap. Gitea and Forgejo made the first pushed branch the default, so a review branch used to become the default and later branches built on its unreviewed commits. BREAKING CHANGE: a GitBranch whose spec.branch is not the empty repository's default branch now creates that default branch first, and stalls with DefaultBranchNotAllowed when spec.allowedGitBranches does not allow it. See docs/UPGRADING.md. Co-Authored-By: Claude Opus 5.5 --- api/v1alpha3/gitbranch_types.go | 16 +- .../bases/configbutler.ai_gitbranches.yaml | 13 +- docs/INDEX.md | 2 +- docs/TODO.md | 5 - docs/UPGRADING.md | 25 ++ docs/configuration.md | 22 +- docs/design/branch-worker-event-model.md | 4 +- docs/design/gittarget-parent-branch.md | 18 +- .../gittarget-parent-empty-repository.md | 225 ++++++------------ docs/design/gittarget-parent-hardening.md | 6 +- internal/authz/kubeprovider_admission.go | 20 ++ internal/controller/gitbranch_controller.go | 45 +++- .../controller/gitbranch_controller_test.go | 44 ++++ internal/git/branch_worker.go | 30 ++- internal/git/parent_recovery.go | 19 +- internal/git/unborn_default_branch.go | 180 ++++++++++++++ internal/git/unborn_head_test.go | 144 +++++++++-- test/e2e/new_branch_parent_e2e_test.go | 73 ++++-- 18 files changed, 663 insertions(+), 228 deletions(-) create mode 100644 internal/git/unborn_default_branch.go diff --git a/api/v1alpha3/gitbranch_types.go b/api/v1alpha3/gitbranch_types.go index 6cfc6339..5e8a6a84 100644 --- a/api/v1alpha3/gitbranch_types.go +++ b/api/v1alpha3/gitbranch_types.go @@ -48,7 +48,14 @@ type GitBranchSpec struct { // ParentBranch is the branch spec.branch is created from, and compared // against while spec.branch does not exist on the remote. Omitted, it is the - // remote's default branch, or no history at all in an empty repository. + // remote's default branch. + // + // In an empty repository with this omitted, a spec.branch that is not the + // default branch starts from it: the default branch is created first, with + // one empty commit, when spec.allowedGitBranches lets this GitBranch write + // it. Otherwise this GitBranch reports Stalled=True, reason + // DefaultBranchNotAllowed, and writes nothing. A server that does not name + // its default branch (Git protocol v0) gets spec.branch as its first branch. // // When set, it must exist on the remote for spec.branch to be created; // otherwise this GitBranch reports Stalled=True, reason ParentBranchNotFound, @@ -171,7 +178,8 @@ const ( // branch it carries. GitBranchParentMissing GitBranchParentState = "Missing" // GitBranchParentUnborn means the repository is empty, with no refs at all (tags included), and - // no parent is configured, so the first commit starts the branch with no history. + // no parent is configured. The first commit starts the branch with no history when it is the + // default branch; otherwise the default branch is created first, with one empty commit. GitBranchParentUnborn GitBranchParentState = "Unborn" ) @@ -180,8 +188,8 @@ const ( type GitBranchParentStatus struct { // State is what the branch would be created from: "Found" (the parent branch's // tip), "Missing" (the parent branch is not on the remote, so nothing is - // written) or "Unborn" (an empty repository and no spec.parentBranch, so the - // first commit has no history). + // written) or "Unborn" (an empty repository and no spec.parentBranch: the + // default branch is created first, unless it is spec.branch). // +required State GitBranchParentState `json:"state"` diff --git a/config/crd/bases/configbutler.ai_gitbranches.yaml b/config/crd/bases/configbutler.ai_gitbranches.yaml index 4a2e558b..569e949e 100644 --- a/config/crd/bases/configbutler.ai_gitbranches.yaml +++ b/config/crd/bases/configbutler.ai_gitbranches.yaml @@ -252,7 +252,14 @@ spec: description: |- ParentBranch is the branch spec.branch is created from, and compared against while spec.branch does not exist on the remote. Omitted, it is the - remote's default branch, or no history at all in an empty repository. + remote's default branch. + + In an empty repository with this omitted, a spec.branch that is not the + default branch starts from it: the default branch is created first, with + one empty commit, when spec.allowedGitBranches lets this GitBranch write + it. Otherwise this GitBranch reports Stalled=True, reason + DefaultBranchNotAllowed, and writes nothing. A server that does not name + its default branch (Git protocol v0) gets spec.branch as its first branch. When set, it must exist on the remote for spec.branch to be created; otherwise this GitBranch reports Stalled=True, reason ParentBranchNotFound, @@ -404,8 +411,8 @@ spec: description: |- State is what the branch would be created from: "Found" (the parent branch's tip), "Missing" (the parent branch is not on the remote, so nothing is - written) or "Unborn" (an empty repository and no spec.parentBranch, so the - first commit has no history). + written) or "Unborn" (an empty repository and no spec.parentBranch: the + default branch is created first, unless it is spec.branch). enum: - Found - Missing diff --git a/docs/INDEX.md b/docs/INDEX.md index c00f07d4..e8a0c49d 100644 --- a/docs/INDEX.md +++ b/docs/INDEX.md @@ -142,7 +142,7 @@ so you never have to read a proposal to find out it already shipped. | [`gittarget-branch-worker-pending-writes.md`](design/gittarget-branch-worker-pending-writes.md) | **built in #413.** Ordered pending writes, one materializer and one retry schedule; a safe admission boundary for the watch; Git call deadlines; intake pause and resume; publication status. Holds the recovery contract for an outage, replay limits, and the acceptance scenarios. | | [`gittarget-parent-branch.md`](design/gittarget-parent-branch.md) | **merged in #407; the field moved to `GitBranch` in #429.** `GitBranch.spec.parentBranch`: a branch stays on standby until a write needs it, then is created from the parent's tip as checked at publication. The contract as built, the defect it started from, prior art and naming. Open: reporting the parent per branch in status. | | [`gittarget-parent-hardening.md`](design/gittarget-parent-hardening.md) | **done in #407.** Parent changes on a live worker, an empty repository gaining a ref, an unresolved default branch, recovery that cannot strand work, and the refused upload, without adding a pull to the fast path; plus the two recovery fixes from the review at `93120caf`. Records the merge boundary and the next separate PRs. | -| [`gittarget-parent-empty-repository.md`](design/gittarget-parent-empty-repository.md) | **open.** Which branch an empty repository's first commit may create when it is not the default: today's bootstrap, and on Gitea its promotion of a review branch to default; refuse, seed, or seed when allowed. Recommends refusing first. Status already names the default branch (#480). | +| [`gittarget-parent-empty-repository.md`](design/gittarget-parent-empty-repository.md) | **built in #480.** An empty repository's first commit: status names the default branch (protocol v2), and a write branch that is not the default starts from it, created first with one empty commit when `allowedGitBranches` allows it, else `DefaultBranchNotAllowed`. Why, and what was deliberately not verified. | | [`gittarget-parent-observation.md`](design/gittarget-parent-observation.md) | **deferred.** Keep the parent visible after the write branch exists: availability first, ancestry only after measuring how often a shallow checkout can answer it. Informational; authorizes no branch mutation. | | [`gittarget-red-status-plan.md`](design/gittarget-red-status-plan.md) | **design, partly built.** The smaller alternative to Git write preflight: make a `GitTarget` turn red and explain why no commit was made, without moving Git checks into the admission webhook and without adding a second state source. Keeps the existing kstatus trio and domain conditions, ranks the most likely operator-facing failures, and has now shipped the first status-transition slice: refusal messages republish when details change, scoped refusals cannot be cleared by unrelated scope success, `BlockMessage()` carries stable issue kind/path/actor detail, and the unsupported-folder e2e covers Git-side break and recovery. | | [`open-asks-priority.md`](design/open-asks-priority.md) | **stale; [`TODO.md`](TODO.md) is the work queue.** Merged three overlapping backlogs (the downstream consumer asks, the API-surface block left unbuilt by the status and configuration-model review, and the config-surface proposal B1–B6) into one ordered queue under four stated tests, and says where we deliberately do **not** do what was asked. B2, B4, F6 and F10 have shipped, and F9 is probably moot since #333 deleted the `ClusterWatchRule` `scope` field. Still the record of one design call: **Option C sibling inference was deleted** rather than given an off-switch, and "what the deletion taught" records what building it found | diff --git a/docs/TODO.md b/docs/TODO.md index d676f90a..98a50a92 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -135,11 +135,6 @@ Their argument for each is in [gitops-api-open-asks.md](future/gitops-api-open-a `GitTarget`, plus the durable queue, plus whether a replica warms its fact index before starting a watch it took over. Redis becomes required in this mode. [ha-gittarget-distribution-plan.md](future/ha-gittarget-distribution-plan.md). -- [ ] **Decide which branch an empty repository's first commit may create.** Today a review branch - becomes the repository's first branch; on Gitea it then becomes the default, and later branches - build on its unreviewed commits. Recommended: refuse until the default branch exists (B), then - seed it when the provider may write it (C/D). - [gittarget-parent-empty-repository.md](design/gittarget-parent-empty-repository.md). - [ ] **Drop `unbornHead` once go-git reports an empty repository's unborn HEAD.** Upstream main (`d6fa0275`) adds `LsRefsOutput.UnbornHead` but `GetRemoteRefs` still returns `ErrEmptyRemoteRepository` before building `RemoteRefs`. An upstream PR returning the name there, diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index 76c07e57..168cb98d 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -7,6 +7,31 @@ guidance that the changelog's breaking-change entries link to. We are pre-1.0, so breaking changes bump the **minor** version (release-please is configured with `bump-minor-pre-major`) rather than the major. Read the relevant entry before upgrading across it. +## An empty repository gets its default branch first + +In an empty repository with `spec.parentBranch` omitted, a `GitBranch` whose `spec.branch` is not +the repository's default branch no longer creates `spec.branch` as the repository's first branch. +The default branch the server names (`main` on GitHub, Gitea and Forgejo) is created first, with one +empty commit authored and signed like the provider's other commits, and `spec.branch` starts from +it. On Gitea and Forgejo the first branch pushed became the default branch, so a review branch used +to become the default, and later branches built on it. + +That is a write to the default branch, so the provider's `spec.allowedGitBranches` must allow it for +the `GitBranch`'s namespace. If it does not, the `GitBranch` reports `Ready=False`, `Stalled=True`, +reason `DefaultBranchNotAllowed`, its `GitTarget`s report `GitBranchNotReady`, and nothing is +written until you allow the branch or push a first commit to it: + +```yaml +allowedGitBranches: + - consumerNamespaces: {from: Same} + branchPatterns: ["main", "reverser/edits"] +``` + +A `GitBranch` whose `spec.branch` is the default branch, as in the quickstart, is unaffected. So is +a server that does not name its default branch (Git protocol v0), where `spec.branch` is still +created as the first branch. `status.remote.parent.branch` now names the default branch of an empty +repository when the server reports it. + ## A `CommitRequest` reports each phase as the worker reaches it A request's progress and outcome are written as soon as its branch worker reports them, instead of diff --git a/docs/configuration.md b/docs/configuration.md index 3ec86ef4..2f35f9c6 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -1243,6 +1243,7 @@ at, and `Ready`; | `PushSessionFailed` | The credential cannot open a push session; a holder keeps the branch meanwhile | | `BranchConflict` | Another `GitBranch` holds this repository branch (`Stalled=True`) | | `ParentBranchNotFound` | Neither the branch nor its parent is on the remote (`Stalled=True`) | +| `DefaultBranchNotAllowed` | The repository is empty; the branch would start from the default branch, which `spec.allowedGitBranches` does not let this `GitBranch` create (`Stalled=True`) | | `RecoveringParentBranch` | The parent is back and the worker is publishing what it held | ### Deleting a `GitBranch` @@ -1551,9 +1552,17 @@ spec: until those are published the `GitBranch` reports `Ready=False` with reason `RecoveringParentBranch`. - An omitted parent is the remote's default branch, as last discovered. In an empty repository (no - refs at all, tags included) the first commit starts the write branch with no history, whatever - branch the repository's `HEAD` names; change the repository's default branch afterwards if you - want another one. A repository that + refs at all, tags included) whose default branch is `spec.branch`, the first commit starts it with + no history. When `spec.branch` is another branch, the default branch is created first, with one + empty commit authored and signed like every other commit, and `spec.branch` starts from it: the + history starts on the default branch, which stays the default, and the first pull request into it + carries everything written. That is a write to the default branch, so the provider's + `allowedGitBranches` must allow it for this namespace (`branchPatterns: ["main", "reverser/edits"]`). + Without that the `GitBranch` reports `DefaultBranchNotAllowed`, its `GitTarget`s report + `GitBranchNotReady`, and nothing is written until the branch is allowed or someone pushes a first + commit to it. This needs the server to name its default branch, which Git protocol v2 does + (GitHub, Gitea, Forgejo); over v0 `spec.branch` is created as the repository's first branch. + A repository that is not empty but whose `HEAD` names no branch it carries also reports `ParentBranchNotFound`, and writes nothing until `HEAD` is fixed or `parentBranch` is set; that never starts an orphan. - A detached `HEAD`, or a server that does not say what `HEAD` points at, resolves the way go-git @@ -1716,9 +1725,10 @@ status: created from, observed together with the branch's absence. `state` is `Found` (the first commit builds on `commit`, the parent's tip), `Missing` (the parent is not on the remote either; the `GitBranch` reports `ParentBranchNotFound` and nothing is written) or `Unborn` (an empty repository, with - no refs at all, and no parent configured; the first commit starts the branch with no history). For - `Unborn`, `branch` is the branch the empty repository's `HEAD` names, which only a server speaking - Git protocol v2 reports (GitHub does); it is empty when the server does not say. See + no refs at all, and no parent configured). For `Unborn`, `branch` is the branch the empty + repository's `HEAD` names, which only a server speaking Git protocol v2 reports, and empty when the + server does not say. Unless it is `spec.branch`, it is created first and `spec.branch` starts from + it. See [starting the write branch from another branch](#starting-the-write-branch-from-another-branch-specparentbranch). `kubectl get gitbranch` shows `commit` in the `Commit` column, and `-o wide` shows diff --git a/docs/design/branch-worker-event-model.md b/docs/design/branch-worker-event-model.md index 0a0b4925..336b9385 100644 --- a/docs/design/branch-worker-event-model.md +++ b/docs/design/branch-worker-event-model.md @@ -579,8 +579,8 @@ The detailed design records continue to own adjacent policy: the parent after its advertisement. - [Parent observation](gittarget-parent-observation.md): deferred status work that adds no remote connections or publication permission. Incomplete ancestry evidence remains `Unknown`. -- [Empty repositories](gittarget-parent-empty-repository.md): protocol v2 discovery names the - default branch (#480); which branch the first commit may create is open. +- [Empty repositories](gittarget-parent-empty-repository.md): the default branch is created first + when the write branch is another branch and the provider allows it (#480). - [Target status](gittarget-red-status-plan.md): the writer proves refusals and controllers project them. A sibling scope's success cannot clear another scope's refusal. - [Configuration freshness](gittarget-configuration-freshness.md): watch-plan application and diff --git a/docs/design/gittarget-parent-branch.md b/docs/design/gittarget-parent-branch.md index 0e935b70..84a8c7e0 100644 --- a/docs/design/gittarget-parent-branch.md +++ b/docs/design/gittarget-parent-branch.md @@ -2,9 +2,9 @@ > **design**: built in #407, with #408's `spec.parentBranch` folded in and the hardening pass > ([`gittarget-parent-hardening.md`](gittarget-parent-hardening.md), done) applied. This page -> records the contract as built and the reasoning behind it. Deferred: -> [`gittarget-parent-empty-repository.md`](gittarget-parent-empty-repository.md) and -> [`gittarget-parent-observation.md`](gittarget-parent-observation.md). +> records the contract as built and the reasoning behind it. Empty repositories: +> [`gittarget-parent-empty-repository.md`](gittarget-parent-empty-repository.md) (built in #480). +> Deferred: [`gittarget-parent-observation.md`](gittarget-parent-observation.md). > Related: [`push-notification-and-reconcile-trigger.md`](push-notification-and-reconcile-trigger.md), > [`../definitions.md`](../definitions.md), and `architecture.md` (Ground rules; "A branch that does > not exist yet"). @@ -57,7 +57,7 @@ check is what protects the first write. | no | yes | compare against the parent; create the write branch from its tip, checked at publication | | no | no, **set** | refuse: `Ready=False`, `Stalled=True`, reason `ParentBranchNotFound`; nothing written, no orphan branch | | no | no, omitted, not empty (`HEAD` resolves to no branch, or only tags) | refuse the same way: `ParentBranchNotFound`, with a message to fix `HEAD` or set the field | -| no | no, omitted, empty repository (no refs at all) | unborn `HEAD`; the first commit is a root commit on the write branch, whatever branch the repository's `HEAD` names | +| no | no, omitted, empty repository (no refs at all) | the write branch is the default branch, or the server does not name it: the first commit is a root commit on the write branch. Otherwise the default branch is created first with one empty commit if allowed, else `DefaultBranchNotAllowed` | - **Recovery from a missing parent** is held open by the branch worker until the held work lands, whatever the parent's observed state. It latches when an attempt fails on the parent and clears @@ -150,12 +150,10 @@ The cost of the name: it can never hold a commit or a tag. Omitted is lenient: the default branch, and a root commit when the repository is empty, because that is the first-install case. Set is strict: it must exist, or the target refuses. -In an empty repository the first commit creates the write branch, whatever branch the -repository's `HEAD` names. Status shows that name (`{state: Unborn, branch: main}`) when the server -speaks protocol v2, read by `unbornHead` in `internal/git/git_smart_fetch.go`; it is never guessed. -Whether a write branch that is not the default should be created there is open: on Gitea it -becomes the repository's default branch, and later branches build on it. See -[`gittarget-parent-empty-repository.md`](gittarget-parent-empty-repository.md). +In an empty repository whose server names its default branch, a write branch that is not the +default starts from it: the default branch is created first with one empty commit, when the +provider allows this `GitBranch` to write it, and the `GitBranch` reports `DefaultBranchNotAllowed` +otherwise. See [`gittarget-parent-empty-repository.md`](gittarget-parent-empty-repository.md). **Deferred, additive when someone asks:** diff --git a/docs/design/gittarget-parent-empty-repository.md b/docs/design/gittarget-parent-empty-repository.md index 2a429c8e..7a675164 100644 --- a/docs/design/gittarget-parent-empty-repository.md +++ b/docs/design/gittarget-parent-empty-repository.md @@ -1,7 +1,7 @@ -# An empty repository's first commit: behavior analysis +# An empty repository's first commit -> **design**: open. Analysis dated 2026-11-10, written after #480 made the evidence available. -> Nothing here is built beyond #480's status field. +> **design**: built in #480 (2026-11-10). Records the behavior, why it was chosen over the +> alternatives, and what was deliberately not verified. > > Related: [`gittarget-parent-branch.md`](gittarget-parent-branch.md), the parent contract as > built; [`gittarget-parent-hardening.md`](gittarget-parent-hardening.md), which defines "empty" @@ -11,151 +11,78 @@ An empty repository has no refs, but its `HEAD` already names a branch: `main` on Gitea, Forgejo and GitHub by default, or whatever `init.defaultBranch` or an organization setting chose. That -branch is unborn: it has a name and no commit. +branch is unborn: it has a name and no commit. Protocol v2 servers report the name (`ls-refs` +`unborn`); #480 reads it through go-git's public `transport.Commander` (`unbornHead` in +`internal/git/git_smart_fetch.go`), and status shows it as `status.remote.parent: {state: Unborn, +branch: main}`. A `GitBranch` writes `spec.branch`, created from `spec.parentBranch`, or from the remote's default -branch when that is omitted. In an empty repository the omitted parent is the unborn branch. Two -cases: - -- **`spec.branch` is the unborn branch** (the quickstart: `branch: main`). The first commit creates - the default branch. Every option below agrees on this, and nothing about it is weird. -- **`spec.branch` is another branch** (a review-branch workflow: `branch: reverser/edits`). This - page is about this case. - -What we know about the unborn branch, since #480: - -| Server | Reports the unborn name | How we know | -|---|---|---| -| GitHub (HTTPS) | yes, `main` | live test `TestCheckRepo_PublicConnectivityEmpty` | -| Gitea, Forgejo (HTTPS) | yes | both forward `Git-Protocol` to git (`routers/web/repo/githttp.go`); the #480 e2e spec checks Gitea | -| canonical `git http-backend` | yes, from git 2.31 | `unborn_head_test.go` | -| any server over protocol v0/v1 | no | `unborn_head_test.go` | -| Azure DevOps | unknown | its v2 support is unverified; the test harness assumes v0 | -| any server over SSH | unknown | needs `GIT_PROTOCOL` passed through the SSH session; unverified | - -## 2. What happens today - -The first write creates `reverser/edits` as a root commit. `main` does not exist afterwards. What -happens next depends on the server, because the server, not us, decides what `HEAD` becomes. - -**Gitea and Forgejo adopt the first pushed branch as the default.** On the first push into an empty -repository, `services/repository/push.go` sets the repository's default branch to the pushed ref -and moves `HEAD` to it (verified in both sources). So after our first push: - -1. The repository's default branch is `reverser/edits`, a review branch nobody reviewed. -2. A second `GitBranch` on the repository, `branch: reverser/other` with no parent, resolves its - parent to the default branch, so its first commit **builds on the first GitBranch's - unreviewed commits**. Its PR into anything carries them. This is the weird case. -3. A PR `reverser/edits` → `main` cannot be opened: `main` does not exist. A human who creates - `main` from the forge's UI (from `reverser/edits`) gets a `main` that already holds the edits, - so the first review never happens. A human who pushes their own first commit to `main` creates - an unrelated history, and the PR needs `--allow-unrelated-histories`. - -**GitHub, GitLab and Azure DevOps reportedly do the same** (the first pushed branch becomes the -default). Unverified; verifying needs a throwaway repository on each. - -**A plain git server keeps `HEAD` at `main`, which now dangles.** The repository is no longer empty -and its default branch resolves to nothing, so the second `GitBranch` with no parent is refused -with `ParentBranchNotFound`: "the remote's default branch (HEAD -> 'main') does not exist". Over v2 -the message names `main` (#480); over v0 it says there is no default branch. Nothing is written -until a human fixes `HEAD` or sets `parentBranch`. Safe, but the human is now repairing a -repository we shaped. - -**Two GitBranches whose first writes race** both create root commits, on two unrelated branches. -On Gitea whichever lands first becomes the default. - -So today's behavior is not wrong for one `GitBranch`, but it makes the repository's history -start on a review branch, and on the common forges it promotes that branch to default. - -## 3. Options - -Each option changes only the case in §1 where `spec.branch` is not the unborn branch and -`spec.parentBranch` is omitted. A set `parentBranch` already behaves like B: it must exist, -or the target refuses and recovers when it appears. - -### A. Keep today's bootstrap - -The write branch is the repository's first branch, with the consequences in §2. No evidence -needed, so it behaves the same on every server. - -### B. Refuse until the default branch exists - -`Ready=False`, `Stalled=True`, reason `ParentBranchNotFound`, with a message that names the branch -the server reported: "the repository is empty and its default branch is 'main': push a first commit -to 'main', or set spec.branch: main". Writes are held, as for a missing configured parent. When -`main` appears, the existing parent probe finds it, and the held writes land on `main`'s tip -(`TestBranchWorker_AnEmptyRepositoryThatGainsABranchIsNotOrphaned` shows that replay works). - -- Same rule as a set `parentBranch`: the omitted parent must exist too, unless it is the write - branch itself. Easy to explain. -- Nothing is written that was not asked for, and the history starts on `main`. -- A human has to create the first commit. On GitHub and Gitea that is the "initialize with a - README" checkbox, so many empty repositories never reach this case. -- **Needs the evidence.** Over v0 we cannot tell whether `spec.branch` is the unborn branch, so we - cannot refuse only the review-branch case. Over v0 B has to fall back to A, or refuse every - empty repository, including the quickstart's `branch: main`. - -### C. The operator seeds the default branch - -One atomic push creates two refs: `main` with an empty root commit ("Initialize repository", -authored as the provider's commit identity), and `reverser/edits` with the content on top of it. - -- `main` stays the default. On Gitea the default branch follows the first ref in the push - (`pushUpdates` handles the updates in order), so `main` goes first. Unverified on the other - forges. -- The first PR, `reverser/edits` → `main`, shows the whole initial snapshot as one reviewable - change. This is the outcome a review workflow wants. -- A second `GitBranch` builds on `main`, the empty commit, not on the first one's edits. -- Racing first writes converge: one seed wins, the other push is refused (it expected `main` to be - absent) and replays onto the winner's `main`. -- **It writes a branch nobody named in `spec.branch`.** That needs authorization: - `main` must be in the provider's `allowedGitBranches`. If it is not, C cannot apply. -- **Needs the evidence**, like B: without the name, there is nothing to seed. -- Cost: today's push sends one ref update (`git_atomic_push.go`); this needs two, with - compare-and-swap on both. The protocol and the `atomic` capability we already request - support that. - -### D. Seed when allowed, otherwise refuse - -C when `allowedGitBranches` covers the unborn branch, B otherwise, and A when the server did not -report the name. No new field: the existing allowlist decides whether we may write `main`. - -- Best outcome where it applies, honest refusal where it does not. -- Implicit: whether we create a commit on `main` depends on an allowlist entry, which a reader of - the `GitBranch` cannot see. Status and an event must say "seeded 'main'" when it happens. - -## 4. How the options compare - -| | A (today) | B (refuse) | C (seed) | D (seed or refuse) | -|---|---|---|---|---| -| History starts on | the review branch | `main`, by a human | `main`, by the operator | `main` | -| Gitea default branch afterwards | the review branch | `main` | `main` | `main` | -| A second no-parent `GitBranch` builds on | the first one's edits (Gitea), or is refused (plain git) | `main` | `main` | `main` | -| First PR into `main` possible | no | yes | yes | yes | -| Writes a branch not in `spec.branch` | no | no | yes | only if allowed | -| Human action needed | later, to repair | before the first write | none | none, or before the first write | -| Without the unborn name (v0) | same | falls back to A, or refuses all | falls back to A | falls back to A | - -## 5. Recommendation - -**D, built as B first.** - -1. **B now**: refuse the review-branch case when the server reported the name, keep A when it did - not. It is small (`fallbackBranch` already refuses a missing configured parent, and the unborn - name reaches it through `smartFetch`). It uses the existing parent probe and replay. It ends - the Gitea stacking. -2. **C behind the allowlist, if the human step in B turns out to be friction**: the two-ref atomic - push is the real work. Both steps keep the quickstart (`branch: main`) unchanged. - -A loses because, on the forges people use, it promotes an unreviewed branch to default and stacks -later branches on it. C alone loses because it writes `main` without permission. B alone is -acceptable but leaves the human step. - -## 6. To verify before building - -- What GitHub, GitLab and Azure DevOps do on the first push into an empty repository: does the - pushed branch become the default? Use a throwaway repository, never `ConfigButler/empty.git`. -- Whether Azure DevOps speaks protocol v2, and whether our SSH transport carries it. Where it does - not, B and C fall back to A. -- For C: that Gitea takes the first ref of an atomic two-ref push as the default branch, and what - the other forges do with it. +branch when that is omitted. In an empty repository the omitted parent is the unborn branch. + +## 2. Behavior + +With `spec.parentBranch` omitted, in an empty repository: + +| The server names its default branch | `spec.branch` is it | The provider allows this `GitBranch` to write it | What happens | +|---|---|---|---| +| yes | yes | (it is `spec.branch`, so yes) | the first commit starts `spec.branch` with no history | +| yes | no | yes | the default branch is created first with one empty commit; `spec.branch` starts from it | +| yes | no | no | nothing is written: `GitBranch` `Ready=False`, `Stalled=True`, reason `DefaultBranchNotAllowed`; its `GitTarget`s `GitBranchNotReady` | +| no (protocol v0/v1) | unknown | | the first commit starts `spec.branch` as the repository's first branch | + +- **The empty commit** is authored and signed like every other commit the provider makes + (`spec.commit`), with the message `chore: initialize repository`. It is pushed alone, with + compare-and-swap on the branch being absent, before anything is written to `spec.branch`. After + it, the repository is no longer empty, and `spec.branch` is an ordinary absent branch whose parent + is `Found`. +- **Permission is the existing allowlist.** Creating the default branch is a write to a branch + other than `spec.branch`, so the provider's `spec.allowedGitBranches` must allow this `GitBranch`'s + namespace to write it, as it would for a `GitBranch` on that branch. There is no setting to opt in + or out. +- **Without permission, writes are held**, the same way as for a missing configured parent: the + worker latches parent recovery and probes on its retry schedule. A probe counts the parent as + found once the default branch exists (somebody pushed a first commit) or once the provider allows + creating it; either way the held writes land on it. +- **A default branch that appears during our push** (another writer, or another `GitBranch`) makes + the server refuse our empty commit. The sync that follows finds theirs and `spec.branch` starts + from it; the refusal is not reported as an error. +- `ParentUnborn` stays the state of an empty repository, and `ErrDefaultBranchUnresolved` stays the + refusal for a repository with commits whose `HEAD` names a branch it does not carry. Over v2 that + refusal now names the branch. + +Code: `internal/git/unborn_default_branch.go` (create or refuse), +`internal/git/parent_recovery.go` (`probeParent`), `internal/controller/gitbranch_controller.go` +(`gateUnbornDefault`), `internal/authz` (`GitBranchMayWrite`, shared by both). + +## 3. Why + +Starting `spec.branch` as the repository's first branch looked harmless, but on the common forges +it is not. Gitea and Forgejo make the first branch pushed into an empty repository the default +(`services/repository/push.go`, verified in both sources). So a review branch such as +`reverser/edits` became the repository's default branch without anyone reviewing it, and every +later `GitBranch` without a parent built on its unreviewed commits. A PR into `main` was impossible, +because `main` did not exist. On a plain git server `HEAD` kept naming `main`, which then dangled, +and later `GitBranch`es were refused until someone repaired the repository. + +The alternatives considered: + +- **Keep the bootstrap**: the outcome above. +- **Refuse until the default branch exists**: clean, but every review-branch user has to push a + first commit by hand before anything works. +- **Always create the default branch**: writes a branch nobody allowed. +- **Create it when the allowlist allows it, refuse otherwise** (built): the history starts on the + default branch, which stays the default; later write branches build on an empty commit, not on + someone's edits; and the first PR into the default branch carries everything written, so the + initial snapshot gets reviewed. Creating a commit nobody explicitly asked for still feels odd, but + leaving users of these platforms with a review branch as their default branch is worse. + +## 4. Deliberately not verified + +What other platforms (GitHub, GitLab, Azure DevOps, Bitbucket) do on a first push into an empty +repository is not pinned down, and does not need to be for now. When the server names its default +branch, the default branch is pushed first, alone, so whatever a forge does with "the first pushed +branch" lands on the right branch. It matters only on servers that do not name it (v0/v1), where +`spec.branch` is still created as the first branch. + +Also unverified: whether Azure DevOps speaks protocol v2, and whether our SSH transport carries it. +Where they do not, the server does not name the default branch, and the last row of §2 applies. diff --git a/docs/design/gittarget-parent-hardening.md b/docs/design/gittarget-parent-hardening.md index aa4827bb..87fc7a40 100644 --- a/docs/design/gittarget-parent-hardening.md +++ b/docs/design/gittarget-parent-hardening.md @@ -22,8 +22,8 @@ > (empty-repository bootstrap rules, ancestry status) is not part of it. > > Related: [`gittarget-parent-branch.md`](gittarget-parent-branch.md), the feature as built; -> [`gittarget-parent-empty-repository.md`](gittarget-parent-empty-repository.md) and -> [`gittarget-parent-observation.md`](gittarget-parent-observation.md), deferred follow-ups. +> [`gittarget-parent-empty-repository.md`](gittarget-parent-empty-repository.md), built in #480; and +> [`gittarget-parent-observation.md`](gittarget-parent-observation.md), a deferred follow-up. ## Merge review at `93120caf` @@ -94,7 +94,7 @@ resolving `Committed`. ### Out of scope for this pass - [`gittarget-parent-empty-repository.md`](gittarget-parent-empty-repository.md): which branch an - empty repository's first commit may create. Keep today's bootstrap behavior here. + empty repository's first commit may create. Kept here; changed later in #480. - [`gittarget-parent-observation.md`](gittarget-parent-observation.md): the parent's state after the write branch exists. diff --git a/internal/authz/kubeprovider_admission.go b/internal/authz/kubeprovider_admission.go index e00dba11..116c33a0 100644 --- a/internal/authz/kubeprovider_admission.go +++ b/internal/authz/kubeprovider_admission.go @@ -181,6 +181,26 @@ func GitBranchClaimAdmitted( return Decision{Allowed: true}, nil } +// GitBranchMayWrite reports whether the GitBranch named gitBranch may write branch through provider: +// the same spec.allowedGitBranches decision as GitBranchClaimAdmitted, for a branch other than its +// own spec.branch. The branch worker asks it before it creates the default branch of an empty +// repository for its write branch to start from, and the GitBranch reconciler asks it to say why it +// did not. A non-NotFound read of the Namespace is returned as err. +func GitBranchMayWrite( + ctx context.Context, + reader client.Reader, + gitBranch k8stypes.NamespacedName, + provider *configv1alpha3.GitProvider, + branch string, +) (bool, error) { + nsLabels, err := namespaceLabels(ctx, reader, gitBranch.Namespace) + if err != nil { + return false, err + } + admission, _ := provider.AdmitsGitBranch(gitBranch, nsLabels, branch) + return admission.Admitted && admission.Allowed, nil +} + // ApplierClaimAdmitted reports whether an applier claim may hold its applier through the KubeProvider // it names (spec.kubeProviderRef): a grant in that provider's spec.allowedApplierClaims admits the // applier claim's namespace, and the applier lives in one of the applierNamespaces those grants diff --git a/internal/controller/gitbranch_controller.go b/internal/controller/gitbranch_controller.go index fe86b80b..608f934a 100644 --- a/internal/controller/gitbranch_controller.go +++ b/internal/controller/gitbranch_controller.go @@ -20,6 +20,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/handler" + logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/predicate" ctrlreconcile "sigs.k8s.io/controller-runtime/pkg/reconcile" "sigs.k8s.io/controller-runtime/pkg/source" @@ -56,6 +57,11 @@ const ( // GitBranchReasonParentBranchNotFound is the stall for a branch that cannot be created: the // remote carries neither it nor the parent it would be created from. GitBranchReasonParentBranchNotFound = "ParentBranchNotFound" + // GitBranchReasonDefaultBranchNotAllowed is the stall for a write branch in an empty repository + // whose default branch the branch worker would create first, with one empty commit, for the + // write branch to start from, but the GitProvider's spec.allowedGitBranches does not allow this + // GitBranch to write it. + GitBranchReasonDefaultBranchNotAllowed = "DefaultBranchNotAllowed" // GitBranchReasonRecoveringParentBranch is progress while the parent branch has been found again // but the branch worker has not yet published the writes it kept while the parent was missing. GitBranchReasonRecoveringParentBranch = "RecoveringParentBranch" @@ -233,13 +239,14 @@ func (r *GitBranchReconciler) evaluate( if !held { return rd, 0, nil } - heldFor := r.observeWorker(st, rd, branch, key, &provider) + heldFor := r.observeWorker(ctx, st, rd, branch, key, &provider) return rd, heldFor, nil } // observeWorker publishes what the holder's branch worker last proved about the remote, and the // gates that come from it: a parent branch that is missing, and work held back while it was. func (r *GitBranchReconciler) observeWorker( + ctx context.Context, st *reconcileStatus, rd *readiness, branch *configbutleraiv1alpha3.GitBranch, @@ -259,6 +266,9 @@ func (r *GitBranchReconciler) observeWorker( observed.ParentRequested == branch.Spec.ParentBranch && !differentRepository(observed.Repo, repo) { rd.stalled(GitBranchReasonParentBranchNotFound, parentBranchNotFoundMessage(branch, observed.ParentBranch)) } + if seen && !differentRepository(observed.Repo, repo) { + r.gateUnbornDefault(ctx, rd, branch, provider, observed) + } if r.WorkerManager == nil { return heldFor } @@ -272,6 +282,39 @@ func (r *GitBranchReconciler) observeWorker( return heldFor } +// gateUnbornDefault stalls a GitBranch whose write branch would start from an empty repository's +// default branch that it may not create: the worker creates that branch with one empty commit only +// when spec.allowedGitBranches allows this GitBranch to write it, and holds the writes otherwise. +// Allowed, the worker creates it on its next look, so nothing is reported. +func (r *GitBranchReconciler) gateUnbornDefault( + ctx context.Context, + rd *readiness, + branch *configbutleraiv1alpha3.GitBranch, + provider *configbutleraiv1alpha3.GitProvider, + observed git.RemoteObservation, +) { + unborn := observed.ParentBranch + if observed.ParentState != git.ParentUnborn || unborn == "" || unborn == branch.Spec.Branch || + branch.Spec.ParentBranch != "" { + return + } + allowed, err := authz.GitBranchMayWrite(ctx, r.Client, client.ObjectKeyFromObject(branch), provider, unborn) + if err != nil { + logf.FromContext(ctx).V(1).Info("Could not check spec.allowedGitBranches for the default branch", + "defaultBranch", unborn, "error", err.Error()) + return + } + if allowed { + return + } + rd.stalled(GitBranchReasonDefaultBranchNotAllowed, fmt.Sprintf( + "The repository is empty and its default branch '%s' does not exist yet. Branch '%s' starts from "+ + "it, so it is created first with one empty commit, but GitProvider '%s' does not allow this "+ + "GitBranch to write '%s' (spec.allowedGitBranches). Allow '%s' there, or push a first commit to "+ + "'%s'; nothing is written until then", + unborn, branch.Spec.Branch, branch.GitProviderKey(), unborn, unborn, unborn)) +} + // lastPushNotice is when a Git host last reported a push to the repository branch key, which this // GitBranch holds, zero when its worker has seen none. A worker still serving the GitBranch for the // repository its GitProvider left has none to give: its notices are about that repository. diff --git a/internal/controller/gitbranch_controller_test.go b/internal/controller/gitbranch_controller_test.go index 488a99e2..34f1070f 100644 --- a/internal/controller/gitbranch_controller_test.go +++ b/internal/controller/gitbranch_controller_test.go @@ -352,6 +352,50 @@ func TestGitBranch_AnUnresolvedDefaultBranchStallsToo(t *testing.T) { assert.Contains(t, readyOf(got).Message, "spec.parentBranch") } +// TestGitBranch_AnEmptyRepositorysDefaultBranchMustBeAllowed: in an empty repository whose HEAD +// names main, a write branch that is not main starts from main, which is created first. A GitBranch +// whose provider does not allow it to write main stalls with DefaultBranchNotAllowed; one that may, +// or whose write branch is main, does not. +func TestGitBranch_AnEmptyRepositorysDefaultBranchMustBeAllowed(t *testing.T) { + for name, tc := range map[string]struct { + allowed []configbutleraiv1alpha3.BranchPattern + branch string + wantStall bool + }{ + "main not allowed": {allowed: []configbutleraiv1alpha3.BranchPattern{"edits"}, branch: "edits", wantStall: true}, + "main allowed": {allowed: []configbutleraiv1alpha3.BranchPattern{"edits", "main"}, branch: "edits"}, + "the write branch is main": {allowed: []configbutleraiv1alpha3.BranchPattern{"main"}, branch: "main"}, + } { + t.Run(name, func(t *testing.T) { + f := newGitBranchFixture(t, branchProvider("platform", "acme", tc.allowed...), + branchObject("platform", "edits", "acme", tc.branch, time.Hour)) + f.reconcile(t, "platform", "edits") + key := k8stypes.NamespacedName{Namespace: "platform", Name: "edits"} + require.NoError(t, f.workers.EnsureWorker(context.Background(), git.WorkerRequest{ + GitBranch: key, Provider: k8stypes.NamespacedName{Namespace: "platform", Name: "acme"}, + Branch: tc.branch, Repo: git.RepoIdentity{ProviderUID: "uid-acme", URL: gitBranchTestURL}, + })) + worker, _ := f.workers.WorkerForGitBranch(key) + + worker.RecordUnbornDefaultBranchForTest("main") + got := f.reconcile(t, "platform", "edits") + require.NotNil(t, got.Status.Remote) + require.NotNil(t, got.Status.Remote.Parent) + assert.Equal(t, configbutleraiv1alpha3.GitBranchParentUnborn, got.Status.Remote.Parent.State) + assert.Equal(t, "main", got.Status.Remote.Parent.Branch) + if !tc.wantStall { + assert.NotEqual(t, GitBranchReasonDefaultBranchNotAllowed, readyOf(got).Reason) + return + } + assert.Equal(t, GitBranchReasonDefaultBranchNotAllowed, readyOf(got).Reason) + assert.Equal(t, metav1.ConditionFalse, readyOf(got).Status) + assert.True(t, conditionIsTrue(got.Status.Conditions, ConditionTypeStalled)) + assert.Contains(t, readyOf(got).Message, "default branch 'main'") + assert.Contains(t, readyOf(got).Message, "spec.allowedGitBranches") + }) + } +} + // TestGitBranch_ARepointedProviderMovesTheClaim: a GitProvider recreated against another repository // is another key, so the GitBranch's claim moves with it. func TestGitBranch_ARepointedProviderMovesTheClaim(t *testing.T) { diff --git a/internal/git/branch_worker.go b/internal/git/branch_worker.go index 7a807744..f9fa3f92 100644 --- a/internal/git/branch_worker.go +++ b/internal/git/branch_worker.go @@ -2788,7 +2788,16 @@ func (w *BranchWorker) prepareBranch( ctx context.Context, repoPath string, auth []gitclient.Option, ) (*PullReport, error) { parent := w.parentSnapshot() - report, err := PrepareBranchFrom(ctx, w.repo.URL, repoPath, w.Branch, parent.name, auth) + prepare := func() (*PullReport, error) { + return PrepareBranchFrom(ctx, w.repo.URL, repoPath, w.Branch, parent.name, auth) + } + report, err := prepare() + if err == nil && w.unbornDefaultToCreate(report, parent) != "" { + var repo *gogit.Repository + if repo, err = gogit.PlainOpen(repoPath); err == nil { + report, err = w.startFromUnbornDefault(ctx, repo, report, parent, auth, prepare) + } + } w.noteFetchParent(parent, err) return report, err } @@ -2798,7 +2807,13 @@ func (w *BranchWorker) syncToRemote( ctx context.Context, repo *gogit.Repository, auth []gitclient.Option, ) (*PullReport, error) { parent := w.parentSnapshot() - report, err := syncToRemoteFn(ctx, repo, plumbing.NewBranchReferenceName(w.Branch), parent.name, auth) + sync := func() (*PullReport, error) { + return syncToRemoteFn(ctx, repo, plumbing.NewBranchReferenceName(w.Branch), parent.name, auth) + } + report, err := sync() + if err == nil { + report, err = w.startFromUnbornDefault(ctx, repo, report, parent, auth, sync) + } w.noteFetchParent(parent, err) return report, err } @@ -2812,9 +2827,13 @@ func (w *BranchWorker) noteFetchParent(parent parentConfig, err error) { return } var unresolved *DefaultBranchUnresolvedError + var notAllowed *DefaultBranchNotAllowedError switch { case errors.As(err, &unresolved): w.recordMissingParent(parent.name, unresolved.Head) + case errors.As(err, ¬Allowed): + // The repository is still empty, and its default branch is named: Unborn, as observed. + w.recordAbsentBranch(notAllowed.Branch, "", ObservedByFetch) case errors.Is(err, ErrParentBranchNotFound): w.recordMissingParent(parent.name, parent.name) } @@ -3361,6 +3380,13 @@ func (w *BranchWorker) RecordUnresolvedDefaultBranchForTest(head string) { w.recordMissingParent("", head) } +// RecordUnbornDefaultBranchForTest records the observation a fetch makes of an empty repository +// whose HEAD names head, with the write branch absent and no parent configured, for tests in +// another package. +func (w *BranchWorker) RecordUnbornDefaultBranchForTest(head string) { + w.recordAbsentBranch(head, "", ObservedByFetch) +} + // BaseTrustedForTest exposes the flag to tests in another package. func (w *BranchWorker) BaseTrustedForTest() bool { return w.baseTrusted() } diff --git a/internal/git/parent_recovery.go b/internal/git/parent_recovery.go index f175f53f..e936f71e 100644 --- a/internal/git/parent_recovery.go +++ b/internal/git/parent_recovery.go @@ -37,9 +37,11 @@ var errAwaitingParentProbe = fmt.Errorf("%w (unchanged since the last look; wait ErrParentBranchNotFound) // isParentUnavailable reports an error caused by the parent a new write branch would be created -// from: a configured parent that is absent, or a default branch that does not resolve. +// from: a configured parent that is absent, a default branch that does not resolve, or an empty +// repository's default branch that this GitBranch may not create. func isParentUnavailable(err error) bool { - return errors.Is(err, ErrParentBranchNotFound) || errors.Is(err, ErrDefaultBranchUnresolved) + return errors.Is(err, ErrParentBranchNotFound) || errors.Is(err, ErrDefaultBranchUnresolved) || + errors.Is(err, ErrDefaultBranchNotAllowed) } // parentRecovery is the loop's latch on parent recovery. Loop-goroutine only. @@ -145,7 +147,18 @@ func (l *branchWorkerEventLoop) probeParent() (bool, error) { if err != nil { return true, fmt.Errorf("read the remote advertisement: %w", err) } - return w.recordAdvertisement(advertisement, parent.name), nil + missing := w.recordAdvertisement(advertisement, parent.name) + unborn := advertisement.parentBranch + if missing || !advertisement.empty || parent.name != "" || unborn == "" || unborn == w.Branch { + return missing, nil + } + // Still empty, and the write branch would start from a default branch the worker has to create + // first: that is found only once this GitBranch may create it. + allowed, err := w.mayCreateUnbornDefault(w.ctx, provider, unborn) + if err != nil { + return true, fmt.Errorf("check spec.allowedGitBranches for %q: %w", unborn, err) + } + return !allowed, nil } // publishRecovery mirrors the latch for the readers outside the loop: the controller, and the diff --git a/internal/git/unborn_default_branch.go b/internal/git/unborn_default_branch.go new file mode 100644 index 00000000..4b1fe643 --- /dev/null +++ b/internal/git/unborn_default_branch.go @@ -0,0 +1,180 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "context" + "errors" + "fmt" + "time" + + gogit "github.com/go-git/go-git/v6" + "github.com/go-git/go-git/v6/plumbing" + gitclient "github.com/go-git/go-git/v6/plumbing/client" + + configv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" + "github.com/ConfigButler/gitops-reverser/internal/authz" +) + +// An empty repository's HEAD names a branch with no commit yet: its default branch, unborn. When +// spec.parentBranch is omitted and the write branch is another branch, the worker creates that +// default branch first, with one empty commit, and starts the write branch from it. The history +// then starts on the default branch, which stays the default on every forge, and the first PR from +// the write branch into it carries everything the operator wrote. Starting the write branch as the +// repository's first branch instead would make it the default on Gitea and Forgejo, and every later +// write branch without a parent would build on its unreviewed commits. +// +// Creating the default branch is a write to a branch other than spec.branch, so it needs the same +// permission: the GitProvider's spec.allowedGitBranches must allow this GitBranch to write it. +// Without that, nothing is written and the GitBranch reports DefaultBranchNotAllowed until the +// branch is allowed or exists. A server that does not report the unborn name (protocol v0/v1) gives +// nothing to create; the write branch then starts as the first branch, as before. + +// ErrDefaultBranchNotAllowed reports an empty repository whose default branch the worker would +// create for the write branch to start from, but the GitProvider does not allow this GitBranch to +// write it. DefaultBranchNotAllowedError names the branch. +var ErrDefaultBranchNotAllowed = errors.New("the empty repository's default branch may not be created") + +// DefaultBranchNotAllowedError is ErrDefaultBranchNotAllowed with the branch the server named. +type DefaultBranchNotAllowedError struct { + // Branch is the short name of the unborn default branch. + Branch string +} + +func (e *DefaultBranchNotAllowedError) Error() string { + return fmt.Sprintf("the repository is empty and its default branch %q does not exist; "+ + "spec.allowedGitBranches does not allow creating it", e.Branch) +} + +// Is makes the error match ErrDefaultBranchNotAllowed. +func (e *DefaultBranchNotAllowedError) Is(target error) bool { + return target == ErrDefaultBranchNotAllowed +} + +// initializeCommitMessage is the message of the empty commit that creates the default branch. +const initializeCommitMessage = "chore: initialize repository\n\n" + + "Created by GitOps Reverser so that branch '%s' has the default branch to start from.\n" + +// unbornDefaultToCreate is the default branch a sync left unborn that the worker must create before +// the write branch can start from it, or empty when there is none: the write branch exists, a parent +// is configured, the repository is not empty, the server did not name its default branch, or the +// write branch is that default branch. +func (w *BranchWorker) unbornDefaultToCreate(report *PullReport, parent parentConfig) string { + if report == nil || parent.name != "" || report.ExistsOnRemote || !report.HEAD.Unborn || + report.ParentBranch == "" || report.ParentBranch == w.Branch { + return "" + } + return report.ParentBranch +} + +// mayCreateUnbornDefault reports whether this worker's GitBranch may write branch. A worker that +// serves no GitBranch (the CLI, and tests) is allowed nothing. +func (w *BranchWorker) mayCreateUnbornDefault( + ctx context.Context, provider *configv1alpha3.GitProvider, branch string, +) (bool, error) { + if w.GitBranch.Name == "" { + return false, nil + } + return authz.GitBranchMayWrite(ctx, w.Client, w.GitBranch, provider, branch) +} + +// createUnbornDefault creates branch on the remote with one empty commit, signed and authored as the +// worker's other commits are, from a checkout that a sync just left unborn (an empty index). It is +// refused with DefaultBranchNotAllowedError when the GitBranch may not write branch. +// +// The push's own failure comes back as an unbornDefaultPushError: a push refused because somebody +// else created branch meanwhile (the advertisement, or the server's compare-and-swap at upload) is +// not a failure, and only the sync that follows can tell the two apart. +func (w *BranchWorker) createUnbornDefault( + ctx context.Context, repo *gogit.Repository, branch string, auth []gitclient.Option, +) error { + provider, err := w.getGitProvider(ctx) + if err != nil { + return err + } + allowed, err := w.mayCreateUnbornDefault(ctx, provider, branch) + if err != nil { + return fmt.Errorf("check spec.allowedGitBranches for %q: %w", branch, err) + } + if !allowed { + return &DefaultBranchNotAllowedError{Branch: branch} + } + signer, err := getCommitSigner(ctx, w.Client, provider) + if err != nil { + return err + } + + if err := setHead(repo, branch); err != nil { + return fmt.Errorf("point HEAD at %q: %w", branch, err) + } + worktree, err := repo.Worktree() + if err != nil { + return err + } + operator := operatorSignature(ResolveCommitConfig(provider.Spec.Commit), time.Now()) + commit, err := worktree.Commit(fmt.Sprintf(initializeCommitMessage, w.Branch), &gogit.CommitOptions{ + Author: operator, + Committer: operator, + Signer: signer, + AllowEmptyCommits: true, + }) + if err != nil { + return fmt.Errorf("commit the first commit of %q: %w", branch, err) + } + + if _, err := PushAtomic(ctx, repo, plumbing.ZeroHash, plumbing.NewBranchReferenceName(branch), auth); err != nil { + return &unbornDefaultPushError{branch: branch, err: err} + } + w.Log.Info("Created the empty repository's default branch for the write branch to start from", + "branch", w.Branch, "defaultBranch", branch, "commit", commit.String()) + return nil +} + +// unbornDefaultPushError is the push of the default branch's first commit failing, which the sync +// after it judges: see createUnbornDefault. +type unbornDefaultPushError struct { + branch string + err error +} + +func (e *unbornDefaultPushError) Error() string { + return fmt.Sprintf("create the default branch %q: %v", e.branch, e.err) +} + +func (e *unbornDefaultPushError) Unwrap() error { return e.err } + +// startFromUnbornDefault follows a sync that left the write branch unborn in an empty repository +// whose default branch is another branch: it creates that branch and syncs again, so the write +// branch starts from it. resync is the same sync. Any other report is returned unchanged. +func (w *BranchWorker) startFromUnbornDefault( + ctx context.Context, + repo *gogit.Repository, + report *PullReport, + parent parentConfig, + auth []gitclient.Option, + resync func() (*PullReport, error), +) (*PullReport, error) { + branch := w.unbornDefaultToCreate(report, parent) + if branch == "" { + return report, nil + } + var pushErr *unbornDefaultPushError + if err := w.createUnbornDefault(ctx, repo, branch, auth); err != nil && !errors.As(err, &pushErr) { + return nil, err + } + report, err := resync() + if err != nil { + return nil, err + } + if w.unbornDefaultToCreate(report, parent) != "" { + if pushErr != nil { + return nil, pushErr + } + return nil, fmt.Errorf("created the default branch %q, but the remote does not show it", branch) + } + if pushErr != nil { + w.Log.Info("The default branch appeared on the remote before ours; starting from it", + "branch", w.Branch, "defaultBranch", branch, "pushError", pushErr.Error()) + } + return report, nil +} diff --git a/internal/git/unborn_head_test.go b/internal/git/unborn_head_test.go index ac9d62cc..6cf24804 100644 --- a/internal/git/unborn_head_test.go +++ b/internal/git/unborn_head_test.go @@ -16,6 +16,8 @@ import ( "github.com/go-git/go-git/v6/plumbing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + configv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" ) // emptyRepoServedOver serves an empty bare repository whose HEAD names trunk, over v2 or v0. @@ -91,19 +93,75 @@ func TestListRemoteRefs_ARepositoryWithCommitsHasNoUnbornHeadOverV2(t *testing.T assert.ElementsMatch(t, []string{"HEAD", "refs/heads/main", "refs/tags/v1"}, names, "peeled refs left out") } -// The worker reports the name with ParentUnborn from every look at the remote: the fetch, a -// refresh, and a push, which cannot see HEAD and keeps what discovery read. -func TestBranchWorker_AnEmptyRepositoryReportsItsUnbornHeadOverV2(t *testing.T) { - f := newRealServerNewBranch(t, "unborn-head-v2", func(repoDir string) { +// allowBranches lets the fixture's GitBranch write patterns through its GitProvider. +func (f *realServerNewBranch) allowBranches(patterns ...configv1alpha3.BranchPattern) { + f.t.Helper() + provider, err := f.worker.getGitProvider(f.worker.ctx) + require.NoError(f.t, err) + provider.Spec.AllowedGitBranches = []configv1alpha3.GitProviderGitBranchGrant{{ + ConsumerNamespaces: configv1alpha3.ConsumerNamespaces{From: configv1alpha3.GrantFromSame}, + BranchPatterns: patterns, + }} + require.NoError(f.t, f.worker.Client.Update(f.worker.ctx, provider)) +} + +// emptyTrunkRepoOverV2 is an empty repository whose HEAD names trunk, served over v2, with the +// write branch feature. +func emptyTrunkRepoOverV2(t *testing.T, slug string) *realServerNewBranch { + t.Helper() + f := newRealServerNewBranch(t, slug, func(repoDir string) { gitIn(t, repoDir, "symbolic-ref", "HEAD", "refs/heads/trunk") }) f.sim.protocolV2.Store(true) + return f +} + +// assertFeatureStartsOnInitializedTrunk checks trunk is one empty root commit, and feature's first commit sits on it. +func (f *realServerNewBranch) assertFeatureStartsOnInitializedTrunk() { + f.t.Helper() + trunk := f.ref("trunk") + require.False(f.t, trunk.IsZero(), "trunk is created") + assert.Empty(f.t, f.parentOf(trunk), "trunk starts with a root commit") + assert.Empty(f.t, gitIn(f.t, f.repoDir, "ls-tree", "-r", trunk.String()), "an empty one") + assert.Contains(f.t, gitIn(f.t, f.repoDir, "log", "-1", "--format=%s", trunk.String()), + "chore: initialize repository") + + tip := f.ref("feature") + require.False(f.t, tip.IsZero(), "feature is created") + assert.Equal(f.t, trunk.String(), gitIn(f.t, f.repoDir, "merge-base", "feature", "trunk"), + "feature starts from trunk") + assert.Contains(f.t, gitIn(f.t, f.repoDir, "ls-tree", "-r", "--name-only", "feature"), cmFile("cm1")) +} + +// The write branch is not the default branch, and the GitBranch may write the default branch: it is +// created first with one empty commit, and the write branch starts from it. +func TestBranchWorker_AnEmptyRepositoryGetsItsDefaultBranchFirst(t *testing.T) { + f := emptyTrunkRepoOverV2(t, "unborn-default-created") + f.allowBranches("trunk", "feature") require.NoError(t, f.worker.ensureRepositoryInitialized(f.worker.ctx)) o := f.observed() + assert.Equal(t, ParentFound, o.ParentState, "trunk exists now") + assert.Equal(t, "trunk", o.ParentBranch) + assert.Equal(t, f.ref("trunk").String(), o.ParentCommit) + assert.True(t, f.ref("feature").IsZero(), "nothing written yet") + + liveWrite(f.loop, "cm1") + f.assertFeatureStartsOnInitializedTrunk() +} + +// The GitBranch may not write the default branch: nothing is written, the repository is reported +// Unborn with the name, and the write is held. Allowing the branch is enough: the next probe creates +// it and the held write lands on it. +func TestBranchWorker_AnEmptyRepositoryWaitsUntilItsDefaultBranchIsAllowed(t *testing.T) { + f := emptyTrunkRepoOverV2(t, "unborn-default-not-allowed") + f.allowBranches("feature") + + err := f.worker.ensureRepositoryInitialized(f.worker.ctx) + require.ErrorIs(t, err, ErrDefaultBranchNotAllowed) + o := f.observed() assert.Equal(t, ParentUnborn, o.ParentState) assert.Equal(t, "trunk", o.ParentBranch) - assert.Empty(t, o.ParentCommit) provider, err := f.worker.getGitProvider(f.worker.ctx) require.NoError(t, err) @@ -111,32 +169,66 @@ func TestBranchWorker_AnEmptyRepositoryReportsItsUnbornHeadOverV2(t *testing.T) assert.Equal(t, ParentUnborn, f.observed().ParentState, "the refresh agrees") assert.Equal(t, "trunk", f.observed().ParentBranch) - noOpWrite(f.loop) - o = f.observed() - assert.Equal(t, ObservedByPush, o.By) - assert.Equal(t, ParentUnborn, o.ParentState) - assert.Equal(t, "trunk", o.ParentBranch, "a push keeps the name discovery read") + liveWrite(f.loop, "cm1") + assert.True(t, f.ref("trunk").IsZero(), "trunk is not created") + assert.True(t, f.ref("feature").IsZero(), "nothing is written") + assert.Len(t, f.loop.pendingWrites, 1, "the write is held") + + pastProbeDeadline(f.worker) + f.loop.runParentProbe() + assert.True(t, f.ref("trunk").IsZero(), "a probe without permission changes nothing") + assert.Len(t, f.loop.pendingWrites, 1) + + f.allowBranches("trunk", "feature") + pastProbeDeadline(f.worker) + f.loop.runParentProbe() + f.assertFeatureStartsOnInitializedTrunk() + assert.Empty(t, f.loop.pendingWrites, "the held write landed") } -// Knowing the name changes no behavior: the first write is still a root commit on the write branch, -// and the branch HEAD names is not created. -func TestBranchWorker_AnEmptyRepositoryStillBootstrapsOverV2(t *testing.T) { - f := newRealServerNewBranch(t, "unborn-bootstrap-v2", func(repoDir string) { - gitIn(t, repoDir, "symbolic-ref", "HEAD", "refs/heads/trunk") +// Somebody else creates the default branch between our advertisement and our upload. The server +// refuses our first commit, and the write branch starts from theirs instead. +func TestBranchWorker_AnEmptyRepositoryWhoseDefaultBranchAppearsDuringOurPushStartsFromIt(t *testing.T) { + f := emptyTrunkRepoOverV2(t, "unborn-default-race") + f.allowBranches("trunk", "feature") + + var theirs plumbing.Hash + afterPushValidation = func() { + afterPushValidation = nil + theirs = branchOnDisk(t, f.repoDir, "trunk", "theirs\n") + } + t.Cleanup(func() { afterPushValidation = nil }) + + require.NoError(t, f.worker.ensureRepositoryInitialized(f.worker.ctx)) + require.False(t, theirs.IsZero(), "trunk was created during our push") + assert.Equal(t, theirs, f.ref("trunk"), "theirs stands") + assert.Equal(t, ParentFound, f.observed().ParentState) + assert.Equal(t, theirs.String(), f.observed().ParentCommit) + + liveWrite(f.loop, "cm1") + assert.Equal(t, theirs.String(), gitIn(t, f.repoDir, "merge-base", "feature", "trunk")) +} + +// The write branch IS the default branch: it starts the history, nothing else is created, and a +// push, which cannot see HEAD, keeps the name the fetch read. +func TestBranchWorker_AnEmptyRepositoryWhoseDefaultIsTheWriteBranchStartsIt(t *testing.T) { + f := newRealServerNewBranch(t, "unborn-default-is-write-branch", func(repoDir string) { + gitIn(t, repoDir, "symbolic-ref", "HEAD", "refs/heads/feature") }) f.sim.protocolV2.Store(true) + require.NoError(t, f.worker.ensureRepositoryInitialized(f.worker.ctx)) - require.Equal(t, "trunk", f.observed().ParentBranch) + assert.Equal(t, ParentUnborn, f.observed().ParentState) + assert.Equal(t, "feature", f.observed().ParentBranch) - liveWrite(f.loop, "cm1") - tip := f.ref("feature") - require.False(t, tip.IsZero()) - assert.Empty(t, f.parentOf(tip), "a root commit") - assert.True(t, f.ref("trunk").IsZero(), "trunk is not created") + noOpWrite(f.loop) + assert.Equal(t, ObservedByPush, f.observed().By) + assert.Equal(t, "feature", f.observed().ParentBranch, "a push keeps the name the fetch read") } -// Over v0 the same repository is Unborn with no name: the server did not say, so nothing is guessed. -func TestBranchWorker_AnEmptyRepositoryNamesNoBranchOverV0(t *testing.T) { +// Over v0 the server does not name the default branch: nothing is guessed, and the write branch +// starts as the repository's first branch. +func TestBranchWorker_AnEmptyRepositoryOverV0StartsTheWriteBranch(t *testing.T) { f := newRealServerNewBranch(t, "unborn-head-v0", func(repoDir string) { gitIn(t, repoDir, "symbolic-ref", "HEAD", "refs/heads/trunk") }) @@ -144,6 +236,12 @@ func TestBranchWorker_AnEmptyRepositoryNamesNoBranchOverV0(t *testing.T) { require.NoError(t, f.worker.ensureRepositoryInitialized(f.worker.ctx)) assert.Equal(t, ParentUnborn, f.observed().ParentState) assert.Empty(t, f.observed().ParentBranch) + + liveWrite(f.loop, "cm1") + tip := f.ref("feature") + require.False(t, tip.IsZero()) + assert.Empty(t, f.parentOf(tip), "a root commit") + assert.True(t, f.ref("trunk").IsZero()) } // §4.3 test 1 over v2. A repository that is not empty but whose HEAD dangles is still refused, and diff --git a/test/e2e/new_branch_parent_e2e_test.go b/test/e2e/new_branch_parent_e2e_test.go index 27fae4d4..21270e63 100644 --- a/test/e2e/new_branch_parent_e2e_test.go +++ b/test/e2e/new_branch_parent_e2e_test.go @@ -46,7 +46,7 @@ var _ = Describe("New write branch starts from its parent", Label("manager"), fu Context("with spec.parentBranch naming a branch that does not exist yet", Ordered, func() { describeMissingParentRecoverySpec() }) - Context("in an empty repository with spec.parentBranch omitted", Ordered, func() { + Context("in an empty repository with spec.parentBranch omitted", Ordered, Label("empty-repo"), func() { describeEmptyRepositoryBootstrapSpec() }) }) @@ -199,10 +199,12 @@ func describeMissingParentRecoverySpec() { }) } -// describeEmptyRepositoryBootstrapSpec: an empty repository's HEAD names main (Gitea's default). -// Gitea speaks protocol v2, so the GitBranch reports that name before anything is written. The -// first write still creates the requested branch as a root commit, and main is not created: the -// branch asked for is the branch written. +// describeEmptyRepositoryBootstrapSpec: an empty Gitea repository's HEAD names main, and Gitea +// speaks protocol v2, so the GitBranch knows it. The write branch is not main, so it starts from +// main, which is created first with one empty commit. Creating it is a write to main: while the +// GitProvider does not allow that, the GitBranch reports DefaultBranchNotAllowed, its GitTarget +// GitBranchNotReady, and nothing is written. Allowing main is the whole fix: the held edit lands on +// the write branch, on top of main's empty first commit. func describeEmptyRepositoryBootstrapSpec() { const ( providerName = "empty-repo-provider" @@ -216,6 +218,30 @@ func describeEmptyRepositoryBootstrapSpec() { writeBranch string ) + applyProvider := func(patterns ...string) { + GinkgoHelper() + quoted := make([]string, 0, len(patterns)) + for _, p := range patterns { + quoted = append(quoted, fmt.Sprintf("%q", p)) + } + provider := fmt.Sprintf(`apiVersion: configbutler.ai/v1alpha3 +kind: GitProvider +metadata: + name: %s + namespace: %s +spec: + url: %s + allowedGitBranches: + - consumerNamespaces: {from: Same} + branchPatterns: [%s] + secretRef: + name: %s +`, providerName, testNs, repo.RepoURLHTTP, strings.Join(quoted, ", "), repo.GitSecretHTTP) + out, err := kubectlRunWithStdin(testNs, provider, "apply", "-f", "-") + Expect(err).NotTo(HaveOccurred(), "apply GitProvider: %s", out) + verifyResourceStatus("gitprovider", providerName, testNs, "True", "Succeeded", "") + } + BeforeAll(func() { testNs = testNamespaceFor("manager-empty-repo") _, _ = kubectlRun("create", "namespace", testNs) @@ -224,7 +250,7 @@ func describeEmptyRepositoryBootstrapSpec() { _, err := kubectlRunInNamespace(testNs, "apply", "-f", repo.SecretsYAML) Expect(err).NotTo(HaveOccurred(), "failed to apply git secrets to test namespace") applySOPSAgeKeyToNamespace(testNs) - applyParentSpecProvider(testNs, providerName, repo, writeBranch) + applyProvider(writeBranch) }) AfterAll(func() { @@ -234,33 +260,48 @@ func describeEmptyRepositoryBootstrapSpec() { cleanupNamespace(testNs) }) - It("names the unborn default branch, then creates the requested branch as a root commit", func() { + It("creates main first, once it may, and starts the requested branch from it", func() { createGitTargetWithParentBranch(destName, testNs, providerName, gitPath, writeBranch, "") branchName := gitBranchName(providerName, writeBranch) + applyParentSpecWatchRule(testNs, ruleName, destName) + applyParentConfigMap(testNs, "first-write") - By("the GitBranch reports the empty repository and the branch its HEAD names") + By("main may not be written: the GitBranch and its GitTarget say so, and nothing is written") Eventually(func(g Gomega) { g.Expect(gitBranchRemoteField(g, branchName, testNs, "parent.state")).To(Equal("Unborn")) g.Expect(gitBranchRemoteField(g, branchName, testNs, "parent.branch")).To(Equal("main"), "Gitea answers protocol v2 ls-refs unborn") + g.Expect(gitBranchReadyReasonOf(g, branchName, testNs)).To(Equal("DefaultBranchNotAllowed")) + g.Expect(readyReasonOf(g, destName, testNs)).To(Equal("GitBranchNotReady")) }, 3*time.Minute, 3*time.Second).Should(Succeed()) + Expect(remoteBranchHeadOf(Default, repo.CheckoutDir, "main")).To(BeEmpty(), "main is not created") + Expect(remoteBranchHeadOf(Default, repo.CheckoutDir, writeBranch)).To(BeEmpty(), "nothing is written") - By("the first write creates the requested branch, not main") - applyParentSpecWatchRule(testNs, ruleName, destName) - applyParentConfigMap(testNs, "first-write") + By("allowing main creates it with one empty commit, and the held edit lands on top of it") + applyProvider("main", writeBranch) editPath := path.Join(gitPath, testNs, "configmaps", "first-write.yaml") Eventually(func(g Gomega) { + mainTip := remoteBranchHeadOf(g, repo.CheckoutDir, "main") + g.Expect(mainTip).NotTo(BeEmpty(), "main is created") tip := remoteBranchHeadOf(g, repo.CheckoutDir, writeBranch) - g.Expect(tip).NotTo(BeEmpty(), "the write creates the requested branch") - _, fetchErr := gitRun(repo.CheckoutDir, "fetch", "origin", writeBranch) + g.Expect(tip).NotTo(BeEmpty(), "the write branch is created") + _, fetchErr := gitRun(repo.CheckoutDir, "fetch", "origin", "main", writeBranch) g.Expect(fetchErr).NotTo(HaveOccurred()) roots, revErr := gitRun(repo.CheckoutDir, "rev-list", "--max-parents=0", tip) g.Expect(revErr).NotTo(HaveOccurred()) - g.Expect(strings.Fields(roots)).To(HaveLen(1), "one history, starting on the requested branch") + g.Expect(strings.TrimSpace(roots)).To(Equal(mainTip), "the write branch's history starts at main") + files, lsErr := gitRun(repo.CheckoutDir, "ls-tree", "-r", "--name-only", mainTip) + g.Expect(lsErr).NotTo(HaveOccurred()) + g.Expect(strings.TrimSpace(files)).To(BeEmpty(), "main's first commit is empty") _, showErr := gitRun(repo.CheckoutDir, "cat-file", "-e", tip+":"+editPath) - g.Expect(showErr).NotTo(HaveOccurred(), "the write is on the branch") + g.Expect(showErr).NotTo(HaveOccurred(), "the held edit is on the write branch") }, 5*time.Minute, 5*time.Second).Should(Succeed()) - Expect(remoteBranchHeadOf(Default, repo.CheckoutDir, "main")).To(BeEmpty(), "main is not created") + + Eventually(func(g Gomega) { + g.Expect(gitBranchReadyReasonOf(g, branchName, testNs)).NotTo( + BeElementOf("DefaultBranchNotAllowed", "RecoveringParentBranch")) + g.Expect(readyReasonOf(g, destName, testNs)).NotTo(Equal("GitBranchNotReady")) + }, 2*time.Minute, 3*time.Second).Should(Succeed()) }) } From e25884b9a2bccacebd9b4efbf936d9c39b6b5119 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 10 Nov 2026 11:44:45 +0000 Subject: [PATCH 05/10] test(git): give the annotated tag in the unborn listing test its own identity CI has no global git identity, so 'git tag -a' failed with 'Committer identity unknown'. Co-Authored-By: Claude Opus 5.5 --- internal/git/unborn_head_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/git/unborn_head_test.go b/internal/git/unborn_head_test.go index 6cf24804..3dbcdeee 100644 --- a/internal/git/unborn_head_test.go +++ b/internal/git/unborn_head_test.go @@ -78,7 +78,8 @@ func TestListRemoteRefs_ARepositoryWithCommitsHasNoUnbornHeadOverV2(t *testing.T repoDir := filepath.Join(projectRoot, "repo.git") createBareRepo(t, repoDir) main := simulateClientCommitOnDisk(t, repoDir, "main", "README.md", "x\n") - gitIn(t, repoDir, "tag", "-a", "-m", "annotated", "v1", main.String()) + gitIn(t, repoDir, "-c", "user.name=Outside", "-c", "user.email=o@example.com", + "tag", "-a", "-m", "annotated", "v1", main.String()) sim := startRealGitServer(t, projectRoot, repoDir) sim.protocolV2.Store(true) From 0e19f33bfad6e722473e070559d3d4076657a4c9 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 10 Nov 2026 11:45:46 +0000 Subject: [PATCH 06/10] test(e2e): the platform initializes main before tenants claim their branches A tenant may write only teams//*, so in an empty repository it can no longer start its branch as the first branch: the default branch is created first, which the tenant may not write, and its GitBranch reports DefaultBranchNotAllowed. A shared repository's main belongs to the platform, which pushes its first commit. Co-Authored-By: Claude Opus 5.5 --- test/e2e/provider_access_e2e_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/test/e2e/provider_access_e2e_test.go b/test/e2e/provider_access_e2e_test.go index fec19109..f794029d 100644 --- a/test/e2e/provider_access_e2e_test.go +++ b/test/e2e/provider_access_e2e_test.go @@ -46,6 +46,13 @@ var _ = Describe("Manager branch grants", Label("manager"), Ordered, func() { _, err := kubectlRunInNamespace(platformNs, "apply", "-f", repo.SecretsYAML) Expect(err).NotTo(HaveOccurred(), "failed to apply git secrets to the platform namespace") + // The platform owns main and initializes it. A tenant may write only teams//*, so in an + // empty repository it could not create the default branch its branch starts from, and its + // GitBranch would report DefaultBranchNotAllowed. + By("the platform pushes a first commit to main") + commitFilesToBranchFromOutside(repo, platformNs, "main", "e2e: the platform initializes main", + map[string]string{"README.md": "shared repository\n"}) + By("creating the platform's GitProvider: main for itself, teams/{consumer.namespace}/* for tenants") _, err = kubectlRunWithStdin("", fmt.Sprintf(`apiVersion: configbutler.ai/v1alpha3 kind: GitProvider From 832095dfe105ef752f6115bb92255e4f577ef386 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 10 Nov 2026 12:23:38 +0000 Subject: [PATCH 07/10] docs: describe the empty repository's default branch in architecture and the multi-tenant setup Architecture's GitBranch fields and its new-write-branch paragraph now say that an empty repository's default branch is created first, which the grants must allow. The multi-tenant example notes that tenants limited to teams//* need a repository with a first commit on main. Co-Authored-By: Claude Opus 5.5 --- docs/architecture.md | 12 +++++++++--- docs/configuration.md | 5 +++++ 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index 659ccbd1..1c226e28 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -545,7 +545,8 @@ it. Every field but `spec.allowedGitTargets` is immutable: - `spec.parentBranch`: optional branch the write branch is created from and compared against while it does not exist. Omitted means the remote's default branch. An explicit parent must exist when creating the write branch (`ParentBranchNotFound`). The grants need not allow it, because it is - only read. + only read. The exception is an empty repository with the parent omitted: its default branch is + created first, which is a write, so the grants must allow it (`DefaultBranchNotAllowed`). - `spec.allowedGitTargets`: which namespaces' `GitTarget`s may write through it. Omitted, its own namespace's; a written list replaces that default, and `[]` admits none. - `status.repository`: the normalized repository URL, half of the key one `GitBranch` holds. @@ -1611,8 +1612,13 @@ parent like a moved branch: fetch, reset, replay, check again. A branch somebody meantime is a moved remote too, so the replay builds on their commit instead of overwriting it; that holds when the server refuses our upload too, because the fallback reads both refs from one advertisement. A cycle planned on an empty repository that has since gained refs is refused at the -advertisement (`RepositoryNotEmptyError`) and replayed, never pushed as an orphan. A parent that is -missing, or a default branch that does not resolve, opens parent recovery on the worker: it +advertisement (`RepositoryNotEmptyError`) and replayed, never pushed as an orphan. In an **empty +repository** whose server names its unborn default branch (protocol v2 `ls-refs`, read by +`unbornHead`), a write branch that is not that branch starts from it: the worker pushes the default +branch first, alone, with one empty commit, and from then on the write branch is an ordinary absent +branch with a found parent. Over v0 the server names nothing and the write branch is the first +branch. A parent that is missing, a default branch that does not resolve, or an empty repository's +default branch the grants do not let the `GitBranch` create, opens parent recovery on the worker: it probes on its own backoff and publishes what it held back once the parent exists (see `docs/design/gittarget-parent-branch.md`). Proved end to end by the e2e spec "New write branch starts from its parent"; that publication alone finds a moved parent is proved by worker tests diff --git a/docs/configuration.md b/docs/configuration.md index 2f35f9c6..06e1b5b6 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -569,6 +569,11 @@ It also applies the `GitProvider` and `KubeProvider` above, with their grants, a `main` in `platform` that the platform keeps for itself (`from: Same` in `allowedGitBranches`). The Role also needs whatever team A's own workloads require. +Start from a repository that has a first commit on `main`. A tenant may write only +`teams//*`, so in an empty repository it cannot create the default branch its branch +starts from, and its `GitBranch` reports `DefaultBranchNotAllowed` until `main` exists +([starting the write branch](#starting-the-write-branch-from-another-branch-specparentbranch)). + | Identity | Manages | Cannot | |---|---|---| | The platform's reconciler | `GitProvider`, `KubeProvider` and their grants; Namespaces and their labels; tenant RBAC; platform `GitBranch`es such as `main` | | From 0f906c0c59ee4166ca9af7c93a465b4a68bdfcc5 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 10 Nov 2026 12:51:20 +0000 Subject: [PATCH 08/10] refactor(git): rename the reason to DefaultBranchCreationNotAllowed Beside BranchNotAllowed (spec.branch is not allowed), DefaultBranchNotAllowed read as "the default branch is not allowed". What is refused is creating it. Co-Authored-By: Claude Opus 5.5 --- api/v1alpha3/gitbranch_types.go | 5 ++-- .../bases/configbutler.ai_gitbranches.yaml | 5 ++-- docs/INDEX.md | 2 +- docs/UPGRADING.md | 2 +- docs/architecture.md | 2 +- docs/configuration.md | 6 ++--- docs/design/gittarget-parent-branch.md | 4 ++-- .../gittarget-parent-empty-repository.md | 2 +- internal/controller/gitbranch_controller.go | 6 ++--- .../controller/gitbranch_controller_test.go | 6 ++--- internal/git/branch_worker.go | 2 +- internal/git/parent_recovery.go | 2 +- internal/git/unborn_default_branch.go | 24 +++++++++---------- internal/git/unborn_head_test.go | 2 +- test/e2e/new_branch_parent_e2e_test.go | 6 ++--- test/e2e/provider_access_e2e_test.go | 2 +- 16 files changed, 40 insertions(+), 38 deletions(-) diff --git a/api/v1alpha3/gitbranch_types.go b/api/v1alpha3/gitbranch_types.go index 5e8a6a84..e7c41661 100644 --- a/api/v1alpha3/gitbranch_types.go +++ b/api/v1alpha3/gitbranch_types.go @@ -54,8 +54,9 @@ type GitBranchSpec struct { // default branch starts from it: the default branch is created first, with // one empty commit, when spec.allowedGitBranches lets this GitBranch write // it. Otherwise this GitBranch reports Stalled=True, reason - // DefaultBranchNotAllowed, and writes nothing. A server that does not name - // its default branch (Git protocol v0) gets spec.branch as its first branch. + // DefaultBranchCreationNotAllowed, and writes nothing. A server that does + // not name its default branch (Git protocol v0) gets spec.branch as its + // first branch. // // When set, it must exist on the remote for spec.branch to be created; // otherwise this GitBranch reports Stalled=True, reason ParentBranchNotFound, diff --git a/config/crd/bases/configbutler.ai_gitbranches.yaml b/config/crd/bases/configbutler.ai_gitbranches.yaml index 569e949e..7242098d 100644 --- a/config/crd/bases/configbutler.ai_gitbranches.yaml +++ b/config/crd/bases/configbutler.ai_gitbranches.yaml @@ -258,8 +258,9 @@ spec: default branch starts from it: the default branch is created first, with one empty commit, when spec.allowedGitBranches lets this GitBranch write it. Otherwise this GitBranch reports Stalled=True, reason - DefaultBranchNotAllowed, and writes nothing. A server that does not name - its default branch (Git protocol v0) gets spec.branch as its first branch. + DefaultBranchCreationNotAllowed, and writes nothing. A server that does + not name its default branch (Git protocol v0) gets spec.branch as its + first branch. When set, it must exist on the remote for spec.branch to be created; otherwise this GitBranch reports Stalled=True, reason ParentBranchNotFound, diff --git a/docs/INDEX.md b/docs/INDEX.md index e8a0c49d..76b6b8ed 100644 --- a/docs/INDEX.md +++ b/docs/INDEX.md @@ -142,7 +142,7 @@ so you never have to read a proposal to find out it already shipped. | [`gittarget-branch-worker-pending-writes.md`](design/gittarget-branch-worker-pending-writes.md) | **built in #413.** Ordered pending writes, one materializer and one retry schedule; a safe admission boundary for the watch; Git call deadlines; intake pause and resume; publication status. Holds the recovery contract for an outage, replay limits, and the acceptance scenarios. | | [`gittarget-parent-branch.md`](design/gittarget-parent-branch.md) | **merged in #407; the field moved to `GitBranch` in #429.** `GitBranch.spec.parentBranch`: a branch stays on standby until a write needs it, then is created from the parent's tip as checked at publication. The contract as built, the defect it started from, prior art and naming. Open: reporting the parent per branch in status. | | [`gittarget-parent-hardening.md`](design/gittarget-parent-hardening.md) | **done in #407.** Parent changes on a live worker, an empty repository gaining a ref, an unresolved default branch, recovery that cannot strand work, and the refused upload, without adding a pull to the fast path; plus the two recovery fixes from the review at `93120caf`. Records the merge boundary and the next separate PRs. | -| [`gittarget-parent-empty-repository.md`](design/gittarget-parent-empty-repository.md) | **built in #480.** An empty repository's first commit: status names the default branch (protocol v2), and a write branch that is not the default starts from it, created first with one empty commit when `allowedGitBranches` allows it, else `DefaultBranchNotAllowed`. Why, and what was deliberately not verified. | +| [`gittarget-parent-empty-repository.md`](design/gittarget-parent-empty-repository.md) | **built in #480.** An empty repository's first commit: status names the default branch (protocol v2), and a write branch that is not the default starts from it, created first with one empty commit when `allowedGitBranches` allows it, else `DefaultBranchCreationNotAllowed`. Why, and what was deliberately not verified. | | [`gittarget-parent-observation.md`](design/gittarget-parent-observation.md) | **deferred.** Keep the parent visible after the write branch exists: availability first, ancestry only after measuring how often a shallow checkout can answer it. Informational; authorizes no branch mutation. | | [`gittarget-red-status-plan.md`](design/gittarget-red-status-plan.md) | **design, partly built.** The smaller alternative to Git write preflight: make a `GitTarget` turn red and explain why no commit was made, without moving Git checks into the admission webhook and without adding a second state source. Keeps the existing kstatus trio and domain conditions, ranks the most likely operator-facing failures, and has now shipped the first status-transition slice: refusal messages republish when details change, scoped refusals cannot be cleared by unrelated scope success, `BlockMessage()` carries stable issue kind/path/actor detail, and the unsupported-folder e2e covers Git-side break and recovery. | | [`open-asks-priority.md`](design/open-asks-priority.md) | **stale; [`TODO.md`](TODO.md) is the work queue.** Merged three overlapping backlogs (the downstream consumer asks, the API-surface block left unbuilt by the status and configuration-model review, and the config-surface proposal B1–B6) into one ordered queue under four stated tests, and says where we deliberately do **not** do what was asked. B2, B4, F6 and F10 have shipped, and F9 is probably moot since #333 deleted the `ClusterWatchRule` `scope` field. Still the record of one design call: **Option C sibling inference was deleted** rather than given an off-switch, and "what the deletion taught" records what building it found | diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index 168cb98d..ab6d76ac 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -18,7 +18,7 @@ to become the default, and later branches built on it. That is a write to the default branch, so the provider's `spec.allowedGitBranches` must allow it for the `GitBranch`'s namespace. If it does not, the `GitBranch` reports `Ready=False`, `Stalled=True`, -reason `DefaultBranchNotAllowed`, its `GitTarget`s report `GitBranchNotReady`, and nothing is +reason `DefaultBranchCreationNotAllowed`, its `GitTarget`s report `GitBranchNotReady`, and nothing is written until you allow the branch or push a first commit to it: ```yaml diff --git a/docs/architecture.md b/docs/architecture.md index 1c226e28..68816d54 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -546,7 +546,7 @@ it. Every field but `spec.allowedGitTargets` is immutable: it does not exist. Omitted means the remote's default branch. An explicit parent must exist when creating the write branch (`ParentBranchNotFound`). The grants need not allow it, because it is only read. The exception is an empty repository with the parent omitted: its default branch is - created first, which is a write, so the grants must allow it (`DefaultBranchNotAllowed`). + created first, which is a write, so the grants must allow it (`DefaultBranchCreationNotAllowed`). - `spec.allowedGitTargets`: which namespaces' `GitTarget`s may write through it. Omitted, its own namespace's; a written list replaces that default, and `[]` admits none. - `status.repository`: the normalized repository URL, half of the key one `GitBranch` holds. diff --git a/docs/configuration.md b/docs/configuration.md index 06e1b5b6..881f4718 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -571,7 +571,7 @@ Role also needs whatever team A's own workloads require. Start from a repository that has a first commit on `main`. A tenant may write only `teams//*`, so in an empty repository it cannot create the default branch its branch -starts from, and its `GitBranch` reports `DefaultBranchNotAllowed` until `main` exists +starts from, and its `GitBranch` reports `DefaultBranchCreationNotAllowed` until `main` exists ([starting the write branch](#starting-the-write-branch-from-another-branch-specparentbranch)). | Identity | Manages | Cannot | @@ -1248,7 +1248,7 @@ at, and `Ready`; | `PushSessionFailed` | The credential cannot open a push session; a holder keeps the branch meanwhile | | `BranchConflict` | Another `GitBranch` holds this repository branch (`Stalled=True`) | | `ParentBranchNotFound` | Neither the branch nor its parent is on the remote (`Stalled=True`) | -| `DefaultBranchNotAllowed` | The repository is empty; the branch would start from the default branch, which `spec.allowedGitBranches` does not let this `GitBranch` create (`Stalled=True`) | +| `DefaultBranchCreationNotAllowed` | The repository is empty; the branch would start from the default branch, which `spec.allowedGitBranches` does not let this `GitBranch` create (`Stalled=True`) | | `RecoveringParentBranch` | The parent is back and the worker is publishing what it held | ### Deleting a `GitBranch` @@ -1563,7 +1563,7 @@ spec: history starts on the default branch, which stays the default, and the first pull request into it carries everything written. That is a write to the default branch, so the provider's `allowedGitBranches` must allow it for this namespace (`branchPatterns: ["main", "reverser/edits"]`). - Without that the `GitBranch` reports `DefaultBranchNotAllowed`, its `GitTarget`s report + Without that the `GitBranch` reports `DefaultBranchCreationNotAllowed`, its `GitTarget`s report `GitBranchNotReady`, and nothing is written until the branch is allowed or someone pushes a first commit to it. This needs the server to name its default branch, which Git protocol v2 does (GitHub, Gitea, Forgejo); over v0 `spec.branch` is created as the repository's first branch. diff --git a/docs/design/gittarget-parent-branch.md b/docs/design/gittarget-parent-branch.md index 84a8c7e0..7cdadaac 100644 --- a/docs/design/gittarget-parent-branch.md +++ b/docs/design/gittarget-parent-branch.md @@ -57,7 +57,7 @@ check is what protects the first write. | no | yes | compare against the parent; create the write branch from its tip, checked at publication | | no | no, **set** | refuse: `Ready=False`, `Stalled=True`, reason `ParentBranchNotFound`; nothing written, no orphan branch | | no | no, omitted, not empty (`HEAD` resolves to no branch, or only tags) | refuse the same way: `ParentBranchNotFound`, with a message to fix `HEAD` or set the field | -| no | no, omitted, empty repository (no refs at all) | the write branch is the default branch, or the server does not name it: the first commit is a root commit on the write branch. Otherwise the default branch is created first with one empty commit if allowed, else `DefaultBranchNotAllowed` | +| no | no, omitted, empty repository (no refs at all) | the write branch is the default branch, or the server does not name it: the first commit is a root commit on the write branch. Otherwise the default branch is created first with one empty commit if allowed, else `DefaultBranchCreationNotAllowed` | - **Recovery from a missing parent** is held open by the branch worker until the held work lands, whatever the parent's observed state. It latches when an attempt fails on the parent and clears @@ -152,7 +152,7 @@ that is the first-install case. Set is strict: it must exist, or the target refu In an empty repository whose server names its default branch, a write branch that is not the default starts from it: the default branch is created first with one empty commit, when the -provider allows this `GitBranch` to write it, and the `GitBranch` reports `DefaultBranchNotAllowed` +provider allows this `GitBranch` to write it, and the `GitBranch` reports `DefaultBranchCreationNotAllowed` otherwise. See [`gittarget-parent-empty-repository.md`](gittarget-parent-empty-repository.md). **Deferred, additive when someone asks:** diff --git a/docs/design/gittarget-parent-empty-repository.md b/docs/design/gittarget-parent-empty-repository.md index 7a675164..313edeee 100644 --- a/docs/design/gittarget-parent-empty-repository.md +++ b/docs/design/gittarget-parent-empty-repository.md @@ -27,7 +27,7 @@ With `spec.parentBranch` omitted, in an empty repository: |---|---|---|---| | yes | yes | (it is `spec.branch`, so yes) | the first commit starts `spec.branch` with no history | | yes | no | yes | the default branch is created first with one empty commit; `spec.branch` starts from it | -| yes | no | no | nothing is written: `GitBranch` `Ready=False`, `Stalled=True`, reason `DefaultBranchNotAllowed`; its `GitTarget`s `GitBranchNotReady` | +| yes | no | no | nothing is written: `GitBranch` `Ready=False`, `Stalled=True`, reason `DefaultBranchCreationNotAllowed`; its `GitTarget`s `GitBranchNotReady` | | no (protocol v0/v1) | unknown | | the first commit starts `spec.branch` as the repository's first branch | - **The empty commit** is authored and signed like every other commit the provider makes diff --git a/internal/controller/gitbranch_controller.go b/internal/controller/gitbranch_controller.go index 608f934a..a1d8b797 100644 --- a/internal/controller/gitbranch_controller.go +++ b/internal/controller/gitbranch_controller.go @@ -57,11 +57,11 @@ const ( // GitBranchReasonParentBranchNotFound is the stall for a branch that cannot be created: the // remote carries neither it nor the parent it would be created from. GitBranchReasonParentBranchNotFound = "ParentBranchNotFound" - // GitBranchReasonDefaultBranchNotAllowed is the stall for a write branch in an empty repository + // GitBranchReasonDefaultBranchCreationNotAllowed is the stall for a write branch in an empty repository // whose default branch the branch worker would create first, with one empty commit, for the // write branch to start from, but the GitProvider's spec.allowedGitBranches does not allow this // GitBranch to write it. - GitBranchReasonDefaultBranchNotAllowed = "DefaultBranchNotAllowed" + GitBranchReasonDefaultBranchCreationNotAllowed = "DefaultBranchCreationNotAllowed" // GitBranchReasonRecoveringParentBranch is progress while the parent branch has been found again // but the branch worker has not yet published the writes it kept while the parent was missing. GitBranchReasonRecoveringParentBranch = "RecoveringParentBranch" @@ -307,7 +307,7 @@ func (r *GitBranchReconciler) gateUnbornDefault( if allowed { return } - rd.stalled(GitBranchReasonDefaultBranchNotAllowed, fmt.Sprintf( + rd.stalled(GitBranchReasonDefaultBranchCreationNotAllowed, fmt.Sprintf( "The repository is empty and its default branch '%s' does not exist yet. Branch '%s' starts from "+ "it, so it is created first with one empty commit, but GitProvider '%s' does not allow this "+ "GitBranch to write '%s' (spec.allowedGitBranches). Allow '%s' there, or push a first commit to "+ diff --git a/internal/controller/gitbranch_controller_test.go b/internal/controller/gitbranch_controller_test.go index 34f1070f..085720f5 100644 --- a/internal/controller/gitbranch_controller_test.go +++ b/internal/controller/gitbranch_controller_test.go @@ -354,7 +354,7 @@ func TestGitBranch_AnUnresolvedDefaultBranchStallsToo(t *testing.T) { // TestGitBranch_AnEmptyRepositorysDefaultBranchMustBeAllowed: in an empty repository whose HEAD // names main, a write branch that is not main starts from main, which is created first. A GitBranch -// whose provider does not allow it to write main stalls with DefaultBranchNotAllowed; one that may, +// whose provider does not allow it to write main stalls with DefaultBranchCreationNotAllowed; one that may, // or whose write branch is main, does not. func TestGitBranch_AnEmptyRepositorysDefaultBranchMustBeAllowed(t *testing.T) { for name, tc := range map[string]struct { @@ -384,10 +384,10 @@ func TestGitBranch_AnEmptyRepositorysDefaultBranchMustBeAllowed(t *testing.T) { assert.Equal(t, configbutleraiv1alpha3.GitBranchParentUnborn, got.Status.Remote.Parent.State) assert.Equal(t, "main", got.Status.Remote.Parent.Branch) if !tc.wantStall { - assert.NotEqual(t, GitBranchReasonDefaultBranchNotAllowed, readyOf(got).Reason) + assert.NotEqual(t, GitBranchReasonDefaultBranchCreationNotAllowed, readyOf(got).Reason) return } - assert.Equal(t, GitBranchReasonDefaultBranchNotAllowed, readyOf(got).Reason) + assert.Equal(t, GitBranchReasonDefaultBranchCreationNotAllowed, readyOf(got).Reason) assert.Equal(t, metav1.ConditionFalse, readyOf(got).Status) assert.True(t, conditionIsTrue(got.Status.Conditions, ConditionTypeStalled)) assert.Contains(t, readyOf(got).Message, "default branch 'main'") diff --git a/internal/git/branch_worker.go b/internal/git/branch_worker.go index f9fa3f92..61573c2a 100644 --- a/internal/git/branch_worker.go +++ b/internal/git/branch_worker.go @@ -2827,7 +2827,7 @@ func (w *BranchWorker) noteFetchParent(parent parentConfig, err error) { return } var unresolved *DefaultBranchUnresolvedError - var notAllowed *DefaultBranchNotAllowedError + var notAllowed *DefaultBranchCreationNotAllowedError switch { case errors.As(err, &unresolved): w.recordMissingParent(parent.name, unresolved.Head) diff --git a/internal/git/parent_recovery.go b/internal/git/parent_recovery.go index e936f71e..7ffa1fbb 100644 --- a/internal/git/parent_recovery.go +++ b/internal/git/parent_recovery.go @@ -41,7 +41,7 @@ var errAwaitingParentProbe = fmt.Errorf("%w (unchanged since the last look; wait // repository's default branch that this GitBranch may not create. func isParentUnavailable(err error) bool { return errors.Is(err, ErrParentBranchNotFound) || errors.Is(err, ErrDefaultBranchUnresolved) || - errors.Is(err, ErrDefaultBranchNotAllowed) + errors.Is(err, ErrDefaultBranchCreationNotAllowed) } // parentRecovery is the loop's latch on parent recovery. Loop-goroutine only. diff --git a/internal/git/unborn_default_branch.go b/internal/git/unborn_default_branch.go index 4b1fe643..a6b982fd 100644 --- a/internal/git/unborn_default_branch.go +++ b/internal/git/unborn_default_branch.go @@ -26,29 +26,29 @@ import ( // // Creating the default branch is a write to a branch other than spec.branch, so it needs the same // permission: the GitProvider's spec.allowedGitBranches must allow this GitBranch to write it. -// Without that, nothing is written and the GitBranch reports DefaultBranchNotAllowed until the +// Without that, nothing is written and the GitBranch reports DefaultBranchCreationNotAllowed until the // branch is allowed or exists. A server that does not report the unborn name (protocol v0/v1) gives // nothing to create; the write branch then starts as the first branch, as before. -// ErrDefaultBranchNotAllowed reports an empty repository whose default branch the worker would +// ErrDefaultBranchCreationNotAllowed reports an empty repository whose default branch the worker would // create for the write branch to start from, but the GitProvider does not allow this GitBranch to -// write it. DefaultBranchNotAllowedError names the branch. -var ErrDefaultBranchNotAllowed = errors.New("the empty repository's default branch may not be created") +// write it. DefaultBranchCreationNotAllowedError names the branch. +var ErrDefaultBranchCreationNotAllowed = errors.New("the empty repository's default branch may not be created") -// DefaultBranchNotAllowedError is ErrDefaultBranchNotAllowed with the branch the server named. -type DefaultBranchNotAllowedError struct { +// DefaultBranchCreationNotAllowedError is ErrDefaultBranchCreationNotAllowed with the branch the server named. +type DefaultBranchCreationNotAllowedError struct { // Branch is the short name of the unborn default branch. Branch string } -func (e *DefaultBranchNotAllowedError) Error() string { +func (e *DefaultBranchCreationNotAllowedError) Error() string { return fmt.Sprintf("the repository is empty and its default branch %q does not exist; "+ "spec.allowedGitBranches does not allow creating it", e.Branch) } -// Is makes the error match ErrDefaultBranchNotAllowed. -func (e *DefaultBranchNotAllowedError) Is(target error) bool { - return target == ErrDefaultBranchNotAllowed +// Is makes the error match ErrDefaultBranchCreationNotAllowed. +func (e *DefaultBranchCreationNotAllowedError) Is(target error) bool { + return target == ErrDefaultBranchCreationNotAllowed } // initializeCommitMessage is the message of the empty commit that creates the default branch. @@ -80,7 +80,7 @@ func (w *BranchWorker) mayCreateUnbornDefault( // createUnbornDefault creates branch on the remote with one empty commit, signed and authored as the // worker's other commits are, from a checkout that a sync just left unborn (an empty index). It is -// refused with DefaultBranchNotAllowedError when the GitBranch may not write branch. +// refused with DefaultBranchCreationNotAllowedError when the GitBranch may not write branch. // // The push's own failure comes back as an unbornDefaultPushError: a push refused because somebody // else created branch meanwhile (the advertisement, or the server's compare-and-swap at upload) is @@ -97,7 +97,7 @@ func (w *BranchWorker) createUnbornDefault( return fmt.Errorf("check spec.allowedGitBranches for %q: %w", branch, err) } if !allowed { - return &DefaultBranchNotAllowedError{Branch: branch} + return &DefaultBranchCreationNotAllowedError{Branch: branch} } signer, err := getCommitSigner(ctx, w.Client, provider) if err != nil { diff --git a/internal/git/unborn_head_test.go b/internal/git/unborn_head_test.go index 3dbcdeee..b745ffe5 100644 --- a/internal/git/unborn_head_test.go +++ b/internal/git/unborn_head_test.go @@ -159,7 +159,7 @@ func TestBranchWorker_AnEmptyRepositoryWaitsUntilItsDefaultBranchIsAllowed(t *te f.allowBranches("feature") err := f.worker.ensureRepositoryInitialized(f.worker.ctx) - require.ErrorIs(t, err, ErrDefaultBranchNotAllowed) + require.ErrorIs(t, err, ErrDefaultBranchCreationNotAllowed) o := f.observed() assert.Equal(t, ParentUnborn, o.ParentState) assert.Equal(t, "trunk", o.ParentBranch) diff --git a/test/e2e/new_branch_parent_e2e_test.go b/test/e2e/new_branch_parent_e2e_test.go index 21270e63..e0bf0112 100644 --- a/test/e2e/new_branch_parent_e2e_test.go +++ b/test/e2e/new_branch_parent_e2e_test.go @@ -202,7 +202,7 @@ func describeMissingParentRecoverySpec() { // describeEmptyRepositoryBootstrapSpec: an empty Gitea repository's HEAD names main, and Gitea // speaks protocol v2, so the GitBranch knows it. The write branch is not main, so it starts from // main, which is created first with one empty commit. Creating it is a write to main: while the -// GitProvider does not allow that, the GitBranch reports DefaultBranchNotAllowed, its GitTarget +// GitProvider does not allow that, the GitBranch reports DefaultBranchCreationNotAllowed, its GitTarget // GitBranchNotReady, and nothing is written. Allowing main is the whole fix: the held edit lands on // the write branch, on top of main's empty first commit. func describeEmptyRepositoryBootstrapSpec() { @@ -271,7 +271,7 @@ spec: g.Expect(gitBranchRemoteField(g, branchName, testNs, "parent.state")).To(Equal("Unborn")) g.Expect(gitBranchRemoteField(g, branchName, testNs, "parent.branch")).To(Equal("main"), "Gitea answers protocol v2 ls-refs unborn") - g.Expect(gitBranchReadyReasonOf(g, branchName, testNs)).To(Equal("DefaultBranchNotAllowed")) + g.Expect(gitBranchReadyReasonOf(g, branchName, testNs)).To(Equal("DefaultBranchCreationNotAllowed")) g.Expect(readyReasonOf(g, destName, testNs)).To(Equal("GitBranchNotReady")) }, 3*time.Minute, 3*time.Second).Should(Succeed()) Expect(remoteBranchHeadOf(Default, repo.CheckoutDir, "main")).To(BeEmpty(), "main is not created") @@ -299,7 +299,7 @@ spec: Eventually(func(g Gomega) { g.Expect(gitBranchReadyReasonOf(g, branchName, testNs)).NotTo( - BeElementOf("DefaultBranchNotAllowed", "RecoveringParentBranch")) + BeElementOf("DefaultBranchCreationNotAllowed", "RecoveringParentBranch")) g.Expect(readyReasonOf(g, destName, testNs)).NotTo(Equal("GitBranchNotReady")) }, 2*time.Minute, 3*time.Second).Should(Succeed()) }) diff --git a/test/e2e/provider_access_e2e_test.go b/test/e2e/provider_access_e2e_test.go index f794029d..1e3bd3d6 100644 --- a/test/e2e/provider_access_e2e_test.go +++ b/test/e2e/provider_access_e2e_test.go @@ -48,7 +48,7 @@ var _ = Describe("Manager branch grants", Label("manager"), Ordered, func() { // The platform owns main and initializes it. A tenant may write only teams//*, so in an // empty repository it could not create the default branch its branch starts from, and its - // GitBranch would report DefaultBranchNotAllowed. + // GitBranch would report DefaultBranchCreationNotAllowed. By("the platform pushes a first commit to main") commitFilesToBranchFromOutside(repo, platformNs, "main", "e2e: the platform initializes main", map[string]string{"README.md": "shared repository\n"}) From 6039e7a9566538334ef81f03786a3ed9dd9314c2 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 10 Nov 2026 13:14:34 +0000 Subject: [PATCH 09/10] fix(git): retry a failed unborn query, and never parent the first commit on a leftover branch From CodeRabbit's review of #480: - unbornHead read a failed ls-refs on a server that offers "unborn" as "the server does not say". Now that the name decides what the worker writes, a transient failure would have started the write branch as the repository's first branch. It is an error now, so the cycle retries; a server that does not offer "unborn" still answers with no name. - createUnbornDefault pointed HEAD at a local default branch it did not clear. One left by a failed earlier attempt, or by history fetched before the remote was emptied, became the first commit's parent and was uploaded with it. The local ref is removed first, so the commit is always a root. - The test that creates the default branch now signs it and checks the signature is there. Each new test was checked to fail without its fix. Co-Authored-By: Claude Opus 5.5 --- internal/git/git_smart_fetch.go | 22 +++++---- internal/git/unborn_default_branch.go | 7 +++ internal/git/unborn_head_test.go | 65 +++++++++++++++++++++++++++ 3 files changed, 85 insertions(+), 9 deletions(-) diff --git a/internal/git/git_smart_fetch.go b/internal/git/git_smart_fetch.go index 38687962..178c34a9 100644 --- a/internal/git/git_smart_fetch.go +++ b/internal/git/git_smart_fetch.go @@ -243,7 +243,8 @@ func advertise( advertised, err := session.GetRemoteRefs(ctx, nil) if errors.Is(err, transport.ErrEmptyRemoteRepository) { - return nil, unbornHead(ctx, session), nil + head, err := unbornHead(ctx, session) + return nil, head, err } if err != nil { return nil, "", err @@ -259,32 +260,35 @@ func advertise( // unbornHead is the branch an empty repository's HEAD names, read from a protocol v2 ls-refs with // the "unborn" argument (git 2.31 and later; GitHub answers it). It is empty when the session -// speaks v0 or v1, when the server does not offer "unborn", when the call fails, and when the -// repository is no longer empty by the time it answers: the name is evidence, never a guess. +// speaks v0 or v1, when the server does not offer "unborn", and when the repository is no longer +// empty by the time it answers: the name is evidence, never a guess. +// +// A server that offers "unborn" and then fails the call is an error, not an empty answer: the name +// decides what the worker writes (unborn_default_branch.go), and reading a transient failure as +// "the server does not say" would start the write branch as the repository's first branch. // // go-git sends the same request inside GetRemoteRefs and then discards the answer for an empty // repository, so this asks again through its public Commander. Delete it once a go-git release // reports the unborn HEAD of an empty repository. -func unbornHead(ctx context.Context, session transport.Session) plumbing.ReferenceName { +func unbornHead(ctx context.Context, session transport.Session) (plumbing.ReferenceName, error) { commander, ok := session.(transport.Commander) if !ok || !slices.Contains(session.Capabilities().Get(capability.LsRefs), lsRefsUnborn) { - return "" + return "", nil } out := &packp.LsRefsOutput{} if err := commander.Command(ctx, "ls-refs", &packp.LsRefsArgs{Symrefs: true, Unborn: true}, out); err != nil { - log.FromContext(ctx).V(1).Info("Could not read an empty repository's unborn HEAD", "error", err.Error()) - return "" + return "", fmt.Errorf("read the empty repository's unborn HEAD: %w", err) } var head plumbing.ReferenceName for _, ref := range out.References { switch { case ref.Type() == plumbing.HashReference: - return "" // it gained a ref since the advertisement: HEAD may well be born now + return "", nil // it gained a ref since the advertisement: HEAD may well be born now case ref.Name() == plumbing.HEAD && ref.Target().IsBranch(): head = ref.Target() } } - return head + return head, nil } // lsRefsUnborn is the ls-refs feature a server offers when it reports an unborn HEAD. Sending the diff --git a/internal/git/unborn_default_branch.go b/internal/git/unborn_default_branch.go index a6b982fd..c360d0d4 100644 --- a/internal/git/unborn_default_branch.go +++ b/internal/git/unborn_default_branch.go @@ -104,6 +104,13 @@ func (w *BranchWorker) createUnbornDefault( return err } + // The first commit has no parent, whatever the checkout holds: a local branch left by an + // earlier attempt whose push failed, or by history fetched before the remote was emptied, + // would become its parent and be uploaded with it. + local := plumbing.NewBranchReferenceName(branch) + if err := repo.Storer.RemoveReference(local); err != nil && !errors.Is(err, plumbing.ErrReferenceNotFound) { + return fmt.Errorf("clear the local %q: %w", branch, err) + } if err := setHead(repo, branch); err != nil { return fmt.Errorf("point HEAD at %q: %w", branch, err) } diff --git a/internal/git/unborn_head_test.go b/internal/git/unborn_head_test.go index b745ffe5..b6a5c007 100644 --- a/internal/git/unborn_head_test.go +++ b/internal/git/unborn_head_test.go @@ -9,14 +9,20 @@ package git import ( "context" + "net/http" "os/exec" "path/filepath" + "strings" + "sync/atomic" "testing" "github.com/go-git/go-git/v6/plumbing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/fluxcd/pkg/apis/meta" + corev1 "k8s.io/api/core/v1" + configv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" ) @@ -106,6 +112,22 @@ func (f *realServerNewBranch) allowBranches(patterns ...configv1alpha3.BranchPat require.NoError(f.t, f.worker.Client.Update(f.worker.ctx, provider)) } +// signCommits makes the fixture's GitProvider sign its commits with a fresh SSH key. +func (f *realServerNewBranch) signCommits() { + f.t.Helper() + privateKey, _, err := GenerateSSHSigningKeyPair(nil) + require.NoError(f.t, err) + secret := &corev1.Secret{Data: map[string][]byte{signingKeyDataKey: privateKey}} + secret.Name, secret.Namespace = "signing-key", "default" + require.NoError(f.t, f.worker.Client.Create(f.worker.ctx, secret)) + provider, err := f.worker.getGitProvider(f.worker.ctx) + require.NoError(f.t, err) + provider.Spec.Commit = &configv1alpha3.CommitSpec{ + Signing: &configv1alpha3.CommitSigningSpec{SecretRef: meta.LocalObjectReference{Name: secret.Name}}, + } + require.NoError(f.t, f.worker.Client.Update(f.worker.ctx, provider)) +} + // emptyTrunkRepoOverV2 is an empty repository whose HEAD names trunk, served over v2, with the // write branch feature. func emptyTrunkRepoOverV2(t *testing.T, slug string) *realServerNewBranch { @@ -139,6 +161,7 @@ func (f *realServerNewBranch) assertFeatureStartsOnInitializedTrunk() { func TestBranchWorker_AnEmptyRepositoryGetsItsDefaultBranchFirst(t *testing.T) { f := emptyTrunkRepoOverV2(t, "unborn-default-created") f.allowBranches("trunk", "feature") + f.signCommits() require.NoError(t, f.worker.ensureRepositoryInitialized(f.worker.ctx)) o := f.observed() @@ -147,6 +170,9 @@ func TestBranchWorker_AnEmptyRepositoryGetsItsDefaultBranchFirst(t *testing.T) { assert.Equal(t, f.ref("trunk").String(), o.ParentCommit) assert.True(t, f.ref("feature").IsZero(), "nothing written yet") + assert.Contains(t, gitIn(t, f.repoDir, "cat-file", "-p", "trunk"), "gpgsig -----BEGIN SSH SIGNATURE-----", + "the first commit is signed like every other commit") + liveWrite(f.loop, "cm1") f.assertFeatureStartsOnInitializedTrunk() } @@ -187,6 +213,45 @@ func TestBranchWorker_AnEmptyRepositoryWaitsUntilItsDefaultBranchIsAllowed(t *te assert.Empty(t, f.loop.pendingWrites, "the held write landed") } +// A server that offers "unborn" and then fails the query is an error, so the cycle retries: read as +// "the server does not say", it would start the write branch as the repository's first branch. +func TestListRemoteRefs_AFailedUnbornQueryIsAnError(t *testing.T) { + sim := emptyRepoServedOver(t, true) + var posts atomic.Int32 + around := func(w http.ResponseWriter, r *http.Request, backend http.Handler) { + if r.Method == http.MethodPost && strings.HasSuffix(r.URL.Path, "/git-upload-pack") && posts.Add(1) == 2 { + http.Error(w, "unavailable", http.StatusServiceUnavailable) + return + } + backend.ServeHTTP(w, r) + } + sim.around.Store(&around) + + _, unborn, err := listRemoteRefs(context.Background(), sim.RepoURL, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "unborn HEAD") + assert.Empty(t, unborn) +} + +// A local default branch the checkout still holds (an earlier attempt whose push failed, or history +// fetched before the remote was emptied) is never the first commit's parent, and is not uploaded. +func TestBranchWorker_TheDefaultBranchsFirstCommitIgnoresALeftoverLocalBranch(t *testing.T) { + f := emptyTrunkRepoOverV2(t, "unborn-default-leftover") + f.allowBranches("feature") + require.ErrorIs(t, f.worker.ensureRepositoryInitialized(f.worker.ctx), ErrDefaultBranchCreationNotAllowed) + liveWrite(f.loop, "cm1") + require.Len(t, f.loop.pendingWrites, 1) + + leftover := branchOnDisk(t, f.worker.repoPath(), "trunk", "never on the remote\n") + + f.allowBranches("trunk", "feature") + pastProbeDeadline(f.worker) + f.loop.runParentProbe() + f.assertFeatureStartsOnInitializedTrunk() + assert.Error(t, exec.Command("git", "-C", f.repoDir, "cat-file", "-e", leftover.String()).Run(), + "the leftover commit was not uploaded") +} + // Somebody else creates the default branch between our advertisement and our upload. The server // refuses our first commit, and the write branch starts from theirs instead. func TestBranchWorker_AnEmptyRepositoryWhoseDefaultBranchAppearsDuringOurPushStartsFromIt(t *testing.T) { From 9e991627bcb391e289c311a64ddec873347f8cb8 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 10 Nov 2026 13:25:02 +0000 Subject: [PATCH 10/10] test(git): show the default branch is created when work is picked up, and a hand-made one is used Two tests and one sentence per document make the timing explicit: - An empty repository's default branch is created as soon as the worker picks up work for the branch, even work that changes nothing; a refresh only looks, and spec.branch still waits for a change. - While the GitBranch may not create it, nothing is written; a default branch somebody pushes by hand is then used as it is, and the held write lands on it with no initialize commit of ours. Co-Authored-By: Claude Opus 5.5 --- docs/UPGRADING.md | 7 +-- docs/configuration.md | 5 +- .../gittarget-parent-empty-repository.md | 6 +++ internal/git/unborn_head_test.go | 49 +++++++++++++++++++ 4 files changed, 63 insertions(+), 4 deletions(-) diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index ab6d76ac..79976bf3 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -12,9 +12,10 @@ We are pre-1.0, so breaking changes bump the **minor** version (release-please i In an empty repository with `spec.parentBranch` omitted, a `GitBranch` whose `spec.branch` is not the repository's default branch no longer creates `spec.branch` as the repository's first branch. The default branch the server names (`main` on GitHub, Gitea and Forgejo) is created first, with one -empty commit authored and signed like the provider's other commits, and `spec.branch` starts from -it. On Gitea and Forgejo the first branch pushed became the default branch, so a review branch used -to become the default, and later branches built on it. +empty commit authored and signed like the provider's other commits, as soon as the worker picks up +work for the branch, and `spec.branch` starts from it once there is a change. On Gitea and Forgejo +the first branch pushed became the default branch, so a review branch used to become the default, +and later branches built on it. That is a write to the default branch, so the provider's `spec.allowedGitBranches` must allow it for the `GitBranch`'s namespace. If it does not, the `GitBranch` reports `Ready=False`, `Stalled=True`, diff --git a/docs/configuration.md b/docs/configuration.md index 881f4718..d5ad6497 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -1561,7 +1561,10 @@ spec: no history. When `spec.branch` is another branch, the default branch is created first, with one empty commit authored and signed like every other commit, and `spec.branch` starts from it: the history starts on the default branch, which stays the default, and the first pull request into it - carries everything written. That is a write to the default branch, so the provider's + carries everything written. The default branch is created as soon as the worker picks up work for + the branch, even work that changes nothing, so a misconfigured credential or signing key shows up + at setup; `spec.branch` still waits for a change + ([the tests](../internal/git/unborn_head_test.go)). That is a write to the default branch, so the provider's `allowedGitBranches` must allow it for this namespace (`branchPatterns: ["main", "reverser/edits"]`). Without that the `GitBranch` reports `DefaultBranchCreationNotAllowed`, its `GitTarget`s report `GitBranchNotReady`, and nothing is written until the branch is allowed or someone pushes a first diff --git a/docs/design/gittarget-parent-empty-repository.md b/docs/design/gittarget-parent-empty-repository.md index 313edeee..64fc88f3 100644 --- a/docs/design/gittarget-parent-empty-repository.md +++ b/docs/design/gittarget-parent-empty-repository.md @@ -35,6 +35,12 @@ With `spec.parentBranch` omitted, in an empty repository: compare-and-swap on the branch being absent, before anything is written to `spec.branch`. After it, the repository is no longer empty, and `spec.branch` is an ordinary absent branch whose parent is `Found`. +- **When:** as soon as the worker picks up work for the branch, even work that changes nothing; a + refresh only looks. It prepares the base the work builds on, and a bad credential, signing key or + branch protection fails at setup instead of at the first change. `spec.branch` still waits for a + change. A default branch somebody creates by hand meanwhile is used as it is + (`TestBranchWorker_TheDefaultBranchIsCreatedWhenWorkIsPickedUp`, + `TestBranchWorker_ADefaultBranchCreatedByHandIsPickedUp`). - **Permission is the existing allowlist.** Creating the default branch is a write to a branch other than `spec.branch`, so the provider's `spec.allowedGitBranches` must allow this `GitBranch`'s namespace to write it, as it would for a `GitBranch` on that branch. There is no setting to opt in diff --git a/internal/git/unborn_head_test.go b/internal/git/unborn_head_test.go index b6a5c007..2725d2dd 100644 --- a/internal/git/unborn_head_test.go +++ b/internal/git/unborn_head_test.go @@ -15,6 +15,7 @@ import ( "strings" "sync/atomic" "testing" + "time" "github.com/go-git/go-git/v6/plumbing" "github.com/stretchr/testify/assert" @@ -177,6 +178,54 @@ func TestBranchWorker_AnEmptyRepositoryGetsItsDefaultBranchFirst(t *testing.T) { f.assertFeatureStartsOnInitializedTrunk() } +// The default branch is prepared when the worker picks up work for the branch, before any change: +// a write that changes nothing creates it, and the write branch still waits for a change. A refresh +// only looks. +func TestBranchWorker_TheDefaultBranchIsCreatedWhenWorkIsPickedUp(t *testing.T) { + f := emptyTrunkRepoOverV2(t, "unborn-default-eager") + f.allowBranches("trunk", "feature") + + f.loop.handleRefreshRequest(&RefreshRequest{Target: refreshTarget(), MaxAge: time.Nanosecond}) + assert.True(t, f.ref("trunk").IsZero(), "a refresh only looks") + assert.Equal(t, ParentUnborn, f.observed().ParentState) + + deleted := configMapTargetEvent("never-written", "alice", newBranchTarget) + deleted.Operation = "DELETE" + deleted.Object = nil + f.loop.handleQueueItem(WorkItem{Request: &WriteRequest{Events: []Event{deleted}}}) + + trunk := f.ref("trunk") + require.False(t, trunk.IsZero(), "work picked up, nothing changed: trunk is created") + assert.Empty(t, f.parentOf(trunk)) + assert.True(t, f.ref("feature").IsZero(), "the write branch waits for a change") + assert.Equal(t, ParentFound, f.observed().ParentState) +} + +// A default branch somebody creates by hand, while this GitBranch may not create it, is enough: the +// held write lands on their branch, and nothing of ours is added to it. +func TestBranchWorker_ADefaultBranchCreatedByHandIsPickedUp(t *testing.T) { + f := emptyTrunkRepoOverV2(t, "unborn-default-by-hand") + f.allowBranches("feature") + + liveWrite(f.loop, "cm1") + assert.True(t, f.ref("trunk").IsZero(), "not allowed: trunk is not created") + assert.True(t, f.ref("feature").IsZero(), "nothing is written") + require.Len(t, f.loop.pendingWrites, 1, "the write is held") + assert.Equal(t, ParentUnborn, f.observed().ParentState) + assert.Equal(t, "trunk", f.observed().ParentBranch) + + theirs := simulateClientCommitOnDisk(t, f.repoDir, "trunk", "README.md", "created by hand\n") + pastProbeDeadline(f.worker) + f.loop.runParentProbe() + + assert.Equal(t, theirs, f.ref("trunk"), "their trunk, untouched") + require.False(t, f.ref("feature").IsZero(), "the held write is published") + assert.Equal(t, theirs.String(), gitIn(t, f.repoDir, "merge-base", "feature", "trunk"), "on their trunk") + assert.Contains(t, gitIn(t, f.repoDir, "ls-tree", "-r", "--name-only", "feature"), cmFile("cm1")) + assert.NotContains(t, gitIn(t, f.repoDir, "log", "--format=%s", "feature"), "chore: initialize repository") + assert.Empty(t, f.loop.pendingWrites) +} + // The GitBranch may not write the default branch: nothing is written, the repository is reported // Unborn with the name, and the write is held. Allowing the branch is enough: the next probe creates // it and the held write lands on it.