From 81a855f5ab34904b25ce192317c6be54c8a60899 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20T=C3=B6lle?= Date: Mon, 9 Jun 2025 10:52:09 +0200 Subject: [PATCH] feat: avoid pushing release branch only for rebasing (#114) Right now releaser-pleaser pushes the branch even if it is only for a "rebase", this wastes CI resources. Instead, it should only push when there are changes to the files it owns. - **Old**: Push when there is a diff origin/release-pr..release-pr - **New**: Push when the these two diffs are not the same: origin/main..release-pr $(git merge-base origin/main origin/release-pr)..release-pr Closes #92 --- go.mod | 2 +- internal/git/git.go | 68 ++++++++++++-- internal/git/git_test.go | 126 +++++++++++++++++++++++++ internal/git/util_test.go | 189 ++++++++++++++++++++++++++++++++++++++ releaserpleaser.go | 2 +- 5 files changed, 376 insertions(+), 11 deletions(-) create mode 100644 internal/git/util_test.go diff --git a/go.mod b/go.mod index 1cb7b8b..e8038f4 100644 --- a/go.mod +++ b/go.mod @@ -6,6 +6,7 @@ toolchain go1.24.4 require ( github.com/blang/semver/v4 v4.0.0 + github.com/go-git/go-billy/v5 v5.6.2 github.com/go-git/go-git/v5 v5.16.2 github.com/google/go-github/v72 v72.0.0 github.com/leodido/go-conventionalcommits v0.12.0 @@ -25,7 +26,6 @@ require ( github.com/davecgh/go-spew v1.1.1 // indirect github.com/emirpasic/gods v1.18.1 // indirect github.com/go-git/gcfg v1.5.1-0.20230307220236-3a3c6141e376 // indirect - github.com/go-git/go-billy/v5 v5.6.2 // indirect github.com/golang/groupcache v0.0.0-20241129210726-2c02b8208cf8 // indirect github.com/google/go-querystring v1.1.0 // indirect github.com/hashicorp/go-cleanhttp v0.5.2 // indirect diff --git a/internal/git/git.go b/internal/git/git.go index 95f05e0..136a828 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -193,8 +193,27 @@ func (r *Repository) Commit(_ context.Context, message string, author Author) (C }, nil } -func (r *Repository) HasChangesWithRemote(ctx context.Context, branch string) (bool, error) { - remoteRef, err := r.r.Reference(plumbing.NewRemoteReferenceName(remoteName, branch), false) +// HasChangesWithRemote checks if the following two diffs are equal: +// +// - **Local**: remote/main..branch +// - **Remote**: (git merge-base remote/main remote/branch)..remote/branch +// +// This is done to avoid pushing when the only change would be a rebase of remote/branch onto the current remote/main. +func (r *Repository) HasChangesWithRemote(ctx context.Context, mainBranch, prBranch string) (bool, error) { + return r.hasChangesWithRemote(ctx, + plumbing.NewRemoteReferenceName(remoteName, mainBranch), + plumbing.NewBranchReferenceName(prBranch), + plumbing.NewRemoteReferenceName(remoteName, prBranch), + ) +} + +func (r *Repository) hasChangesWithRemote(ctx context.Context, mainBranchRef, localPRBranchRef, remotePRBranchRef plumbing.ReferenceName) (bool, error) { + commitOnRemoteMain, err := r.commitFromRef(mainBranchRef) + if err != nil { + return false, err + } + + commitOnRemotePRBranch, err := r.commitFromRef(remotePRBranchRef) if err != nil { if err.Error() == "reference not found" { // No remote branch means that there are changes @@ -204,29 +223,60 @@ func (r *Repository) HasChangesWithRemote(ctx context.Context, branch string) (b return false, err } - remoteCommit, err := r.r.CommitObject(remoteRef.Hash()) + currentRemotePRMergeBase, err := r.mergeBase(commitOnRemoteMain, commitOnRemotePRBranch) + if err != nil { + return false, err + } + if currentRemotePRMergeBase == nil { + // If there is no merge base something weird has happened with the + // remote main branch, and we should definitely push updates. + return false, nil + } + + remoteDiff, err := commitOnRemotePRBranch.PatchContext(ctx, currentRemotePRMergeBase) if err != nil { return false, err } - localRef, err := r.r.Reference(plumbing.NewBranchReferenceName(branch), false) + commitOnLocalPRBranch, err := r.commitFromRef(localPRBranchRef) if err != nil { return false, err } - localCommit, err := r.r.CommitObject(localRef.Hash()) + localDiff, err := commitOnRemoteMain.PatchContext(ctx, commitOnLocalPRBranch) if err != nil { return false, err } - diff, err := localCommit.PatchContext(ctx, remoteCommit) + return remoteDiff.String() == localDiff.String(), nil +} + +func (r *Repository) commitFromRef(refName plumbing.ReferenceName) (*object.Commit, error) { + ref, err := r.r.Reference(refName, false) if err != nil { - return false, err + return nil, err } - hasChanges := len(diff.FilePatches()) > 0 + commit, err := r.r.CommitObject(ref.Hash()) + if err != nil { + return nil, err + } - return hasChanges, nil + return commit, nil +} + +func (r *Repository) mergeBase(a, b *object.Commit) (*object.Commit, error) { + mergeBases, err := a.MergeBase(b) + if err != nil { + return nil, err + } + + if len(mergeBases) == 0 { + return nil, nil + } + + // :shrug: We dont really care which commit we pick, at worst we do an unnecessary push. + return mergeBases[0], nil } func (r *Repository) ForcePush(ctx context.Context, branch string) error { diff --git a/internal/git/git_test.go b/internal/git/git_test.go index 399c2ba..3d05538 100644 --- a/internal/git/git_test.go +++ b/internal/git/git_test.go @@ -1,12 +1,15 @@ package git import ( + "context" "reflect" "strconv" "testing" "time" + "github.com/go-git/go-git/v5/plumbing" "github.com/go-git/go-git/v5/plumbing/object" + "github.com/stretchr/testify/assert" ) func TestAuthor_signature(t *testing.T) { @@ -44,3 +47,126 @@ func TestAuthor_String(t *testing.T) { }) } } + +const testMainBranch = "main" +const testPRBranch = "releaser-pleaser" + +func TestRepository_HasChangesWithRemote(t *testing.T) { + // go-git/v5 has a bug where it tries to delete the repo root dir (".") multiple times if there is no file left in it. + // this happens while switching branches in worktree.go rmFileAndDirsIfEmpty. + // TODO: Fix bug upstream + // For now I just make sure that there is always at least one file left in the dir by adding an empty "README.md" in the test util. + + mainBranchRef := plumbing.NewBranchReferenceName(testMainBranch) + localPRBranchRef := plumbing.NewBranchReferenceName(testPRBranch) + remotePRBranchRef := plumbing.NewBranchReferenceName("remote/" + testPRBranch) + + tests := []struct { + name string + repo TestRepo + want bool + wantErr assert.ErrorAssertionFunc + }{ + { + name: "no remote pr branch", + repo: WithTestRepo( + WithCommit( + "chore: release v1.0.0", + WithFile("VERSION", "v1.0.0"), + ), + WithCommit( + "chore: release v1.1.0", + OnBranch(mainBranchRef), + AsNewBranch(localPRBranchRef), + WithFile("VERSION", "v1.1.0"), + ), + ), + want: true, + wantErr: assert.NoError, + }, + { + name: "remote pr branch matches local", + repo: WithTestRepo( + WithCommit( + "chore: release v1.0.0", + WithFile("VERSION", "v1.0.0"), + ), + WithCommit( + "chore: release v1.1.0", + OnBranch(mainBranchRef), + AsNewBranch(remotePRBranchRef), + WithFile("VERSION", "v1.1.0"), + ), + WithCommit( + "chore: release v1.1.0", + OnBranch(mainBranchRef), + AsNewBranch(localPRBranchRef), + WithFile("VERSION", "v1.1.0"), + ), + ), + want: false, + wantErr: assert.NoError, + }, + { + name: "remote pr only needs rebase", + repo: WithTestRepo( + WithCommit( + "chore: release v1.0.0", + WithFile("VERSION", "v1.0.0"), + ), + WithCommit( + "chore: release v1.1.0", + OnBranch(mainBranchRef), + AsNewBranch(remotePRBranchRef), + WithFile("VERSION", "v1.1.0"), + ), + WithCommit( + "feat: new feature on remote", + OnBranch(mainBranchRef), + WithFile("feature", "yes"), + ), + WithCommit( + "chore: release v1.1.0", + OnBranch(mainBranchRef), + AsNewBranch(localPRBranchRef), + WithFile("VERSION", "v1.1.0"), + ), + ), + want: false, + wantErr: assert.NoError, + }, + { + name: "needs update", + repo: WithTestRepo( + WithCommit( + "chore: release v1.0.0", + WithFile("VERSION", "v1.0.0"), + ), + WithCommit( + "chore: release v1.1.0", + OnBranch(mainBranchRef), + AsNewBranch(remotePRBranchRef), + WithFile("VERSION", "v1.1.0"), + ), + WithCommit( + "chore: release v1.2.0", + OnBranch(mainBranchRef), + AsNewBranch(localPRBranchRef), + WithFile("VERSION", "v1.2.0"), + ), + ), + want: false, + wantErr: assert.NoError, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + repo := tt.repo(t) + got, err := repo.hasChangesWithRemote(context.Background(), mainBranchRef, localPRBranchRef, remotePRBranchRef) + if !tt.wantErr(t, err) { + return + } + assert.Equal(t, tt.want, got) + }) + } +} diff --git a/internal/git/util_test.go b/internal/git/util_test.go new file mode 100644 index 0000000..eff947f --- /dev/null +++ b/internal/git/util_test.go @@ -0,0 +1,189 @@ +package git + +import ( + "fmt" + "io" + "log/slog" + "os" + "testing" + "time" + + "github.com/go-git/go-billy/v5/memfs" + "github.com/go-git/go-git/v5" + "github.com/go-git/go-git/v5/plumbing" + "github.com/go-git/go-git/v5/plumbing/object" + "github.com/go-git/go-git/v5/storage/memory" + "github.com/stretchr/testify/require" +) + +var ( + author = &object.Signature{ + Name: "releaser-pleaser", + When: time.Date(2020, 01, 01, 01, 01, 01, 01, time.UTC), + } +) + +type CommitOption func(*commitOptions) +type commitOptions struct { + cleanFiles bool + files []commitFile + tags []string + newRef plumbing.ReferenceName + parentRef plumbing.ReferenceName +} +type commitFile struct { + path string + content string +} + +type TestCommit func(*testing.T, *Repository) error +type TestRepo func(*testing.T) *Repository + +func WithCommit(message string, options ...CommitOption) TestCommit { + return func(t *testing.T, repo *Repository) error { + t.Helper() + + require.NotEmpty(t, message, "commit message is required") + + opts := &commitOptions{} + for _, opt := range options { + opt(opts) + } + + wt, err := repo.r.Worktree() + require.NoError(t, err) + + if opts.parentRef != "" { + checkoutOptions := &git.CheckoutOptions{} + + if opts.newRef != "" { + parentRef, err := repo.r.Reference(opts.parentRef, false) + require.NoError(t, err) + + checkoutOptions.Create = true + checkoutOptions.Hash = parentRef.Hash() + checkoutOptions.Branch = opts.newRef + } else { + checkoutOptions.Branch = opts.parentRef + } + + err = wt.Checkout(checkoutOptions) + require.NoError(t, err) + } + + // Yeet all files + if opts.cleanFiles { + files, err := wt.Filesystem.ReadDir(".") + require.NoError(t, err, "failed to get current files") + + for _, fileInfo := range files { + err = wt.Filesystem.Remove(fileInfo.Name()) + require.NoError(t, err, "failed to remove file %q", fileInfo.Name()) + } + } + + // Create new files + for _, fileInfo := range opts.files { + file, err := wt.Filesystem.Create(fileInfo.path) + require.NoError(t, err, "failed to create file %q", fileInfo.path) + + _, err = file.Write([]byte(fileInfo.content)) + _ = file.Close() + require.NoError(t, err, "failed to write content to file %q", fileInfo.path) + + _, err = wt.Add(fileInfo.path) + require.NoError(t, err, "failed to stage changes to file %q", fileInfo.path) + + } + + // Commit + commitHash, err := wt.Commit(message, &git.CommitOptions{ + All: true, + AllowEmptyCommits: true, + Author: author, + Committer: author, + }) + require.NoError(t, err, "failed to commit") + + // Create tags + for _, tagName := range opts.tags { + _, err = repo.r.CreateTag(tagName, commitHash, nil) + require.NoError(t, err, "failed to create tag %q", tagName) + } + + return nil + } +} + +func WithFile(path, content string) CommitOption { + return func(opts *commitOptions) { + opts.files = append(opts.files, commitFile{path: path, content: content}) + } +} + +// WithCleanFiles removes all previous files from the repo. Make sure to leave at least one file in the root +// directory when switching branches! +func WithCleanFiles() CommitOption { + return func(opts *commitOptions) { + opts.cleanFiles = true + } +} + +func AsNewBranch(ref plumbing.ReferenceName) CommitOption { + return func(opts *commitOptions) { + opts.newRef = ref + } +} + +func OnBranch(ref plumbing.ReferenceName) CommitOption { + return func(opts *commitOptions) { + opts.parentRef = ref + } +} + +func WithTag(name string) CommitOption { + return func(opts *commitOptions) { + opts.tags = append(opts.tags, name) + } +} + +// Can be useful to debug git issues by using it in a terminal +const useOnDiskTestRepository = false + +func WithTestRepo(commits ...TestCommit) TestRepo { + return func(t *testing.T) *Repository { + t.Helper() + + repo := &Repository{ + logger: slog.New(slog.NewTextHandler(io.Discard, nil)), + } + + var err error + + initOptions := git.InitOptions{DefaultBranch: plumbing.Main} + + if useOnDiskTestRepository { + dir, err := os.MkdirTemp(os.TempDir(), "rp-test-repo-") + require.NoError(t, err, "failed to create temp directory") + + repo.r, err = git.PlainInitWithOptions(dir, &git.PlainInitOptions{InitOptions: initOptions}) + require.NoError(t, err, "failed to create fs repository") + + fmt.Printf("using temp directory: %s", dir) + } else { + repo.r, err = git.InitWithOptions(memory.NewStorage(), memfs.New(), initOptions) + require.NoError(t, err, "failed to create in-memory repository") + } + + // Make initial commit + err = WithCommit("chore: init", WithFile("README.md", "# git test util"))(t, repo) + require.NoError(t, err, "failed to create init commit") + + for i, commit := range commits { + err = commit(t, repo) + require.NoError(t, err, "failed to create commit %d", i) + } + + return repo + } +} diff --git a/releaserpleaser.go b/releaserpleaser.go index 13e2381..3c316ae 100644 --- a/releaserpleaser.go +++ b/releaserpleaser.go @@ -269,7 +269,7 @@ func (rp *ReleaserPleaser) runReconcileReleasePR(ctx context.Context) error { logger.InfoContext(ctx, "created release commit", "commit.hash", releaseCommit.Hash, "commit.message", releaseCommit.Message, "commit.author", releaseCommitAuthor) // Check if anything changed in comparison to the remote branch (if exists) - newReleasePRChanges, err := repo.HasChangesWithRemote(ctx, rpBranch) + newReleasePRChanges, err := repo.HasChangesWithRemote(ctx, rp.targetBranch, rpBranch) if err != nil { return err }