diff --git a/runway/extension/merger/git/BUILD.bazel b/runway/extension/merger/git/BUILD.bazel index 110b70f7..4633a955 100644 --- a/runway/extension/merger/git/BUILD.bazel +++ b/runway/extension/merger/git/BUILD.bazel @@ -6,6 +6,7 @@ go_library( "author.go", "changeref.go", "git_merger.go", + "headbranch.go", "objects.go", ], importpath = "github.com/uber/submitqueue/runway/extension/merger/git", @@ -25,7 +26,10 @@ go_library( go_test( name = "go_default_test", - srcs = ["git_merger_test.go"], + srcs = [ + "git_merger_test.go", + "headbranch_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 3122ad5d..08241732 100644 --- a/runway/extension/merger/git/README.md +++ b/runway/extension/merger/git/README.md @@ -79,7 +79,23 @@ 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 the final push (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. 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. +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. ## Failure classification diff --git a/runway/extension/merger/git/git_merger.go b/runway/extension/merger/git/git_merger.go index abe75186..42a1662a 100644 --- a/runway/extension/merger/git/git_merger.go +++ b/runway/extension/merger/git/git_merger.go @@ -36,16 +36,26 @@ // on the commits SQUASH_REBASE and MERGE mint, from the author recorded on the // commit each change's URI pins. See author.go. // -// Atomicity: for a committing merge nothing reaches the remote until the final -// push (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. +// 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. // // 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. @@ -141,6 +151,27 @@ 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 @@ -161,14 +192,15 @@ 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 + checkoutPath string + remote string + target string + defaultStrategy mergestrategypb.Strategy + runtime GitRuntime + maxPushAttempts int + fetchRefspecs []string + checkStaleness bool + updateHeadBranch bool // allowUnrelatedHistories permits a MERGE across disjoint history graphs. allowUnrelatedHistories bool @@ -219,14 +251,15 @@ 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, + 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, allowUnrelatedHistories: params.AllowUnrelatedHistories, committerName: committerName, @@ -401,8 +434,12 @@ 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) + baseSHA, stepResults, err := m.tryApply(ctx, steps, commit, tracked) if err == nil { if !commit { // Discard the local commits the dry run created so the checkout @@ -455,8 +492,9 @@ 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. -func (m *gitMerger) tryApply(ctx context.Context, steps []resolvedStep, commit bool) (string, []*runwaymq.StepResult, error) { +// 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) { if err := m.resetToRemote(ctx); err != nil { coremetrics.NamedCounter(m.metricsScope, "merge", "reset_errors", 1) return "", nil, err @@ -466,7 +504,7 @@ func (m *gitMerger) tryApply(ctx context.Context, steps []resolvedStep, commit b return "", nil, err } - stepResults, err := m.applySteps(ctx, steps) + stepResults, heads, 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. @@ -474,6 +512,15 @@ 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 @@ -482,42 +529,53 @@ 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. +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, error) { +func (m *gitMerger) applySteps(ctx context.Context, steps []resolvedStep) ([]*runwaymq.StepResult, []headUpdate, error) { results := make([]*runwaymq.StepResult, 0, len(steps)) + var heads []headUpdate for _, rs := range steps { var ( - outputs []*runwaymq.StepOutput - err error + out applied + err error ) switch rs.strategy { case mergestrategypb.Strategy_REBASE: - outputs, err = m.applyRebase(ctx, rs) + out, err = m.applyRebase(ctx, rs) case mergestrategypb.Strategy_SQUASH_REBASE: - outputs, err = m.applySquashRebase(ctx, rs) + out, err = m.applySquashRebase(ctx, rs) case mergestrategypb.Strategy_MERGE: - outputs, err = m.applyMerge(ctx, rs) + out, err = m.applyMerge(ctx, rs) default: // resolveAndValidate rejects anything else; defensive. - return nil, fmt.Errorf("%w: unsupported strategy %v", merger.ErrInvalidRequest, rs.strategy) + return nil, nil, fmt.Errorf("%w: unsupported strategy %v", merger.ErrInvalidRequest, rs.strategy) } if err != nil { - return nil, err + return nil, nil, err } - results = append(results, &runwaymq.StepResult{StepId: rs.step.GetStepId(), Outputs: outputs}) + results = append(results, &runwaymq.StepResult{StepId: rs.step.GetStepId(), Outputs: out.outputs}) + heads = append(heads, out.heads...) } - return results, nil + return results, heads, 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) ([]*runwaymq.StepOutput, error) { - picked, err := m.pickStepChanges(ctx, rs) +func (m *gitMerger) applyRebase(ctx context.Context, rs resolvedStep) (applied, error) { + picked, heads, err := m.pickStepChanges(ctx, rs) if err != nil { - return nil, err + return applied{}, err } - return toOutputs(picked), nil + return applied{outputs: toOutputs(picked), heads: heads}, nil } // applySquashRebase collapses each change in the step into a single commit. @@ -528,18 +586,19 @@ func (m *gitMerger) applyRebase(ctx context.Context, rs resolvedStep) ([]*runway // 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 +func (m *gitMerger) applySquashRebase(ctx context.Context, rs resolvedStep) (applied, error) { + var out applied for _, ref := range rs.refs { sha, squashed, err := m.squashChange(ctx, rs.step, ref) if err != nil { - return nil, err + return applied{}, err } if squashed { - outputs = append(outputs, &runwaymq.StepOutput{Id: sha}) + out.outputs = append(out.outputs, &runwaymq.StepOutput{Id: sha}) + out.heads = append(out.heads, headUpdate{ref: ref, newSHA: sha}) } } - return outputs, nil + return out, nil } // squashChange replays one change and collapses whatever it produced into a @@ -604,12 +663,15 @@ 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. -func (m *gitMerger) applyMerge(ctx context.Context, rs resolvedStep) ([]*runwaymq.StepOutput, error) { - var outputs []*runwaymq.StepOutput +// +// 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 { contained, err := m.isAncestor(ctx, ref.SHA, "HEAD") if err != nil { - return nil, err + return applied{}, err } if contained { continue @@ -624,23 +686,23 @@ func (m *gitMerger) applyMerge(ctx context.Context, rs resolvedStep) ([]*runwaym // change's own commits keep their authors through the second parent. author, err := m.commitAuthor(ctx, ref.SHA) if err != nil { - return nil, err + return applied{}, err } - out, err := m.runCombinedAs(ctx, author, nil, append(args, ref.SHA)...) + o, err := m.runCombinedAs(ctx, author, 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) + return applied{}, m.classifyMergeFailure(ref, o, conflicted) } mergeSHA, err := m.headSHA(ctx) if err != nil { - return nil, err + return applied{}, err } - outputs = append(outputs, &runwaymq.StepOutput{Id: mergeSHA}) + out.outputs = append(out.outputs, &runwaymq.StepOutput{Id: mergeSHA}) } - return outputs, nil + return out, nil } // promote fast-forwards the target to an already-existing commit. It is only @@ -740,17 +802,22 @@ 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). -func (m *gitMerger) pickStepChanges(ctx context.Context, rs resolvedStep) ([]string, error) { +// 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) { var picked []string + var heads []headUpdate for _, ref := range rs.refs { created, err := m.pickRange(ctx, ref) if err != nil { - return nil, err + return nil, nil, err } picked = append(picked, created...) + if len(created) > 0 { + heads = append(heads, headUpdate{ref: ref, newSHA: created[len(created)-1]}) + } } - return picked, nil + return picked, heads, 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 d4a20c7c..0e8e4b30 100644 --- a/runway/extension/merger/git/git_merger_test.go +++ b/runway/extension/merger/git/git_merger_test.go @@ -1625,6 +1625,17 @@ 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 + target_pushed=1 + fi +done +if [ "$target_pushed" -eq 0 ]; then + exit 0 +fi counter_file="$GIT_DIR/hooks/race-counter" race_sha_file="$GIT_DIR/hooks/race-shas" count=$(cat "$counter_file" 2>/dev/null || echo 0) @@ -1645,6 +1656,31 @@ 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 new file mode 100644 index 00000000..49c54c45 --- /dev/null +++ b/runway/extension/merger/git/headbranch.go @@ -0,0 +1,208 @@ +// 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 new file mode 100644 index 00000000..c5233e13 --- /dev/null +++ b/runway/extension/merger/git/headbranch_test.go @@ -0,0 +1,341 @@ +// 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) + } +}