refactor(changeprovider): make the git provider pure logic over a contract

Move the git transport plumbing — the bare local copy, fetch, commit resolution, merge base, the git command environment — and the Auth contract out of the change provider extension into platform/git/repo, built on platform/git/exec. The provider now depends on a small Repository interface it defines and holds no os/exec and no credential handling: it parses the change URI, picks the stack baseline, reads the diff and author, and shapes the result, nothing more.

Auth moves with the copy it configures, resolving the review point that authentication did not belong in the change provider. The wiring's tokenAuth now implements gitrepo.Auth and SetConfig is gitrepo.SetConfig. Repository-plumbing tests move to platform/git/repo; the provider's behavior tests stay and drive a real gitrepo.Repo through the interface.

Author: behinddwalls

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",
+    ],
+)

…eue/extension/changeprovider/git/auth.go platform/git/repo/auth.gosubmitqueue/extension/changeprovider/git/auth.go renamed to platform/git/repo/auth.go submitqueue/extension/changeprovider/git/auth.go renamed to 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.

…eue/extension/changeprovider/git/repo.go platform/git/repo/repo.gosubmitqueue/extension/changeprovider/git/repo.go renamed to platform/git/repo/repo.go submitqueue/extension/changeprovider/git/repo.go renamed to 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,15 +36,17 @@ import (
 	"path/filepath"
 	"strings"
 	"sync"
+
+	gitexec "github.com/uber/submitqueue/platform/git/exec"
 )
 
 // RepoConfig describes one local copy of a remote.
 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,52 +58,54 @@ 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
 }
 
 // NewRepo returns a Repo for cfg, resolving the git binary. It touches no disk;
 // 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
 	}
 	cfg.Git = git
 	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,55 +195,50 @@ 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)
 	}
 	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
-}