From bec528c6e9e694c5516502e68ffb934ceefd356e Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Fri, 31 Jul 2026 11:35:17 -0700 Subject: [PATCH] feat(runway): credit the change author on squash and merge commits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary ### Why? Git records an author and a committer separately, and the merger was collapsing the two: `command()` pins `user.name`/`user.email` to the configured merger identity on every invocation, so any commit the merger creates is both authored and committed by `SubmitQueue Runway `. A landed change shows the service, not the person who wrote it — in `git log`, in blame, and in every tool built on them. `REBASE` was never affected: cherry-pick carries each commit's author across on its own. The strategies that mint a fresh commit are the ones that lose it — `SQUASH_REBASE` builds its commit with `reset --soft` + `git commit`, and `MERGE` creates a `--no-ff` merge commit. `PROMOTE` creates no commit at all, so there is nothing to attribute. ### What? The committer stays the merger — it is what applied the change, and that is what a committer means. The author now comes from the change: the author recorded on the commit its URI pins. For a change spanning several commits by different people that is the head commit's author, which is the one identity the request actually names. The author is read out of the local object store (`git show --no-patch --format=%an%x00%ae`), so this needs nothing on the wire and costs no network — every referenced commit is already fetched and verified before a step is applied. No change to the `Change` proto, and no resolver, keeping the property that the merger reads change URIs straight from the payload. A commit recording no usable author (either half missing) falls back to the committer identity rather than failing the merge. The identity travels through `GIT_AUTHOR_NAME`/`GIT_AUTHOR_EMAIL` rather than `git commit --author`. Two reasons: `git merge` has no `--author` flag, so the merge commit could not use one; and the environment carries the name and address as separate values, where `--author` takes a single `Name
` string that git parses back apart — a display name containing an angle bracket splits in the wrong place, and one that parses to nothing makes git search history for a matching author and fail the commit outright. New `author.go` holds `authorIdent` and `commitAuthor`. `run`/`runCombined`/`command` gained `…As` variants taking an author; the existing signatures delegate to them with a zero author, so every other call site is unchanged. ## Test Plan ✅ `bazel test //runway/...` — 6/6 targets pass. New tests in `git_merger_test.go`: squash credits the change author, the `--no-ff` merge commit credits the change author, a multi-author change credits its head commit's author, and `REBASE` keeps each commit's own author. Every one also asserts the committer is still the merger, which is the invariant that separates attribution from impersonation. `TestAuthorIdent` covers the fallback when either half is missing, and a name containing angle brackets surviving verbatim. Verified the tests actually catch the bug: reverting just the two attribution call sites fails exactly the three attribution tests and leaves the `REBASE` one passing, which is the expected profile since `REBASE` was already correct. Ref: the equivalent change on the gitfarm merger, `uber-code/go-code` 454437cc — same intent, different mechanism, because that merger resolves the PR author over RPC where this one has the commits locally. --- runway/extension/merger/git/BUILD.bazel | 1 + runway/extension/merger/git/README.md | 10 + runway/extension/merger/git/author.go | 92 +++++++++ runway/extension/merger/git/git_merger.go | 50 ++++- .../extension/merger/git/git_merger_test.go | 175 ++++++++++++++++++ 5 files changed, 323 insertions(+), 5 deletions(-) create mode 100644 runway/extension/merger/git/author.go diff --git a/runway/extension/merger/git/BUILD.bazel b/runway/extension/merger/git/BUILD.bazel index a173bd6b..110b70f7 100644 --- a/runway/extension/merger/git/BUILD.bazel +++ b/runway/extension/merger/git/BUILD.bazel @@ -3,6 +3,7 @@ load("@rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "go_default_library", srcs = [ + "author.go", "changeref.go", "git_merger.go", "objects.go", diff --git a/runway/extension/merger/git/README.md b/runway/extension/merger/git/README.md index d381567d..3122ad5d 100644 --- a/runway/extension/merger/git/README.md +++ b/runway/extension/merger/git/README.md @@ -55,6 +55,16 @@ Fetching by SHA guarantees the merger applies exactly the commit a URI names — `PROMOTE` is exclusive because a pre-existing commit cannot descend from commits an earlier transforming step produced, and it advances the ref to an exact SHA rather than to the locally-built HEAD. Mixing it with any other step is rejected as an invalid request. +## Authorship + +Git records an author and a committer separately, and the merger keeps them apart: the committer is always the merger's configured identity, because it is what applied the change, while the author is the person who wrote it. + +`REBASE` gets this for free — cherry-pick carries each commit's author across, so every landed commit keeps whoever wrote it. The strategies that mint a fresh commit do not: the squash commit and the `--no-ff` merge commit are new objects, and without attribution both would be credited to the service. Each is therefore authored as the author recorded on the commit its change's URI pins. For a change spanning several commits by different people, that is the head commit's author — the one identity the request actually names. + +The author is read out of the local object store, so this costs no network and needs nothing on the wire: every referenced commit is already fetched and verified before a step is applied. A commit that records no usable author (either half missing) falls back to the committer identity rather than failing the merge. + +The author travels to git through `GIT_AUTHOR_NAME`/`GIT_AUTHOR_EMAIL` rather than `git commit --author`, because `git merge` has no `--author` flag and because the environment keeps the name and address as separate values — a single `Name
` string has to be parsed back apart, which a display name containing an angle bracket breaks. + ## Importing an unrelated history A repository migration arrives as an ordinary change in the target repo whose branch carries the source repo's entire history. Living in the target repo is what makes its commits fetchable; it says nothing about ancestry, and the two graphs still share no common ancestor, so git refuses the merge by default. diff --git a/runway/extension/merger/git/author.go b/runway/extension/merger/git/author.go new file mode 100644 index 00000000..408effb9 --- /dev/null +++ b/runway/extension/merger/git/author.go @@ -0,0 +1,92 @@ +// 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" +) + +// authorIdent is the person a commit is credited to, as distinct from the +// committer that produced it. +// +// Git records the two separately and the merger keeps them separate: the +// merger is always the committer, because it is what applied the change, while +// the author is carried over from the change itself. A strategy that mints a +// fresh commit would otherwise credit the service for work someone else wrote. +type authorIdent struct { + // Name is the author's display name, verbatim from the commit it was read + // from. Empty when the commit did not record one. + Name string + // Email is the author's email address, verbatim from the commit it was read + // from. Empty when the commit did not record one. + Email string +} + +// complete reports whether both halves are present. A half-known identity is +// unusable — git wants a name and an address, and substituting a placeholder +// for the missing half would attribute the commit to someone who does not +// exist, which is worse than not attributing it at all. +func (a authorIdent) complete() bool { + return a.Name != "" && a.Email != "" +} + +// env renders the identity as the environment git reads an author from. It +// returns nil for an incomplete identity, which leaves the configured committer +// identity to stand in for both roles — the behavior from before attribution +// existed. +// +// The identity travels as GIT_AUTHOR_* rather than as `git commit --author` +// for two reasons. `git merge` has no --author flag at all, so the merge +// strategy could not use one. And the environment carries name and address as +// two values, where --author takes a single "Name
" string that git +// has to parse back apart — a name containing an angle bracket splits in the +// wrong place, and one that parses to nothing makes git search the history for +// a matching author and fail the commit outright. +func (a authorIdent) env() []string { + if !a.complete() { + return nil + } + return []string{ + "GIT_AUTHOR_NAME=" + a.Name, + "GIT_AUTHOR_EMAIL=" + a.Email, + } +} + +// commitAuthor reads the author recorded on a commit. +// +// The commit is the one the change's URI pins, so the attribution follows what +// the request names rather than anything the merger infers. It is already +// present locally: every referenced commit is fetched and verified before a +// step is applied, so this reads from the object store and costs no network. +// +// A commit with no readable author is not an error. It yields an incomplete +// identity, and the caller falls back to the committer identity rather than +// failing a merge over attribution. +func (m *gitMerger) commitAuthor(ctx context.Context, sha string) (authorIdent, error) { + // NUL-separated: a display name may contain spaces, angle brackets, quotes + // or anything else a person put in their git config, and none of those can + // be mistaken for the field separator. + out, err := m.run(ctx, nil, "show", "--no-patch", "--format=%an%x00%ae", sha) + if err != nil { + return authorIdent{}, fmt.Errorf("git show --no-patch --format author %s: %w", sha, err) + } + name, email, ok := strings.Cut(strings.TrimRight(string(out), "\n"), "\x00") + if !ok { + return authorIdent{}, nil + } + return authorIdent{Name: name, Email: email}, nil +} diff --git a/runway/extension/merger/git/git_merger.go b/runway/extension/merger/git/git_merger.go index f148452f..abe75186 100644 --- a/runway/extension/merger/git/git_merger.go +++ b/runway/extension/merger/git/git_merger.go @@ -30,6 +30,12 @@ // - DEFAULT: resolved to the instance's configured DefaultStrategy // before any step runs. // +// Authorship: the committer is always the merger's configured identity, since +// it is what applied the change. The author is the person who wrote it — +// carried across automatically by cherry-pick under REBASE, and set explicitly +// on the commits SQUASH_REBASE and MERGE mint, from the author recorded on the +// commit each change's URI pins. See author.go. +// // 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 @@ -577,7 +583,13 @@ func (m *gitMerger) squashChange(ctx context.Context, step *runwaymq.MergeStep, if _, err := m.run(ctx, nil, "reset", "--soft", preSHA); err != nil { return "", false, fmt.Errorf("git reset --soft %s: %w", preSHA, err) } - if _, err := m.run(ctx, nil, "commit", "-m", squashMessage(step, ref)); err != nil { + // The squash is a new commit, so it carries no authorship of its own. + // Credit it to the change's own author rather than to the merger. + author, err := m.commitAuthor(ctx, ref.SHA) + if err != nil { + return "", false, err + } + if _, err := m.runAs(ctx, author, nil, "commit", "-m", squashMessage(step, ref)); err != nil { return "", false, fmt.Errorf("git commit (squash): %w", err) } sha, err := m.headSHA(ctx) @@ -607,7 +619,14 @@ func (m *gitMerger) applyMerge(ctx context.Context, rs resolvedStep) ([]*runwaym if m.allowUnrelatedHistories { args = append(args, "--allow-unrelated-histories") } - out, err := m.runCombined(ctx, nil, append(args, ref.SHA)...) + // The merge commit is minted by the merger, so it would otherwise be + // authored by the service. Credit it to the change being merged; the + // change's own commits keep their authors through the second parent. + author, err := m.commitAuthor(ctx, ref.SHA) + if err != nil { + return nil, err + } + out, 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. @@ -938,7 +957,13 @@ func (m *gitMerger) push(ctx context.Context) error { // run executes a `git` command in the checkout. Returns captured stdout and an // error that includes captured stderr for diagnostics. func (m *gitMerger) run(ctx context.Context, stdin []byte, args ...string) ([]byte, error) { - cmd := m.command(ctx, args...) + return m.runAs(ctx, authorIdent{}, stdin, args...) +} + +// runAs is run with any commit the command creates credited to the given +// author. A zero author leaves the committer identity to author it too. +func (m *gitMerger) runAs(ctx context.Context, author authorIdent, stdin []byte, args ...string) ([]byte, error) { + cmd := m.commandAs(ctx, author, args...) if stdin != nil { cmd.Stdin = bytes.NewReader(stdin) } @@ -954,7 +979,13 @@ func (m *gitMerger) run(ctx context.Context, stdin []byte, args ...string) ([]by // runCombined is like run but returns combined stdout+stderr both on success // and failure. Used when the caller needs to inspect git's diagnostic output. func (m *gitMerger) runCombined(ctx context.Context, stdin []byte, args ...string) ([]byte, error) { - cmd := m.command(ctx, args...) + return m.runCombinedAs(ctx, authorIdent{}, stdin, args...) +} + +// runCombinedAs is runCombined with any commit the command creates credited to +// the given author. +func (m *gitMerger) runCombinedAs(ctx context.Context, author authorIdent, stdin []byte, args ...string) ([]byte, error) { + cmd := m.commandAs(ctx, author, args...) if stdin != nil { cmd.Stdin = bytes.NewReader(stdin) } @@ -964,6 +995,13 @@ func (m *gitMerger) runCombined(ctx context.Context, stdin []byte, args ...strin // command builds a git command with the committer identity injected via -c // flags, on top of the pinned runtime and scrubbed environment. func (m *gitMerger) command(ctx context.Context, args ...string) *exec.Cmd { + return m.commandAs(ctx, authorIdent{}, args...) +} + +// commandAs is command with the author supplied through the environment. The +// committer identity is injected either way, so the author overrides only who +// the commit is credited to, never who is recorded as having applied it. +func (m *gitMerger) commandAs(ctx context.Context, author authorIdent, args ...string) *exec.Cmd { withIdentity := make([]string, 0, len(args)+6) withIdentity = append(withIdentity, "-c", "user.name="+m.committerName, @@ -971,7 +1009,9 @@ func (m *gitMerger) command(ctx context.Context, args ...string) *exec.Cmd { "-c", "commit.gpgsign=false", ) withIdentity = append(withIdentity, args...) - return newGitCommand(ctx, m.runtime, m.checkoutPath, withIdentity...) + cmd := newGitCommand(ctx, m.runtime, m.checkoutPath, withIdentity...) + cmd.Env = append(cmd.Env, author.env()...) + return cmd } // newGitCommand constructs a Git command without inheriting the caller's diff --git a/runway/extension/merger/git/git_merger_test.go b/runway/extension/merger/git/git_merger_test.go index c887694e..d4a20c7c 100644 --- a/runway/extension/merger/git/git_merger_test.go +++ b/runway/extension/merger/git/git_merger_test.go @@ -995,6 +995,147 @@ func TestMerge_Rebase_StackedMultiCommitChanges(t *testing.T) { } } +// --- authorship --- + +func TestAuthorIdent(t *testing.T) { + tests := []struct { + name string + ident authorIdent + complete bool + wantEnv []string + }{ + { + name: "both halves present", + ident: authorIdent{Name: "Jane Dev", Email: "jane@example.com"}, + complete: true, + wantEnv: []string{"GIT_AUTHOR_NAME=Jane Dev", "GIT_AUTHOR_EMAIL=jane@example.com"}, + }, + { + // Passed through verbatim: the environment carries the two halves + // separately, so nothing has to survive being parsed back apart. + name: "name containing angle brackets", + ident: authorIdent{Name: "Jane Doe", Email: "jane@example.com"}, + complete: true, + wantEnv: []string{"GIT_AUTHOR_NAME=Jane Doe", "GIT_AUTHOR_EMAIL=jane@example.com"}, + }, + { + name: "missing email falls back", + ident: authorIdent{Name: "Jane Dev"}, + }, + { + name: "missing name falls back", + ident: authorIdent{Email: "jane@example.com"}, + }, + { + name: "empty falls back", + ident: authorIdent{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.complete, tt.ident.complete()) + assert.Equal(t, tt.wantEnv, tt.ident.env()) + }) + } +} + +func TestMerge_SquashRebase_CreditsChangeAuthor(t *testing.T) { + // A squash mints a fresh commit, so without attribution it would be + // authored by the merger. The person who wrote the change should be its + // author; the merger stays the committer, since it is what applied it. + f := setupGitFixture(t) + sha := f.pushPRCommit(t, "feature/a", "hello.txt", "hello\nearth\n", "tweak hello") + + m := f.newMerger(t, mergestrategypb.Strategy_REBASE) + res, err := m.Merge(context.Background(), req("b", + stepOf(mergestrategypb.Strategy_SQUASH_REBASE, "s1", uri(sha)), + )) + require.NoError(t, err) + require.Equal(t, runwaypb.Outcome_SUCCEEDED, res.GetOutcome()) + + author, committer := f.remoteIdent(t, f.remoteHEAD(t)) + assert.Equal(t, "author ", author) + assert.Equal(t, "Test ", committer) +} + +func TestMerge_Merge_CreditsChangeAuthorOnMergeCommit(t *testing.T) { + // `git merge` has no --author flag, so the merge commit is the case that + // only the environment can attribute. + f := setupGitFixture(t) + sha := f.pushPRCommit(t, "feature/a", "hello.txt", "hello\nearth\n", "tweak hello") + + m := f.newMerger(t, mergestrategypb.Strategy_REBASE) + res, err := m.Merge(context.Background(), req("b", + stepOf(mergestrategypb.Strategy_MERGE, "s1", uri(sha)), + )) + require.NoError(t, err) + require.Equal(t, runwaypb.Outcome_SUCCEEDED, res.GetOutcome()) + + mergeSHA := f.remoteHEAD(t) + require.Len(t, f.parents(t, mergeSHA), 2, "expected a merge commit") + author, committer := f.remoteIdent(t, mergeSHA) + assert.Equal(t, "author ", author) + assert.Equal(t, "Test ", committer) +} + +func TestMerge_SquashRebase_CreditsAuthorOfHeadCommit(t *testing.T) { + // The URI pins a head SHA, and that commit's author is the one the request + // names. A change whose commits have different authors is credited to the + // head's, not to whoever happened to start it. + f := setupGitFixture(t) + head := f.pushMultiCommitPRAs(t, "feature/multi", + authoredCommit{spec: commitSpec{path: "a.go", contents: "package a\n", message: "add a"}, + name: "First Author", email: "first@example.com"}, + authoredCommit{spec: commitSpec{path: "b.go", contents: "package b\n", message: "add b"}, + name: "Head Author", email: "head@example.com"}, + ) + + m := f.newMerger(t, mergestrategypb.Strategy_REBASE) + res, err := m.Merge(context.Background(), req("b", + stepOf(mergestrategypb.Strategy_SQUASH_REBASE, "s1", uri(head)), + )) + require.NoError(t, err) + require.Equal(t, runwaypb.Outcome_SUCCEEDED, res.GetOutcome()) + + author, committer := f.remoteIdent(t, f.remoteHEAD(t)) + assert.Equal(t, "Head Author ", author) + assert.Equal(t, "Test ", committer) + // Both commits' content still landed — attribution did not change what the + // squash contains. + assert.Equal(t, "package a\n", f.remoteFile(t, "a.go")) + assert.Equal(t, "package b\n", f.remoteFile(t, "b.go")) +} + +func TestMerge_Rebase_PreservesEachCommitsOwnAuthor(t *testing.T) { + // REBASE cherry-picks, which carries the author across on its own. Nothing + // collapses, so each commit keeps the person who wrote it rather than + // everything being credited to the head's author. + f := setupGitFixture(t) + head := f.pushMultiCommitPRAs(t, "feature/multi", + authoredCommit{spec: commitSpec{path: "a.go", contents: "package a\n", message: "add a"}, + name: "First Author", email: "first@example.com"}, + authoredCommit{spec: commitSpec{path: "b.go", contents: "package b\n", message: "add b"}, + name: "Head Author", email: "head@example.com"}, + ) + + m := f.newMerger(t, mergestrategypb.Strategy_REBASE) + res, err := m.Merge(context.Background(), req("b", + stepOf(mergestrategypb.Strategy_REBASE, "s1", uri(head)), + )) + require.NoError(t, err) + require.Equal(t, runwaypb.Outcome_SUCCEEDED, res.GetOutcome()) + + landed := f.remoteCommitsSinceSeed(t) + require.Len(t, landed, 2) + firstAuthor, firstCommitter := f.remoteIdent(t, landed[0]) + headAuthor, headCommitter := f.remoteIdent(t, landed[1]) + assert.Equal(t, "First Author ", firstAuthor) + assert.Equal(t, "Head Author ", headAuthor) + assert.Equal(t, "Test ", firstCommitter) + assert.Equal(t, "Test ", headCommitter) +} + // --- object availability and staleness --- func TestMerge_UnavailableCommitIsInvalidRequestNotConflict(t *testing.T) { @@ -1551,6 +1692,40 @@ func (f gitFixture) parents(t *testing.T, sha string) []string { return fields[1:] } +// remoteIdent returns the author and committer recorded on a commit, each +// formatted "Name ". The two differ whenever the merger creates a commit +// for someone else's change. +func (f gitFixture) remoteIdent(t *testing.T, sha string) (author, committer string) { + t.Helper() + out := mustGitOutput(t, f.remoteDir, "show", "--no-patch", "--format=%an <%ae>%n%cn <%ce>", sha) + lines := strings.Split(strings.TrimSpace(string(out)), "\n") + require.Len(t, lines, 2) + return lines[0], lines[1] +} + +// authoredCommit is a commitSpec plus the identity to author it with, for the +// tests that need a change whose commits are not all by the same person. +type authoredCommit struct { + spec commitSpec + name string + email string +} + +// pushMultiCommitPRAs is pushMultiCommitPR with an explicit author per commit. +func (f gitFixture) pushMultiCommitPRAs(t *testing.T, branch string, commits ...authoredCommit) string { + t.Helper() + mustGit(t, f.authorDir, "fetch", "origin") + mustGit(t, f.authorDir, "checkout", "-B", branch, "origin/main") + for _, c := range commits { + require.NoError(t, writeFile(filepath.Join(f.authorDir, c.spec.path), c.spec.contents)) + mustGit(t, f.authorDir, "add", ".") + mustGit(t, f.authorDir, "commit", "-m", c.spec.message, + "--author", fmt.Sprintf("%s <%s>", c.name, c.email)) + } + mustGit(t, f.authorDir, "push", "-f", "origin", branch) + return strings.TrimSpace(string(mustGitOutput(t, f.authorDir, "rev-parse", "HEAD"))) +} + func mustGit(t *testing.T, dir string, args ...string) { t.Helper() cmd := newGitCommand(context.Background(), testGitRuntime(t), dir, args...)