diff --git a/api/v1alpha3/gitbranch_types.go b/api/v1alpha3/gitbranch_types.go index ad454b9e..e7c41661 100644 --- a/api/v1alpha3/gitbranch_types.go +++ b/api/v1alpha3/gitbranch_types.go @@ -48,7 +48,15 @@ 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 + // 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, @@ -171,7 +179,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,14 +189,15 @@ 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"` // 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..7242098d 100644 --- a/config/crd/bases/configbutler.ai_gitbranches.yaml +++ b/config/crd/bases/configbutler.ai_gitbranches.yaml @@ -252,7 +252,15 @@ 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 + 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, @@ -391,8 +399,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: |- @@ -403,8 +412,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 415a9d60..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) | **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) | **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/TODO.md b/docs/TODO.md index 740bec86..98a50a92 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -135,15 +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). -- [ ] **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). +- [ ] **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 (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/UPGRADING.md b/docs/UPGRADING.md index 76c07e57..79976bf3 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -7,6 +7,32 @@ 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, 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`, +reason `DefaultBranchCreationNotAllowed`, 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/architecture.md b/docs/architecture.md index 659ccbd1..68816d54 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 (`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. @@ -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 ddf9ae0e..d5ad6497 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 `DefaultBranchCreationNotAllowed` 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` | | @@ -1243,6 +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`) | +| `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` @@ -1551,7 +1557,20 @@ 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) 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. 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 + 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 @@ -1714,7 +1733,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). 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 b3855a11..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): default-branch protocol discovery - and authorization to bootstrap it remain separate decisions. +- [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 374ca647..7cdadaac 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 | +| 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 @@ -148,9 +148,12 @@ 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 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 `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 58aa02df..64fc88f3 100644 --- a/docs/design/gittarget-parent-empty-repository.md +++ b/docs/design/gittarget-parent-empty-repository.md @@ -1,198 +1,94 @@ -# GitTarget parent branch: an empty repository's default branch +# An empty repository's first commit -> **design**: deferred; not part of #407. The experiment in §2 is done, nothing else is built. -> Dated 2026-10-01. +> **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 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, and unchanged on -upstream main at `fc18716c`: - -- 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 - -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. - -## 5. Steps once go-git carries it - -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. - -## 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 `. +> 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. 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. + +## 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 `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 + (`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`. +- **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 + 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/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/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..a1d8b797 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" + // 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. + 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" @@ -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(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 "+ + "'%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..085720f5 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 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 { + 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, GitBranchReasonDefaultBranchCreationNotAllowed, readyOf(got).Reason) + return + } + 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'") + 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/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..61573c2a 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 *DefaultBranchCreationNotAllowedError 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) } @@ -3179,10 +3198,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 +3220,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 +3233,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. @@ -3349,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/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..178c34a9 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,95 @@ 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 + head, err := unbornHead(ctx, session) + return nil, head, err } 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", 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, error) { + commander, ok := session.(transport.Commander) + if !ok || !slices.Contains(session.Capabilities().Get(capability.LsRefs), lsRefsUnborn) { + return "", nil } - return refs, nil + out := &packp.LsRefsOutput{} + if err := commander.Command(ctx, "ls-refs", &packp.LsRefsArgs{Symrefs: true, Unborn: true}, out); err != nil { + 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 "", 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, nil } +// 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/parent_recovery.go b/internal/git/parent_recovery.go index f175f53f..7ffa1fbb 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, ErrDefaultBranchCreationNotAllowed) } // 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/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_default_branch.go b/internal/git/unborn_default_branch.go new file mode 100644 index 00000000..c360d0d4 --- /dev/null +++ b/internal/git/unborn_default_branch.go @@ -0,0 +1,187 @@ +// 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 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. + +// 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. DefaultBranchCreationNotAllowedError names the branch. +var ErrDefaultBranchCreationNotAllowed = errors.New("the empty repository's default branch may not be created") + +// 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 *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 ErrDefaultBranchCreationNotAllowed. +func (e *DefaultBranchCreationNotAllowedError) Is(target error) bool { + return target == ErrDefaultBranchCreationNotAllowed +} + +// 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 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 +// 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 &DefaultBranchCreationNotAllowedError{Branch: branch} + } + signer, err := getCommitSigner(ctx, w.Client, provider) + if err != nil { + 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) + } + 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 new file mode 100644 index 00000000..2725d2dd --- /dev/null +++ b/internal/git/unborn_head_test.go @@ -0,0 +1,375 @@ +// 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" + "net/http" + "os/exec" + "path/filepath" + "strings" + "sync/atomic" + "testing" + "time" + + "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" +) + +// 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, "-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) + + 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") +} + +// 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)) +} + +// 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 { + 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") + f.signCommits() + + 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") + + 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() +} + +// 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. +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, ErrDefaultBranchCreationNotAllowed) + o := f.observed() + assert.Equal(t, ParentUnborn, o.ParentState) + assert.Equal(t, "trunk", o.ParentBranch) + + 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) + + 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") +} + +// 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) { + 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)) + assert.Equal(t, ParentUnborn, f.observed().ParentState) + assert.Equal(t, "feature", f.observed().ParentBranch) + + 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 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") + }) + + 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 +// 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) +} diff --git a/test/e2e/new_branch_parent_e2e_test.go b/test/e2e/new_branch_parent_e2e_test.go index 9e24fc2d..e0bf0112 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, Label("empty-repo"), func() { + describeEmptyRepositoryBootstrapSpec() + }) }) const parentBranchSelectorLabel = "e2e.configbutler.ai/parent-branch" @@ -196,6 +199,112 @@ 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 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() { + const ( + providerName = "empty-repo-provider" + destName = "empty-repo-dest" + ruleName = "empty-repo-rule" + gitPath = "apps/empty-repo" + ) + var ( + testNs string + repo *RepoArtifacts + 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) + 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) + applyProvider(writeBranch) + }) + + AfterAll(func() { + cleanupWatchRule(ruleName, testNs) + cleanupGitTarget(destName, testNs) + deleteGitProvider(testNs, providerName) + cleanupNamespace(testNs) + }) + + 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("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("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") + Expect(remoteBranchHeadOf(Default, repo.CheckoutDir, writeBranch)).To(BeEmpty(), "nothing is written") + + 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 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.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 held edit is on the write branch") + }, 5*time.Minute, 5*time.Second).Should(Succeed()) + + Eventually(func(g Gomega) { + g.Expect(gitBranchReadyReasonOf(g, branchName, testNs)).NotTo( + BeElementOf("DefaultBranchCreationNotAllowed", "RecoveringParentBranch")) + g.Expect(readyReasonOf(g, destName, testNs)).NotTo(Equal("GitBranchNotReady")) + }, 2*time.Minute, 3*time.Second).Should(Succeed()) + }) +} + func describeNewWriteBranchSpec(parentBranch, parent, slug string) { const ( providerName = "parent-provider" diff --git a/test/e2e/provider_access_e2e_test.go b/test/e2e/provider_access_e2e_test.go index fec19109..1e3bd3d6 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 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"}) + By("creating the platform's GitProvider: main for itself, teams/{consumer.namespace}/* for tenants") _, err = kubectlRunWithStdin("", fmt.Sprintf(`apiVersion: configbutler.ai/v1alpha3 kind: GitProvider