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

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

submitqueue/extension/changeprovider/git/auth.go → 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.

submitqueue/extension/changeprovider/git/repo.go → 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
-}