From 5ae36dd841d36609b26cb908309d29a58bb656ab Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Tue, 28 Jul 2026 14:55:09 -0700 Subject: [PATCH] feat(runway): git merger SQUASH_REBASE and MERGE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary ### Why? The git merger landed with REBASE only. `SQUASH_REBASE` and `MERGE` are part of the wire contract SubmitQueue already publishes against, and until they apply here a request naming either is rejected as an invalid request. This adds the two remaining transforming strategies on top of the shared apply machinery. ### What? **SQUASH_REBASE** applies each change exactly like REBASE — replaying every commit it introduces — then collapses what that change produced into a single commit. The squash unit is the change: a change of ten commits becomes one, and a step whose change carries several URIs yields one commit per URI. Those URIs are a stack of pull requests, and squashing them together would erase the per-PR boundary the stack exists to express. Two degenerate cases produce no output rather than an empty commit. A change already present on the target creates no commits, so there is nothing to squash. A change that does create commits whose net tree matches the base would squash to an empty commit, so the intermediates are dropped. Both keep redelivery idempotent. **MERGE** creates a `--no-ff` merge commit per change, which keeps the change's original commits reachable through second-parent history — the property that separates it from the picking strategies, which rewrite those hashes. A change already contained in HEAD is skipped rather than merged again. **Not every failed merge is a conflict.** `applyMerge` previously reported any `git merge` failure as `ErrConflict`, which tells the client its change collides with the target even when nothing collided. Failures are now classified, and the case that matters in practice is an unrelated history. **Importing an unrelated history.** A repository migration arrives as an ordinary change in the target repo whose branch carries the source repo's whole history. Being in the target repo is what makes the commits fetchable; it says nothing about ancestry, and git refuses to merge two graphs with no common ancestor. `AllowUnrelatedHistories` lifts that refusal for a queue that exists to perform such imports. It is off by default because the refusal is a genuine safeguard — with it always on, merging the wrong object silently produces a nonsense result instead of failing. Without the option, the refusal is now reported as an invalid request rather than a conflict. MERGE is the only strategy that can serve a migration: it is the only one that preserves the imported commits' original hashes, and the picking strategies have no range to compute across disjoint graphs, so they reject such a change explicitly and say so. ## Test Plan ✅ `bazel test //runway/extension/merger/git:go_default_test` — passes (58s) New coverage: SQUASH_REBASE collapsing a multi-commit change into one commit while landing all of its content, a two-URI change yielding one squashed commit per URI in application order, SQUASH_REBASE over an already-landed change producing none, MERGE creating one merge commit for a multi-commit change, MERGE skipping a change already an ancestor of the tip, and the MERGE dry-run path. For migration specifically: importing a three-commit unrelated history and asserting every imported commit is reachable **under its original hash**, the merge commit has two parents, and the target keeps its own files; redelivery of the same request being a no-op; the import rejected as an invalid request (explicitly not a conflict) when the option is off, leaving the remote and checkout untouched; and both picking strategies rejecting an unrelated history rather than rewriting it. --- runway/extension/merger/git/README.md | 16 +- runway/extension/merger/git/git_merger.go | 233 +++++++++++-- .../extension/merger/git/git_merger_test.go | 313 +++++++++++++++++- 3 files changed, 540 insertions(+), 22 deletions(-) diff --git a/runway/extension/merger/git/README.md b/runway/extension/merger/git/README.md index 1f119c90..a5b58600 100644 --- a/runway/extension/merger/git/README.md +++ b/runway/extension/merger/git/README.md @@ -38,9 +38,21 @@ Fetching by SHA guarantees the merger applies exactly the commit a URI names — | Strategy | What it does | Outputs | |---|---|---| | `REBASE` | Cherry-picks every commit each change introduces onto the tip, in order. A commit already present on the target is skipped (no output), as is one that was empty to begin with. | one revision per newly-created commit | +| `SQUASH_REBASE` | Applies each change like `REBASE`, then collapses the commits it produced into a single commit (squash unit = the change, not the step). | one revision per change, or none for a change already present | +| `MERGE` | Creates a `--no-ff` merge commit per change, keeping the change's original commits reachable through second-parent history. A commit already contained in the tip is skipped. | the merge-commit revision(s) | | `DEFAULT` | Resolved to the instance's configured default strategy before any step runs. | per the resolved strategy | -`REBASE` is the only strategy implemented so far. `SQUASH_REBASE`, `MERGE`, and `PROMOTE` are defined by the wire contract but not yet applied here — a step naming one is rejected as an invalid request. +`PROMOTE` is defined by the wire contract but not yet applied here — a step naming it is rejected as an invalid request. + +## Importing an unrelated history + +A repository migration arrives as an ordinary change in the target repo whose branch carries the source repo's entire history. Living in the target repo is what makes its commits fetchable; it says nothing about ancestry, and the two graphs still share no common ancestor, so git refuses the merge by default. + +`MERGE` is the only strategy that can serve this, because it is the only one that leaves the imported commits reachable under their original hashes — the picking strategies would rewrite every one of them, and have no range to compute in the first place, so they reject such a change outright. + +The refusal is lifted by a per-instance option rather than always: it is a real safeguard, and without it a merge of the wrong object fails loudly instead of quietly producing a nonsense result. A queue that exists to perform imports turns it on. A refusal that surfaces without the option is reported as an invalid request, not as a conflict — nothing collided. + +Redelivery is safe: once imported, the source head is contained in the target, so the change is skipped rather than merged twice. ## Committing, dry-run, atomicity, contention @@ -56,7 +68,7 @@ The distinction between the last two matters operationally: a commit that is mis ## 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 `REBASE` strategy requires. +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. 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/git_merger.go b/runway/extension/merger/git/git_merger.go index 320a33fd..8e1c41bf 100644 --- a/runway/extension/merger/git/git_merger.go +++ b/runway/extension/merger/git/git_merger.go @@ -19,11 +19,15 @@ // Strategy → git operation: // // - REBASE: cherry-pick each URI's head commit onto the target tip. +// - SQUASH_REBASE: cherry-pick the step's changes, then collapse them into a +// single commit (squash unit = the step). +// - MERGE: create a --no-ff merge commit per URI, preserving the +// original commit hashes in second-parent history. // - DEFAULT: resolved to the instance's configured DefaultStrategy // before any step runs. // -// REBASE is the only strategy implemented so far. A step naming any other -// strategy is rejected as merger.ErrInvalidRequest until its apply path lands. +// PROMOTE is not implemented yet; a step naming it is rejected as +// merger.ErrInvalidRequest until its apply path lands. // // Atomicity: for a committing merge nothing reaches the remote until the final // push. A step that fails to apply aborts the in-progress git operation and @@ -77,8 +81,8 @@ 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 the commit-creating REBASE strategy needs one supplied -// explicitly. +// ambient identity, so commit-creating strategies (REBASE/SQUASH_REBASE/MERGE) +// need one supplied explicitly. const ( defaultCommitterName = "SubmitQueue Runway" defaultCommitterEmail = "runway@submitqueue.invalid" @@ -112,7 +116,7 @@ type Params struct { // Target is the destination branch ref on the remote (e.g. "main"). Target string // DefaultStrategy resolves a step whose strategy is DEFAULT. Must be a - // concrete strategy (currently only REBASE). + // concrete strategy (REBASE, SQUASH_REBASE, or MERGE). DefaultStrategy mergestrategypb.Strategy // Runtime is the pinned Git runtime used for every invocation. Runtime GitRuntime @@ -129,6 +133,13 @@ type Params struct { // CheckStaleness enables verifying, before applying, that each change's // canonical ref still points at the commit its URI names. CheckStaleness bool + // AllowUnrelatedHistories lets a MERGE step integrate a change that shares + // no ancestry with the target — importing one repository's history into + // another. Off by default: the refusal it lifts is a real safeguard, since + // without it a merge of the wrong object fails loudly instead of quietly + // 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 @@ -150,10 +161,13 @@ type gitMerger struct { maxPushAttempts int fetchRefspecs []string checkStaleness bool - committerName string - committerEmail string - logger *zap.SugaredLogger - metricsScope tally.Scope + + // 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. @@ -182,7 +196,7 @@ func NewMerger(params Params) (merger.Merger, error) { return nil, err } if !isConcreteStrategy(params.DefaultStrategy) { - return nil, fmt.Errorf("default strategy must be concrete (currently only REBASE), got %v", params.DefaultStrategy) + return nil, fmt.Errorf("default strategy must be concrete (REBASE, SQUASH_REBASE, or MERGE), got %v", params.DefaultStrategy) } maxAttempts := params.MaxPushAttempts if maxAttempts <= 0 { @@ -205,10 +219,12 @@ func NewMerger(params Params) (merger.Merger, error) { maxPushAttempts: maxAttempts, fetchRefspecs: params.FetchRefspecs, checkStaleness: params.CheckStaleness, - committerName: committerName, - committerEmail: committerEmail, - logger: params.Logger.Named("git_merger"), - metricsScope: params.MetricsScope.SubScope("git_merger"), + + allowUnrelatedHistories: params.AllowUnrelatedHistories, + committerName: committerName, + committerEmail: committerEmail, + logger: params.Logger.Named("git_merger"), + metricsScope: params.MetricsScope.SubScope("git_merger"), }, nil } @@ -307,7 +323,8 @@ func (m *gitMerger) resolveAndValidate(req *runwaymq.MergeRequest) ([]resolvedSt } // applyTransforming runs the reset/apply/push cycle for the transforming -// strategies, retrying on remote contention when committing. For a dry run it applies the steps locally then discards them. +// strategies (REBASE, SQUASH_REBASE, MERGE), retrying on remote contention when +// committing. For a dry run it applies the steps locally then discards them. func (m *gitMerger) applyTransforming(ctx context.Context, req *runwaymq.MergeRequest, steps []resolvedStep, commit bool) (*runwaymq.MergeResult, error) { // Fetch and vet every change the request names before the first attempt, so // an unusable request fails without having touched the checkout. This sits @@ -416,6 +433,10 @@ func (m *gitMerger) applySteps(ctx context.Context, steps []resolvedStep) ([]*ru switch rs.strategy { case mergestrategypb.Strategy_REBASE: outputs, err = m.applyRebase(ctx, rs) + case mergestrategypb.Strategy_SQUASH_REBASE: + outputs, err = m.applySquashRebase(ctx, rs) + case mergestrategypb.Strategy_MERGE: + outputs, err = m.applyMerge(ctx, rs) default: // resolveAndValidate rejects anything else; defensive. return nil, fmt.Errorf("%w: unsupported strategy %v", merger.ErrInvalidRequest, rs.strategy) @@ -438,6 +459,140 @@ func (m *gitMerger) applyRebase(ctx context.Context, rs resolvedStep) ([]*runway return toOutputs(picked), nil } +// applySquashRebase collapses each change in the step into a single commit. +// +// The squash unit is the change, not the step: a change is one pull request, +// and squashing it is what "squash the PR" means. A step whose change carries +// several URIs is a stack of pull requests, and collapsing those into one +// commit would erase the per-PR boundary the stack exists to express. So each +// URI is picked as its own range and squashed on its own, in order, yielding +// one commit — and one output — per change that had anything to contribute. +func (m *gitMerger) applySquashRebase(ctx context.Context, rs resolvedStep) ([]*runwaymq.StepOutput, error) { + var outputs []*runwaymq.StepOutput + for _, ref := range rs.refs { + sha, squashed, err := m.squashChange(ctx, rs.step, ref) + if err != nil { + return nil, err + } + if squashed { + outputs = append(outputs, &runwaymq.StepOutput{Id: sha}) + } + } + return outputs, nil +} + +// squashChange replays one change and collapses whatever it produced into a +// single commit, reporting squashed=false when it produced nothing worth +// keeping. +// +// Two cases produce nothing. A change already present on the target creates no +// commits at all. A change that does create commits whose net tree matches the +// base — its effect was already there, spread differently — would squash to an +// empty commit, so the intermediates are dropped instead. Both keep redelivery +// idempotent. +func (m *gitMerger) squashChange(ctx context.Context, step *runwaymq.MergeStep, ref changeRef) (string, bool, error) { + preSHA, err := m.headSHA(ctx) + if err != nil { + return "", false, err + } + + created, err := m.pickRange(ctx, ref) + if err != nil { + return "", false, err + } + if len(created) == 0 { + return "", false, nil + } + + preTree, err := m.commitTreeSHA(ctx, preSHA) + if err != nil { + return "", false, err + } + postTree, err := m.commitTreeSHA(ctx, "HEAD") + if err != nil { + return "", false, err + } + if preTree == postTree { + if _, err := m.run(ctx, nil, "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 { + return "", false, fmt.Errorf("git reset --soft %s: %w", preSHA, err) + } + if _, err := m.run(ctx, nil, "commit", "-m", squashMessage(step, ref)); err != nil { + return "", false, fmt.Errorf("git commit (squash): %w", err) + } + sha, err := m.headSHA(ctx) + if err != nil { + return "", false, err + } + return sha, true, nil +} + +// applyMerge creates a --no-ff merge commit for every change in the step, +// keeping the change's original commits reachable through second-parent +// history — the property that distinguishes MERGE from the picking strategies, +// which rewrite those commits. A change already contained in HEAD produces no +// output, which is what makes redelivery idempotent. +func (m *gitMerger) applyMerge(ctx context.Context, rs resolvedStep) ([]*runwaymq.StepOutput, error) { + var outputs []*runwaymq.StepOutput + for _, ref := range rs.refs { + contained, err := m.isAncestor(ctx, ref.SHA, "HEAD") + if err != nil { + return nil, err + } + if contained { + continue + } + + args := []string{"merge", "--no-ff", "--no-edit"} + if m.allowUnrelatedHistories { + args = append(args, "--allow-unrelated-histories") + } + out, err := m.runCombined(ctx, nil, 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") + return nil, m.classifyMergeFailure(ref, out, conflicted) + } + mergeSHA, err := m.headSHA(ctx) + if err != nil { + return nil, err + } + outputs = append(outputs, &runwaymq.StepOutput{Id: mergeSHA}) + } + return outputs, nil +} + +// classifyMergeFailure decides what a failed `git merge` actually means, given +// whether the index was left holding conflicted entries. +// +// Not every refusal is a conflict, and reporting one as such tells the client +// its change collides with the target when nothing of the sort happened. Two +// non-conflicts land here: an import of an unrelated history, refused outright +// and fixed by configuration rather than by rebasing, and any other way git +// can exit non-zero — a missing object, an unreadable repository, a killed +// process — which is infrastructure and should be retried, not made terminal. +func (m *gitMerger) classifyMergeFailure(ref changeRef, out []byte, conflicted bool) error { + detail := strings.TrimSpace(string(out)) + if strings.Contains(detail, "refusing to merge unrelated histories") { + coremetrics.NamedCounter(m.metricsScope, "merge", "unrelated_histories", 1) + return fmt.Errorf("%w: %s shares no history with the merge target; integrating an imported history requires the merger to allow unrelated histories", + merger.ErrInvalidRequest, ref.Label) + } + if !conflicted { + coremetrics.NamedCounter(m.metricsScope, "merge", "merge_errors", 1) + return fmt.Errorf("git merge %s: %s", ref.SHA, detail) + } + coremetrics.NamedCounter(m.metricsScope, "merge", "merge_conflicts", 1) + return fmt.Errorf("%w: git merge %s: %s", merger.ErrConflict, ref.SHA, detail) +} + // pickStepChanges applies every change in the step, in order, returning the // SHAs of the commits created on the target (empty for a change whose content // was already present). @@ -618,6 +773,33 @@ func (m *gitMerger) refetchTipSHA(ctx context.Context) (string, error) { return strings.TrimSpace(string(out)), nil } +// isAncestor reports whether ancestor is an ancestor of (or equal to) +// 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) { + cmd := m.command(ctx, "merge-base", "--is-ancestor", ancestor, descendant) + var stderr bytes.Buffer + cmd.Stderr = &stderr + err := cmd.Run() + if err == nil { + return true, nil + } + var exitErr *exec.ExitError + if errors.As(err, &exitErr) && exitErr.ExitCode() == 1 { + return false, nil + } + return false, fmt.Errorf("git merge-base --is-ancestor %s %s: %w: %s", ancestor, descendant, err, strings.TrimSpace(stderr.String())) +} + +// 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}") + if err != nil { + return "", fmt.Errorf("git rev-parse %s^{tree}: %w", ref, err) + } + return strings.TrimSpace(string(out)), nil +} + // 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 @@ -750,12 +932,13 @@ func passthroughEnv(extra []string) []string { } // isConcreteStrategy reports whether s names a concrete integration strategy -// (i.e. not DEFAULT and not an unknown value). Only REBASE is implemented so -// far; the remaining strategies are rejected as invalid requests until their -// apply paths land. +// (i.e. not DEFAULT and not an unknown value). PROMOTE is not implemented yet +// and is rejected as an invalid request until its apply path lands. func isConcreteStrategy(s mergestrategypb.Strategy) bool { switch s { - case mergestrategypb.Strategy_REBASE: + case mergestrategypb.Strategy_REBASE, + mergestrategypb.Strategy_SQUASH_REBASE, + mergestrategypb.Strategy_MERGE: return true default: return false @@ -770,6 +953,18 @@ func isRedundantCherryPick(out []byte) bool { strings.Contains(s, "nothing to commit") } +// squashMessage synthesizes a commit message for one squashed change. The wire +// carries no upstream commit message, so the change is named by its provider +// label — which is why this goes through the resolver rather than one +// provider's fields, so a non-GitHub change is named too instead of being +// silently omitted. +func squashMessage(step *runwaymq.MergeStep, ref changeRef) string { + if step.GetStepId() != "" { + return fmt.Sprintf("squash: %s (%s)", step.GetStepId(), ref.Label) + } + return fmt.Sprintf("squash: %s", ref.Label) +} + // toOutputs wraps commit SHAs as StepOutputs in order. func toOutputs(shas []string) []*runwaymq.StepOutput { if len(shas) == 0 { diff --git a/runway/extension/merger/git/git_merger_test.go b/runway/extension/merger/git/git_merger_test.go index 1c81cee1..0b0c17ee 100644 --- a/runway/extension/merger/git/git_merger_test.go +++ b/runway/extension/merger/git/git_merger_test.go @@ -20,6 +20,7 @@ import ( "errors" "fmt" "os" + "os/exec" "path/filepath" "strconv" "strings" @@ -421,6 +422,81 @@ func TestMerge_Rebase_GivesUpAfterMaxAttempts(t *testing.T) { assert.Equal(t, raceSHAs[1], f.remoteHEAD(t)) } +// --- SQUASH_REBASE --- + +func TestMerge_SquashRebase_OneCommitPerChange(t *testing.T) { + // Two URIs in one change are a stack of two pull requests. Each squashes on + // its own — collapsing them together would erase the boundary the stack + // exists to express. + f := setupGitFixture(t) + sha1, sha2 := f.pushStack(t) + + m := f.newMerger(t, mergestrategypb.Strategy_REBASE) + res, err := m.Merge(context.Background(), req("b", + stepOf(mergestrategypb.Strategy_SQUASH_REBASE, "s1", uri(sha1), uri(sha2)), + )) + require.NoError(t, err) + assert.Equal(t, runwaypb.Outcome_SUCCEEDED, res.GetOutcome()) + require.Len(t, res.GetSteps(), 1) + + outputs := res.GetSteps()[0].GetOutputs() + require.Len(t, outputs, 2, "one squashed commit per change") + commits := f.remoteCommitsSinceSeed(t) + require.Len(t, commits, 2) + assert.Equal(t, []string{outputs[0].GetId(), outputs[1].GetId()}, commits, "in application order") + assert.Equal(t, "hello\nearth\ngoodbye\n", f.remoteFile(t, "hello.txt")) +} + +func TestMerge_SquashRebase_AlreadyLanded(t *testing.T) { + f := setupGitFixture(t) + sha := f.pushPRCommit(t, "feature/a", "hello.txt", "hello\nearth\n", "tweak hello") + f.landOnMain(t, sha) + mainBefore := f.remoteHEAD(t) + + m := f.newMerger(t, mergestrategypb.Strategy_REBASE) + res, err := m.Merge(context.Background(), req("b", stepOf(mergestrategypb.Strategy_SQUASH_REBASE, "s1", uri(sha)))) + require.NoError(t, err) + require.Len(t, res.GetSteps(), 1) + assert.Empty(t, res.GetSteps()[0].GetOutputs()) + assert.Equal(t, mainBefore, f.remoteHEAD(t)) +} + +// --- MERGE --- + +func TestMerge_Merge_FreshChangeCreatesMergeCommit(t *testing.T) { + f := setupGitFixture(t) + freshSHA := f.pushPRCommit(t, "feature/a", "hello.txt", "hello\nearth\n", "tweak hello") + + m := f.newMerger(t, mergestrategypb.Strategy_REBASE) + res, err := m.Merge(context.Background(), req("b", stepOf(mergestrategypb.Strategy_MERGE, "s1", uri(freshSHA)))) + require.NoError(t, err) + assert.Equal(t, runwaypb.Outcome_SUCCEEDED, res.GetOutcome()) + require.Len(t, res.GetSteps(), 1) + require.Len(t, res.GetSteps()[0].GetOutputs(), 1) + + mergeSHA := res.GetSteps()[0].GetOutputs()[0].GetId() + assert.Equal(t, mergeSHA, f.remoteHEAD(t)) + assert.Len(t, f.parents(t, mergeSHA), 2, "a --no-ff merge commit has two parents") + assert.Contains(t, f.parents(t, mergeSHA), freshSHA, + "the original head commit is preserved as the merge's second parent") +} + +func TestMerge_Merge_AlreadyAncestor(t *testing.T) { + f := setupGitFixture(t) + freshSHA := f.pushPRCommit(t, "feature/a", "hello.txt", "hello\nearth\n", "tweak hello") + f.advanceMain(t, freshSHA) // fast-forward main directly to freshSHA + mainBefore := f.remoteHEAD(t) + + m := f.newMerger(t, mergestrategypb.Strategy_REBASE) + res, err := m.Merge(context.Background(), req("b", stepOf(mergestrategypb.Strategy_MERGE, "s1", uri(freshSHA)))) + require.NoError(t, err) + require.Len(t, res.GetSteps(), 1) + assert.Empty(t, res.GetSteps()[0].GetOutputs()) + assert.Equal(t, mainBefore, f.remoteHEAD(t)) +} + +// --- PROMOTE --- + func TestMerge_Default_ResolvesToRebase(t *testing.T) { f := setupGitFixture(t) sha := f.pushPRCommit(t, "feature/a", "hello.txt", "hello\nearth\n", "tweak hello") @@ -434,6 +510,206 @@ func TestMerge_Default_ResolvesToRebase(t *testing.T) { assert.Equal(t, []string{res.GetSteps()[0].GetOutputs()[0].GetId()}, f.remoteCommitsSinceSeed(t)) } +func TestClassifyMergeFailure(t *testing.T) { + f := setupGitFixture(t) + m := f.newMerger(t, mergestrategypb.Strategy_REBASE).(*gitMerger) + ref := changeRef{SHA: fakeSHA, Label: "uber/repo#1"} + + tests := []struct { + name string + out string + conflicted bool + wantConflict bool + wantInvalid bool + }{ + { + name: "content conflict", + out: "CONFLICT (content): Merge conflict in a.txt", + conflicted: true, + wantConflict: true, + }, + { + name: "unrelated histories", + out: "fatal: refusing to merge unrelated histories", + conflicted: false, + wantInvalid: true, + }, + { + name: "infrastructure failure", + out: "fatal: unable to read tree", + conflicted: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := m.classifyMergeFailure(ref, []byte(tt.out), tt.conflicted) + require.Error(t, err) + assert.Equal(t, tt.wantConflict, errors.Is(err, merger.ErrConflict)) + assert.Equal(t, tt.wantInvalid, errors.Is(err, merger.ErrInvalidRequest)) + }) + } +} + +// --- repo migration (unrelated histories) --- +// +// A repository migration reaches Runway as an ordinary change in the target +// repo whose branch carries the *source* repo's entire history. Being in the +// same repository is what makes the commits fetchable; it says nothing about +// ancestry, and the two graphs still share no common ancestor. + +// pushImportedHistory builds a branch with its own root commit — an imported +// history, unrelated to the target's — and publishes it in the target repo as +// a change ref, the way such a migration is actually proposed. Returns the head +// SHA and every commit of the imported history, oldest first. +func (f gitFixture) pushImportedHistory(t *testing.T, prNumber int, commits ...commitSpec) (string, []string) { + t.Helper() + dir := filepath.Join(f.root, fmt.Sprintf("import-%d", prNumber)) + mustGit(t, f.root, "init", "-b", "main", dir) + configRepo(t, dir, "importer", "importer@example.com") + for _, c := range commits { + require.NoError(t, writeFile(filepath.Join(dir, c.path), c.contents)) + mustGit(t, dir, "add", ".") + mustGit(t, dir, "commit", "-m", c.message) + } + head := strings.TrimSpace(string(mustGitOutput(t, dir, "rev-parse", "HEAD"))) + mustGit(t, dir, "remote", "add", "origin", f.remoteDir) + mustGit(t, dir, "push", "-f", "origin", fmt.Sprintf("HEAD:refs/pull/%d/head", prNumber)) + + out := mustGitOutput(t, dir, "rev-list", "--reverse", "HEAD") + return head, strings.Fields(string(out)) +} + +func TestMerge_Merge_ImportsUnrelatedHistory(t *testing.T) { + f := setupGitFixture(t) + head, imported := f.pushImportedHistory(t, 1, + commitSpec{"src/lib.go", "v1\n", "micro: initial"}, + commitSpec{"src/lib.go", "v2\n", "micro: feature A"}, + commitSpec{"src/util.go", "u\n", "micro: feature B"}, + ) + require.Len(t, imported, 3) + + m := f.newMergerWith(t, func(p *Params) { p.AllowUnrelatedHistories = true }) + res, err := m.Merge(context.Background(), req("b", stepOf(mergestrategypb.Strategy_MERGE, "s1", uri(head)))) + require.NoError(t, err) + assert.Equal(t, runwaypb.Outcome_SUCCEEDED, res.GetOutcome()) + + // The target keeps its own file and gains the imported tree. + assert.Equal(t, "hello\nworld\n", f.remoteFile(t, "hello.txt")) + assert.Equal(t, "v2\n", f.remoteFile(t, "src/lib.go")) + assert.Equal(t, "u\n", f.remoteFile(t, "src/util.go")) + + // The point of using MERGE for a migration: every imported commit survives + // under its original hash rather than being rewritten. + for _, sha := range imported { + assert.True(t, f.remoteHasCommit(t, sha), "imported commit %s must be reachable unchanged", sha) + } + + // The merge commit joins both graphs. + require.Len(t, res.GetSteps(), 1) + require.Len(t, res.GetSteps()[0].GetOutputs(), 1) + assert.Len(t, f.remoteParents(t, res.GetSteps()[0].GetOutputs()[0].GetId()), 2) +} + +func TestMerge_Merge_ImportedHistoryRedeliveryIsNoOp(t *testing.T) { + f := setupGitFixture(t) + head, _ := f.pushImportedHistory(t, 1, + commitSpec{"src/lib.go", "v1\n", "micro: initial"}, + commitSpec{"src/lib.go", "v2\n", "micro: feature A"}, + ) + + m := f.newMergerWith(t, func(p *Params) { p.AllowUnrelatedHistories = true }) + request := req("b", stepOf(mergestrategypb.Strategy_MERGE, "s1", uri(head))) + _, err := m.Merge(context.Background(), request) + require.NoError(t, err) + afterFirst := f.remoteHEAD(t) + + res, err := m.Merge(context.Background(), request) + require.NoError(t, err) + assert.Equal(t, afterFirst, f.remoteHEAD(t), "redelivery must not merge the import twice") + require.Len(t, res.GetSteps(), 1) + assert.Empty(t, res.GetSteps()[0].GetOutputs()) +} + +func TestMerge_Merge_UnrelatedHistoryRejectedByDefault(t *testing.T) { + // Without the option the merge is refused. It must not be reported as a + // conflict: nothing collided, the merger simply declined to join two + // unrelated graphs. + f := setupGitFixture(t) + head, _ := f.pushImportedHistory(t, 1, commitSpec{"src/lib.go", "v1\n", "micro: initial"}) + mainBefore := f.remoteHEAD(t) + + m := f.newMerger(t, mergestrategypb.Strategy_REBASE) + _, err := m.Merge(context.Background(), req("b", stepOf(mergestrategypb.Strategy_MERGE, "s1", uri(head)))) + require.Error(t, err) + assert.True(t, errors.Is(err, merger.ErrInvalidRequest)) + assert.False(t, errors.Is(err, merger.ErrConflict)) + assert.Equal(t, mainBefore, f.remoteHEAD(t)) + assert.True(t, f.checkoutClean(t)) +} + +func TestMerge_Rebase_UnrelatedHistoryRejected(t *testing.T) { + // A picking strategy cannot serve a migration at all: there is no range to + // compute, and replaying the import commit-by-commit would rewrite every + // hash in it — the opposite of preserving the history. + f := setupGitFixture(t) + head, _ := f.pushImportedHistory(t, 1, + commitSpec{"src/lib.go", "v1\n", "micro: initial"}, + commitSpec{"src/lib.go", "v2\n", "micro: feature A"}, + ) + + for _, strategy := range []mergestrategypb.Strategy{ + mergestrategypb.Strategy_REBASE, + mergestrategypb.Strategy_SQUASH_REBASE, + } { + t.Run(strategy.String(), func(t *testing.T) { + m := f.newMergerWith(t, func(p *Params) { p.AllowUnrelatedHistories = true }) + _, err := m.Merge(context.Background(), req("b", stepOf(strategy, "s1", uri(head)))) + require.Error(t, err) + assert.True(t, errors.Is(err, merger.ErrInvalidRequest)) + assert.False(t, errors.Is(err, merger.ErrConflict)) + }) + } +} + +// --- multi-commit changes under the remaining strategies --- + +func TestMerge_SquashRebase_MultiCommitChangeSingleURI(t *testing.T) { + f := setupGitFixture(t) + head := f.pushMultiCommitPR(t, "feature/multi", + commitSpec{"a.txt", "a\n", "add a"}, + commitSpec{"b.txt", "b\n", "add b"}, + commitSpec{"c.txt", "c\n", "add c"}, + ) + + m := f.newMerger(t, mergestrategypb.Strategy_REBASE) + res, err := m.Merge(context.Background(), req("b", stepOf(mergestrategypb.Strategy_SQUASH_REBASE, "s1", uri(head)))) + require.NoError(t, err) + require.Len(t, res.GetSteps(), 1) + assert.Len(t, res.GetSteps()[0].GetOutputs(), 1, "a change of any length collapses into one commit") + // All three commits' content is present despite the single output. + assert.Equal(t, "a\n", f.remoteFile(t, "a.txt")) + assert.Equal(t, "b\n", f.remoteFile(t, "b.txt")) + assert.Equal(t, "c\n", f.remoteFile(t, "c.txt")) + assert.Len(t, f.remoteCommitsSinceSeed(t), 1) +} + +func TestMerge_Merge_MultiCommitChangeSingleURI(t *testing.T) { + f := setupGitFixture(t) + head := f.pushMultiCommitPR(t, "feature/multi", + commitSpec{"a.txt", "a\n", "add a"}, + commitSpec{"b.txt", "b\n", "add b"}, + ) + + m := f.newMerger(t, mergestrategypb.Strategy_REBASE) + res, err := m.Merge(context.Background(), req("b", stepOf(mergestrategypb.Strategy_MERGE, "s1", uri(head)))) + require.NoError(t, err) + assert.Equal(t, "a\n", f.remoteFile(t, "a.txt")) + assert.Equal(t, "b\n", f.remoteFile(t, "b.txt")) + require.Len(t, res.GetSteps(), 1) + assert.Len(t, res.GetSteps()[0].GetOutputs(), 1, "one merge commit regardless of change size") +} + // --- multi-commit changes (one URI, many commits) --- // // A change URI pins a pull request to a single head SHA, so these are the tests @@ -691,7 +967,7 @@ func TestMerge_InvalidRequests(t *testing.T) { }, { name: "unsupported strategy", - req: req("b", stepOf(mergestrategypb.Strategy_MERGE, "s1", uri(fakeSHA))), + req: req("b", stepOf(mergestrategypb.Strategy_PROMOTE, "s1", uri(fakeSHA))), }, { name: "malformed URI", @@ -726,6 +1002,20 @@ func TestCheckMergeability_RebaseMergeable(t *testing.T) { assert.Equal(t, mainBefore, f.remoteHEAD(t), "a dry run does not advance the remote tip") } +func TestCheckMergeability_MergeMergeable(t *testing.T) { + f := setupGitFixture(t) + sha := f.pushPRCommit(t, "feature/a", "hello.txt", "hello\nearth\n", "tweak hello") + mainBefore := f.remoteHEAD(t) + + m := f.newMerger(t, mergestrategypb.Strategy_REBASE) + res, err := m.CheckMergeability(context.Background(), req("b", stepOf(mergestrategypb.Strategy_MERGE, "s1", uri(sha)))) + require.NoError(t, err) + assert.Equal(t, runwaypb.Outcome_SUCCEEDED, res.GetOutcome()) + require.Len(t, res.GetSteps(), 1) + assert.Empty(t, res.GetSteps()[0].GetOutputs()) + assert.Equal(t, mainBefore, f.remoteHEAD(t)) +} + func TestCheckMergeability_Conflict(t *testing.T) { f := setupGitFixture(t) mainBefore, conflictingSHA := f.setupConflict(t) @@ -760,6 +1050,24 @@ func stepOf(strategy mergestrategypb.Strategy, stepID string, uris ...string) *r // --- merger + fixture helpers --- +// remoteHasCommit reports whether sha is reachable in the remote repository. +func (f gitFixture) remoteHasCommit(t *testing.T, sha string) bool { + t.Helper() + mustGit(t, f.checkoutDir, "fetch", "origin") + cmd := exec.Command("git", "cat-file", "-e", sha+"^{commit}") + cmd.Dir = f.checkoutDir + return cmd.Run() == nil +} + +// remoteParents returns the parent SHAs of a commit on the remote. +func (f gitFixture) remoteParents(t *testing.T, sha string) []string { + t.Helper() + mustGit(t, f.checkoutDir, "fetch", "origin") + out := mustGitOutput(t, f.checkoutDir, "rev-list", "--parents", "-n", "1", sha) + fields := strings.Fields(string(out)) + return fields[1:] +} + // commitSpec describes one commit to build on a change branch. type commitSpec struct { path string @@ -1121,5 +1429,8 @@ func absoluteTestPath(t *testing.T, name string) string { } func writeFile(path, contents string) error { + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + return err + } return os.WriteFile(path, []byte(contents), 0o644) }