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...)