feat: a git change provider, and a git rung that reads its own repository - #605
Open
behinddwalls wants to merge 4 commits into
Open
feat: a git change provider, and a git rung that reads its own repository#605behinddwalls wants to merge 4 commits into
behinddwalls wants to merge 4 commits into
Conversation
## Summary ### Why? Five test files each carried their own copy of the same twenty lines: read `SUBMITQUEUE_TEST_GIT`, notice that rules_go expanded `$(location)` to an execroot-relative path, and re-root it under `TEST_SRCDIR` so the binary can actually be executed. They had already drifted. Two spellings handled a leading `external/` as well as an embedded `/external/`; three handled only the embedded form. Two verified the resolved path with `os.Stat`; one returned it unchecked. Two were named for what they returned (`testGit`, `pinnedGit`), two for how they worked (`runfilePath`, `absoluteTestPath`). One carried a comment pointing at another copy as the explanation for why any of it was necessary. None of that is behaviour anyone chose — it is what five independent transcriptions of the same workaround look like after a while. The next test that shells out to git was going to be a sixth. ### What? `platform/gitexec/gitexectest` — `Git(t)` for the common case, `Runfile(t, name)` for a target that pins something else as well, which today is the merger's commit-template directory. Colocated with `gitexec` and named for it, following `httptest`: the thing a caller wants is the binary `gitexec` will run, and the runfiles indirection is Bazel's business rather than theirs. The promoted implementation is the most complete of the five (`service/runway/server`'s): it accepts both spellings of an external path and stats what it resolves, so a misconfigured target fails saying so rather than handing back a path that does not exist. The three thinner copies gain that; nothing loses anything. Deliberately not in `test/testutil`. That package is the Docker Compose harness and pulls in the MySQL driver and gRPC — dependencies a unit test that only needs a git binary should not acquire to find one. Pure refactor: no test changes what it asserts, and no production code is touched. ## Test Plan - ✅ all five converted targets pass — `//tool/gitsandbox`, `//service/submitqueue/demo/requests`, `//runway/extension/merger/git`, `//service/runway/server`, and `make e2e-git-test` - ✅ `make fmt`, `make gazelle` The E2E is the one that matters here: it runs the pinned git both from the test process and inside the containers, so a helper that resolved the wrong path would fail it rather than silently fall back to the host's git. Local note, unrelated to this change: `make e2e-git-test` needs `--sandbox_writable_path=$HOME/.docker` on macOS, or the build fails on `open ~/.docker/buildx/activity/…: operation not permitted` before any test runs.
## Summary ### Why? Every change provider so far asks a service what a change contains. GitHub and Phabricator both have an API that already knows; `fake` invents an answer; `routing` picks between the first two. A plain git remote has no such service, so there has been no way to run the queue against one and have it know what a change touched. That gap is visible in the demo's git rung, where the merge is real and everything upstream of it is not. Because the fake provider cannot read a repository, `make demo-requests` writes the paths it committed onto the change URI itself (`sq-files=…`) and the fake reads them back, so the conflict analyzer has something to key on. It works for changes the demo creates and for nothing else: a change pushed by hand carries no marker and conflicts with nothing. It is also why change URIs run into the 255-byte storage limit and had to be budgeted down to one path per directory. The queue's own logic — batching, conflict analysis, scoring — is built on what a change touched. Deriving that from git makes a plain remote a first-class source rather than a rung where those features are simulated. ### What? `submitqueue/extension/changeprovider/git` keeps its own copy of a remote and computes each change from the commits: `--numstat` for files and line counts, the commit itself for the author. **The baseline chains through a stack, and this is the part worth reviewing.** A `git://` URI names a commit and a ref and nothing else — unlike a pull request it carries no base, so the baseline has to be derived. It cannot be the target branch: a stack's changes are cut one from the next, so measuring each against the target reports the second change as containing the first, and anything summing line counts across a batch counts them twice. The first change is measured from where it diverged from the target, each one after it from where it diverged from its predecessor. The order of `Change.URIs` is the stack order and is load-bearing. This is wrong by default and fails silently — no error, just inflated numbers and scores that look slightly off — so it is the first thing the tests pin. **Authentication is injected, never derived.** The provider takes an `Auth` implementation and calls it before each fetch. It never reads an environment variable, never encodes a token, never decides what a credential is. `tokenEnv` in configuration is one implementation of that interface, supplied by the wiring layer; an integrator with a secrets manager or short-lived minted tokens supplies a different one and nothing in the extension changes. Calling it per fetch rather than once is what lets an expiring credential be refreshed. A nil `Auth` means the remote needs none, which covers a local path and an SSH remote served by the host's own SSH config and agent. The environment a fetch needs to reach a remote is passed through; the configuration that could change what a diff says is not. **The copy is bare and its own.** Nothing is ever checked out — the provider answers questions about commits and produces none — so there is no working tree to leave dirty and no index to corrupt. It is independent of any checkout a merger keeps, which is the point: each service configures its own remote for a queue, and a bind-mounted bare repository and `https://github.com/…` are the same code path. Provisioning runs at wiring time rather than on first use. Resolving a provider happens once per message on the validate path, so provisioning there would put a clone inside a retry loop and hide an unreachable remote behind queue processing rather than failing the service that owns the configuration. Nothing is wired to this yet — no queue selects `type: git`, and the orchestrator image still has no git binary. Those are the next steps, kept separate so this one is reviewable on its own. ## Test Plan Hermetic tests driving the pinned `@git//:git` against throwaway repositories — a bare "remote", a working clone that authors changes, and the provider reading through its own third copy, which is the same three-way arrangement the deployment has. - ✅ **three-step stack reports per change, not cumulatively** — `pkg/a` / `pkg/b` / `pkg/c` and 1 / 2 / 3 lines, each change carrying only its own - ✅ **mutation-tested that assertion**: reverting the baseline to always use the target fails it on exactly the two claims it exists for ("the second change must not carry the first's files", "nor the third the first two's") and nothing else fails — so it is not passing by construction - ✅ a multi-commit change reports every file across its commits, not just its tip - ✅ a binary file is reported as touched with no line counts, rather than dropped or refused — `--numstat` gives `-` for both counts, which a plain `Atoi` would fail on - ✅ a rename is reported at its new path, which is the case that splits one record across extra NUL-delimited fields and that a naive parse turns into an empty path - ✅ a commit the copy has not seen is fetched — the normal case, since the copy is its own - ✅ an unknown commit, a malformed URI, and a change sharing no history with the target are each errors; the last is deliberately not reported as a change that touches everything - ✅ four concurrent `Get`s against one copy return the right answer each, exercising the shared lock - ✅ re-provisioning keeps objects already fetched - ✅ `make fmt`, `make gazelle`, `make lint` The rename token layout was the one thing not taken from documentation: the test against a real renamed file is what pinned it.
## Summary
### Why?
The previous change added a change provider that reads a git repository. Nothing could select it: `profiles.yaml` had no `type: git`, and the wiring had no case for one. This connects the two.
### What?
`changeProvider: {type: git, git: {…}}` in `profiles.yaml`, nested under a named block like `github` and `phabricator` are, carrying what a copy of a repository needs: `remoteUrl`, `target`, `repoPath`, and optionally `tokenEnv`/`tokenUser`. `remote` defaults to `origin`, `tokenUser` to `x-access-token`.
Three values are required because none has a default that could be right: there is no public git remote to fall back on, no universal trunk name, and nowhere obvious to keep a copy. The block deliberately reads like Runway's merger block — each service says for itself where its copy fetches from, so a queue's provider and its merger can name the same remote while being configured independently.
**Two queues may share a `repoPath`, and should when they are on the same repository** — that is how they come to share one copy and one lock. Sharing a path while disagreeing about the remote or target is rejected at startup, because whichever queue was built first would silently decide what the other one reads.
**The default `Auth` lives here, not in the extension.** `tokenAuth` reads a named environment variable and writes git an `http.extraheader` fragment inside the repository, mode 0600, included from its config — never into the remote URL, which git echoes back into error messages and from there into logs and dead-letter payloads. The extension only knows it has an `Auth` and calls it; a deployment that mints short-lived tokens or reads a secrets manager replaces this one file's worth of behaviour and changes nothing else. It is applied before each fetch rather than once, which is what lets an expiring credential be refreshed.
A remote that needs no credential gets a nil `Auth` — a local path, or SSH served by the host's own configuration and agent.
**Provisioning now fetches.** It runs at wiring time, and the reason given for that was to fail the misconfigured service rather than bury the failure in a queue's retry loop. Writing the test for it showed the claim was not yet true: initializing a directory and recording a remote succeeds whether or not the remote exists, so a wrong URL would have surfaced later, per message, as a validate failure. Provisioning fetches the target branch, which is the step that actually proves the remote is reachable and the credential works — and it warms the copy, so the first request does not pay for a clone.
`newProfiles` takes a context so that provisioning can be cancelled: it reaches the network during startup, and a shutdown then should stop it rather than wait it out.
Still nothing selects it — no demo queue is switched over and the orchestrator image has no git binary yet. Those are the next two steps.
## Test Plan
- ✅ the seam test: configuration in, a provider out that reports `pkg/a/one.go` with 2 added lines from a repository the test built — which only passes if the config block, provisioning, the injected auth and the factory case all line up
- ✅ an unreachable remote fails `newProfiles`. This is the test that found the gap above; before provisioning fetched, it passed startup and would have failed per message instead
- ✅ defaults applied (`remote`, `tokenUser`) and each of the three required values rejected when missing
- ✅ two queues sharing a `repoPath` with different remotes are rejected; two that agree are allowed, since sharing a copy is the point
- ✅ `make test`, `make lint`, `make gazelle`
The config block is nested under `git:` rather than flat as the plan sketched. Flat would have matched `merge.yaml` more literally, but this file's own grammar is a named block per provider, and consistency inside the file a reader is editing seemed worth more than symmetry with a file in another service.
## Summary
### Why?
The git rung merged for real and made up everything upstream of it. Because no change provider could read a plain git remote, `demo-queue` and `e2e-git-queue` both used the fake one, and `make demo-requests` compensated by writing the paths it had just committed onto the change URI (`sq-files=…`) for the fake to read back.
That worked for changes the demo created and for nothing else. A branch pushed by hand carried no marker, so as far as the conflict analyzer could tell it touched nothing and conflicted with nothing — on the rung whose entire purpose is that the repository is real. It was also what pushed change URIs into the 255-byte storage limit and forced the paths to be budgeted down to one per directory.
### What?
Both queues select the git change provider added in the previous two commits, and `gitSource` stops stating anything about what it touched. The orchestrator keeps its own copy of the same bare repository Runway merges into, points its own remote at it, and reads each change out of the commits.
Each queue gets its own copy, because two copies at one path with different configuration would let whichever queue was built first decide what the other reads — which the config layer now rejects outright.
**The orchestrator image gains git and a writable directory**, the same two things Runway's needed for the same reasons: git because the provider shells out to it, and `/var/submitqueue/changerepos` pre-created `0777` because Docker seeds a named volume from the image and the container's user is deployment-configurable, so a root-owned directory would leave a non-root service unable to provision.
**The compose overlay mounts the sandbox into the orchestrator read-only** — it only ever fetches — plus a named volume for the copies. The volume rather than a bind mount for the reason Runway's checkout already is one: this is where git writes objects, and on macOS a freshly written loose object can read back as corrupt across the host filesystem bridge.
The ladder table in the quickstart gains a "Read from" column. The rung's honesty was the point of the change and was not visible in the summary a reader skims.
## Test Plan
Against a live `PROVIDER=git` stack, in order:
- ✅ the container first, with the orchestrator still on `fake`: `git version 2.39.5`, `/srv/git` holding `sandbox.git`, and the repos directory `drwxrwxrwx`
- ✅ **`FOLDERS=1`, six changes, no marker on any URI → a full dependency chain**, every batch depending on all the later ones. With `sq-files=` gone, the only way `pathoverlap` could see a shared directory is from paths the provider read out of the repository
- ✅ **`FOLDERS=50`, five changes → no dependencies at all**, and they land in 3s rather than 10-14s. So it is genuinely keying on paths, not serializing everything
- ✅ **the case that was impossible before**: two branches pushed by hand, carrying no marker, were described correctly — `{"author": {"name": "Hand", …}, "changed_files": [{"path": "shared/a.txt", "lines_added": 1, …}]}` read straight out of the `change` table
- ✅ `make e2e-git-test` with both queues switched
- ✅ `make test`, `make lint`, `make gazelle`
**The risk flagged when planning this did not materialise.** `TestLand_ResubmittedAfterLanding_IsRejectedAsStale` asserts only that a resubmission errors, and the worry was that it would now error inside the provider — the head branch has moved, so the original SHA is no longer reachable from the ref — and pass while no longer testing staleness. Reproduced by hand: it still fails on `refs/heads/hand/one now points at 5b96c363…`, the staleness check. The provider's copy keeps the object it fetched the first time, so it resolves the commit locally and never refetches.
Worth knowing about what the E2E does and does not buy: `e2e-git-queue` uses `analyzer: {type: none}`, so nothing there consumes the provider's output. Switching it proves the provider runs end to end without breaking a land; it does not check the metadata. That check is the `FOLDERS` runs above, which are hand-observed rather than asserted in CI.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Why?
Every change provider so far asks a service what a change contains. GitHub and Phabricator each have an API that knows;
fakeinvents an answer;routingpicks between the first two. A plain git remote has no such service, so there was no way to run the queue against one and have it know what a change touched.That gap is what the demo's git rung was built around. The merge there is real — a real fetch, cherry-pick and push — and everything upstream of it was not. Because the fake provider cannot read a repository,
make demo-requestswrote the paths it had just committed onto the change URI (sq-files=…) and the fake read them back, so the conflict analyzer had something to key on.That worked for changes the demo created and for nothing else. A branch pushed by hand carried no marker, so as far as the analyzer could tell it touched nothing and conflicted with nothing — on the rung whose whole point is that the repository is real. It was also what pushed change URIs into the 255-byte storage limit and forced the paths to be budgeted down to one per directory.
What?
Four commits, each independently revertible:
refactor(test)— five test files each carried their own transcription of "resolve the Bazel-pinned git from runfiles", and they had drifted: two handled both spellings of an external path, three handled one; two verified what they resolved, one did not. Extracted toplatform/gitexec/gitexectestbefore adding a sixth.feat(changeprovider)— the provider. It keeps its own copy of a remote and computes each change from the commits:--numstatfor files and line counts, the commit for the author.The baseline chains through a stack, and that is the part worth reviewing. A
git://URI names a commit and a ref and nothing else — unlike a pull request it carries no base. It cannot be the target branch: a stack's changes are cut one from the next, so measuring each against the target reports the second change as containing the first, and anything summing line counts across a batch counts them twice. The first change is measured from where it diverged from the target, each one after it from its predecessor. This is wrong by default and fails silently, so it is the first thing the tests pin — and the assertion is mutation-tested.Authentication is injected, never derived. The provider takes an
Authand calls it before each fetch; it never reads an environment variable or decides what a credential is. A nilAuthcovers a local path and SSH served by the host's own config and agent.feat(orchestrator)—changeProvider: {type: git, git: {…}}, and the defaultAuthimplementation, which lives in the wiring layer where a deployment can replace it. Two queues may share a copy — that is how they share its lock — but sharing a path while disagreeing about the remote is rejected at startup.Provisioning fetches. It runs at wiring time to fail a misconfigured service rather than bury the failure in a queue's retry loop, and writing the test showed that claim was not yet true: initializing a directory and recording a remote succeeds whether or not the remote exists.
feat(demo)— both git-rung queues switch to it,gitSourcestops stating anything about what it touched, and the orchestrator image gains git and a writable directory, the same two things Runway's image needed for the same reasons.Test Plan
FOLDERS=1, six changes, no marker on any URI → a full dependency chain. Withsq-files=gone, the only waypathoverlapcould see a shared directory is from paths read out of the repositoryFOLDERS=50, five changes → no dependencies, landing in 3s rather than 10-14s, so it is keying on paths rather than serializing everything{"author": {"name": "Hand", …}, "changed_files": [{"path": "shared/a.txt", "lines_added": 1, …}]}straight out of thechangetablefake: git present,/srv/gitmounted, repos directory writablemake e2e-git-testwith both queues switched;make test(106); integration suites;make lint,make gazelleA risk flagged when planning did not materialise.
TestLand_ResubmittedAfterLanding_IsRejectedAsStaleasserts only that a resubmission errors, and the worry was it would now error inside the provider — the head branch has moved, so the original SHA is unreachable from the ref — and stay green while no longer testing staleness. Reproduced by hand: it still fails on the staleness check, because the copy keeps the object it fetched the first time.Worth knowing what the E2E does not buy:
e2e-git-queueusesanalyzer: {type: none}, so nothing there consumes the provider's output. Switching it proves the provider runs end to end without breaking a land; the metadata checks are theFOLDERSruns above, hand-observed rather than asserted in CI.Local note:
make e2e-git-testand the integration suites need--sandbox_writable_path=$HOME/.dockeron macOS, or they fail onopen ~/.docker/buildx/activity/…: operation not permittedbefore any test runs.Note for reviewers
The config block is nested under
git:rather than flat as the plan sketched. Flat would have matchedmerge.yamlmore literally, but this file's grammar is a named block per provider, and consistency inside the file being edited seemed worth more than symmetry with another service's file.GitHub mode deliberately stays on the GitHub API provider: it reads pull request identity a git remote cannot give. So
githubandgitanswer "what changed" by different routes.