From 6cbd1054c3356c388b3bdc200c62f5c1cd44f892 Mon Sep 17 00:00:00 2001 From: sergeyb Date: Wed, 22 Jul 2026 23:41:40 +0000 Subject: [PATCH] feat: pin git runtime for git pusher --- MODULE.bazel | 6 + submitqueue/extension/pusher/git/BUILD.bazel | 14 ++ .../extension/pusher/git/git_pusher.go | 75 ++++++++- .../extension/pusher/git/git_pusher_test.go | 149 ++++++++++++++++-- 4 files changed, 226 insertions(+), 18 deletions(-) diff --git a/MODULE.bazel b/MODULE.bazel index 61bdeb2a..8202f504 100644 --- a/MODULE.bazel +++ b/MODULE.bazel @@ -1,10 +1,16 @@ # Listed first so its protoc toolchain wins resolution over the protobuf module's, # which is pulled in transitively by rules_go/rules_proto. bazel_dep(name = "toolchains_protoc", version = "0.6.1") +bazel_dep(name = "git", version = "2.55.0") bazel_dep(name = "rules_go", version = "0.57.0") bazel_dep(name = "gazelle", version = "0.45.0") bazel_dep(name = "rules_proto", version = "7.1.0") +single_version_override( + module_name = "git", + version = "2.55.0", +) + # Direct dep so the well-known type proto_library targets (e.g. # @protobuf//:descriptor_proto, needed by the message queue topics option that # extends google.protobuf.MessageOptions) are visible by apparent name. diff --git a/submitqueue/extension/pusher/git/BUILD.bazel b/submitqueue/extension/pusher/git/BUILD.bazel index 6f0538e3..d1891bd6 100644 --- a/submitqueue/extension/pusher/git/BUILD.bazel +++ b/submitqueue/extension/pusher/git/BUILD.bazel @@ -20,7 +20,21 @@ go_library( go_test( name = "go_default_test", srcs = ["git_pusher_test.go"], + data = [ + "@git", + "@git//:git-remote-http", + "@git//:git_receive_pack", + "@git//:git_remote_https", + "@git//:git_upload_archive", + "@git//:git_upload_pack", + "@git//:templates", + "@git//:templates/description", + ], embed = [":go_default_library"], + env = { + "SUBMITQUEUE_TEST_GIT": "$(location @git//:git)", + "SUBMITQUEUE_TEST_GIT_TEMPLATE_DESCRIPTION": "$(location @git//:templates/description)", + }, deps = [ "//platform/base/change:go_default_library", "//submitqueue/core/changeset/fake:go_default_library", diff --git a/submitqueue/extension/pusher/git/git_pusher.go b/submitqueue/extension/pusher/git/git_pusher.go index 56e406fc..78e99e2e 100644 --- a/submitqueue/extension/pusher/git/git_pusher.go +++ b/submitqueue/extension/pusher/git/git_pusher.go @@ -54,7 +54,9 @@ import ( "bytes" "context" "fmt" + "os" "os/exec" + "path/filepath" "strings" "sync" @@ -74,6 +76,17 @@ import ( // pathologically busy remote. const defaultMaxPushAttempts = 10 +// GitRuntime identifies the explicitly provided Git runtime used by the Pusher. +type GitRuntime struct { + // Executable is the absolute path to the Git executable. + Executable string + // ExecPath is the absolute directory containing Git's helper executables. + ExecPath string + // TemplateDir is the absolute directory containing Git's repository + // templates. + TemplateDir string +} + // Params holds the dependencies for the git Pusher. type Params struct { // CheckoutPath is the absolute path to an existing git checkout that the @@ -90,6 +103,8 @@ type Params struct { Logger *zap.SugaredLogger // MetricsScope is the metrics scope for instrumentation. MetricsScope tally.Scope + // Runtime is the pinned Git runtime used for every invocation. + Runtime GitRuntime // MaxPushAttempts caps how many times Push retries the full // fetch/reset/cherry-pick/push cycle when the remote tip moves under // it. Defaults to defaultMaxPushAttempts when zero or negative. @@ -105,6 +120,7 @@ type gitPusher struct { resolver changeset.Resolver logger *zap.SugaredLogger metricsScope tally.Scope + runtime GitRuntime maxPushAttempts int // mu serializes concurrent Push calls — the underlying checkout cannot @@ -117,7 +133,11 @@ var _ pusher.Pusher = (*gitPusher)(nil) // NewPusher constructs a new git-backed Pusher operating against the given // checkout. The checkout must already exist and have the configured remote. -func NewPusher(params Params) pusher.Pusher { +// Runtime paths must be absolute. +func NewPusher(params Params) (pusher.Pusher, error) { + if err := params.Runtime.validate(); err != nil { + return nil, err + } maxAttempts := params.MaxPushAttempts if maxAttempts <= 0 { maxAttempts = defaultMaxPushAttempts @@ -129,8 +149,25 @@ func NewPusher(params Params) pusher.Pusher { resolver: params.Resolver, logger: params.Logger.Named("git_pusher"), metricsScope: params.MetricsScope.SubScope("git_pusher"), + runtime: params.Runtime, maxPushAttempts: maxAttempts, + }, nil +} + +func (r GitRuntime) validate() error { + for name, path := range map[string]string{ + "executable": r.Executable, + "exec path": r.ExecPath, + "template dir": r.TemplateDir, + } { + if path == "" { + return fmt.Errorf("git runtime %s is required", name) + } + if !filepath.IsAbs(path) { + return fmt.Errorf("git runtime %s must be absolute: %q", name, path) + } } + return nil } // Push fulfils the pusher.Pusher contract. @@ -431,8 +468,7 @@ func (p *gitPusher) push(ctx context.Context) error { // run executes a `git` command in the checkout. Returns captured stdout and // an error that includes captured stderr for diagnostics. func (p *gitPusher) run(ctx context.Context, stdin []byte, args ...string) ([]byte, error) { - cmd := exec.CommandContext(ctx, "git", args...) - cmd.Dir = p.checkoutPath + cmd := newGitCommand(ctx, p.runtime, p.checkoutPath, args...) if stdin != nil { cmd.Stdin = bytes.NewReader(stdin) } @@ -449,14 +485,43 @@ func (p *gitPusher) run(ctx context.Context, stdin []byte, args ...string) ([]by // and failure. Used when the caller needs to inspect git's diagnostic // output (e.g., to detect "previous cherry-pick is now empty"). func (p *gitPusher) runCombined(ctx context.Context, stdin []byte, args ...string) ([]byte, error) { - cmd := exec.CommandContext(ctx, "git", args...) - cmd.Dir = p.checkoutPath + cmd := newGitCommand(ctx, p.runtime, p.checkoutPath, args...) if stdin != nil { cmd.Stdin = bytes.NewReader(stdin) } return cmd.CombinedOutput() } +// newGitCommand constructs a Git command without inheriting the caller's +// environment. The executable and helper paths come from the pinned runtime; +// repository-local configuration remains an intentional input. +func newGitCommand(ctx context.Context, runtime GitRuntime, dir string, args ...string) *exec.Cmd { + gitArgs := make([]string, 0, len(args)+3) + gitArgs = append(gitArgs, + "--exec-path="+runtime.ExecPath, + "-c", "init.templateDir="+runtime.TemplateDir, + ) + gitArgs = append(gitArgs, args...) + + cmd := exec.CommandContext(ctx, runtime.Executable, gitArgs...) + cmd.Dir = dir + cmd.Env = []string{ + "HOME=" + filepath.Join(dir, ".submitqueue-git-home"), + "XDG_CONFIG_HOME=" + filepath.Join(dir, ".submitqueue-git-home", "xdg"), + "GIT_CONFIG_NOSYSTEM=1", + "GIT_CONFIG_GLOBAL=" + os.DevNull, + "GIT_ATTR_NOSYSTEM=1", + "GIT_TERMINAL_PROMPT=0", + "GIT_PAGER=cat", + "GIT_EDITOR=:", + "GIT_EXEC_PATH=" + runtime.ExecPath, + "GIT_TEMPLATE_DIR=" + runtime.TemplateDir, + "LC_ALL=C", + "LANG=C", + } + return cmd +} + // isRedundantCherryPick reports whether git's cherry-pick output indicates // the pick was rejected because the change is already present on target // (i.e. applying it would produce no diff). diff --git a/submitqueue/extension/pusher/git/git_pusher_test.go b/submitqueue/extension/pusher/git/git_pusher_test.go index 40b5428f..27791298 100644 --- a/submitqueue/extension/pusher/git/git_pusher_test.go +++ b/submitqueue/extension/pusher/git/git_pusher_test.go @@ -20,7 +20,6 @@ import ( "errors" "fmt" "os" - "os/exec" "path/filepath" "strconv" "strings" @@ -37,6 +36,8 @@ import ( "github.com/uber/submitqueue/submitqueue/extension/pusher" ) +const pinnedGitVersion = "2.55.0" + // gitFixture provides a bare "remote" repository plus a working checkout // that pushes to it. Tests run real `git` commands so we exercise the same // code path as production. @@ -55,10 +56,6 @@ type gitFixture struct { func setupGitFixture(t *testing.T) gitFixture { t.Helper() - if _, err := exec.LookPath("git"); err != nil { - t.Skip("git not available on PATH") - } - root := t.TempDir() remoteDir := filepath.Join(root, "remote.git") checkoutDir := filepath.Join(root, "checkout") @@ -88,6 +85,74 @@ func setupGitFixture(t *testing.T) gitFixture { } } +func TestGitRuntimeValidate(t *testing.T) { + abs, err := filepath.Abs("git") + require.NoError(t, err) + tests := []struct { + name string + runtime GitRuntime + wantErr bool + }{ + { + name: "valid", + runtime: GitRuntime{ + Executable: abs, + ExecPath: abs, + TemplateDir: abs, + }, + }, + { + name: "missing executable", + runtime: GitRuntime{ + ExecPath: abs, + TemplateDir: abs, + }, + wantErr: true, + }, + { + name: "relative exec path", + runtime: GitRuntime{ + Executable: abs, + ExecPath: "git-core", + TemplateDir: abs, + }, + wantErr: true, + }, + { + name: "relative template dir", + runtime: GitRuntime{ + Executable: abs, + ExecPath: abs, + TemplateDir: "templates", + }, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := tt.runtime.validate() + if tt.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + }) + } +} + +func TestNewGitCommandDoesNotInheritEnvironment(t *testing.T) { + t.Setenv("SUBMITQUEUE_GIT_AMBIENT", "ambient") + runtime := testGitRuntime(t) + + cmd := newGitCommand(context.Background(), runtime, t.TempDir(), "--version") + + assert.Equal(t, runtime.Executable, cmd.Path) + assert.NotContains(t, cmd.Env, "SUBMITQUEUE_GIT_AMBIENT=ambient") + assert.Contains(t, cmd.Env, "GIT_CONFIG_NOSYSTEM=1") + assert.Contains(t, cmd.Env, "GIT_EXEC_PATH="+runtime.ExecPath) +} + // batchFor seeds the fixture resolver so batch id resolves to the given changes, // and returns the batch to pass to Push. func (f gitFixture) batchFor(id string, changes ...change.Change) entity.Batch { @@ -159,14 +224,17 @@ func uri(sha string) string { } func (f gitFixture) newPusher(t *testing.T) pusher.Pusher { - return NewPusher(Params{ + p, err := NewPusher(Params{ CheckoutPath: f.checkoutDir, Remote: "origin", Target: "main", Resolver: f.resolver, Logger: zaptest.NewLogger(t).Sugar(), MetricsScope: tally.NoopScope, + Runtime: testGitRuntime(t), }) + require.NoError(t, err) + return p } // installRaceHook writes a pre-receive hook on the bare remote that @@ -204,7 +272,7 @@ fi # Pre-receive runs in git's quarantine env; unset its markers so update-ref # is allowed to mutate the live ref store. unset GIT_QUARANTINE_PATH GIT_OBJECT_DIRECTORY GIT_ALTERNATE_OBJECT_DIRECTORIES -git update-ref refs/heads/main "$next_sha" +"$GIT_EXEC_PATH/git" update-ref refs/heads/main "$next_sha" echo "race hook moved main to $next_sha and rejected push" >&2 exit 1 ` @@ -472,16 +540,18 @@ func TestPusher_Push_GivesUpAfterMaxAttempts(t *testing.T) { featureSHA := f.pushPRCommit(t, "feature/a", "hello.txt", "hello\nearth\n", "tweak hello") f.installRaceHook(t, raceSHAs) - p := NewPusher(Params{ + p, err := NewPusher(Params{ CheckoutPath: f.checkoutDir, Remote: "origin", Target: "main", Resolver: f.resolver, Logger: zaptest.NewLogger(t).Sugar(), MetricsScope: tally.NoopScope, + Runtime: testGitRuntime(t), MaxPushAttempts: 2, }) - _, err := p.Push(context.Background(), []entity.Batch{ + require.NoError(t, err) + _, err = p.Push(context.Background(), []entity.Batch{ f.batchFor("b", change.Change{URIs: []string{uri(featureSHA)}}), }) require.Error(t, err) @@ -494,10 +564,14 @@ func TestPusher_Push_GivesUpAfterMaxAttempts(t *testing.T) { // --- helpers --- +func TestPinnedGitVersion(t *testing.T) { + out := mustGitOutput(t, t.TempDir(), "--version") + assert.Equal(t, "git version "+pinnedGitVersion, strings.TrimSpace(string(out))) +} + func mustGit(t *testing.T, dir string, args ...string) { t.Helper() - cmd := exec.Command("git", args...) - cmd.Dir = dir + cmd := newGitCommand(context.Background(), testGitRuntime(t), dir, args...) var stderr bytes.Buffer cmd.Stderr = &stderr require.NoError(t, cmd.Run(), "git %s: %s", strings.Join(args, " "), stderr.String()) @@ -505,8 +579,7 @@ func mustGit(t *testing.T, dir string, args ...string) { func mustGitOutput(t *testing.T, dir string, args ...string) []byte { t.Helper() - cmd := exec.Command("git", args...) - cmd.Dir = dir + cmd := newGitCommand(context.Background(), testGitRuntime(t), dir, args...) var stdout, stderr bytes.Buffer cmd.Stdout = &stdout cmd.Stderr = &stderr @@ -514,6 +587,56 @@ func mustGitOutput(t *testing.T, dir string, args ...string) []byte { return stdout.Bytes() } +func testGitRuntime(t *testing.T) GitRuntime { + t.Helper() + executable := absoluteTestPath(t, "SUBMITQUEUE_TEST_GIT") + templateDescription := absoluteTestPath(t, "SUBMITQUEUE_TEST_GIT_TEMPLATE_DESCRIPTION") + return GitRuntime{ + Executable: executable, + ExecPath: filepath.Dir(executable), + TemplateDir: filepath.Dir(templateDescription), + } +} + +func absoluteTestPath(t *testing.T, name string) string { + t.Helper() + path := os.Getenv(name) + require.NotEmpty(t, path) + if filepath.IsAbs(path) { + _, err := os.Stat(path) + require.NoError(t, err) + return path + } + if absolute, err := filepath.Abs(path); err == nil { + if _, err := os.Stat(absolute); err == nil { + return absolute + } + } + + // rules_go expands $(location) to an execroot-relative path. Tests run + // from their main-repository runfiles directory, so translate an external + // output to its canonical repository path under TEST_SRCDIR. + const externalMarker = "/external/" + slashed := filepath.ToSlash(path) + externalPath := "" + if strings.HasPrefix(slashed, "external/") { + externalPath = strings.TrimPrefix(slashed, "external/") + } else if i := strings.Index(slashed, externalMarker); i >= 0 { + externalPath = slashed[i+len(externalMarker):] + } + if externalPath != "" { + runfilesRoot := os.Getenv("TEST_SRCDIR") + require.NotEmpty(t, runfilesRoot) + candidate := filepath.Join(runfilesRoot, filepath.FromSlash(externalPath)) + _, err := os.Stat(candidate) + require.NoError(t, err) + return candidate + } + + require.FailNowf(t, "resolve test path", "%s=%q is not a runfile", name, path) + return "" +} + func writeFile(path, contents string) error { return os.WriteFile(path, []byte(contents), 0o644) }