Skip to content

Commit 172c4fc

Browse files
committed
feat(changeprovider): read change metadata from a git remote
## 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.
1 parent 3f72adb commit 172c4fc

7 files changed

Lines changed: 1058 additions & 0 deletions

File tree

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
load("@rules_go//go:def.bzl", "go_library", "go_test")
2+
3+
go_library(
4+
name = "go_default_library",
5+
srcs = [
6+
"auth.go",
7+
"numstat.go",
8+
"provider.go",
9+
"repo.go",
10+
],
11+
importpath = "github.com/uber/submitqueue/submitqueue/extension/changeprovider/git",
12+
visibility = ["//visibility:public"],
13+
deps = [
14+
"//platform/base/change/git:go_default_library",
15+
"//platform/metrics:go_default_library",
16+
"//submitqueue/entity:go_default_library",
17+
"//submitqueue/extension/changeprovider:go_default_library",
18+
"@com_github_uber_go_tally//:go_default_library",
19+
"@org_uber_go_zap//:go_default_library",
20+
],
21+
)
22+
23+
go_test(
24+
name = "go_default_test",
25+
srcs = ["provider_test.go"],
26+
# The pinned git, so these assertions describe the build the services run
27+
# rather than whatever the host happens to have.
28+
data = ["@git"],
29+
embed = [":go_default_library"],
30+
env = {
31+
"SUBMITQUEUE_TEST_GIT": "$(location @git//:git)",
32+
},
33+
deps = [
34+
"//platform/base/change:go_default_library",
35+
"//platform/gitexec/gitexectest:go_default_library",
36+
"//submitqueue/entity:go_default_library",
37+
"//submitqueue/extension/changeprovider:go_default_library",
38+
"@com_github_stretchr_testify//assert:go_default_library",
39+
"@com_github_stretchr_testify//require:go_default_library",
40+
"@com_github_uber_go_tally//:go_default_library",
41+
"@org_uber_go_zap//:go_default_library",
42+
],
43+
)
Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
# Git change provider
2+
3+
Reads change metadata — files, line counts, author — out of a git repository, for a remote that offers no API to ask.
4+
5+
The GitHub and Phabricator providers query a service that already knows what a change contains. This one derives it. It keeps its own copy of the remote and computes each change from the commits themselves, which makes a plain git remote a first-class source of change metadata with nothing in front of it: an internal host, a mirror, or a bare repository on disk.
6+
7+
## What a change is measured against
8+
9+
A `git://` change URI names a commit and the ref it lives on, and nothing else. A pull request carries a base; this does not, so the baseline has to be derived — and for a stack it cannot be the target branch.
10+
11+
A stack's changes are cut one from the next. Measuring every change against the target would report the second as containing the first, and any consumer that sums line counts across a batch would count them twice. So the first change in a request is measured from where it diverged from the target, and each one after it from where it diverged from the change before it. The order of the URIs is the stack order, and it is load-bearing.
12+
13+
A change that shares no history with what it claims to land on is an error, not a change that touches nothing.
14+
15+
## Its own copy
16+
17+
Each service keeps its own copy of a queue's repository and configures its own remote for it, so this provider's copy is independent of anything a merger keeps. Where that copy fetches from is configuration: a bind-mounted bare repository and a remote host are the same code path, differing only in the URL.
18+
19+
The copy is bare. Nothing here checks anything out — the provider answers questions about commits and never produces one — so there is no working tree to leave dirty and no index to corrupt.
20+
21+
Git commands against one repository cannot safely interleave, so every provider sharing a copy shares its lock.
22+
23+
Provisioning happens once, at wiring time, rather than on first use: resolving a provider happens per message on the validate path, so a copy created there would put a clone inside a retry loop and hide an unreachable remote behind queue processing instead of failing the service that owns the configuration.
24+
25+
## Authentication
26+
27+
The provider does not decide what a credential is. It takes an `Auth` implementation and calls it before each fetch; an integrator supplies one that reads an environment variable, calls a secrets manager, or mints a short-lived token, and only that implementation changes when the answer does. `Auth` is called per fetch rather than once so an expiring credential can be refreshed.
28+
29+
A nil `Auth` means the remote needs none. That covers a local path, and an SSH remote served by the host's own SSH configuration and agent — the environment a fetch needs to reach a remote is passed through, while the configuration that could change what a diff says is not.
30+
31+
## Tests
32+
33+
Hermetic, against throwaway repositories, driving the Bazel-pinned git rather than the host's. The test that matters most is the three-step stack: reporting a stack cumulatively is wrong by default, never fails loudly, and shows up only as odd-looking scores.
Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
// Copyright (c) 2025 Uber Technologies, Inc.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package git
16+
17+
import "context"
18+
19+
// Auth prepares a local repository to authenticate to its remote.
20+
//
21+
// This provider never decides what a credential is, where it comes from, or how
22+
// long it lives. An integrator wires an implementation in — reading an
23+
// environment variable, calling a secrets manager, minting a short-lived token —
24+
// and only that implementation changes when the answer does.
25+
//
26+
// Apply runs immediately before every fetch rather than once at provisioning,
27+
// so an implementation backed by an expiring credential can refresh it. It must
28+
// therefore be cheap and idempotent.
29+
//
30+
// A nil Auth means the remote needs none, which covers a local path and an SSH
31+
// remote served by the host's own SSH configuration and agent.
32+
type Auth interface {
33+
// Apply configures repoPath so that git commands run against remoteURL from
34+
// inside it can authenticate.
35+
Apply(ctx context.Context, repoPath, remoteURL string) error
36+
}
Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
// Copyright (c) 2025 Uber Technologies, Inc.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package git
16+
17+
import (
18+
"fmt"
19+
"strconv"
20+
"strings"
21+
22+
"github.com/uber/submitqueue/submitqueue/entity"
23+
)
24+
25+
// parseNumstat reads the output of `git diff --numstat -z`.
26+
//
27+
// The NUL-delimited form is used rather than the line-based one because it is
28+
// the only one that survives a path containing a newline, and because it is how
29+
// a rename becomes unambiguous: a normal record is one NUL-terminated field
30+
// holding "added\tdeleted\tpath", while a rename leaves the path empty and
31+
// follows the record with the old and new paths as two further fields.
32+
//
33+
// A binary file reports "-" for both counts. Those are recorded as a changed
34+
// path with no line counts, which is true — a binary has no lines — rather than
35+
// dropped, since a path-keyed conflict analyzer still needs to see it.
36+
func parseNumstat(out string) ([]entity.ChangedFile, error) {
37+
fields := strings.Split(out, "\x00")
38+
var files []entity.ChangedFile
39+
40+
for i := 0; i < len(fields); i++ {
41+
record := fields[i]
42+
if record == "" {
43+
continue
44+
}
45+
46+
parts := strings.SplitN(record, "\t", 3)
47+
if len(parts) != 3 {
48+
return nil, fmt.Errorf("unparseable numstat record %q", record)
49+
}
50+
51+
added, err := parseCount(parts[0])
52+
if err != nil {
53+
return nil, fmt.Errorf("numstat record %q: %w", record, err)
54+
}
55+
deleted, err := parseCount(parts[1])
56+
if err != nil {
57+
return nil, fmt.Errorf("numstat record %q: %w", record, err)
58+
}
59+
60+
path := parts[2]
61+
if path == "" {
62+
// A rename or copy. The two fields that follow are the old path and
63+
// the new one; only the new one is kept, because a ChangedFile names
64+
// one path and the new one is where the content now lives.
65+
if i+2 >= len(fields) {
66+
return nil, fmt.Errorf("numstat rename record %q is missing its paths", record)
67+
}
68+
path = fields[i+2]
69+
i += 2
70+
}
71+
72+
files = append(files, entity.ChangedFile{
73+
Path: path,
74+
LinesAdded: added,
75+
LinesDeleted: deleted,
76+
})
77+
}
78+
return files, nil
79+
}
80+
81+
// parseCount reads one side of a numstat record. "-" means the file is binary,
82+
// which is reported as zero rather than refused.
83+
func parseCount(field string) (int, error) {
84+
if field == "-" {
85+
return 0, nil
86+
}
87+
n, err := strconv.Atoi(field)
88+
if err != nil {
89+
return 0, fmt.Errorf("%q is not a line count: %w", field, err)
90+
}
91+
return n, nil
92+
}
Lines changed: 181 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,181 @@
1+
// Copyright (c) 2025 Uber Technologies, Inc.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
// Package git provides a changeprovider.ChangeProvider that reads change
16+
// metadata out of a git repository, for a remote that offers no API to ask.
17+
//
18+
// Where the GitHub and Phabricator providers query a service that already knows
19+
// what a change contains, this one derives it: it keeps its own copy of the
20+
// remote and computes each change's files, line counts and author from the
21+
// commits themselves. That makes a plain git remote — an internal host, a
22+
// mirror, a bare repository on disk — a first-class source of change metadata
23+
// with no service in front of it.
24+
//
25+
// # What a change is measured against
26+
//
27+
// A git:// change URI names a commit and the ref it lives on, and nothing else.
28+
// Unlike a pull request it carries no base, so the baseline has to be derived,
29+
// and for a stack it cannot be the target branch: a stack's changes are cut one
30+
// from the next, so measuring each against the target would report the second
31+
// change as containing the first as well. Each change is therefore measured
32+
// from where it diverged from the change before it, and only the first from the
33+
// target. Callers get per-change numbers that sum, which is what any consumer
34+
// aggregating over a batch depends on.
35+
package git
36+
37+
import (
38+
"context"
39+
"fmt"
40+
41+
"github.com/uber-go/tally"
42+
"go.uber.org/zap"
43+
44+
changegit "github.com/uber/submitqueue/platform/base/change/git"
45+
coremetrics "github.com/uber/submitqueue/platform/metrics"
46+
"github.com/uber/submitqueue/submitqueue/entity"
47+
"github.com/uber/submitqueue/submitqueue/extension/changeprovider"
48+
)
49+
50+
const opName = "git_changeprovider"
51+
52+
// Params carries what a provider needs. The Repo is built once per repository
53+
// and shared by every queue reading it.
54+
type Params struct {
55+
Config changeprovider.Config
56+
Repo *Repo
57+
Logger *zap.SugaredLogger
58+
MetricsScope tally.Scope
59+
}
60+
61+
// provider reads change metadata from a local copy of a git remote.
62+
type provider struct {
63+
cfg changeprovider.Config
64+
repo *Repo
65+
logger *zap.SugaredLogger
66+
metricsScope tally.Scope
67+
}
68+
69+
// New returns a changeprovider.ChangeProvider reading from repo.
70+
func New(params Params) changeprovider.ChangeProvider {
71+
return &provider{
72+
cfg: params.Config,
73+
repo: params.Repo,
74+
logger: params.Logger.Named(opName),
75+
metricsScope: params.MetricsScope.SubScope(opName),
76+
}
77+
}
78+
79+
// Get returns one ChangeInfo per URI, in the order the URIs were given.
80+
//
81+
// The order is load-bearing: it is the stack order, and each change after the
82+
// first is measured from the one before it.
83+
func (p *provider) Get(ctx context.Context, request entity.Request) (_ []entity.ChangeInfo, retErr error) {
84+
op := coremetrics.Begin(p.metricsScope, "get", coremetrics.LongLatencyBuckets)
85+
defer func() { op.Complete(retErr) }()
86+
87+
uris := request.Change.URIs
88+
infos := make([]entity.ChangeInfo, 0, len(uris))
89+
90+
p.repo.mu.Lock()
91+
defer p.repo.mu.Unlock()
92+
93+
if err := p.repo.fetchTarget(ctx); err != nil {
94+
coremetrics.NamedCounter(p.metricsScope, "get", "fetch_errors", 1)
95+
return nil, fmt.Errorf("failed to update target branch %s: %w", p.repo.cfg.Target, err)
96+
}
97+
98+
previous := ""
99+
for _, uri := range uris {
100+
id, err := changegit.ParseChangeID(uri)
101+
if err != nil {
102+
return nil, fmt.Errorf("failed to parse change URI: %w", err)
103+
}
104+
if err := p.repo.ensureCommit(ctx, id.CommitSHA, id.Ref); err != nil {
105+
coremetrics.NamedCounter(p.metricsScope, "get", "commit_unavailable", 1)
106+
return nil, err
107+
}
108+
109+
// The first change stands on the target; each one after it stands on the
110+
// change before it.
111+
against := p.repo.cfg.Remote + "/" + p.repo.cfg.Target
112+
if previous != "" {
113+
against = previous
114+
}
115+
116+
details, err := p.describe(ctx, against, id.CommitSHA)
117+
if err != nil {
118+
return nil, fmt.Errorf("failed to describe change %s: %w", uri, err)
119+
}
120+
121+
infos = append(infos, entity.ChangeInfo{URI: uri, Details: details})
122+
previous = id.CommitSHA
123+
}
124+
return infos, nil
125+
}
126+
127+
// describe reports what sha changed relative to where it diverged from against.
128+
func (p *provider) describe(ctx context.Context, against, sha string) (entity.ChangeDetails, error) {
129+
base, err := p.repo.mergeBase(ctx, against, sha)
130+
if err != nil {
131+
return entity.ChangeDetails{}, err
132+
}
133+
134+
// -M so a rename reads as one moved file rather than a whole file deleted
135+
// and another added; the scrubbed environment leaves git's own default off.
136+
raw, err := p.repo.output(ctx, "diff", "--numstat", "-M", "-z", base, sha)
137+
if err != nil {
138+
return entity.ChangeDetails{}, err
139+
}
140+
files, err := parseNumstat(raw)
141+
if err != nil {
142+
return entity.ChangeDetails{}, err
143+
}
144+
145+
author, err := p.author(ctx, sha)
146+
if err != nil {
147+
return entity.ChangeDetails{}, err
148+
}
149+
return entity.ChangeDetails{Author: author, ChangedFiles: files}, nil
150+
}
151+
152+
// author reads the commit's author, NUL-separated because a display name can
153+
// contain anything a friendlier separator would collide with.
154+
func (p *provider) author(ctx context.Context, sha string) (entity.Author, error) {
155+
out, err := p.repo.output(ctx, "show", "--no-patch", "--format=%an%x00%ae", sha)
156+
if err != nil {
157+
return entity.Author{}, err
158+
}
159+
name, email, found := cut(out)
160+
if !found {
161+
return entity.Author{}, fmt.Errorf("unreadable author for commit %s", sha)
162+
}
163+
return entity.Author{Name: name, Email: email}, nil
164+
}
165+
166+
// cut splits the author format's two fields, trimming the newline git appends.
167+
func cut(out string) (name, email string, found bool) {
168+
for i := 0; i < len(out); i++ {
169+
if out[i] == 0 {
170+
return out[:i], trimNewline(out[i+1:]), true
171+
}
172+
}
173+
return "", "", false
174+
}
175+
176+
func trimNewline(s string) string {
177+
for len(s) > 0 && (s[len(s)-1] == '\n' || s[len(s)-1] == '\r') {
178+
s = s[:len(s)-1]
179+
}
180+
return s
181+
}

0 commit comments

Comments
 (0)