From 4ae0fccacdd391ab20c4336a72396630196e873e Mon Sep 17 00:00:00 2001 From: Chenghan Ying <125171961+roychying@users.noreply.github.com> Date: Wed, 28 Oct 2026 14:23:13 -0700 Subject: [PATCH] refactor(runway): Run the git merger's commands through a gitworkspace.Workspace --- runway/extension/merger/git/BUILD.bazel | 4 +- runway/extension/merger/git/README.md | 12 +- runway/extension/merger/git/author.go | 48 ++- runway/extension/merger/git/git_merger.go | 276 ++++++++---------- .../extension/merger/git/git_merger_test.go | 79 +++-- runway/extension/merger/git/objects.go | 10 +- service/runway/server/main.go | 24 +- 7 files changed, 221 insertions(+), 232 deletions(-) diff --git a/runway/extension/merger/git/BUILD.bazel b/runway/extension/merger/git/BUILD.bazel index bd74dba16..064573a28 100644 --- a/runway/extension/merger/git/BUILD.bazel +++ b/runway/extension/merger/git/BUILD.bazel @@ -16,8 +16,7 @@ go_library( "//api/runway/messagequeue/protopb:go_default_library", "//platform/base/change/git:go_default_library", "//platform/base/change/github:go_default_library", - "//platform/extension/gitworkspace/local:go_default_library", - "//platform/git/exec:go_default_library", + "//platform/extension/gitworkspace:go_default_library", "//platform/metrics:go_default_library", "//runway/extension/merger:go_default_library", "@com_github_uber_go_tally//:go_default_library", @@ -46,6 +45,7 @@ go_test( "//api/base/mergestrategy/protopb:go_default_library", "//api/runway/messagequeue:go_default_library", "//api/runway/messagequeue/protopb:go_default_library", + "//platform/extension/gitworkspace:go_default_library", "//platform/extension/gitworkspace/local:go_default_library", "//platform/git/exec:go_default_library", "//platform/git/exectest:go_default_library", diff --git a/runway/extension/merger/git/README.md b/runway/extension/merger/git/README.md index 0f7721b1f..0d14b7659 100644 --- a/runway/extension/merger/git/README.md +++ b/runway/extension/merger/git/README.md @@ -1,6 +1,6 @@ # git merger -A `merger.Merger` backed by the `git` CLI operating on a local checkout. It applies a merge request's ordered steps onto a target branch, honoring each step's strategy, and — for a committing merge — pushes the result. It is constructed by the wiring layer (see [`service/runway`](../../../../service/runway)) with the checkout path, remote, target branch, pinned git runtime, committer identity, and the default strategy; the request itself carries only a queue name and the changes, so the merger reads change URIs straight from the payload (no store, no resolver). +A `merger.Merger` that drives the `git` CLI through a [`gitworkspace.Workspace`](../../../../platform/extension/gitworkspace). It applies a merge request's ordered steps onto a target branch, honoring each step's strategy, and — for a committing merge — pushes the result. It is constructed by the wiring layer (see [`service/runway`](../../../../service/runway)) with a workspace factory, remote, target branch, and the default strategy; the request itself carries only a queue name and the changes, so the merger reads change URIs straight from the payload (no store, no resolver). ## Model @@ -57,13 +57,11 @@ Fetching by SHA guarantees the merger applies exactly the commit a URI names — ## Authorship -Git records an author and a committer separately, and the merger keeps them apart: the committer is always the merger's configured identity, because it is what applied the change, while the author is the person who wrote it. +Git records an author and a committer separately, and the merger keeps them apart: the committer is always the workspace's configured identity, because it is what applied the change, while the author is the person who wrote it. `REBASE` gets this for free — cherry-pick carries each commit's author across, so every landed commit keeps whoever wrote it. The strategies that mint a fresh commit do not: the squash commit and the `--no-ff` merge commit are new objects, and without attribution both would be credited to the service. Each is therefore authored as the author recorded on the commit its change's URI pins. For a change spanning several commits by different people, that is the head commit's author — the one identity the request actually names. -The author is read out of the local object store, so this costs no network and needs nothing on the wire: every referenced commit is already fetched and verified before a step is applied. A commit that records no usable author (either half missing) falls back to the committer identity rather than failing the merge. - -The author travels to git through `GIT_AUTHOR_NAME`/`GIT_AUTHOR_EMAIL` rather than `git commit --author`, because `git merge` has no `--author` flag and because the environment keeps the name and address as separate values — a single `Name
` string has to be parsed back apart, which a display name containing an angle bracket breaks. +The author is read out of the local object store, so this costs no network and needs nothing on the wire: every referenced commit is already fetched and verified before a step is applied. The author travels as `git commit --author`, since a workspace command carries arguments only. `git merge` has no `--author` flag, so a `MERGE` step merges with `--no-commit` and then commits the merge with the author. A commit that records no usable author falls back to the committer identity rather than failing the merge: either half missing, or a name or address containing an angle bracket or line break, which `--author`'s single `Name
` string cannot carry. ## Importing an unrelated history @@ -87,9 +85,9 @@ A merge conflict surfaces as `merger.ErrConflict`. An unusable request surfaces The distinction between the last two matters operationally: a commit that is missing while the remote answers is a property of the request, whereas a remote that will not answer is a property of the moment. -## Runtime and identity +## Workspace, runtime and identity -Every git invocation uses the pinned runtime (explicit executable, exec-path, and template dir) and a scrubbed environment: no ambient configuration, no system or global git config, no interactive prompts. Because that leaves no ambient identity, the committer name and email are injected per-invocation, which the commit-creating strategies (`REBASE`, `SQUASH_REBASE`, `MERGE`) require. +The merger opens one workspace per operation and runs every git command in it, one command per batch. Where git runs, which git it is, and who it commits as belong to the workspace, not the merger. The reference wiring uses the local workspace over the configured checkout, with the pinned runtime (explicit executable, exec-path, and template dir) and a scrubbed environment: no ambient configuration, no system or global git config, no interactive prompts. Because that leaves no ambient identity, the workspace injects the committer name and email into every git command, which the commit-creating strategies (`REBASE`, `SQUASH_REBASE`, `MERGE`) require. Scrubbing denies git ambient *configuration* — anything that could change what a merge produces. It deliberately does not deny it the means to reach the remote, so the agent socket, `PATH`, ssh-command, TLS and proxy variables are inherited when set. Without them an SSH remote cannot authenticate and git cannot even exec `ssh`; none of them can influence merge semantics. A deployment needing more can name additional variables on the runtime. diff --git a/runway/extension/merger/git/author.go b/runway/extension/merger/git/author.go index 408effb95..80a25a2b8 100644 --- a/runway/extension/merger/git/author.go +++ b/runway/extension/merger/git/author.go @@ -36,34 +36,32 @@ type authorIdent struct { Email string } -// complete reports whether both halves are present. A half-known identity is -// unusable — git wants a name and an address, and substituting a placeholder -// for the missing half would attribute the commit to someone who does not -// exist, which is worse than not attributing it at all. -func (a authorIdent) complete() bool { - return a.Name != "" && a.Email != "" +// usable reports whether the identity can be passed to `git commit --author`. +// +// A half-known identity is unusable — git wants a name and an address, and +// substituting a placeholder for the missing half would attribute the commit +// to someone who does not exist, which is worse than not attributing it at +// all. So is one containing an angle bracket or a line break: --author takes a +// single "Name
" string that git parses back apart, and such a value +// splits in the wrong place or makes git search the history for a matching +// author and fail the commit outright. +func (a authorIdent) usable() bool { + if a.Name == "" || a.Email == "" { + return false + } + return !strings.ContainsAny(a.Name+a.Email, "<>\n") } -// env renders the identity as the environment git reads an author from. It -// returns nil for an incomplete identity, which leaves the configured committer -// identity to stand in for both roles — the behavior from before attribution -// existed. -// -// The identity travels as GIT_AUTHOR_* rather than as `git commit --author` -// for two reasons. `git merge` has no --author flag at all, so the merge -// strategy could not use one. And the environment carries name and address as -// two values, where --author takes a single "Name
" string that git -// has to parse back apart — a name containing an angle bracket splits in the -// wrong place, and one that parses to nothing makes git search the history for -// a matching author and fail the commit outright. -func (a authorIdent) env() []string { - if !a.complete() { +// args renders the identity as the `git commit` arguments that credit it. It +// returns nil for an unusable identity, which leaves the configured committer +// identity to stand in for both roles. The identity travels as an argument +// rather than through the environment because a workspace command carries +// arguments only. +func (a authorIdent) args() []string { + if !a.usable() { return nil } - return []string{ - "GIT_AUTHOR_NAME=" + a.Name, - "GIT_AUTHOR_EMAIL=" + a.Email, - } + return []string{"--author", a.Name + " <" + a.Email + ">"} } // commitAuthor reads the author recorded on a commit. @@ -80,7 +78,7 @@ func (m *gitMerger) commitAuthor(ctx context.Context, sha string) (authorIdent, // NUL-separated: a display name may contain spaces, angle brackets, quotes // or anything else a person put in their git config, and none of those can // be mistaken for the field separator. - out, err := m.run(ctx, nil, "show", "--no-patch", "--format=%an%x00%ae", sha) + out, err := m.run(ctx, "show", "--no-patch", "--format=%an%x00%ae", sha) if err != nil { return authorIdent{}, fmt.Errorf("git show --no-patch --format author %s: %w", sha, err) } diff --git a/runway/extension/merger/git/git_merger.go b/runway/extension/merger/git/git_merger.go index 9dec7adcd..981f02333 100644 --- a/runway/extension/merger/git/git_merger.go +++ b/runway/extension/merger/git/git_merger.go @@ -12,9 +12,10 @@ // See the License for the specific language governing permissions and // limitations under the License. -// Package git is a Merger implementation backed by a local git checkout. It -// applies a MergeRequest's ordered steps onto a target branch, honoring each -// step's merge strategy, and (for a committing Merge) pushes the result. +// Package git is a Merger implementation that drives git through a +// gitworkspace.Workspace. It applies a MergeRequest's ordered steps onto a +// target branch, honoring each step's merge strategy, and (for a committing +// Merge) pushes the result. // // Strategy → git operation: // @@ -30,8 +31,8 @@ // - DEFAULT: resolved to the instance's configured DefaultStrategy // before any step runs. // -// Authorship: the committer is always the merger's configured identity, since -// it is what applied the change. The author is the person who wrote it — +// Authorship: the committer is always the workspace's configured identity, +// since it is what applied the change. The author is the person who wrote it — // carried across automatically by cherry-pick under REBASE, and set explicitly // on the commits SQUASH_REBASE and MERGE mint, from the author recorded on the // commit each change's URI pins. See author.go. @@ -68,7 +69,6 @@ import ( "context" "errors" "fmt" - "os/exec" "strings" "sync" @@ -78,8 +78,7 @@ import ( mergestrategypb "github.com/uber/submitqueue/api/base/mergestrategy/protopb" runwaymq "github.com/uber/submitqueue/api/runway/messagequeue" runwaypb "github.com/uber/submitqueue/api/runway/messagequeue/protopb" - gitlocal "github.com/uber/submitqueue/platform/extension/gitworkspace/local" - gitexec "github.com/uber/submitqueue/platform/git/exec" + "github.com/uber/submitqueue/platform/extension/gitworkspace" coremetrics "github.com/uber/submitqueue/platform/metrics" "github.com/uber/submitqueue/runway/extension/merger" ) @@ -88,20 +87,14 @@ import ( // under us. Bounded to prevent an infinite loop against a busy remote. const defaultMaxPushAttempts = 10 -// Default committer identity used when Params leaves it unset. The scrubbed -// environment (GIT_CONFIG_NOSYSTEM, GIT_CONFIG_GLOBAL=/dev/null) leaves no -// ambient identity, so commit-creating strategies (REBASE/SQUASH_REBASE/MERGE) -// need one supplied explicitly. -const ( - defaultCommitterName = "SubmitQueue Runway" - defaultCommitterEmail = "runway@submitqueue.invalid" -) - // Params holds the dependencies for the git Merger. type Params struct { - // CheckoutPath is the absolute path to an existing git checkout that the - // Merger will operate against. The Merger owns this working tree. - CheckoutPath string + // Workspaces opens the git workspace every operation runs in. The workspace + // owns the checkout, the git runtime and the committer identity, and the + // Merger must be the only user of the checkout it opens. + Workspaces gitworkspace.Factory + // Repo names the repository Workspaces opens. + Repo string // Remote is the name of the git remote to fetch from and push to // (e.g. "origin"). Remote string @@ -110,8 +103,6 @@ type Params struct { // DefaultStrategy resolves a step whose strategy is DEFAULT. Must be a // concrete strategy (REBASE, SQUASH_REBASE, MERGE, or PROMOTE). DefaultStrategy mergestrategypb.Strategy - // Runtime is the pinned Git runtime used for every invocation. - Runtime gitlocal.Runtime // MaxPushAttempts caps how many times a committing merge retries the full // reset/apply/push cycle when the remote tip moves under it. Defaults to // defaultMaxPushAttempts when zero or negative. @@ -132,38 +123,33 @@ type Params struct { // producing a nonsense result. Enable it on a queue that exists to perform // such an import. AllowUnrelatedHistories bool - // CommitterName / CommitterEmail identify the author/committer of - // service-created commits. Defaults are used when empty. - CommitterName string - CommitterEmail string // Logger is the structured logger. Logger *zap.SugaredLogger // MetricsScope is the metrics scope for instrumentation. MetricsScope tally.Scope } -// gitMerger implements merger.Merger by shelling out to the `git` CLI against a -// local checkout. +// gitMerger implements merger.Merger by running `git` commands in a workspace. type gitMerger struct { - checkoutPath string + workspaces gitworkspace.Factory + repo string remote string target string defaultStrategy mergestrategypb.Strategy - runtime gitlocal.Runtime maxPushAttempts int fetchRefspecs []string checkStaleness bool // allowUnrelatedHistories permits a MERGE across disjoint history graphs. allowUnrelatedHistories bool - committerName string - committerEmail string logger *zap.SugaredLogger metricsScope tally.Scope // mu serializes concurrent operations — the underlying checkout cannot be // safely shared between operations. mu sync.Mutex + // ws is the workspace of the operation holding mu, nil between operations. + ws gitworkspace.Workspace } // Verify gitMerger implements merger.Merger at compile time. @@ -180,12 +166,12 @@ type resolvedStep struct { refs []changeRef } -// NewMerger constructs a git-backed Merger operating against the given checkout. -// The checkout must already exist and have the configured remote. Runtime paths -// must be absolute and DefaultStrategy must be a concrete strategy. +// NewMerger constructs a git-backed Merger running in the workspaces Params +// opens. The checkout must already exist and have the configured remote, and +// DefaultStrategy must be a concrete strategy. func NewMerger(params Params) (merger.Merger, error) { - if err := params.Runtime.Validate(); err != nil { - return nil, err + if params.Workspaces == nil { + return nil, errors.New("git workspace factory is required") } if !isConcreteStrategy(params.DefaultStrategy) { return nil, fmt.Errorf("default strategy must be concrete (REBASE, SQUASH_REBASE, MERGE, or PROMOTE), got %v", params.DefaultStrategy) @@ -194,27 +180,17 @@ func NewMerger(params Params) (merger.Merger, error) { if maxAttempts <= 0 { maxAttempts = defaultMaxPushAttempts } - committerName := params.CommitterName - if committerName == "" { - committerName = defaultCommitterName - } - committerEmail := params.CommitterEmail - if committerEmail == "" { - committerEmail = defaultCommitterEmail - } return &gitMerger{ - checkoutPath: params.CheckoutPath, + workspaces: params.Workspaces, + repo: params.Repo, remote: params.Remote, target: params.Target, defaultStrategy: params.DefaultStrategy, - runtime: params.Runtime, maxPushAttempts: maxAttempts, fetchRefspecs: params.FetchRefspecs, checkStaleness: params.CheckStaleness, allowUnrelatedHistories: params.AllowUnrelatedHistories, - committerName: committerName, - committerEmail: committerEmail, logger: params.Logger.Named("git_merger"), metricsScope: params.MetricsScope.SubScope("git_merger"), }, nil @@ -252,6 +228,18 @@ func (m *gitMerger) process(ctx context.Context, req *runwaymq.MergeRequest, com m.mu.Lock() defer m.mu.Unlock() + ws, err := m.workspaces.For(ctx, m.repo) + if err != nil { + return nil, fmt.Errorf("open git workspace for %s: %w", m.repo, err) + } + m.ws = ws + defer func() { + m.ws = nil + if err := ws.Close(); err != nil { + m.logger.Warnw("closing git workspace failed", "repo", m.repo, "err", err) + } + }() + m.logger.Debugw("starting merge", "id", req.GetId(), "target", m.target, @@ -548,13 +536,13 @@ func (m *gitMerger) squashChange(ctx context.Context, step *runwaymq.MergeStep, return "", false, err } if preTree == postTree { - if _, err := m.run(ctx, nil, "reset", "--hard", preSHA); err != nil { + if _, err := m.run(ctx, "reset", "--hard", preSHA); err != nil { return "", false, fmt.Errorf("git reset --hard %s after empty squash: %w", preSHA, err) } return "", false, nil } - if _, err := m.run(ctx, nil, "reset", "--soft", preSHA); err != nil { + if _, err := m.run(ctx, "reset", "--soft", preSHA); err != nil { return "", false, fmt.Errorf("git reset --soft %s: %w", preSHA, err) } // The squash is a new commit, so it carries no authorship of its own. @@ -563,7 +551,7 @@ func (m *gitMerger) squashChange(ctx context.Context, step *runwaymq.MergeStep, if err != nil { return "", false, err } - if _, err := m.runAs(ctx, author, nil, "commit", "-m", squashMessage(step, ref)); err != nil { + if _, err := m.run(ctx, append([]string{"commit", "-m", squashMessage(step, ref)}, author.args()...)...); err != nil { return "", false, fmt.Errorf("git commit (squash): %w", err) } sha, err := m.headSHA(ctx) @@ -589,25 +577,31 @@ func (m *gitMerger) applyMerge(ctx context.Context, rs resolvedStep) (applied, e continue } - args := []string{"merge", "--no-ff", "--no-edit"} + args := []string{"merge", "--no-ff", "--no-commit"} if m.allowUnrelatedHistories { args = append(args, "--allow-unrelated-histories") } - // The merge commit is minted by the merger, so it would otherwise be - // authored by the service. Credit it to the change being merged; the - // change's own commits keep their authors through the second parent. - author, err := m.commitAuthor(ctx, ref.SHA) - if err != nil { - return applied{}, err - } - o, err := m.runCombinedAs(ctx, author, nil, append(args, ref.SHA)...) + o, err := m.runCombined(ctx, append(args, ref.SHA)...) if err != nil { // Read the index before aborting clears it; a non-zero exit alone // does not establish that anything collided. conflicted := m.hasUnmergedPaths(ctx) - _, _ = m.run(ctx, nil, "merge", "--abort") + _, _ = m.run(ctx, "merge", "--abort") return applied{}, m.classifyMergeFailure(ref, o, conflicted, err) } + // The merge commit is minted by the merger, so it would otherwise be + // authored by the service. git merge takes no --author, so the merge + // stops short of committing and the commit credits the change's author; + // the change's own commits keep theirs through the second parent. + author, err := m.commitAuthor(ctx, ref.SHA) + if err != nil { + _, _ = m.run(ctx, "merge", "--abort") + return applied{}, err + } + if _, err := m.run(ctx, append([]string{"commit", "--no-edit"}, author.args()...)...); err != nil { + _, _ = m.run(ctx, "merge", "--abort") + return applied{}, fmt.Errorf("git commit (merge %s): %w", ref.SHA, err) + } mergeSHA, err := m.headSHA(ctx) if err != nil { return applied{}, err @@ -674,7 +668,7 @@ func (m *gitMerger) promote(ctx context.Context, req *runwaymq.MergeRequest, rs } refspec := sha + ":refs/heads/" + m.target - if _, err := m.run(ctx, nil, "push", m.remote, refspec); err != nil { + if _, err := m.run(ctx, "push", m.remote, refspec); err != nil { // The target may have moved under us; re-fetch and re-classify. coremetrics.NamedCounter(m.metricsScope, "promote", "push_retries", 1) m.logger.Warnw("promote push failed, re-classifying", @@ -776,17 +770,17 @@ func (m *gitMerger) pickRange(ctx context.Context, ref changeRef) ([]string, err // the attempt, and the in-progress pick is aborted so the checkout stays // usable for the next request. func (m *gitMerger) cherryPickRange(ctx context.Context, base, head string) error { - out, err := m.runCombined(ctx, nil, "cherry-pick", base+".."+head) + out, err := m.runCombined(ctx, "cherry-pick", base+".."+head) for err != nil { if isRedundantCherryPick(out) { - out, err = m.runCombined(ctx, nil, "cherry-pick", "--skip") + out, err = m.runCombined(ctx, "cherry-pick", "--skip") continue } // Ask the index what happened before aborting clears it. A non-zero // exit alone does not establish a conflict; without unmerged paths, // preserve the command error for infrastructure classification. conflicted := m.hasUnmergedPaths(ctx) - _, _ = m.run(ctx, nil, "cherry-pick", "--abort") + _, _ = m.run(ctx, "cherry-pick", "--abort") detail := strings.TrimSpace(string(out)) if !conflicted { coremetrics.NamedCounter(m.metricsScope, "merge", "cherry_pick_errors", 1) @@ -803,7 +797,7 @@ func (m *gitMerger) cherryPickRange(ctx context.Context, base, head string) erro // invocation exited non-zero, and it has to be read before the operation is // aborted, since aborting clears the index. func (m *gitMerger) hasUnmergedPaths(ctx context.Context) bool { - out, err := m.run(ctx, nil, "ls-files", "--unmerged") + out, err := m.run(ctx, "ls-files", "--unmerged") return err == nil && len(bytes.TrimSpace(out)) > 0 } @@ -811,15 +805,17 @@ func (m *gitMerger) hasUnmergedPaths(ctx context.Context) bool { // share no history. git exits 1 for the latter, which is an answer rather than // a failure. func (m *gitMerger) mergeBase(ctx context.Context, a, b string) (string, error) { - out, err := m.run(ctx, nil, "merge-base", a, b) - if err == nil { - return strings.TrimSpace(string(out)), nil + cmd, out, err := m.execGit("merge-base", a, b) + if err != nil { + return "", err } - var exitErr *exec.ExitError - if errors.As(err, &exitErr) && exitErr.ExitCode() == 1 { + switch out.ExitCode { + case 0: + return strings.TrimSpace(out.Stdout), nil + case 1: return "", nil } - return "", fmt.Errorf("git merge-base %s %s: %w", a, b, err) + return "", fmt.Errorf("git merge-base %s %s: %w", a, b, gitworkspace.CommandFailure(ctx, cmd, out)) } // revList lists the commits reachable from `to` but not from `from`, oldest @@ -827,7 +823,7 @@ func (m *gitMerger) mergeBase(ctx context.Context, a, b string) (string, error) // rather than tracked per-invocation, which stays correct when the sequencer // drops some of them. func (m *gitMerger) revList(ctx context.Context, from, to string) ([]string, error) { - out, err := m.run(ctx, nil, "rev-list", "--reverse", from+".."+to) + out, err := m.run(ctx, "rev-list", "--reverse", from+".."+to) if err != nil { return nil, fmt.Errorf("git rev-list %s..%s: %w", from, to, err) } @@ -838,22 +834,22 @@ func (m *gitMerger) revList(ctx context.Context, from, to string) ([]string, err // HEAD to refs/remotes//, producing a clean working tree pinned // to the latest remote tip. func (m *gitMerger) resetToRemote(ctx context.Context) error { - if _, err := m.run(ctx, nil, "fetch", m.remote); err != nil { + if _, err := m.run(ctx, "fetch", m.remote); err != nil { return fmt.Errorf("git fetch %s: %w", m.remote, err) } // Deployment-supplied refspecs, for a server that refuses a fetch for an // unadvertised object and so cannot satisfy the by-SHA fetch in // ensureObject. Normally empty. for _, refspec := range m.fetchRefspecs { - if _, err := m.run(ctx, nil, "fetch", m.remote, refspec); err != nil { + if _, err := m.run(ctx, "fetch", m.remote, refspec); err != nil { return fmt.Errorf("git fetch %s %s: %w", m.remote, refspec, err) } } remoteRef := m.remote + "/" + m.target - if _, err := m.run(ctx, nil, "reset", "--hard", remoteRef); err != nil { + if _, err := m.run(ctx, "reset", "--hard", remoteRef); err != nil { return fmt.Errorf("git reset --hard %s: %w", remoteRef, err) } - if _, err := m.run(ctx, nil, "clean", "-fdx"); err != nil { + if _, err := m.run(ctx, "clean", "-fdx"); err != nil { return fmt.Errorf("git clean: %w", err) } return nil @@ -861,7 +857,7 @@ func (m *gitMerger) resetToRemote(ctx context.Context) error { // headSHA returns the SHA at HEAD in the local checkout. func (m *gitMerger) headSHA(ctx context.Context) (string, error) { - out, err := m.run(ctx, nil, "rev-parse", "HEAD") + out, err := m.run(ctx, "rev-parse", "HEAD") if err != nil { return "", fmt.Errorf("git rev-parse HEAD: %w", err) } @@ -872,11 +868,11 @@ func (m *gitMerger) headSHA(ctx context.Context) (string, error) { // refs/remotes//. Used after a push failure to detect whether // the remote tip moved under us. func (m *gitMerger) refetchTipSHA(ctx context.Context) (string, error) { - if _, err := m.run(ctx, nil, "fetch", m.remote); err != nil { + if _, err := m.run(ctx, "fetch", m.remote); err != nil { return "", fmt.Errorf("git fetch %s: %w", m.remote, err) } remoteRef := m.remote + "/" + m.target - out, err := m.run(ctx, nil, "rev-parse", remoteRef) + out, err := m.run(ctx, "rev-parse", remoteRef) if err != nil { return "", fmt.Errorf("git rev-parse %s: %w", remoteRef, err) } @@ -887,29 +883,23 @@ func (m *gitMerger) refetchTipSHA(ctx context.Context) (string, error) { // descendant. `git merge-base --is-ancestor` exits 0 for true, 1 for false; // any other exit is a real error. func (m *gitMerger) isAncestor(ctx context.Context, ancestor, descendant string) (bool, error) { - args := []string{"merge-base", "--is-ancestor", ancestor, descendant} - cmd := m.command(ctx, args...) - var stderr bytes.Buffer - cmd.Stderr = &stderr - err := cmd.Run() - if err == nil { - return true, nil + cmd, out, err := m.execGit("merge-base", "--is-ancestor", ancestor, descendant) + if err != nil { + return false, err } - var exitErr *exec.ExitError - if errors.As(err, &exitErr) && exitErr.ExitCode() == 1 { + switch out.ExitCode { + case 0: + return true, nil + case 1: return false, nil } - message := err.Error() - if detail := strings.TrimSpace(stderr.String()); detail != "" { - message += ": " + detail - } return false, fmt.Errorf("git merge-base --is-ancestor %s %s: %w", - ancestor, descendant, gitexec.CommandFailure(ctx, args, message, err)) + ancestor, descendant, gitworkspace.CommandFailure(ctx, cmd, out)) } // commitTreeSHA returns the tree SHA recorded in the commit object at ref. func (m *gitMerger) commitTreeSHA(ctx context.Context, ref string) (string, error) { - out, err := m.run(ctx, nil, "rev-parse", ref+"^{tree}") + out, err := m.run(ctx, "rev-parse", ref+"^{tree}") if err != nil { return "", fmt.Errorf("git rev-parse %s^{tree}: %w", ref, err) } @@ -919,82 +909,48 @@ func (m *gitMerger) commitTreeSHA(ctx context.Context, ref string) (string, erro // push pushes the current HEAD to refs/heads/ on the remote. func (m *gitMerger) push(ctx context.Context) error { refspec := "HEAD:refs/heads/" + m.target - if _, err := m.run(ctx, nil, "push", m.remote, refspec); err != nil { + if _, err := m.run(ctx, "push", m.remote, refspec); err != nil { return fmt.Errorf("git push %s %s: %w", m.remote, refspec, err) } return nil } -// run executes a `git` command in the checkout. Returns captured stdout and an -// error that includes captured stderr for diagnostics. -func (m *gitMerger) run(ctx context.Context, stdin []byte, args ...string) ([]byte, error) { - return m.runAs(ctx, authorIdent{}, stdin, args...) -} - -// runAs is run with any commit the command creates credited to the given -// author. A zero author leaves the committer identity to author it too. -func (m *gitMerger) runAs(ctx context.Context, author authorIdent, stdin []byte, args ...string) ([]byte, error) { - cmd := m.commandAs(ctx, author, args...) - if stdin != nil { - cmd.Stdin = bytes.NewReader(stdin) - } - var stdout, stderr bytes.Buffer - cmd.Stdout = &stdout - cmd.Stderr = &stderr - if err := cmd.Run(); err != nil { - message := err.Error() - if detail := strings.TrimSpace(stderr.String()); detail != "" { - message += ": " + detail - } - return nil, gitexec.CommandFailure(ctx, args, message, err) +// execGit runs one git command in the operation's workspace. A non-zero exit +// is reported in the Output; the error is reserved for not running it at all. +func (m *gitMerger) execGit(args ...string) (gitworkspace.Command, gitworkspace.Output, error) { + cmd := gitworkspace.Command{Alias: args[0], Bin: "git", Args: args} + outs, err := m.ws.Exec([]gitworkspace.Command{cmd}) + if err != nil { + return cmd, gitworkspace.Output{}, fmt.Errorf("git %s: %w", args[0], err) } - return stdout.Bytes(), nil -} - -// runCombined is like run but returns combined stdout+stderr both on success -// and failure. Used when the caller needs to inspect git's diagnostic output. -func (m *gitMerger) runCombined(ctx context.Context, stdin []byte, args ...string) ([]byte, error) { - return m.runCombinedAs(ctx, authorIdent{}, stdin, args...) + return cmd, outs[0], nil } -// runCombinedAs is runCombined with any commit the command creates credited to -// the given author. -func (m *gitMerger) runCombinedAs(ctx context.Context, author authorIdent, stdin []byte, args ...string) ([]byte, error) { - cmd := m.commandAs(ctx, author, args...) - if stdin != nil { - cmd.Stdin = bytes.NewReader(stdin) - } - out, err := cmd.CombinedOutput() +// run executes a git command and returns its stdout. A non-zero exit is an +// error carrying the command's diagnostics for classification. +func (m *gitMerger) run(ctx context.Context, args ...string) ([]byte, error) { + cmd, out, err := m.execGit(args...) if err != nil { - message := err.Error() - if detail := strings.TrimSpace(string(out)); detail != "" { - message += ": " + detail - } - return out, gitexec.CommandFailure(ctx, args, message, err) + return nil, err } - return out, nil -} - -// command builds a git command with the committer identity injected via -c -// flags, on top of the pinned runtime and scrubbed environment. -func (m *gitMerger) command(ctx context.Context, args ...string) *exec.Cmd { - return m.commandAs(ctx, authorIdent{}, args...) + if out.ExitCode != 0 { + return nil, gitworkspace.CommandFailure(ctx, cmd, out) + } + return []byte(out.Stdout), nil } -// commandAs is command with the author supplied through the environment. The -// committer identity is injected either way, so the author overrides only who -// the commit is credited to, never who is recorded as having applied it. -func (m *gitMerger) commandAs(ctx context.Context, author authorIdent, args ...string) *exec.Cmd { - withIdentity := make([]string, 0, len(args)+6) - withIdentity = append(withIdentity, - "-c", "user.name="+m.committerName, - "-c", "user.email="+m.committerEmail, - "-c", "commit.gpgsign=false", - ) - withIdentity = append(withIdentity, args...) - cmd := m.runtime.Command(ctx, m.checkoutPath, withIdentity...) - cmd.Env = append(cmd.Env, author.env()...) - return cmd +// runCombined is like run but returns stdout and stderr together, on success +// and on failure, for callers that inspect git's diagnostics. +func (m *gitMerger) runCombined(ctx context.Context, args ...string) ([]byte, error) { + cmd, out, err := m.execGit(args...) + if err != nil { + return nil, err + } + combined := []byte(out.Stdout + out.Stderr) + if out.ExitCode != 0 { + return combined, gitworkspace.CommandFailure(ctx, cmd, out) + } + return combined, nil } // isConcreteStrategy reports whether s names a concrete integration strategy diff --git a/runway/extension/merger/git/git_merger_test.go b/runway/extension/merger/git/git_merger_test.go index e660cad98..fcf8e225a 100644 --- a/runway/extension/merger/git/git_merger_test.go +++ b/runway/extension/merger/git/git_merger_test.go @@ -35,6 +35,7 @@ import ( mergestrategypb "github.com/uber/submitqueue/api/base/mergestrategy/protopb" runwaymq "github.com/uber/submitqueue/api/runway/messagequeue" runwaypb "github.com/uber/submitqueue/api/runway/messagequeue/protopb" + "github.com/uber/submitqueue/platform/extension/gitworkspace" gitlocal "github.com/uber/submitqueue/platform/extension/gitworkspace/local" gitexec "github.com/uber/submitqueue/platform/git/exec" gitexectest "github.com/uber/submitqueue/platform/git/exectest" @@ -98,7 +99,7 @@ func TestCherryPickRange_NonConflictFailureIsRetryable(t *testing.T) { // object or an unreadable repository. Reporting those as ErrConflict would // permanently fail a request that a retry could complete. f := setupGitFixture(t) - m := f.newMerger(t, mergestrategypb.Strategy_REBASE).(*gitMerger) + m := f.openMerger(t) err := m.cherryPickRange(context.Background(), "notarevision", "alsonotarevision") require.Error(t, err) @@ -113,7 +114,7 @@ func TestCherryPickRange_RealConflictIsErrConflict(t *testing.T) { mainBefore, conflictingSHA := f.setupConflict(t) require.NotEmpty(t, mainBefore) - m := f.newMerger(t, mergestrategypb.Strategy_REBASE).(*gitMerger) + m := f.openMerger(t) require.NoError(t, m.resetToRemote(context.Background())) require.NoError(t, m.ensureObjects(context.Background(), []changeRef{{SHA: conflictingSHA}})) @@ -297,16 +298,13 @@ func TestMerge_Rebase_GivesUpAfterMaxAttempts(t *testing.T) { f.installRaceHook(t, raceSHAs) m, err := NewMerger(Params{ - CheckoutPath: f.checkoutDir, + Workspaces: f.workspaces(t), Remote: "origin", Target: "main", DefaultStrategy: mergestrategypb.Strategy_REBASE, - Runtime: testGitRuntime(t), MaxPushAttempts: 2, Logger: zaptest.NewLogger(t).Sugar(), MetricsScope: tally.NoopScope, - CommitterName: "Test", - CommitterEmail: "test@example.com", }) require.NoError(t, err) @@ -488,7 +486,7 @@ func TestMerge_Default_ResolvesToRebase(t *testing.T) { func TestClassifyMergeFailure(t *testing.T) { f := setupGitFixture(t) - m := f.newMerger(t, mergestrategypb.Strategy_REBASE).(*gitMerger) + m := f.openMerger(t) ref := changeRef{SHA: fakeSHA, Label: "uber/repo#1"} tests := []struct { @@ -539,8 +537,12 @@ func TestClassifyMergeFailure(t *testing.T) { // stronger claim than observing that the remote did not move. func (f gitFixture) newUnrunnableMerger(t *testing.T) merger.Merger { t.Helper() + runtime := testGitRuntime(t) + runtime.Executable = filepath.Join(t.TempDir(), "no-such-git") + ws, err := gitlocal.NewFactory(gitlocal.Params{Dir: f.checkoutDir, Runtime: runtime}) + require.NoError(t, err) return f.newMergerWith(t, func(p *Params) { - p.Runtime.Executable = filepath.Join(t.TempDir(), "no-such-git") + p.Workspaces = ws }) } @@ -902,22 +904,21 @@ func TestAuthorIdent(t *testing.T) { tests := []struct { name string ident authorIdent - complete bool - wantEnv []string + wantArgs []string }{ { name: "both halves present", ident: authorIdent{Name: "Jane Dev", Email: "jane@example.com"}, - complete: true, - wantEnv: []string{"GIT_AUTHOR_NAME=Jane Dev", "GIT_AUTHOR_EMAIL=jane@example.com"}, + wantArgs: []string{"--author", "Jane Dev "}, }, { - // Passed through verbatim: the environment carries the two halves - // separately, so nothing has to survive being parsed back apart. - name: "name containing angle brackets", - ident: authorIdent{Name: "Jane Doe", Email: "jane@example.com"}, - complete: true, - wantEnv: []string{"GIT_AUTHOR_NAME=Jane Doe", "GIT_AUTHOR_EMAIL=jane@example.com"}, + // --author would split "Name
" in the wrong place. + name: "name containing angle brackets falls back", + ident: authorIdent{Name: "Jane Doe", Email: "jane@example.com"}, + }, + { + name: "line break falls back", + ident: authorIdent{Name: "Jane\nDev", Email: "jane@example.com"}, }, { name: "missing email falls back", @@ -935,8 +936,8 @@ func TestAuthorIdent(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.complete, tt.ident.complete()) - assert.Equal(t, tt.wantEnv, tt.ident.env()) + assert.Equal(t, tt.wantArgs != nil, tt.ident.usable()) + assert.Equal(t, tt.wantArgs, tt.ident.args()) }) } } @@ -962,7 +963,7 @@ func TestMerge_SquashRebase_CreditsChangeAuthor(t *testing.T) { func TestMerge_Merge_CreditsChangeAuthorOnMergeCommit(t *testing.T) { // `git merge` has no --author flag, so the merge commit is the case that - // only the environment can attribute. + // has to be committed separately to be attributed. f := setupGitFixture(t) sha := f.pushPRCommit(t, "feature/a", "hello.txt", "hello\nearth\n", "tweak hello") @@ -1345,20 +1346,31 @@ func (f gitFixture) publishPRRef(t *testing.T, n int, sha string) { fmt.Sprintf("%s:refs/pull/%d/head", sha, n)) } +// workspaces opens the fixture checkout on the pinned runtime, committing as +// the merger's test identity. +func (f gitFixture) workspaces(t *testing.T) gitworkspace.Factory { + t.Helper() + ws, err := gitlocal.NewFactory(gitlocal.Params{ + Dir: f.checkoutDir, + Runtime: testGitRuntime(t), + CommitterName: "Test", + CommitterEmail: "test@example.com", + }) + require.NoError(t, err) + return ws +} + // newMergerWith builds a Merger with non-default Params, for the options that // only some tests exercise. func (f gitFixture) newMergerWith(t *testing.T, mutate func(*Params)) merger.Merger { t.Helper() p := Params{ - CheckoutPath: f.checkoutDir, + Workspaces: f.workspaces(t), Remote: "origin", Target: "main", DefaultStrategy: mergestrategypb.Strategy_REBASE, - Runtime: testGitRuntime(t), Logger: zaptest.NewLogger(t).Sugar(), MetricsScope: tally.NoopScope, - CommitterName: "Test", - CommitterEmail: "test@example.com", } mutate(&p) m, err := NewMerger(p) @@ -1382,18 +1394,27 @@ func (f gitFixture) checkoutClean(t *testing.T) bool { return true } +// openMerger builds a merger with a workspace already open, for tests that +// drive its git helpers directly rather than through an operation. +func (f gitFixture) openMerger(t *testing.T) *gitMerger { + t.Helper() + m := f.newMerger(t, mergestrategypb.Strategy_REBASE).(*gitMerger) + ws, err := m.workspaces.For(context.Background(), m.repo) + require.NoError(t, err) + t.Cleanup(func() { _ = ws.Close() }) + m.ws = ws + return m +} + func (f gitFixture) newMerger(t *testing.T, defaultStrategy mergestrategypb.Strategy) merger.Merger { t.Helper() m, err := NewMerger(Params{ - CheckoutPath: f.checkoutDir, + Workspaces: f.workspaces(t), Remote: "origin", Target: "main", DefaultStrategy: defaultStrategy, - Runtime: testGitRuntime(t), Logger: zaptest.NewLogger(t).Sugar(), MetricsScope: tally.NoopScope, - CommitterName: "Test", - CommitterEmail: "test@example.com", }) require.NoError(t, err) return m diff --git a/runway/extension/merger/git/objects.go b/runway/extension/merger/git/objects.go index 3a919e5c5..febfc1310 100644 --- a/runway/extension/merger/git/objects.go +++ b/runway/extension/merger/git/objects.go @@ -53,11 +53,11 @@ func (m *gitMerger) ensureObject(ctx context.Context, ref changeRef) error { return nil } - if _, err := m.run(ctx, nil, "fetch", m.remote, ref.SHA); err == nil && m.hasCommit(ctx, ref.SHA) { + if _, err := m.run(ctx, "fetch", m.remote, ref.SHA); err == nil && m.hasCommit(ctx, ref.SHA) { return nil } if ref.Ref != "" { - if _, err := m.run(ctx, nil, "fetch", m.remote, ref.Ref); err == nil && m.hasCommit(ctx, ref.SHA) { + if _, err := m.run(ctx, "fetch", m.remote, ref.Ref); err == nil && m.hasCommit(ctx, ref.SHA) { return nil } } @@ -65,7 +65,7 @@ func (m *gitMerger) ensureObject(ctx context.Context, ref changeRef) error { // Both fetches failed. Distinguish "the remote is unreachable right now" // (infrastructure, worth retrying) from "the remote is fine and simply does // not have this commit" (terminal — no number of retries conjures it). - if _, err := m.run(ctx, nil, "ls-remote", "--exit-code", m.remote, "HEAD"); err != nil { + if _, err := m.run(ctx, "ls-remote", "--exit-code", m.remote, "HEAD"); err != nil { return fmt.Errorf("remote %s unreachable while resolving commit %s: %w", m.remote, ref.SHA, err) } @@ -79,7 +79,7 @@ func (m *gitMerger) ensureObject(ctx context.Context, ref changeRef) error { // hasCommit reports whether sha names a commit object in the local checkout. func (m *gitMerger) hasCommit(ctx context.Context, sha string) bool { - _, err := m.run(ctx, nil, "cat-file", "-e", sha+"^{commit}") + _, err := m.run(ctx, "cat-file", "-e", sha+"^{commit}") return err == nil } @@ -101,7 +101,7 @@ func (m *gitMerger) checkStale(ctx context.Context, refs []changeRef) error { if ref.Ref == "" { continue } - out, err := m.run(ctx, nil, "ls-remote", m.remote, ref.Ref) + out, err := m.run(ctx, "ls-remote", m.remote, ref.Ref) if err != nil { return fmt.Errorf("git ls-remote %s %s: %w", m.remote, ref.Ref, err) } diff --git a/service/runway/server/main.go b/service/runway/server/main.go index eaf26732b..b21514b27 100644 --- a/service/runway/server/main.go +++ b/service/runway/server/main.go @@ -15,6 +15,7 @@ package main import ( + "cmp" "context" "database/sql" "errors" @@ -496,6 +497,14 @@ func loadMergeConfigFromEnv(logger *zap.Logger) (mergeConfig, error) { return cfg, nil } +// Committer identity of a git merger whose config leaves it unset. The pinned +// runtime scrubs ambient git configuration, so the commit-creating strategies +// need one supplied explicitly. +const ( + defaultCommitterName = "SubmitQueue Runway" + defaultCommitterEmail = "runway@submitqueue.invalid" +) + // mergerBuilder constructs merger factories, reusing one git instance per merge // target. // @@ -540,18 +549,25 @@ func (b *mergerBuilder) build(cfg mergerConfig, where string) (merger.Factory, e } } + workspaces, err := gitlocal.NewFactory(gitlocal.Params{ + Dir: cfg.CheckoutPath, + Runtime: b.runtime, + CommitterName: cmp.Or(cfg.CommitterName, defaultCommitterName), + CommitterEmail: cmp.Or(cfg.CommitterEmail, defaultCommitterEmail), + }) + if err != nil { + return nil, fmt.Errorf("%s: failed to build git workspace: %w", where, err) + } m, err := gitmerger.NewMerger(gitmerger.Params{ - CheckoutPath: cfg.CheckoutPath, + Workspaces: workspaces, + Repo: cfg.CheckoutPath, Remote: cfg.Remote, Target: cfg.Target, DefaultStrategy: cfg.strategy(), - Runtime: b.runtime, MaxPushAttempts: cfg.MaxPushAttempts, FetchRefspecs: cfg.FetchRefspecs, CheckStaleness: *cfg.CheckStaleness, AllowUnrelatedHistories: cfg.AllowUnrelatedHistories, - CommitterName: cfg.CommitterName, - CommitterEmail: cfg.CommitterEmail, Logger: b.logger.Sugar(), MetricsScope: b.scope, })