diff --git a/platform/git/repo/BUILD.bazel b/platform/git/repo/BUILD.bazel new file mode 100644 index 00000000..e5478812 --- /dev/null +++ b/platform/git/repo/BUILD.bazel @@ -0,0 +1,30 @@ +load("@rules_go//go:def.bzl", "go_library", "go_test") + +go_library( + name = "go_default_library", + srcs = [ + "auth.go", + "repo.go", + ], + importpath = "github.com/uber/submitqueue/platform/git/repo", + visibility = ["//visibility:public"], + deps = ["//platform/git/exec:go_default_library"], +) + +go_test( + name = "go_default_test", + srcs = ["repo_test.go"], + # The pinned git, so these assertions describe the build the services run + # rather than whatever the host happens to have. + data = ["@git"], + embed = [":go_default_library"], + env = { + "SUBMITQUEUE_TEST_GIT": "$(location @git//:git)", + }, + deps = [ + "//platform/git/exec:go_default_library", + "//platform/git/exectest:go_default_library", + "@com_github_stretchr_testify//assert:go_default_library", + "@com_github_stretchr_testify//require:go_default_library", + ], +) diff --git a/submitqueue/extension/changeprovider/git/auth.go b/platform/git/repo/auth.go similarity index 93% rename from submitqueue/extension/changeprovider/git/auth.go rename to platform/git/repo/auth.go index bb98f116..d622de10 100644 --- a/submitqueue/extension/changeprovider/git/auth.go +++ b/platform/git/repo/auth.go @@ -12,13 +12,13 @@ // See the License for the specific language governing permissions and // limitations under the License. -package git +package gitrepo import "context" // Auth prepares a local repository to authenticate to its remote. // -// This provider never decides what a credential is, where it comes from, or how +// This package never decides what a credential is, where it comes from, or how // long it lives. An integrator wires an implementation in — reading an // environment variable, calling a secrets manager, minting a short-lived token — // and only that implementation changes when the answer does. diff --git a/submitqueue/extension/changeprovider/git/repo.go b/platform/git/repo/repo.go similarity index 56% rename from submitqueue/extension/changeprovider/git/repo.go rename to platform/git/repo/repo.go index 6652a3cc..f0aa88f0 100644 --- a/submitqueue/extension/changeprovider/git/repo.go +++ b/platform/git/repo/repo.go @@ -12,7 +12,21 @@ // See the License for the specific language governing permissions and // limitations under the License. -package git +// Package gitrepo keeps a local, bare copy of a git remote and answers +// questions about its commits. +// +// It is transport plumbing, not domain logic: it fetches, resolves commits, +// and computes merge bases, but never decides what those facts mean. A reader +// that derives change metadata — files, line counts, author — drives a copy +// through this package and interprets the raw git output itself. +// +// The copy is bare because nothing here checks anything out, so there is no +// working tree to leave dirty and no index to corrupt. Git commands against one +// copy cannot safely interleave, so a Repo carries a lock (it embeds +// sync.Mutex) that every reader sharing the copy holds across a sequence of +// commands. Command environment and git-binary resolution come from +// platform/git/exec, the one source of truth every git caller shares. +package gitrepo import ( "context" @@ -22,6 +36,8 @@ import ( "path/filepath" "strings" "sync" + + gitexec "github.com/uber/submitqueue/platform/git/exec" ) // RepoConfig describes one local copy of a remote. @@ -29,8 +45,8 @@ type RepoConfig struct { // Git is the path to the git binary. Empty resolves through GIT_EXECUTABLE // and then PATH. Git string - // Path is where this service keeps its own copy. It belongs to this service - // alone: another service reading the same remote keeps its own. + // Path is where this copy lives on disk. It belongs to one owner: another + // reader of the same remote keeps its own. Path string // RemoteURL is where the copy fetches from — a URL or a local path. RemoteURL string @@ -42,15 +58,11 @@ type RepoConfig struct { Auth Auth } -// Repo is one local copy of a remote, shared by every provider built over it. -// -// Bare, because nothing here checks anything out: the copy answers questions -// about commits and never produces one. That also means no index and no working -// tree to leave dirty between operations. +// Repo is one local, bare copy of a remote, shared by every reader built over +// it. The embedded mutex serializes git commands against the copy; a reader +// holds it across any sequence that must see a consistent object set. type Repo struct { - // mu serializes access. Git commands against one repository cannot safely - // interleave, and every provider sharing this copy shares this lock. - mu sync.Mutex + sync.Mutex cfg RepoConfig } @@ -58,19 +70,19 @@ type Repo struct { // Provision does that. func NewRepo(cfg RepoConfig) (*Repo, error) { if cfg.Path == "" { - return nil, fmt.Errorf("git change provider: a repository path is required") + return nil, fmt.Errorf("gitrepo: a repository path is required") } if cfg.RemoteURL == "" { - return nil, fmt.Errorf("git change provider: a remote URL is required") + return nil, fmt.Errorf("gitrepo: a remote URL is required") } if cfg.Target == "" { - return nil, fmt.Errorf("git change provider: a target branch is required") + return nil, fmt.Errorf("gitrepo: a target branch is required") } if cfg.Remote == "" { cfg.Remote = "origin" } - git, err := resolveGit(cfg.Git) + git, err := gitexec.Resolve(cfg.Git) if err != nil { return nil, err } @@ -78,16 +90,22 @@ func NewRepo(cfg RepoConfig) (*Repo, error) { return &Repo{cfg: cfg}, nil } +// Remote is the name the copy records its remote URL under. +func (r *Repo) Remote() string { return r.cfg.Remote } + +// Target is the branch a change's diff is measured against. +func (r *Repo) Target() string { return r.cfg.Target } + // Provision creates the copy if it is not already there and points it at the // remote, leaving an existing copy's objects alone. // -// Callers run this at wiring time rather than on first use: resolving a -// provider happens once per message on the validate path, so a copy created -// there would put a clone inside a retry loop and hide a bad remote behind -// queue processing rather than failing the service that owns it. +// Callers run this at wiring time rather than on first use: a reader is often +// resolved once per message on a retry-driven path, so a copy created there +// would put a clone inside a retry loop and hide a bad remote behind queue +// processing rather than failing the service that owns it. func (r *Repo) Provision(ctx context.Context) error { - r.mu.Lock() - defer r.mu.Unlock() + r.Lock() + defer r.Unlock() if err := os.MkdirAll(r.cfg.Path, 0o755); err != nil { return fmt.Errorf("could not create repository directory %q: %w", r.cfg.Path, err) @@ -110,7 +128,7 @@ func (r *Repo) Provision(ctx context.Context) error { // at all: an unreachable remote, a wrong URL, or a credential that does not // work fails the service that is misconfigured. Initializing a directory and // recording a remote would succeed against a remote that does not exist. - return r.fetchTarget(ctx) + return r.FetchTarget(ctx) } // configureRemote records the remote, correcting it if the configuration @@ -130,24 +148,24 @@ func (r *Repo) configureRemote(ctx context.Context) error { return err } -// ensureCommit guarantees sha is present locally, fetching if it is not. +// EnsureCommit guarantees sha is present locally, fetching if it is not. // // By SHA first, which needs the server to allow a want for an object it does // not advertise (github.com does); the change's own ref is the fallback for a // server that does not. Neither is shallow — a merge base needs ancestry. -func (r *Repo) ensureCommit(ctx context.Context, sha, ref string) error { - if r.hasCommit(ctx, sha) { +func (r *Repo) EnsureCommit(ctx context.Context, sha, ref string) error { + if r.HasCommit(ctx, sha) { return nil } if err := r.applyAuth(ctx); err != nil { return err } - if _, err := r.run(ctx, "fetch", r.cfg.Remote, sha); err == nil && r.hasCommit(ctx, sha) { + if _, err := r.run(ctx, "fetch", r.cfg.Remote, sha); err == nil && r.HasCommit(ctx, sha) { return nil } if ref != "" { - if _, err := r.run(ctx, "fetch", r.cfg.Remote, ref); err == nil && r.hasCommit(ctx, sha) { + if _, err := r.run(ctx, "fetch", r.cfg.Remote, ref); err == nil && r.HasCommit(ctx, sha) { return nil } } @@ -160,9 +178,9 @@ func (r *Repo) ensureCommit(ctx context.Context, sha, ref string) error { return fmt.Errorf("commit %s is not available from remote %s (tried by SHA and via %q)", sha, r.cfg.Remote, ref) } -// fetchTarget updates the target branch, which is the baseline a change's first +// FetchTarget updates the target branch, which is the baseline a change's first // commit is measured from and moves as other changes land. -func (r *Repo) fetchTarget(ctx context.Context) error { +func (r *Repo) FetchTarget(ctx context.Context) error { if err := r.applyAuth(ctx); err != nil { return err } @@ -177,16 +195,17 @@ func (r *Repo) applyAuth(ctx context.Context) error { return r.cfg.Auth.Apply(ctx, r.cfg.Path, r.cfg.RemoteURL) } -func (r *Repo) hasCommit(ctx context.Context, sha string) bool { +// HasCommit reports whether sha is present in the copy. +func (r *Repo) HasCommit(ctx context.Context, sha string) bool { _, err := r.run(ctx, "cat-file", "-e", sha+"^{commit}") return err == nil } -// mergeBase returns the commit two revisions diverged from. Absence of one is +// MergeBase returns the commit two revisions diverged from. Absence of one is // reported as an error rather than an empty diff: a change sharing no history // with what it claims to land on is a fact worth surfacing, not a change that // touches nothing. -func (r *Repo) mergeBase(ctx context.Context, a, b string) (string, error) { +func (r *Repo) MergeBase(ctx context.Context, a, b string) (string, error) { base, err := r.run(ctx, "merge-base", a, b) if err != nil { return "", fmt.Errorf("%s and %s share no history: %w", a, b, err) @@ -194,38 +213,32 @@ func (r *Repo) mergeBase(ctx context.Context, a, b string) (string, error) { return base, nil } -// run executes git inside the copy. -// -// The environment is replaced rather than inherited, for the reason the merger -// records: ambient configuration — a hooks path, a commit template, a signing -// requirement — is exactly what makes a scripted git behave differently on two -// machines. What survives is what reaching a remote needs and what cannot -// change an answer: the SSH agent, TLS roots, and proxy settings. -func (r *Repo) run(ctx context.Context, args ...string) (string, error) { +// command builds a git invocation inside the copy, carrying the shared scrub set +// plus the transport variables a fetch needs. HOME is passed through so git can +// find the user's SSH known_hosts and credential store; it is not in the shared +// transport set, so this package asks for it explicitly. +func (r *Repo) command(ctx context.Context, args ...string) *exec.Cmd { cmd := exec.CommandContext(ctx, r.cfg.Git, args...) cmd.Dir = r.cfg.Path - cmd.Env = commandEnv() + cmd.Env = gitexec.Env(gitexec.EnvOptions{Transport: true, Passthrough: []string{"HOME"}}) + return cmd +} - var stderr strings.Builder - cmd.Stderr = &stderr - out, err := cmd.Output() - if err != nil { - message := strings.TrimSpace(stderr.String()) - if message == "" { - message = err.Error() - } - return "", fmt.Errorf("git %s: %s", strings.Join(args, " "), message) - } - return strings.TrimSpace(string(out)), nil +// run executes git inside the copy and returns trimmed stdout. +func (r *Repo) run(ctx context.Context, args ...string) (string, error) { + out, err := r.outputOf(ctx, args...) + return strings.TrimSpace(out), err } -// output runs git and returns stdout untrimmed, for commands whose output is -// NUL-delimited and whose trailing separator is part of the format. -func (r *Repo) output(ctx context.Context, args ...string) (string, error) { - cmd := exec.CommandContext(ctx, r.cfg.Git, args...) - cmd.Dir = r.cfg.Path - cmd.Env = commandEnv() +// RunRaw executes git inside the copy and returns stdout untrimmed, for commands +// whose output is NUL-delimited and whose trailing separator is part of the +// format. +func (r *Repo) RunRaw(ctx context.Context, args ...string) (string, error) { + return r.outputOf(ctx, args...) +} +func (r *Repo) outputOf(ctx context.Context, args ...string) (string, error) { + cmd := r.command(ctx, args...) var stderr strings.Builder cmd.Stderr = &stderr out, err := cmd.Output() @@ -239,58 +252,16 @@ func (r *Repo) output(ctx context.Context, args ...string) (string, error) { return string(out), nil } -// scrubbedEnv is the configuration-denying half of a git invocation. -var scrubbedEnv = []string{ - "GIT_CONFIG_NOSYSTEM=1", - "GIT_CONFIG_GLOBAL=" + os.DevNull, - "GIT_ATTR_NOSYSTEM=1", - "GIT_TERMINAL_PROMPT=0", - "GIT_PAGER=cat", - "GIT_EDITOR=:", -} - -// transportEnvNames are inherited when set. None can change what a diff says; -// all of them decide whether a remote can be reached at all. -var transportEnvNames = []string{ - "SSH_AUTH_SOCK", - "SSH_AGENT_PID", - "PATH", - "HOME", - "GIT_SSH", - "GIT_SSH_COMMAND", - "GIT_SSH_VARIANT", - "GIT_SSL_CAINFO", - "GIT_SSL_CAPATH", - "SSL_CERT_DIR", - "SSL_CERT_FILE", - "HTTP_PROXY", "HTTPS_PROXY", "NO_PROXY", - "http_proxy", "https_proxy", "no_proxy", -} - -func commandEnv() []string { - env := make([]string, 0, len(scrubbedEnv)+len(transportEnvNames)) - env = append(env, scrubbedEnv...) - for _, name := range transportEnvNames { - if value, ok := os.LookupEnv(name); ok { - env = append(env, name+"="+value) - } - } - return env -} - // SetConfig writes one local configuration value into the repository at path. // // Exported for an Auth implementation, which configures a repository from // outside this package and would otherwise have to find and run git itself. func SetConfig(ctx context.Context, path, key, value string) error { - git, err := resolveGit("") + git, err := gitexec.Resolve("") if err != nil { return err } - cmd := exec.CommandContext(ctx, git, "config", key, value) - cmd.Dir = path - cmd.Env = commandEnv() - + cmd := gitexec.Command(ctx, git, path, "config", key, value) var stderr strings.Builder cmd.Stderr = &stderr if err := cmd.Run(); err != nil { @@ -302,27 +273,3 @@ func SetConfig(ctx context.Context, path, key, value string) error { } return nil } - -// resolveGit locates the git binary, preferring an explicit path, then -// GIT_EXECUTABLE, then PATH — the convention the rest of the repository uses. -func resolveGit(path string) (string, error) { - candidate := strings.TrimSpace(path) - if candidate == "" { - candidate = strings.TrimSpace(os.Getenv("GIT_EXECUTABLE")) - } - if candidate == "" { - found, err := exec.LookPath("git") - if err != nil { - return "", fmt.Errorf("git change provider: no git binary found: %w", err) - } - candidate = found - } - absolute, err := filepath.Abs(candidate) - if err != nil { - return "", fmt.Errorf("git change provider: %q is not a usable path: %w", candidate, err) - } - if info, err := os.Stat(absolute); err != nil || info.IsDir() { - return "", fmt.Errorf("git change provider: %q is not an executable file", absolute) - } - return absolute, nil -} diff --git a/platform/git/repo/repo_test.go b/platform/git/repo/repo_test.go new file mode 100644 index 00000000..abc5287c --- /dev/null +++ b/platform/git/repo/repo_test.go @@ -0,0 +1,116 @@ +// Copyright (c) 2025 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package gitrepo + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + gitexec "github.com/uber/submitqueue/platform/git/exec" + gitexectest "github.com/uber/submitqueue/platform/git/exectest" +) + +// remote is a bare "remote" plus a working clone the test commits into, driving +// the Bazel-pinned git so the assertions describe the build the services run. +type remote struct { + t *testing.T + git string + url string + work string +} + +func newRemote(t *testing.T) *remote { + t.Helper() + root := t.TempDir() + r := &remote{ + t: t, + git: gitexectest.Git(t), + url: filepath.Join(root, "remote.git"), + work: filepath.Join(root, "work"), + } + r.run("", "init", "--bare", "-b", "main", r.url) + r.run("", "clone", r.url, r.work) + r.run(r.work, "config", "user.name", "Test") + r.run(r.work, "config", "user.email", "test@example.invalid") + require.NoError(t, os.WriteFile(filepath.Join(r.work, "seed.txt"), []byte("seed\n"), 0o644)) + r.run(r.work, "add", ".") + r.run(r.work, "commit", "-m", "seed") + r.run(r.work, "push", "origin", "main") + return r +} + +func (r *remote) run(dir string, args ...string) string { + r.t.Helper() + out, err := gitexec.Command(context.Background(), r.git, dir, args...).CombinedOutput() + require.NoError(r.t, err, "git %s: %s", strings.Join(args, " "), out) + return strings.TrimSpace(string(out)) +} + +// pushFeature commits a file on a new branch and returns its head SHA and ref. +func (r *remote) pushFeature(branch, contents string) (sha, ref string) { + r.t.Helper() + r.run(r.work, "checkout", "-B", branch, "main") + require.NoError(r.t, os.WriteFile(filepath.Join(r.work, "feature.txt"), []byte(contents), 0o644)) + r.run(r.work, "add", "-A") + r.run(r.work, "commit", "-m", "feature") + r.run(r.work, "push", "-f", "origin", branch) + return r.run(r.work, "rev-parse", "HEAD"), "refs/heads/" + branch +} + +func TestProvision_IsIdempotentAndKeepsWhatItFetched(t *testing.T) { + ctx := context.Background() + rem := newRemote(t) + head, ref := rem.pushFeature("feature/keep", "keep\n") + + repo, err := NewRepo(RepoConfig{ + Git: rem.git, + Path: filepath.Join(t.TempDir(), "copy.git"), + RemoteURL: rem.url, + Target: "main", + }) + require.NoError(t, err) + require.NoError(t, repo.Provision(ctx)) + + require.False(t, repo.HasCommit(ctx, head), "provisioning fetches only the target") + require.NoError(t, repo.EnsureCommit(ctx, head, ref)) + require.True(t, repo.HasCommit(ctx, head)) + + require.NoError(t, repo.Provision(ctx)) + assert.True(t, repo.HasCommit(ctx, head), + "re-provisioning must not discard objects already fetched") +} + +func TestNewRepo_RejectsAnIncompleteConfiguration(t *testing.T) { + git := gitexectest.Git(t) + for _, tt := range []struct { + name string + cfg RepoConfig + }{ + {name: "no path", cfg: RepoConfig{Git: git, RemoteURL: "u", Target: "main"}}, + {name: "no remote url", cfg: RepoConfig{Git: git, Path: "p", Target: "main"}}, + {name: "no target", cfg: RepoConfig{Git: git, Path: "p", RemoteURL: "u"}}, + } { + t.Run(tt.name, func(t *testing.T) { + _, err := NewRepo(tt.cfg) + require.Error(t, err) + }) + } +} diff --git a/service/submitqueue/orchestrator/server/BUILD.bazel b/service/submitqueue/orchestrator/server/BUILD.bazel index 4f7ec214..53e39a23 100644 --- a/service/submitqueue/orchestrator/server/BUILD.bazel +++ b/service/submitqueue/orchestrator/server/BUILD.bazel @@ -26,6 +26,7 @@ go_library( "//platform/extension/counter:go_default_library", "//platform/extension/counter/mysql:go_default_library", "//platform/extension/messagequeue/mysql:go_default_library", + "//platform/git/repo:go_default_library", "//platform/githubactions:go_default_library", "//platform/http:go_default_library", "//platform/pipeline:go_default_library", diff --git a/service/submitqueue/orchestrator/server/changerepo.go b/service/submitqueue/orchestrator/server/changerepo.go index 10e2d130..d8d47b4b 100644 --- a/service/submitqueue/orchestrator/server/changerepo.go +++ b/service/submitqueue/orchestrator/server/changerepo.go @@ -22,7 +22,7 @@ import ( "path/filepath" "strings" - gitprovider "github.com/uber/submitqueue/submitqueue/extension/changeprovider/git" + gitrepo "github.com/uber/submitqueue/platform/git/repo" ) // credentialFile holds the git configuration fragment carrying a token. It is @@ -31,11 +31,11 @@ import ( // and from there into logs and dead-letter payloads. const credentialFile = "submitqueue-changeprovider-credentials.config" -// tokenAuth is the default gitprovider.Auth: a credential read from an +// tokenAuth is the default gitrepo.Auth: a credential read from an // environment variable and presented to git as an HTTP header. // -// It is deliberately here rather than in the extension. The extension takes an -// Auth and calls it; what a credential is and where it comes from is a +// It is deliberately here in the wiring rather than in gitrepo. gitrepo takes +// an Auth and calls it; what a credential is and where it comes from is a // deployment's business, so a deployment that mints short-lived tokens or // reads a secrets manager supplies its own implementation instead of this one // and changes nothing else. @@ -73,7 +73,7 @@ func (a tokenAuth) Apply(ctx context.Context, repoPath, remoteURL string) error // include.path resolves relative to the config file holding it, so the bare // filename lands beside it. A bare repository's config is at its root. - return gitprovider.SetConfig(ctx, repoPath, "include.path", credentialFile) + return gitrepo.SetConfig(ctx, repoPath, "include.path", credentialFile) } func isHTTPRemote(remoteURL string) bool { @@ -87,13 +87,13 @@ func isHTTPRemote(remoteURL string) bool { // provider is resolved once per message on the validate path, so a clone // started there would sit inside a retry loop and report an unreachable remote // as a queue processing failure instead of a service that is misconfigured. -func newChangeRepo(ctx context.Context, cfg gitProviderConfig) (*gitprovider.Repo, error) { - var auth gitprovider.Auth +func newChangeRepo(ctx context.Context, cfg gitProviderConfig) (*gitrepo.Repo, error) { + var auth gitrepo.Auth if cfg.TokenEnv != "" { auth = tokenAuth{tokenEnv: cfg.TokenEnv, tokenUser: cfg.TokenUser} } - repo, err := gitprovider.NewRepo(gitprovider.RepoConfig{ + repo, err := gitrepo.NewRepo(gitrepo.RepoConfig{ Path: cfg.RepoPath, RemoteURL: cfg.RemoteURL, Remote: cfg.Remote, diff --git a/submitqueue/extension/changeprovider/git/BUILD.bazel b/submitqueue/extension/changeprovider/git/BUILD.bazel index 11bc776b..eb0eab9a 100644 --- a/submitqueue/extension/changeprovider/git/BUILD.bazel +++ b/submitqueue/extension/changeprovider/git/BUILD.bazel @@ -3,10 +3,8 @@ load("@rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "go_default_library", srcs = [ - "auth.go", "numstat.go", "provider.go", - "repo.go", ], importpath = "github.com/uber/submitqueue/submitqueue/extension/changeprovider/git", visibility = ["//visibility:public"], @@ -32,7 +30,9 @@ go_test( }, deps = [ "//platform/base/change:go_default_library", + "//platform/git/exec:go_default_library", "//platform/git/exectest:go_default_library", + "//platform/git/repo:go_default_library", "//submitqueue/entity:go_default_library", "//submitqueue/extension/changeprovider:go_default_library", "@com_github_stretchr_testify//assert:go_default_library", diff --git a/submitqueue/extension/changeprovider/git/README.md b/submitqueue/extension/changeprovider/git/README.md index e91baf1d..7f9cbec1 100644 --- a/submitqueue/extension/changeprovider/git/README.md +++ b/submitqueue/extension/changeprovider/git/README.md @@ -12,21 +12,9 @@ A stack's changes are cut one from the next. Measuring every change against the A change that shares no history with what it claims to land on is an error, not a change that touches nothing. -## Its own copy +## What this package is, and isn't -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. - -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. - -Git commands against one repository cannot safely interleave, so every provider sharing a copy shares its lock. - -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. - -## Authentication - -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. - -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. +This package is only the derivation: parse the URI, pick the baseline, read the diff and author, shape the result. Everything about *reaching* the repository — the local bare copy, fetching, merge bases, authentication, the git command environment — is transport plumbing that lives in [`platform/git/repo`](../../../../platform/git/repo) (built on [`platform/git/exec`](../../../../platform/git/exec)), which the merger and any other git caller share. The provider depends on a small `Repository` interface it defines, so it holds no `os/exec` and no credential handling of its own. ## Tests diff --git a/submitqueue/extension/changeprovider/git/provider.go b/submitqueue/extension/changeprovider/git/provider.go index be07be37..9ae620d9 100644 --- a/submitqueue/extension/changeprovider/git/provider.go +++ b/submitqueue/extension/changeprovider/git/provider.go @@ -37,6 +37,7 @@ package git import ( "context" "fmt" + "sync" "github.com/uber-go/tally" "go.uber.org/zap" @@ -49,11 +50,27 @@ import ( const opName = "git_changeprovider" -// Params carries what a provider needs. The Repo is built once per repository -// and shared by every queue reading it. +// Repository is the local copy this provider reads through. It is the git +// plumbing the provider needs and nothing more: fetching, commit resolution, +// merge bases, and a lock the provider holds across a read so a shared copy's +// object set stays consistent. platform/git/repo.Repo satisfies it. +type Repository interface { + sync.Locker + FetchTarget(ctx context.Context) error + EnsureCommit(ctx context.Context, sha, ref string) error + MergeBase(ctx context.Context, a, b string) (string, error) + // RunRaw returns git's stdout untrimmed, since the diff and author formats + // this provider reads are NUL-delimited with a meaningful trailing byte. + RunRaw(ctx context.Context, args ...string) (string, error) + Remote() string + Target() string +} + +// Params carries what a provider needs. The Repository is built once per +// repository and shared by every queue reading it. type Params struct { Config changeprovider.Config - Repo *Repo + Repo Repository Logger *zap.SugaredLogger MetricsScope tally.Scope } @@ -61,7 +78,7 @@ type Params struct { // provider reads change metadata from a local copy of a git remote. type provider struct { cfg changeprovider.Config - repo *Repo + repo Repository logger *zap.SugaredLogger metricsScope tally.Scope } @@ -87,12 +104,12 @@ func (p *provider) Get(ctx context.Context, request entity.Request) (_ []entity. uris := request.Change.URIs infos := make([]entity.ChangeInfo, 0, len(uris)) - p.repo.mu.Lock() - defer p.repo.mu.Unlock() + p.repo.Lock() + defer p.repo.Unlock() - if err := p.repo.fetchTarget(ctx); err != nil { + if err := p.repo.FetchTarget(ctx); err != nil { coremetrics.NamedCounter(p.metricsScope, "get", "fetch_errors", 1) - return nil, fmt.Errorf("failed to update target branch %s: %w", p.repo.cfg.Target, err) + return nil, fmt.Errorf("failed to update target branch %s: %w", p.repo.Target(), err) } previous := "" @@ -101,14 +118,14 @@ func (p *provider) Get(ctx context.Context, request entity.Request) (_ []entity. if err != nil { return nil, fmt.Errorf("failed to parse change URI: %w", err) } - if err := p.repo.ensureCommit(ctx, id.CommitSHA, id.Ref); err != nil { + if err := p.repo.EnsureCommit(ctx, id.CommitSHA, id.Ref); err != nil { coremetrics.NamedCounter(p.metricsScope, "get", "commit_unavailable", 1) return nil, err } // The first change stands on the target; each one after it stands on the // change before it. - against := p.repo.cfg.Remote + "/" + p.repo.cfg.Target + against := p.repo.Remote() + "/" + p.repo.Target() if previous != "" { against = previous } @@ -126,14 +143,14 @@ func (p *provider) Get(ctx context.Context, request entity.Request) (_ []entity. // describe reports what sha changed relative to where it diverged from against. func (p *provider) describe(ctx context.Context, against, sha string) (entity.ChangeDetails, error) { - base, err := p.repo.mergeBase(ctx, against, sha) + base, err := p.repo.MergeBase(ctx, against, sha) if err != nil { return entity.ChangeDetails{}, err } // -M so a rename reads as one moved file rather than a whole file deleted // and another added; the scrubbed environment leaves git's own default off. - raw, err := p.repo.output(ctx, "diff", "--numstat", "-M", "-z", base, sha) + raw, err := p.repo.RunRaw(ctx, "diff", "--numstat", "-M", "-z", base, sha) if err != nil { return entity.ChangeDetails{}, err } @@ -152,7 +169,7 @@ func (p *provider) describe(ctx context.Context, against, sha string) (entity.Ch // author reads the commit's author, NUL-separated because a display name can // contain anything a friendlier separator would collide with. func (p *provider) author(ctx context.Context, sha string) (entity.Author, error) { - out, err := p.repo.output(ctx, "show", "--no-patch", "--format=%an%x00%ae", sha) + out, err := p.repo.RunRaw(ctx, "show", "--no-patch", "--format=%an%x00%ae", sha) if err != nil { return entity.Author{}, err } diff --git a/submitqueue/extension/changeprovider/git/provider_test.go b/submitqueue/extension/changeprovider/git/provider_test.go index d6dbed8c..02122286 100644 --- a/submitqueue/extension/changeprovider/git/provider_test.go +++ b/submitqueue/extension/changeprovider/git/provider_test.go @@ -19,7 +19,6 @@ import ( "fmt" "net/url" "os" - "os/exec" "path/filepath" "strings" "sync" @@ -31,7 +30,9 @@ import ( "go.uber.org/zap" "github.com/uber/submitqueue/platform/base/change" + gitexec "github.com/uber/submitqueue/platform/git/exec" gitexectest "github.com/uber/submitqueue/platform/git/exectest" + gitrepo "github.com/uber/submitqueue/platform/git/repo" "github.com/uber/submitqueue/submitqueue/entity" "github.com/uber/submitqueue/submitqueue/extension/changeprovider" ) @@ -45,7 +46,7 @@ type fixture struct { remote string work string provider changeprovider.ChangeProvider - repo *Repo + repo *gitrepo.Repo } func newFixture(t *testing.T) *fixture { @@ -70,7 +71,7 @@ func newFixture(t *testing.T) *fixture { f.run(f.work, "commit", "-m", "seed") f.run(f.work, "push", "origin", "main") - repo, err := NewRepo(RepoConfig{ + repo, err := gitrepo.NewRepo(gitrepo.RepoConfig{ Git: git, Path: filepath.Join(root, "copy.git"), RemoteURL: f.remote, @@ -91,9 +92,7 @@ func newFixture(t *testing.T) *fixture { func (f *fixture) run(dir string, args ...string) string { f.t.Helper() - cmd := exec.Command(f.git, args...) - cmd.Dir = dir - cmd.Env = commandEnv() + cmd := gitexec.Command(context.Background(), f.git, dir, args...) out, err := cmd.CombinedOutput() require.NoError(f.t, err, "git %s: %s", strings.Join(args, " "), out) return strings.TrimSpace(string(out)) @@ -264,7 +263,7 @@ func TestGet_FetchesACommitItHasNotSeen(t *testing.T) { f := newFixture(t) head := f.push("feature/later", f.mainSHA(), map[string]string{"late.txt": "late\n"}, "later") - require.False(t, f.repo.hasCommit(context.Background(), head), + require.False(t, f.repo.HasCommit(context.Background(), head), "the fixture must start without the commit for this to prove anything") infos := f.get(f.uri("feature/later", head)) @@ -348,31 +347,3 @@ func TestGet_ConcurrentReadsAreSerialized(t *testing.T) { assert.Equal(t, []string{fmt.Sprintf("pkg/c%d/f.go", i)}, pathsOf(results[i][0])) } } - -func TestProvision_IsIdempotentAndKeepsWhatItFetched(t *testing.T) { - f := newFixture(t) - ctx := context.Background() - head := f.push("feature/keep", f.mainSHA(), map[string]string{"keep.txt": "keep\n"}, "keep") - f.get(f.uri("feature/keep", head)) - - require.NoError(t, f.repo.Provision(ctx)) - assert.True(t, f.repo.hasCommit(ctx, head), - "re-provisioning must not discard objects already fetched") -} - -func TestNewRepo_RejectsAnIncompleteConfiguration(t *testing.T) { - git := gitexectest.Git(t) - for _, tt := range []struct { - name string - cfg RepoConfig - }{ - {name: "no path", cfg: RepoConfig{Git: git, RemoteURL: "u", Target: "main"}}, - {name: "no remote url", cfg: RepoConfig{Git: git, Path: "p", Target: "main"}}, - {name: "no target", cfg: RepoConfig{Git: git, Path: "p", RemoteURL: "u"}}, - } { - t.Run(tt.name, func(t *testing.T) { - _, err := NewRepo(tt.cfg) - require.Error(t, err) - }) - } -}