From db1433e3c201bd47754f7d2a3674d96bc97fafe6 Mon Sep 17 00:00:00 2001 From: sergeyb Date: Thu, 17 Sep 2026 19:26:31 +0000 Subject: [PATCH] refactor(runway): remove head-branch updates from the git merger The git merger no longer moves a change's head branch to its landed commit after a REBASE or SQUASH_REBASE step lands. The `updateHeadBranch` merge-config field and the `MERGE_UPDATE_HEAD_BRANCH` env var are removed along with the mechanism, metrics, docs, and tests. Head-branch finalization (marking a rewritten change's pull request merged) is to be handled by SubmitQueue in a later, separate change. Co-Authored-By: Claude Fable 5.1 --- doc/howto/QUICKSTART.md | 14 +- runway/extension/merger/git/BUILD.bazel | 6 +- runway/extension/merger/git/README.md | 18 +- runway/extension/merger/git/git_merger.go | 128 ++----- .../extension/merger/git/git_merger_test.go | 27 -- runway/extension/merger/git/headbranch.go | 208 ----------- .../extension/merger/git/headbranch_test.go | 341 ------------------ service/runway/README.md | 2 - service/runway/server/config.go | 3 - service/runway/server/config_test.go | 7 - service/runway/server/main.go | 3 - service/submitqueue/demo/provider/README.md | 6 +- .../submitqueue/demo/provider/git/merge.yaml | 12 +- .../demo/provider/github/merge.yaml | 8 - service/submitqueue/docker-compose.git.yml | 4 +- test/e2e/submitqueue/git_suite_test.go | 44 +-- 16 files changed, 52 insertions(+), 779 deletions(-) delete mode 100644 runway/extension/merger/git/headbranch.go delete mode 100644 runway/extension/merger/git/headbranch_test.go diff --git a/doc/howto/QUICKSTART.md b/doc/howto/QUICKSTART.md index 32a24b25b..a094f8a0c 100644 --- a/doc/howto/QUICKSTART.md +++ b/doc/howto/QUICKSTART.md @@ -253,7 +253,7 @@ The last rung is the only one that needs credentials, and the only one where a c ### What you need -A **scratch repository** you are willing to have commits pushed to and branches force-moved on. Do not point this at anything you care about — the merger pushes to the target branch and rewrites the head branch of every change it lands. +A **scratch repository** you are willing to have commits pushed to. Do not point this at anything you care about — the merger pushes every landed change straight to the target branch. A **token** for it, scoped to that one repository. @@ -262,7 +262,7 @@ For a **fine-grained** token, grant these repository permissions. Each is here b | Permission | Access | Needed by | |---|---|---| | Metadata | Read | mandatory on every fine-grained token; GitHub adds it for you | -| Contents | Read and write | the git merger — clone, fetch, push to the target branch, and force-move each landed change's head branch | +| Contents | Read and write | the git merger — clone, fetch, and push to the target branch | | Pull requests | Read | the change provider reads pull request metadata, and `land -pr` reads the head commit | | Pull requests | Read **and write** | only for `make demo-requests`, which opens pull requests | | Actions | Read and write | the build runner — dispatch a run per batch, poll it, cancel it | @@ -306,11 +306,9 @@ It opens real pull requests, enqueues each as it is created, and watches them la demo-queue/3 https://github.com/behinddwalls/sq-demo/pull/524 25s accepted → … → landed ``` -All three show **Merged** on GitHub and their commits are on `main`. +Their commits land on `main`, but the pull requests themselves stay open: nothing here calls GitHub's API to close them, and `SQUASH_REBASE` rewrites the commits, so a pull request's original head never becomes reachable from `main` for GitHub to notice on its own. Closing them requires a separately driven automation, which nothing in this stack provides. -Worth understanding *why* they show merged, because nothing called an API to close them. A provider marks a change merged once its head commit is reachable from the target branch. `SQUASH_REBASE` rewrites the commits, so a pull request's original head is nowhere in `main` — and `updateHeadBranch` therefore moves its branch to the commit it landed as. GitHub draws its own conclusion from that. - -`make demo-requests STACKED=true` submits a chain instead, each pull request targeting the previous one's branch. All of them land as one push to `main`, and all of them show as merged. +`make demo-requests STACKED=true` submits a chain instead, each pull request targeting the previous one's branch. All of them land as one push to `main`, and all of them stay open the same way. **These builds are real.** This rung dispatches your workflow once per speculative batch and polls it to completion, so `landed` here means a build of that batch passed — not that a fake said so. It is the only rung where nothing is faked, and the only one that costs you Actions minutes. [Using real CI](#using-real-ci) below covers what the workflow has to accept and why testing the *combination* is the whole point. @@ -405,9 +403,9 @@ Both MySQL services mount **anonymous** volumes, so a stop/start cycle orphans a **`PROVIDER=github`: the push is rejected on the first try.** Branch protection on `main` — required status checks, or a linear-history or no-force-push rule — applies to the merger like anyone else. Either relax it on the scratch repo or add the token's identity to the bypass list. -**`PROVIDER=github`: the change lands but the pull request stays open.** Two causes, distinguishable in Runway's logs. If the change came from a **fork**, this is expected and permanent: the head branch lives in the contributor's repository, which this stack has no business writing to, and the log says `no head branch on this remote for change`. Otherwise it is **protection on the head branch** blocking the force update, logged as `could not move change head branch`. The land itself succeeded either way — the failure is reported and deliberately not retried, because the push already happened and cannot be undone. +**`PROVIDER=github`: the change lands but the pull request stays open.** Expected under `REBASE` and `SQUASH_REBASE`: nothing here calls GitHub's API to close a pull request, and rewriting its commits leaves its original head unreachable from the target branch, so GitHub has nothing to notice the change by. Closing it requires a separately driven automation, which nothing in this stack provides. -**A change is rejected as stale.** Its head moved after it was submitted, so the commit named is no longer the one under review. Re-submit it. This also happens if you re-land a change that already landed, since landing moved its branch. +**A change is rejected as stale.** Its head moved after it was submitted, so the commit named is no longer the one under review. Re-submit it. **`grpcurl` reports `target server does not expose service`.** The server registers reflection, but its descriptor references `api/base/change/proto/change.proto` while the generated code registers that file as `change.proto`, so reflection cannot resolve the gateway's descriptor. Use the client CLI, which is what every command here does. diff --git a/runway/extension/merger/git/BUILD.bazel b/runway/extension/merger/git/BUILD.bazel index fcaf7ce67..44192d868 100644 --- a/runway/extension/merger/git/BUILD.bazel +++ b/runway/extension/merger/git/BUILD.bazel @@ -6,7 +6,6 @@ go_library( "author.go", "changeref.go", "git_merger.go", - "headbranch.go", "objects.go", ], importpath = "github.com/uber/submitqueue/runway/extension/merger/git", @@ -27,10 +26,7 @@ go_library( go_test( name = "go_default_test", - srcs = [ - "git_merger_test.go", - "headbranch_test.go", - ], + srcs = ["git_merger_test.go"], data = [ "@git", "@git//:git_receive_pack", diff --git a/runway/extension/merger/git/README.md b/runway/extension/merger/git/README.md index 08241732c..0f7721b1f 100644 --- a/runway/extension/merger/git/README.md +++ b/runway/extension/merger/git/README.md @@ -79,23 +79,7 @@ Redelivery is safe: once imported, the source head is contained in the target, s `Merge` commits and reports outputs; `CheckMergeability` runs the identical apply but never pushes, then resets the checkout to discard the local commits and reports empty outputs. A multi-step check commits its intermediate steps locally so it sees the same conflict surface a real merge would. -For a committing merge nothing reaches the remote until every step has applied cleanly (a `PROMOTE` is itself a single atomic fast-forward ref update). A step that fails to apply aborts its in-progress git operation and returns without pushing. With head-branch updates enabled a merge writes two things rather than one — the head branches first, then the target — and only the target's push is the point of no return. If the push fails because the remote tip moved between reset and push, the whole reset/apply/push cycle is retried up to a bounded number of attempts; detection re-fetches the tip and compares it to the SHA the cycle was based on. - -## Head branches - -A provider decides whether a change merged while it processes the push to the target branch, comparing the change's recorded head against what that push makes reachable. `MERGE` and `PROMOTE` satisfy that on their own — the first keeps the change's head reachable through second-parent history, the second fast-forwards the target to it. The picking strategies do not: `REBASE` and `SQUASH_REBASE` produce new commits, so the change's original head appears nowhere in the target's history and the change is recorded as closed after it has, in every meaningful sense, landed. - -Enabling head-branch updates closes that gap. Before the target is pushed, each change's head branch is moved to the commit that change became — its last replayed commit under `REBASE`, its single squashed commit under `SQUASH_REBASE`. The provider records that new head, and when the target push arrives moments later it finds exactly that commit reachable, so it marks the change merged. Nothing here knows what a pull request is: the branch is found by matching the change's pinned head SHA against the remote's branch tips, so the same mechanism serves a GitHub pull request, a GitLab merge request, or a bare branch. - -**The ordering is the mechanism.** Moving the head branch *after* the target has been pushed leaves the provider comparing against the pre-merge head at the only moment it looks, and it records the change closed rather than merged — even though the branch ends up on a commit that is demonstrably in the target. Doing both in a single atomic push behaves the same way, since the provider still evaluates the target update against the head it had recorded beforehand. Only a separate, earlier push works. - -Three cases are declined rather than guessed at. A change whose head matches **no branch** on this remote is normally one proposed from a fork, whose branch lives in another repository and is not this merger's to move — such a change lands normally. A head matching **several branches** is ambiguous, and the URI does not say which one the change was proposed from, so rewriting a guess risks clobbering an unrelated branch. The **target branch itself** is never a candidate, so a change whose head coincides with the target tip cannot make the merger rewrite the branch it just landed on. - -Each push carries a lease against the SHA the change's URI pinned, so an author who pushes in the window between reading the remote's branches and updating them fails the lease instead of losing their work. A failure to move a branch fails the merge, before the target is pushed: landing a change while knowing its head could not be moved produces exactly the half-merged state the option exists to prevent. The three declined cases above are not failures and do not stop the merge. - -A branch a failed attempt already moved is remembered for the next one. Once moved, it no longer sits at the SHA the URI pinned, so a retry could not find it by matching tips and would strand it on a commit that never landed; the attempt's resolved branch and the value the next lease must name are carried forward instead. - -Off by default — moving a branch the merger was not asked to move is a surprise unless a deployment opted in. +For a committing merge nothing reaches the remote until every step has applied cleanly (a `PROMOTE` is itself a single atomic fast-forward ref update). A step that fails to apply aborts its in-progress git operation and returns without pushing, so the target push is the sole point of no return. If the push fails because the remote tip moved between reset and push, the whole reset/apply/push cycle is retried up to a bounded number of attempts; detection re-fetches the tip and compares it to the SHA the cycle was based on. ## Failure classification diff --git a/runway/extension/merger/git/git_merger.go b/runway/extension/merger/git/git_merger.go index f03d05382..af46eac03 100644 --- a/runway/extension/merger/git/git_merger.go +++ b/runway/extension/merger/git/git_merger.go @@ -39,23 +39,14 @@ // Atomicity: for a committing merge nothing reaches the remote until every step // has applied cleanly (PROMOTE excepted, which is itself a single atomic // fast-forward ref update). A step that fails to apply aborts the in-progress -// git operation and returns without pushing. With Params.UpdateHeadBranch the -// merge then writes two things rather than one — the changes' head branches -// first, then the target — and only the target's push is the point of no return. +// git operation and returns without pushing, and the target's push is the point +// of no return. // // Contention: if the push fails because the remote tip moved between reset and // push, the whole reset/apply/push cycle is retried up to Params.MaxPushAttempts // (default 10). Detection re-fetches the remote tip after a push failure and // compares it to the SHA reset to at the start of the cycle. // -// Head branches: a provider decides whether a change merged while it processes -// the push to the target, comparing the change's recorded head against what that -// push makes reachable — a comparison REBASE and SQUASH_REBASE break by -// rewriting the change's commits. With Params.UpdateHeadBranch, a committing -// merge first moves each change's head branch to the commit it became and then -// pushes the target, so the provider sees its own recorded head land and marks -// the change merged rather than closed. See headbranch.go. -// // Dry-run (CheckMergeability) applies the exact same steps but never pushes; // intermediate steps are committed locally so a cumulative multi-step check // sees the same conflict surface, then the checkout is reset to discard them. @@ -151,27 +142,6 @@ type Params struct { // CheckStaleness enables verifying, before applying, that each change's // canonical ref still points at the commit its URI names. CheckStaleness bool - // UpdateHeadBranch moves each change's head branch to the commit that now - // represents it on the target, as its own push immediately before the target - // is pushed. A provider decides merged-versus-closed while processing the - // push to the target, against the head it has recorded at that moment, so - // this is what makes a rewriting strategy have the change marked merged — - // without the merger having to know the provider or call its API. - // - // The ordering is the whole mechanism, not an implementation detail: moving - // the branch after the target has been pushed, or in the same atomic push, - // both leave the provider comparing against the pre-merge head, and it - // records the change as closed instead. - // - // Only REBASE and SQUASH_REBASE need it: MERGE keeps the change's own head - // reachable through second-parent history, and PROMOTE fast-forwards the - // target to that head directly. Off by default, since moving a branch the - // merger was not asked to move is a surprise unless a deployment opted in. - // - // A head branch that cannot be moved fails the merge before the target is - // pushed. A change with no branch of ours to move — proposed from a fork, or - // with a head several branches share — is skipped and lands normally. - UpdateHeadBranch 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 @@ -192,15 +162,14 @@ type Params struct { // gitMerger implements merger.Merger by shelling out to the `git` CLI against a // local checkout. type gitMerger struct { - checkoutPath string - remote string - target string - defaultStrategy mergestrategypb.Strategy - runtime GitRuntime - maxPushAttempts int - fetchRefspecs []string - checkStaleness bool - updateHeadBranch bool + checkoutPath string + remote string + target string + defaultStrategy mergestrategypb.Strategy + runtime GitRuntime + maxPushAttempts int + fetchRefspecs []string + checkStaleness bool // allowUnrelatedHistories permits a MERGE across disjoint history graphs. allowUnrelatedHistories bool @@ -251,15 +220,14 @@ func NewMerger(params Params) (merger.Merger, error) { committerEmail = defaultCommitterEmail } return &gitMerger{ - checkoutPath: params.CheckoutPath, - remote: params.Remote, - target: params.Target, - defaultStrategy: params.DefaultStrategy, - runtime: params.Runtime, - maxPushAttempts: maxAttempts, - fetchRefspecs: params.FetchRefspecs, - checkStaleness: params.CheckStaleness, - updateHeadBranch: params.UpdateHeadBranch, + checkoutPath: params.CheckoutPath, + 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, @@ -434,12 +402,8 @@ func (m *gitMerger) applyTransforming(ctx context.Context, req *runwaymq.MergeRe } var lastErr error - // Head branches an attempt moved before failing to push the target stay - // moved. The tracker carries what that attempt resolved into the next one, so - // the branch is moved on rather than stranded on a commit that never landed. - tracked := make(headBranchTracker) for attempt := 1; attempt <= m.maxPushAttempts; attempt++ { - baseSHA, stepResults, err := m.tryApply(ctx, steps, commit, tracked) + baseSHA, stepResults, err := m.tryApply(ctx, steps, commit) if err == nil { if !commit { // Discard the local commits the dry run created so the checkout @@ -492,9 +456,8 @@ func (m *gitMerger) applyTransforming(ctx context.Context, req *runwaymq.MergeRe // tryApply runs one full reset+apply(+push) cycle. The returned baseSHA is the // SHA the cycle was based on (set as soon as resetToRemote completes) so the -// caller can distinguish concurrent-push contention from other failures. The -// tracker carries head-branch state across attempts; see headBranchTracker. -func (m *gitMerger) tryApply(ctx context.Context, steps []resolvedStep, commit bool, tracked headBranchTracker) (string, []*runwaymq.StepResult, error) { +// caller can distinguish concurrent-push contention from other failures. +func (m *gitMerger) tryApply(ctx context.Context, steps []resolvedStep, commit bool) (string, []*runwaymq.StepResult, error) { if err := m.resetToRemote(ctx); err != nil { coremetrics.NamedCounter(m.metricsScope, "merge", "reset_errors", 1) return "", nil, err @@ -504,7 +467,7 @@ func (m *gitMerger) tryApply(ctx context.Context, steps []resolvedStep, commit b return "", nil, err } - stepResults, heads, err := m.applySteps(ctx, steps) + stepResults, err := m.applySteps(ctx, steps) if err != nil { // The failing apply function aborts its own in-progress git operation; // the next attempt starts with resetToRemote regardless. @@ -512,15 +475,6 @@ func (m *gitMerger) tryApply(ctx context.Context, steps []resolvedStep, commit b } if commit { - // The head branches move first, as their own push. A provider decides - // merged-versus-closed while processing the push to the target, against - // the head it has recorded at that moment, so a head that moves later — - // or in the same atomic push — is recorded too late. See headbranch.go. - if m.updateHeadBranch { - if err := m.updateHeadBranches(ctx, heads, tracked); err != nil { - return baseSHA, nil, err - } - } if err := m.push(ctx); err != nil { coremetrics.NamedCounter(m.metricsScope, "merge", "git_push_errors", 1) return baseSHA, nil, err @@ -529,20 +483,16 @@ func (m *gitMerger) tryApply(ctx context.Context, steps []resolvedStep, commit b return baseSHA, stepResults, nil } -// applied is what one step produced: the commits created on the target, and the -// per-change head updates those commits represent. The two differ because a -// step's outputs are flat while a head update has to stay attributed to the -// change it came from. +// applied is what one step produced: the commits created on the target, in +// application order. type applied struct { outputs []*runwaymq.StepOutput - heads []headUpdate } // applySteps dispatches each step by its resolved strategy, in order, building // up local HEAD and collecting one StepResult per step. -func (m *gitMerger) applySteps(ctx context.Context, steps []resolvedStep) ([]*runwaymq.StepResult, []headUpdate, error) { +func (m *gitMerger) applySteps(ctx context.Context, steps []resolvedStep) ([]*runwaymq.StepResult, error) { results := make([]*runwaymq.StepResult, 0, len(steps)) - var heads []headUpdate for _, rs := range steps { var ( out applied @@ -557,25 +507,24 @@ func (m *gitMerger) applySteps(ctx context.Context, steps []resolvedStep) ([]*ru out, err = m.applyMerge(ctx, rs) default: // resolveAndValidate rejects anything else; defensive. - return nil, nil, fmt.Errorf("%w: unsupported strategy %v", merger.ErrInvalidRequest, rs.strategy) + return nil, fmt.Errorf("%w: unsupported strategy %v", merger.ErrInvalidRequest, rs.strategy) } if err != nil { - return nil, nil, err + return nil, err } results = append(results, &runwaymq.StepResult{StepId: rs.step.GetStepId(), Outputs: out.outputs}) - heads = append(heads, out.heads...) } - return results, heads, nil + return results, nil } // applyRebase cherry-picks the head SHA of every URI of every change in the // step, in order, returning one StepOutput per newly-created commit. func (m *gitMerger) applyRebase(ctx context.Context, rs resolvedStep) (applied, error) { - picked, heads, err := m.pickStepChanges(ctx, rs) + picked, err := m.pickStepChanges(ctx, rs) if err != nil { return applied{}, err } - return applied{outputs: toOutputs(picked), heads: heads}, nil + return applied{outputs: toOutputs(picked)}, nil } // applySquashRebase collapses each change in the step into a single commit. @@ -595,7 +544,6 @@ func (m *gitMerger) applySquashRebase(ctx context.Context, rs resolvedStep) (app } if squashed { out.outputs = append(out.outputs, &runwaymq.StepOutput{Id: sha}) - out.heads = append(out.heads, headUpdate{ref: ref, newSHA: sha}) } } return out, nil @@ -663,9 +611,6 @@ func (m *gitMerger) squashChange(ctx context.Context, step *runwaymq.MergeStep, // 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. -// -// It reports no head updates: the change's own head is already reachable from -// the target, so its branch needs no moving for a provider to call it merged. func (m *gitMerger) applyMerge(ctx context.Context, rs resolvedStep) (applied, error) { var out applied for _, ref := range rs.refs { @@ -796,22 +741,17 @@ func (m *gitMerger) classifyMergeFailure(ref changeRef, out []byte, conflicted b // 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) and, per change that produced any, the last of those -// commits — the one that now represents the change on the target. -func (m *gitMerger) pickStepChanges(ctx context.Context, rs resolvedStep) ([]string, []headUpdate, error) { +// was already present). +func (m *gitMerger) pickStepChanges(ctx context.Context, rs resolvedStep) ([]string, error) { var picked []string - var heads []headUpdate for _, ref := range rs.refs { created, err := m.pickRange(ctx, ref) if err != nil { - return nil, nil, err + return nil, err } picked = append(picked, created...) - if len(created) > 0 { - heads = append(heads, headUpdate{ref: ref, newSHA: created[len(created)-1]}) - } } - return picked, heads, nil + return picked, nil } // pickRange replays every commit the change introduces, not just its head. diff --git a/runway/extension/merger/git/git_merger_test.go b/runway/extension/merger/git/git_merger_test.go index 05a84edf7..7314db6a4 100644 --- a/runway/extension/merger/git/git_merger_test.go +++ b/runway/extension/merger/git/git_merger_test.go @@ -1633,8 +1633,6 @@ func (f gitFixture) installRaceHook(t *testing.T, raceSHAs []string) { strings.Join(raceSHAs, "\n")+"\n", )) const script = `#!/bin/sh -# Contend only on the target branch. A merge that moves change head branches -# pushes those first, and they are not what this hook simulates a race for. target_pushed=0 while read -r _old _new ref; do if [ "$ref" = "refs/heads/main" ]; then @@ -1664,31 +1662,6 @@ exit 1 require.NoError(t, os.WriteFile(hookPath, []byte(script), 0o755)) } -// installRefRejectHook makes the bare remote refuse every push that touches the -// given fully-qualified ref, leaving all other refs alone. Used to fail a head -// branch update without disturbing the target. -func (f gitFixture) installRefRejectHook(t *testing.T, rejectRef string) { - t.Helper() - hookDir := filepath.Join(f.remoteDir, "hooks") - require.NoError(t, os.MkdirAll(hookDir, 0o755)) - // Override the system-wide core.hooksPath so the hook we just wrote actually - // fires on the bare remote. - mustGit(t, f.remoteDir, "config", "core.hooksPath", hookDir) - require.NoError(t, writeFile(filepath.Join(hookDir, "reject-ref"), rejectRef+"\n")) - const script = `#!/bin/sh -reject_ref=$(cat "$GIT_DIR/hooks/reject-ref") -while read -r _old _new ref; do - if [ "$ref" = "$reject_ref" ]; then - echo "hook rejects $ref" >&2 - exit 1 - fi -done -exit 0 -` - hookPath := filepath.Join(hookDir, "pre-receive") - require.NoError(t, os.WriteFile(hookPath, []byte(script), 0o755)) -} - // hookInvocations returns the number of times the pre-receive race hook has // fired. Used by retry tests to verify the loop ran the expected number of // attempts. diff --git a/runway/extension/merger/git/headbranch.go b/runway/extension/merger/git/headbranch.go deleted file mode 100644 index 49c54c456..000000000 --- a/runway/extension/merger/git/headbranch.go +++ /dev/null @@ -1,208 +0,0 @@ -// Copyright (c) 2025 Uber Technologies, Inc. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package git - -import ( - "context" - "fmt" - "strings" - - coremetrics "github.com/uber/submitqueue/platform/metrics" -) - -// headBranchPrefix is the ref namespace a change's head branch lives in. Only -// branches are candidates: a provider's own change refs (refs/pull/*/head, -// refs/merge-requests/*/head) are published by the provider and not writable. -const headBranchPrefix = "refs/heads/" - -// headUpdate pairs a change with the commit that now represents it on the -// target, so its head branch can be moved there before the target is pushed. -type headUpdate struct { - // ref is the change as resolved from its URI, pinning the commit the head - // branch is expected to still be at. - ref changeRef - // newSHA is the commit produced on the target for this change — the last - // commit of its replayed range, or the single commit it squashed to. - newSHA string -} - -// headBranchTracker records, per change, the branch its head was found on and -// the commit this merger last pushed that branch to. It is keyed by the commit -// the change's URI pins, which is fixed for the whole request and so survives -// the retries the value has to cross. -// -// It exists because a branch only answers to the pinned SHA until the first time -// it is moved. An attempt that moves the branch and then fails to push the -// target leaves the branch on a commit that never landed: the next attempt would -// find nothing at the pinned SHA and skip the change as if it had no branch, -// stranding it there. What that attempt learned — which branch, and what value -// the next lease must name — is only available from here. -type headBranchTracker map[string]trackedBranch - -// trackedBranch is one change's resolved head branch and the commit this merger -// last pushed it to. -type trackedBranch struct { - // branch is the fully-qualified ref resolved on the first attempt. - branch string - // sha is the commit the branch was last pushed to, and therefore the value - // the next attempt's lease must name. - sha string -} - -// lookup returns the branch and lease value recorded for a change, if any. -func (t headBranchTracker) lookup(pinnedSHA string) (branch, lease string, ok bool) { - tb, ok := t[pinnedSHA] - return tb.branch, tb.sha, ok -} - -// record notes where a change's head branch now sits. A nil tracker discards the -// record, which is what a caller driving a single update directly wants. -func (t headBranchTracker) record(pinnedSHA, branch, sha string) { - if t == nil { - return - } - t[pinnedSHA] = trackedBranch{branch: branch, sha: sha} -} - -// updateHeadBranches moves each change's head branch to the commit that now -// represents it on the target, as its own push before the target is pushed. -// -// A provider decides whether a change merged while it processes the push to the -// target branch, comparing the change's recorded head against what that push -// makes reachable. The rewriting strategies break that comparison: the commits -// pushed to the target are new objects, so the change's own head appears nowhere -// in the target's history. Repointing the head branch at the commit the change -// became restores it — but only if the provider has already recorded the new -// head by the time it sees the target move. That is why this is a separate, -// earlier push: moving the branch after the target has been pushed, or in the -// same atomic push, both leave the provider comparing against the pre-merge head -// and it records the change as closed rather than merged. -// -// This is deliberately provider-neutral. It matches a SHA against the remote's -// branch tips rather than parsing a change number or calling an API, so it works -// the same for a GitHub pull request, a GitLab merge request, or a plain branch. -// -// A failure to move a branch fails the merge, before the target is pushed. -// Landing a change while knowing its head could not be moved produces exactly -// the half-merged state the option exists to prevent, so the merge stops instead -// of completing into it. Having nothing to move is not a failure: a change -// proposed from a fork, or one whose head several branches share, is skipped and -// the merge carries on. -func (m *gitMerger) updateHeadBranches(ctx context.Context, updates []headUpdate, tracked headBranchTracker) error { - // Resolved lazily: an attempt whose changes were all resolved by an earlier - // one needs no advertisement, and asking for one only adds a way to fail. - var tips map[string][]string - for _, u := range updates { - if u.newSHA == "" || u.newSHA == u.ref.SHA { - continue - } - if _, _, known := tracked.lookup(u.ref.SHA); !known && tips == nil { - var err error - if tips, err = m.remoteBranchTips(ctx); err != nil { - coremetrics.NamedCounter(m.metricsScope, "head_branch", "list_errors", 1) - return fmt.Errorf("list branches on remote %s: %w", m.remote, err) - } - } - if err := m.updateHeadBranchFor(ctx, u, tips, tracked); err != nil { - return err - } - } - return nil -} - -// updateHeadBranchFor moves one change's head branch. It reports an error only -// when a move was attempted and failed; a change with no branch of ours to move -// is a skip, not a failure. -func (m *gitMerger) updateHeadBranchFor(ctx context.Context, u headUpdate, tips map[string][]string, tracked headBranchTracker) error { - if u.newSHA == "" || u.newSHA == u.ref.SHA { - return nil - } - - // A branch an earlier attempt already moved no longer sits at the SHA the URI - // pinned, so the tips will not name it and only the tracker can. - branch, lease, known := tracked.lookup(u.ref.SHA) - if !known { - candidates := tips[u.ref.SHA] - switch len(candidates) { - case 1: - branch, lease = candidates[0], u.ref.SHA - case 0: - // Nothing on this remote is at the change's head. The ordinary cause is - // a change proposed from a fork, whose head branch lives in another - // repository entirely and is not ours to move; a deleted or already - // advanced branch lands here too. All are left alone. - coremetrics.NamedCounter(m.metricsScope, "head_branch", "no_branch", 1) - m.logger.Debugw("no head branch on this remote for change; skipping", - "change", u.ref.Label, "head_sha", u.ref.SHA) - return nil - default: - // Several branches sit on the same commit and nothing in the URI says - // which one the change was proposed from. Guessing risks rewriting an - // unrelated branch, so decline. - coremetrics.NamedCounter(m.metricsScope, "head_branch", "ambiguous", 1) - m.logger.Warnw("several branches point at the change head; skipping", - "change", u.ref.Label, "head_sha", u.ref.SHA, "branches", candidates) - return nil - } - } - - // --force-with-lease names the remote ref and the value it must still hold — - // the SHA the change's URI pinned, or, once an earlier attempt has moved the - // branch, whatever that attempt left it at. An author who pushed while the - // batch was building fails this check, and the merge stops rather than - // discarding their push. The explicit form is what makes the check - // independent of whatever stale remote-tracking refs this checkout holds. - leaseArg := fmt.Sprintf("--force-with-lease=%s:%s", branch, lease) - refspec := u.newSHA + ":" + branch - if _, err := m.run(ctx, nil, "push", leaseArg, m.remote, refspec); err != nil { - coremetrics.NamedCounter(m.metricsScope, "head_branch", "push_errors", 1) - return fmt.Errorf("move head branch %s of change %s from %s to %s: %w", - branch, u.ref.Label, lease, u.newSHA, err) - } - tracked.record(u.ref.SHA, branch, u.newSHA) - - coremetrics.NamedCounter(m.metricsScope, "head_branch", "updated", 1) - m.logger.Infow("moved change head branch to its landed commit", - "change", u.ref.Label, "branch", branch, "from", lease, "to", u.newSHA) - return nil -} - -// remoteBranchTips maps each commit on the remote's branches to the branches -// pointing at it, in one ref advertisement rather than one per change. -// -// The target branch is excluded. It is not a change's head branch, and a change -// whose head happens to equal the target tip would otherwise make this rewrite -// the very branch the merge just landed on. -func (m *gitMerger) remoteBranchTips(ctx context.Context) (map[string][]string, error) { - out, err := m.run(ctx, nil, "ls-remote", "--heads", m.remote) - if err != nil { - return nil, fmt.Errorf("git ls-remote --heads %s: %w", m.remote, err) - } - - targetRef := headBranchPrefix + m.target - tips := make(map[string][]string) - for _, line := range strings.Split(string(out), "\n") { - sha, ref, ok := strings.Cut(strings.TrimSpace(line), "\t") - if !ok { - continue - } - ref = strings.TrimSpace(ref) - if ref == targetRef || !strings.HasPrefix(ref, headBranchPrefix) { - continue - } - tips[sha] = append(tips[sha], ref) - } - return tips, nil -} diff --git a/runway/extension/merger/git/headbranch_test.go b/runway/extension/merger/git/headbranch_test.go deleted file mode 100644 index c5233e136..000000000 --- a/runway/extension/merger/git/headbranch_test.go +++ /dev/null @@ -1,341 +0,0 @@ -// Copyright (c) 2025 Uber Technologies, Inc. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package git - -import ( - "context" - "fmt" - "os/exec" - "strings" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - - mergestrategypb "github.com/uber/submitqueue/api/base/mergestrategy/protopb" -) - -// uriPR builds a change URI for a specific pull request number, so a test can -// carry several distinct changes in one request. -func uriPR(n int, sha string) string { - return fmt.Sprintf("github://github.example.com/uber/submitqueue/pull/%d/%s", n, sha) -} - -// branchSHA returns the SHA at refs/heads/ on the bare remote, and -// whether that branch exists at all. -func (f gitFixture) branchSHA(t *testing.T, branch string) (string, bool) { - t.Helper() - cmd := exec.Command("git", "rev-parse", "--verify", "refs/heads/"+branch) - cmd.Dir = f.remoteDir - out, err := cmd.Output() - if err != nil { - return "", false - } - return strings.TrimSpace(string(out)), true -} - -// mergerWithHeadBranchUpdates builds a merger that moves change head branches -// after a committing merge. -func (f gitFixture) mergerWithHeadBranchUpdates(t *testing.T, strategy mergestrategypb.Strategy) *gitMerger { - t.Helper() - m := f.newMergerWith(t, func(p *Params) { - p.DefaultStrategy = strategy - p.UpdateHeadBranch = true - }) - return m.(*gitMerger) -} - -func TestMerge_Rebase_MovesHeadBranchToLandedCommit(t *testing.T) { - // The rebased commit is a new object, so nothing on the target names the - // change's original head. Moving the branch to the commit the change became - // is what lets a provider see it as merged. - f := setupGitFixture(t) - head := f.pushPRCommit(t, "feature/a", "a.txt", "a\n", "add a") - - m := f.mergerWithHeadBranchUpdates(t, mergestrategypb.Strategy_REBASE) - res, err := m.Merge(context.Background(), req("b", stepOf(mergestrategypb.Strategy_REBASE, "s1", uri(head)))) - require.NoError(t, err) - - landed := res.GetSteps()[0].GetOutputs()[0].GetId() - assert.NotEqual(t, head, landed, "rebase should have produced a new commit") - assert.Equal(t, landed, f.remoteHEAD(t)) - - got, ok := f.branchSHA(t, "feature/a") - require.True(t, ok) - assert.Equal(t, landed, got) -} - -func TestMerge_HeadBranchUntouchedWhenDisabled(t *testing.T) { - f := setupGitFixture(t) - head := f.pushPRCommit(t, "feature/a", "a.txt", "a\n", "add a") - - m := f.newMerger(t, mergestrategypb.Strategy_REBASE) - _, err := m.Merge(context.Background(), req("b", stepOf(mergestrategypb.Strategy_REBASE, "s1", uri(head)))) - require.NoError(t, err) - - got, ok := f.branchSHA(t, "feature/a") - require.True(t, ok) - assert.Equal(t, head, got, "branch must not move unless the deployment opted in") -} - -func TestMerge_SquashRebase_MovesHeadBranchToSquashedCommit(t *testing.T) { - f := setupGitFixture(t) - head := f.pushMultiCommitPR(t, "feature/sq", - commitSpec{"a.txt", "a\n", "add a"}, - commitSpec{"b.txt", "b\n", "add b"}, - ) - - m := f.mergerWithHeadBranchUpdates(t, mergestrategypb.Strategy_SQUASH_REBASE) - res, err := m.Merge(context.Background(), req("b", stepOf(mergestrategypb.Strategy_SQUASH_REBASE, "s1", uri(head)))) - require.NoError(t, err) - - outputs := res.GetSteps()[0].GetOutputs() - require.Len(t, outputs, 1, "squash collapses the change to one commit") - squashed := outputs[0].GetId() - - got, ok := f.branchSHA(t, "feature/sq") - require.True(t, ok) - assert.Equal(t, squashed, got) -} - -func TestMerge_Rebase_MovesEachHeadBranchOfAStack(t *testing.T) { - // Each change in a stack keeps its own identity on the target, so each head - // branch has to land on its own commit rather than all on the final tip. - f := setupGitFixture(t) - first := f.pushPRCommit(t, "feature/one", "one.txt", "one\n", "add one") - second := f.pushPRCommit(t, "feature/two", "two.txt", "two\n", "add two") - - m := f.mergerWithHeadBranchUpdates(t, mergestrategypb.Strategy_REBASE) - res, err := m.Merge(context.Background(), req("b", - stepOf(mergestrategypb.Strategy_REBASE, "s1", uriPR(1, first), uriPR(2, second)), - )) - require.NoError(t, err) - - outputs := res.GetSteps()[0].GetOutputs() - require.Len(t, outputs, 2) - landedFirst, landedSecond := outputs[0].GetId(), outputs[1].GetId() - - gotFirst, ok := f.branchSHA(t, "feature/one") - require.True(t, ok) - assert.Equal(t, landedFirst, gotFirst) - - gotSecond, ok := f.branchSHA(t, "feature/two") - require.True(t, ok) - assert.Equal(t, landedSecond, gotSecond) - - assert.NotEqual(t, gotFirst, gotSecond, "each change lands on its own commit") - assert.Equal(t, landedSecond, f.remoteHEAD(t)) -} - -func TestMerge_HeadBranchSkippedForForkChange(t *testing.T) { - // A change proposed from a fork has no branch in this repository — only the - // provider's read-only change ref. The land must still succeed, and nothing - // here is ours to move. - f := setupGitFixture(t) - head := f.pushPRCommit(t, "feature/forked", "f.txt", "f\n", "add f") - f.publishPRRef(t, 1, head) - mustGit(t, f.authorDir, "push", "origin", "--delete", "feature/forked") - - m := f.mergerWithHeadBranchUpdates(t, mergestrategypb.Strategy_REBASE) - res, err := m.Merge(context.Background(), req("b", stepOf(mergestrategypb.Strategy_REBASE, "s1", uri(head)))) - require.NoError(t, err) - - assert.Equal(t, res.GetSteps()[0].GetOutputs()[0].GetId(), f.remoteHEAD(t)) - _, ok := f.branchSHA(t, "feature/forked") - assert.False(t, ok, "no branch should be resurrected for a fork change") -} - -func TestMerge_HeadBranchSkippedWhenAmbiguous(t *testing.T) { - // Two branches sit on the change's head and the URI does not say which one - // it was proposed from. Rewriting a guess could clobber an unrelated branch. - f := setupGitFixture(t) - head := f.pushPRCommit(t, "feature/a", "a.txt", "a\n", "add a") - mustGit(t, f.authorDir, "push", "origin", head+":refs/heads/feature/a-copy") - - m := f.mergerWithHeadBranchUpdates(t, mergestrategypb.Strategy_REBASE) - _, err := m.Merge(context.Background(), req("b", stepOf(mergestrategypb.Strategy_REBASE, "s1", uri(head)))) - require.NoError(t, err) - - for _, branch := range []string{"feature/a", "feature/a-copy"} { - got, ok := f.branchSHA(t, branch) - require.True(t, ok) - assert.Equal(t, head, got, "%s must be left alone", branch) - } -} - -func TestMerge_Merge_LeavesHeadBranchAlone(t *testing.T) { - // MERGE keeps the change's own head reachable from the target through - // second-parent history, so the branch already satisfies the provider. - f := setupGitFixture(t) - head := f.pushPRCommit(t, "feature/a", "a.txt", "a\n", "add a") - - m := f.mergerWithHeadBranchUpdates(t, mergestrategypb.Strategy_MERGE) - _, err := m.Merge(context.Background(), req("b", stepOf(mergestrategypb.Strategy_MERGE, "s1", uri(head)))) - require.NoError(t, err) - - got, ok := f.branchSHA(t, "feature/a") - require.True(t, ok) - assert.Equal(t, head, got) -} - -func TestMerge_HeadBranchUntouchedForAlreadyLandedChange(t *testing.T) { - // The change contributed no commits, so there is nothing for its branch to - // move to. Redelivery must not disturb it. - f := setupGitFixture(t) - head := f.pushPRCommit(t, "feature/a", "a.txt", "a\n", "add a") - f.advanceMain(t, head) - - m := f.mergerWithHeadBranchUpdates(t, mergestrategypb.Strategy_REBASE) - res, err := m.Merge(context.Background(), req("b", stepOf(mergestrategypb.Strategy_REBASE, "s1", uri(head)))) - require.NoError(t, err) - assert.Empty(t, res.GetSteps()[0].GetOutputs()) - - got, ok := f.branchSHA(t, "feature/a") - require.True(t, ok) - assert.Equal(t, head, got) -} - -func TestCheckMergeability_DoesNotMoveHeadBranch(t *testing.T) { - // A dry run commits nothing, so it has nothing to point a branch at. - f := setupGitFixture(t) - head := f.pushPRCommit(t, "feature/a", "a.txt", "a\n", "add a") - mainBefore := f.remoteHEAD(t) - - m := f.mergerWithHeadBranchUpdates(t, mergestrategypb.Strategy_REBASE) - _, err := m.CheckMergeability(context.Background(), req("b", stepOf(mergestrategypb.Strategy_REBASE, "s1", uri(head)))) - require.NoError(t, err) - - assert.Equal(t, mainBefore, f.remoteHEAD(t)) - got, ok := f.branchSHA(t, "feature/a") - require.True(t, ok) - assert.Equal(t, head, got) -} - -func TestUpdateHeadBranchFor_StaleLeaseFailsAndLeavesBranchAlone(t *testing.T) { - // The guard against the window between reading the remote's branches and - // pushing: if the author moved the branch in between, the lease must refuse - // rather than discard their push. Driven directly, since the race is not - // reproducible through Merge. - f := setupGitFixture(t) - pinned := f.pushPRCommit(t, "feature/a", "a.txt", "a\n", "add a") - moved := f.pushPRCommit(t, "feature/a", "a.txt", "a2\n", "author pushed again") - require.NotEqual(t, pinned, moved) - - landed := f.pushPRCommit(t, "feature/other", "z.txt", "z\n", "some landed commit") - - m := f.mergerWithHeadBranchUpdates(t, mergestrategypb.Strategy_REBASE) - // A stale view of the remote: it claims feature/a is still at the pinned - // SHA, which is exactly what a racing push invalidates. - stale := map[string][]string{pinned: {"refs/heads/feature/a"}} - err := m.updateHeadBranchFor(context.Background(), - headUpdate{ref: changeRef{SHA: pinned, Label: "uber/submitqueue#1"}, newSHA: landed}, - stale, - nil, - ) - require.Error(t, err, "a refused lease must fail the merge, not be swallowed") - - got, ok := f.branchSHA(t, "feature/a") - require.True(t, ok) - assert.Equal(t, moved, got, "the author's push must survive") -} - -func TestMerge_HeadBranchMovesBeforeTheTargetIsPushed(t *testing.T) { - // Ordering is the whole mechanism: a provider compares a change's recorded - // head against the target push while processing it, so the head has to be - // recorded first. Observed by failing the target push and checking the head - // branch moved anyway — which can only be true if it moved first. - f := setupGitFixture(t) - head := f.pushPRCommit(t, "feature/a", "a.txt", "a\n", "add a") - mainBefore := f.remoteHEAD(t) - race := f.pushPRCommit(t, "race", "race.txt", "race\n", "race commit") - f.installRaceHook(t, []string{race}) - - m := f.newMergerWith(t, func(p *Params) { - p.DefaultStrategy = mergestrategypb.Strategy_REBASE - p.UpdateHeadBranch = true - p.MaxPushAttempts = 1 - }) - _, err := m.Merge(context.Background(), req("b", stepOf(mergestrategypb.Strategy_REBASE, "s1", uri(head)))) - require.Error(t, err, "the target push was rejected, so the merge failed") - - got, ok := f.branchSHA(t, "feature/a") - require.True(t, ok) - assert.NotEqual(t, head, got, "the head branch moved before the target push was attempted") - assert.NotEqual(t, mainBefore, f.remoteHEAD(t), "the hook moved the target out from under us") -} - -func TestMerge_HeadBranchMovedAgainAfterTargetContention(t *testing.T) { - // An attempt that moved the branch and then lost the target push leaves it on - // a commit that never landed. The retry has to move it on to the commit that - // did — it can no longer be found by matching the SHA the URI pinned. - f := setupGitFixture(t) - race := f.pushPRCommit(t, "race", "race.txt", "race\n", "race commit") - head := f.pushPRCommit(t, "feature/a", "a.txt", "a\n", "add a") - f.installRaceHook(t, []string{race}) - - m := f.mergerWithHeadBranchUpdates(t, mergestrategypb.Strategy_REBASE) - res, err := m.Merge(context.Background(), req("b", stepOf(mergestrategypb.Strategy_REBASE, "s1", uri(head)))) - require.NoError(t, err) - assert.Equal(t, 2, f.hookInvocations(t), "first target push rejected, second allowed through") - - landed := res.GetSteps()[0].GetOutputs()[0].GetId() - assert.Equal(t, landed, f.remoteHEAD(t)) - - got, ok := f.branchSHA(t, "feature/a") - require.True(t, ok) - assert.Equal(t, landed, got, "the branch follows the commit that actually landed") -} - -func TestMerge_HeadBranchPushFailureFailsTheLand(t *testing.T) { - // Landing a change while knowing its head could not be moved produces exactly - // the half-merged state the option exists to prevent, so the merge stops - // before the target is pushed rather than completing into it. - f := setupGitFixture(t) - head := f.pushPRCommit(t, "feature/a", "a.txt", "a\n", "add a") - mainBefore := f.remoteHEAD(t) - f.installRefRejectHook(t, "refs/heads/feature/a") - - m := f.mergerWithHeadBranchUpdates(t, mergestrategypb.Strategy_REBASE) - _, err := m.Merge(context.Background(), req("b", stepOf(mergestrategypb.Strategy_REBASE, "s1", uri(head)))) - require.Error(t, err) - - assert.Equal(t, mainBefore, f.remoteHEAD(t), "the target must not have been pushed") - got, ok := f.branchSHA(t, "feature/a") - require.True(t, ok) - assert.Equal(t, head, got) -} - -func TestRemoteBranchTips(t *testing.T) { - f := setupGitFixture(t) - head := f.pushPRCommit(t, "feature/a", "a.txt", "a\n", "add a") - mustGit(t, f.authorDir, "push", "origin", head+":refs/heads/feature/a-copy") - f.publishPRRef(t, 7, head) - - m := f.mergerWithHeadBranchUpdates(t, mergestrategypb.Strategy_REBASE) - tips, err := m.remoteBranchTips(context.Background()) - require.NoError(t, err) - - assert.ElementsMatch(t, - []string{"refs/heads/feature/a", "refs/heads/feature/a-copy"}, - tips[head], - "both branches at the head are reported, and the read-only change ref is not a branch", - ) - - // The target is excluded: a change whose head happened to equal the target - // tip would otherwise make the merger rewrite the branch it just landed on. - for sha, refs := range tips { - assert.NotContains(t, refs, "refs/heads/main", "target must never be a candidate (sha %s)", sha) - } -} diff --git a/service/runway/README.md b/service/runway/README.md index 28710a624..d0f848d9d 100644 --- a/service/runway/README.md +++ b/service/runway/README.md @@ -34,7 +34,6 @@ queues: target: main checkoutPath: /var/runway/checkouts/sq-sandbox defaultStrategy: SQUASH_REBASE - updateHeadBranch: true tokenEnv: GITHUB_TOKEN ``` @@ -81,7 +80,6 @@ The Runway controllers themselves live under [`runway/controller/`](../../runway | `MERGE_COMMITTER_EMAIL` | no | Committer email for service-created commits | `runway@submitqueue.invalid` | | `GIT_EXECUTABLE` / `GIT_EXEC_PATH` / `GIT_TEMPLATE_DIR` | no | Absolute paths pinning the git runtime. Each is derived from the installed git when unset — the executable from `PATH`, the exec path from `git --exec-path`, the templates from the matching install prefix. | derived | | `MERGE_CHECK_STALENESS` | no | Verify each change's provider ref still points at the commit its URI names before applying | `true` | -| `MERGE_UPDATE_HEAD_BRANCH` | no | Before pushing the target, move each change's head branch to the commit it landed as, so the provider marks the change merged rather than closed. Only affects `REBASE`/`SQUASH_REBASE`. | `false` | | `MERGE_ALLOW_UNRELATED_HISTORIES` | no | Let a `MERGE` step integrate a change sharing no ancestry with the target (repository imports). Leave off unless the queue exists to perform imports. | `false` | | `MERGE_FETCH_REFSPECS` | no | Comma-separated extra refspecs fetched each cycle. Only needed for a remote that refuses to serve an unadvertised commit by SHA. | — | diff --git a/service/runway/server/config.go b/service/runway/server/config.go index 94b64a09e..d66d31c3d 100644 --- a/service/runway/server/config.go +++ b/service/runway/server/config.go @@ -84,9 +84,6 @@ type mergerConfig struct { // CheckStaleness verifies each change still points at the commit its URI // names before applying it. Defaults to true. CheckStaleness *bool `yaml:"checkStaleness"` - // UpdateHeadBranch moves each change's head branch to the commit it landed - // as, so the provider marks it merged. Defaults to false. - UpdateHeadBranch bool `yaml:"updateHeadBranch"` // AllowUnrelatedHistories lets a MERGE step integrate a change sharing no // ancestry with the target. Defaults to false. AllowUnrelatedHistories bool `yaml:"allowUnrelatedHistories"` diff --git a/service/runway/server/config_test.go b/service/runway/server/config_test.go index a1d24f18a..684a676eb 100644 --- a/service/runway/server/config_test.go +++ b/service/runway/server/config_test.go @@ -65,7 +65,6 @@ queues: assert.Equal(t, mergestrategypb.Strategy_REBASE, demo.strategy()) require.NotNil(t, demo.CheckStaleness) assert.True(t, *demo.CheckStaleness, "staleness checking is on unless turned off") - assert.False(t, demo.UpdateHeadBranch) assert.True(t, cfg.usesGit()) } @@ -83,7 +82,6 @@ queues: checkoutPath: /var/checkouts/r defaultStrategy: SQUASH_REBASE checkStaleness: false - updateHeadBranch: true tokenEnv: SOME_TOKEN tokenUser: oauth2 `) @@ -98,7 +96,6 @@ queues: assert.Equal(t, mergestrategypb.Strategy_SQUASH_REBASE, demo.strategy()) require.NotNil(t, demo.CheckStaleness) assert.False(t, *demo.CheckStaleness) - assert.True(t, demo.UpdateHeadBranch) } func TestLoadMergeConfig_NoopOnlyNeedsNoGit(t *testing.T) { @@ -303,10 +300,6 @@ func TestLoadMergeConfig_RejectsSharedCheckoutWithDivergentMergerFields(t *testi name: "default strategy", b: "{type: git, remoteUrl: https://example.com/o/r.git, checkoutPath: /var/checkouts/r, defaultStrategy: REBASE}", }, - { - name: "update head branch", - b: "{type: git, remoteUrl: https://example.com/o/r.git, checkoutPath: /var/checkouts/r, updateHeadBranch: true}", - }, { name: "staleness checking", b: "{type: git, remoteUrl: https://example.com/o/r.git, checkoutPath: /var/checkouts/r, checkStaleness: false}", diff --git a/service/runway/server/main.go b/service/runway/server/main.go index 46adbe329..263ddb5fa 100644 --- a/service/runway/server/main.go +++ b/service/runway/server/main.go @@ -477,7 +477,6 @@ func loadMergeConfigFromEnv(logger *zap.Logger) (mergeConfig, error) { // graphs, which is a safeguard everywhere except a queue whose purpose // is importing one repository's history into another. AllowUnrelatedHistories: envBool("MERGE_ALLOW_UNRELATED_HISTORIES", false), - UpdateHeadBranch: envBool("MERGE_UPDATE_HEAD_BRANCH", false), FetchRefspecs: splitRefspecs(os.Getenv("MERGE_FETCH_REFSPECS")), CommitterName: os.Getenv("MERGE_COMMITTER_NAME"), CommitterEmail: os.Getenv("MERGE_COMMITTER_EMAIL"), @@ -538,7 +537,6 @@ func (b *mergerBuilder) build(cfg mergerConfig, where string) (merger.Factory, e MaxPushAttempts: cfg.MaxPushAttempts, FetchRefspecs: cfg.FetchRefspecs, CheckStaleness: *cfg.CheckStaleness, - UpdateHeadBranch: cfg.UpdateHeadBranch, AllowUnrelatedHistories: cfg.AllowUnrelatedHistories, CommitterName: cfg.CommitterName, CommitterEmail: cfg.CommitterEmail, @@ -553,7 +551,6 @@ func (b *mergerBuilder) build(cfg mergerConfig, where string) (merger.Factory, e zap.String("checkout", cfg.CheckoutPath), zap.String("target", cfg.Target), zap.String("default_strategy", cfg.strategy().String()), - zap.Bool("update_head_branch", cfg.UpdateHeadBranch), ) f := &gitMergerFactory{merger: m} b.byTarget[cfg.CheckoutPath] = f diff --git a/service/submitqueue/demo/provider/README.md b/service/submitqueue/demo/provider/README.md index 4ee0987d2..19a039068 100644 --- a/service/submitqueue/demo/provider/README.md +++ b/service/submitqueue/demo/provider/README.md @@ -37,12 +37,10 @@ Everything provider-specific is reached through an existing seam, so a new provi The Makefile line is there because a mode's *mounts* are not something the two config files can express: `github` needs a credential in the environment, `git` needs the sandbox and checkout directories bind-mounted, and `fake` needs neither. A provider reaching a remote API over a token is the common case and can reuse `docker-compose.provider.yml` verbatim, so for most new providers that line names an existing file rather than a new one. -What is **not** on that list is the point of it: the merger's apply and push paths, the head-branch update, the orchestrator pipeline, the wire contract, and the hermetic git E2E are all provider-independent and need no change. +What is **not** on that list is the point of it: the merger's apply and push paths, the orchestrator pipeline, the wire contract, and the hermetic git E2E are all provider-independent and need no change. -Two of those deserve explanation. +One of those deserves explanation. **The merger stays provider-neutral** because `resolveChange` reduces every URI to the same three things — the commit to apply, the ref it lives under, and a label — before any git command runs. The apply paths never learn which provider a change came from. -**Marking a change merged needs no provider API.** A provider decides whether a change merged while it processes the push to the target branch, comparing the change's recorded head against what that push makes reachable. `MERGE` and `PROMOTE` satisfy that by construction; the rewriting strategies do not, so `updateHeadBranch` moves the change's head branch to the commit it landed as — as its own push, immediately before the target is pushed. The ordering is the mechanism: a head moved *after* the target has been pushed, or in the same atomic push, is recorded too late, and the provider marks the change closed rather than merged even though its head is demonstrably on the target. That works by matching a SHA against the remote's branch tips — no change number, no API call — so it behaves identically for a GitHub pull request and a GitLab merge request. The one case it cannot serve is a change proposed from a fork, whose head branch lives in another repository: such a change lands and stays open. - See [doc/howto/QUICKSTART.md](../../../../doc/howto/QUICKSTART.md) for running each of these by hand, from the credential-free modes to a real land against GitHub. diff --git a/service/submitqueue/demo/provider/git/merge.yaml b/service/submitqueue/demo/provider/git/merge.yaml index fcba9da95..8a191147d 100644 --- a/service/submitqueue/demo/provider/git/merge.yaml +++ b/service/submitqueue/demo/provider/git/merge.yaml @@ -1,9 +1,9 @@ # Merge targets for the "git" example: a plain git remote with no provider. # # The target is a bare repository on a shared volume, addressed by path. That -# exercises the whole merge machinery (real fetch, cherry-pick, push, -# head-branch update) with no credential, no network, and no provider account, -# which is what lets the hermetic git E2E (`make e2e-git-test`) gate PRs in CI. +# exercises the whole merge machinery (real fetch, cherry-pick, push) with no +# credential, no network, and no provider account, which is what lets the +# hermetic git E2E (`make e2e-git-test`) gate PRs in CI. # # It doubles as the worked example of a non-GitHub target: nothing below names a # provider, because the merger does not have one. See ../README.md. @@ -29,11 +29,6 @@ queues: checkoutPath: /var/runway/checkouts/demo defaultStrategy: SQUASH_REBASE checkStaleness: true - # Rewriting strategies leave the change's original head unreachable from - # the target, so its branch is moved to the commit it landed as. On a - # provider this is what makes the change show as merged; here it is simply - # observable as the branch having moved. - updateHeadBranch: true # Driven by the E2E, which asserts against the repository itself — what # reached the target branch, in what order, and in how many ref updates. These @@ -48,4 +43,3 @@ queues: checkoutPath: /var/runway/checkouts/sandbox defaultStrategy: REBASE checkStaleness: true - updateHeadBranch: true diff --git a/service/submitqueue/demo/provider/github/merge.yaml b/service/submitqueue/demo/provider/github/merge.yaml index 9cacb6ad6..865811848 100644 --- a/service/submitqueue/demo/provider/github/merge.yaml +++ b/service/submitqueue/demo/provider/github/merge.yaml @@ -30,14 +30,6 @@ queues: # Refuse a change whose head has moved since it was submitted, rather # than landing a commit nobody is looking at any more. checkStaleness: true - # SQUASH_REBASE rewrites commits, so the pull request's original head is - # not reachable from the target and GitHub would leave it open. Moving the - # head branch to the commit it landed as is what makes GitHub mark it - # merged — no API call, no close-PR permission. - # - # This cannot work for a pull request opened from a fork: its head branch - # lives in another repository. Such a change lands and stays open. - updateHeadBranch: true # The credential for cloning, fetching, and pushing. Written into the # checkout as an HTTP Authorization header — never into the remote URL, # which git would echo back in error messages. diff --git a/service/submitqueue/docker-compose.git.yml b/service/submitqueue/docker-compose.git.yml index e117575e6..1de039f0e 100644 --- a/service/submitqueue/docker-compose.git.yml +++ b/service/submitqueue/docker-compose.git.yml @@ -11,8 +11,8 @@ # # The merge target is a bare repository on a shared volume, addressed by path. # That exercises the whole merge path (git in the image, checkout provisioning, -# real cherry-pick and push, head-branch updates) with no credential, no -# network, and no account anywhere — which is what lets it gate a pull request. +# real cherry-pick and push) with no credential, no network, and no account +# anywhere — which is what lets it gate a pull request. # # Required in the environment, all owned by the test: # SQ_PROVIDER_CONFIG_DIR profiles.yaml / merge.yaml selecting each queue's diff --git a/test/e2e/submitqueue/git_suite_test.go b/test/e2e/submitqueue/git_suite_test.go index 321051ec0..2cc0be56e 100644 --- a/test/e2e/submitqueue/git_suite_test.go +++ b/test/e2e/submitqueue/git_suite_test.go @@ -195,29 +195,6 @@ func (s *GitMergeSuite) TestLand_Stack_LandsInOrderInOneRefUpdate() { "the whole stack must reach the target in exactly one ref update") } -func (s *GitMergeSuite) TestLand_MovesEachChangeHeadBranchToItsLandedCommit() { - // What makes a provider mark a rebased change merged: its head branch is moved - // to the commit the change became, so the head is reachable from the target. - before := s.mainSHA() - first := s.pushChange("feature/head-1", map[string]string{"h1.txt": "h1\n"}, "add h1") - second := s.pushChange("feature/head-2", map[string]string{"h2.txt": "h2\n"}, "add h2") - - sqid := s.land(gitQueue, s.uri("feature/head-1", first), s.uri("feature/head-2", second)) - s.requireStatus(sqid, entity.RequestStatusLanded) - - landed := s.shasSince(before) - s.Require().Len(landed, 2) - - // Each change gets its own landed commit, not the final tip. - s.Equal(landed[0], s.branchSHA("feature/head-1")) - s.Equal(landed[1], s.branchSHA("feature/head-2")) - s.NotEqual(s.branchSHA("feature/head-1"), s.branchSHA("feature/head-2")) - - // Reachability is the property a provider actually reads. - s.True(s.isAncestorOfMain(s.branchSHA("feature/head-1"))) - s.True(s.isAncestorOfMain(s.branchSHA("feature/head-2"))) -} - func (s *GitMergeSuite) TestLand_Conflict_FailsAndLeavesTheTargetUntouched() { // Two changes editing the same line from the same base: the first lands, // the second cannot be replayed onto it. @@ -236,23 +213,16 @@ func (s *GitMergeSuite) TestLand_Conflict_FailsAndLeavesTheTargetUntouched() { s.Equal(loser, s.branchSHA("feature/conflict-b")) } -func (s *GitMergeSuite) TestLand_ResubmittedAfterLanding_IsRejectedAsStale() { - // Landing a change moves its head branch to the commit it became, so the - // URI that was submitted no longer describes where that branch points. The - // staleness check catches exactly that, which is what stops a change from - // being replayed onto the target a second time. +func (s *GitMergeSuite) TestLand_ResubmittedAfterLanding_IsSuccessfulNoOp() { head := s.pushChange("feature/already", map[string]string{"already.txt": "already\n"}, "add already") s.requireStatus(s.land(gitQueue, s.uri("feature/already", head)), entity.RequestStatusLanded) settled := s.mainSHA() updates := s.mainRefUpdateCount() - landedAs := s.branchSHA("feature/already") - s.NotEqual(head, landedAs, "the head branch moved to the landed commit") - s.requireStatus(s.land(gitQueue, s.uri("feature/already", head)), entity.RequestStatusError) - s.Equal(settled, s.mainSHA(), "a stale resubmission must not move the target") + s.requireStatus(s.land(gitQueue, s.uri("feature/already", head)), entity.RequestStatusLanded) + s.Equal(settled, s.mainSHA(), "a change already on the target must not move it again") s.Equal(updates, s.mainRefUpdateCount(), "and must not push at all") - s.Equal(landedAs, s.branchSHA("feature/already"), "nor disturb the change's branch") } // --- gateway helpers --- @@ -396,14 +366,6 @@ func (s *GitMergeSuite) fileOnMain(path string) string { return s.runGit(s.bare, "show", "refs/heads/main:"+path) + "\n" } -// isAncestorOfMain reports whether a commit is reachable from the target — the -// property a provider reads to decide a change has merged. -func (s *GitMergeSuite) isAncestorOfMain(sha string) bool { - cmd := exec.Command(s.git, "merge-base", "--is-ancestor", sha, "refs/heads/main") - cmd.Dir = s.bare - return cmd.Run() == nil -} - // mainRefUpdateCount is how many times the target branch has been updated, // read from the bare repository's reflog. One land must cost exactly one, // however many changes it carried.