From f94c8f4ec703da5113e1718ebef96d9a07b3d240 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20T=C3=B6lle?= Date: Wed, 20 Nov 2024 22:03:57 +0100 Subject: [PATCH] feat: avoid pushing release branch only for rebasing --- internal/git/git.go | 60 +++++++++++++++--- internal/git/git_test.go | 131 +++++++++++++++++++++++++++++++++++++++ releaserpleaser.go | 2 +- 3 files changed, 183 insertions(+), 10 deletions(-) create mode 100644 internal/git/git_test.go diff --git a/internal/git/git.go b/internal/git/git.go index 128f94c..414d1f4 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -170,8 +170,19 @@ func (r *Repository) Commit(_ context.Context, message string) (Commit, error) { }, 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) { + commitOnRemoteMain, err := r.commitFromRef(plumbing.NewRemoteReferenceName(remoteName, mainBranch)) + if err != nil { + return false, err + } + + commitOnRemotePRBranch, err := r.commitFromRef(plumbing.NewRemoteReferenceName(remoteName, prBranch)) if err != nil { if err.Error() == "reference not found" { // No remote branch means that there are changes @@ -181,29 +192,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 := currentRemotePRMergeBase.PatchContext(ctx, commitOnRemotePRBranch) if err != nil { return false, err } - localRef, err := r.r.Reference(plumbing.NewBranchReferenceName(branch), false) + commitOnLocalPRBranch, err := r.commitFromRef(plumbing.NewBranchReferenceName(prBranch)) 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 new file mode 100644 index 0000000..4ac3e8b --- /dev/null +++ b/internal/git/git_test.go @@ -0,0 +1,131 @@ +package git + +import ( + "context" + "testing" + + "github.com/go-git/go-git/v5/plumbing" + "github.com/stretchr/testify/assert" +) + +const testMainBranch = "main" +const testPRBranch = "releaser-pleaser" + +func TestRepository_HasChangesWithRemote(t *testing.T) { + tests := []struct { + name string + repo TestRepo + want bool + wantErr assert.ErrorAssertionFunc + }{ + { + name: "no remote pr branch", + repo: WithTestRepo( + WithCommit( + "chore: release v1.0.0", + OnBranch(plumbing.NewBranchReferenceName(testMainBranch)), + AsNewBranch(plumbing.NewRemoteReferenceName(remoteName, testMainBranch)), + WithFile("VERSION", "v1.0.0"), + ), + WithCommit( + "chore: release v1.1.0", + OnBranch(plumbing.NewRemoteReferenceName(remoteName, testMainBranch)), + AsNewBranch(plumbing.NewBranchReferenceName(testPRBranch)), + WithFile("VERSION", "v1.1.0"), + ), + ), + want: true, + wantErr: assert.NoError, + }, + { + name: "remote pr branch matches local", + repo: WithTestRepo( + WithCommit( + "chore: release v1.0.0", + OnBranch(plumbing.NewBranchReferenceName(testMainBranch)), + AsNewBranch(plumbing.NewRemoteReferenceName(remoteName, testMainBranch)), + WithFile("VERSION", "v1.0.0"), + ), + WithCommit( + "chore: release v1.1.0", + OnBranch(plumbing.NewRemoteReferenceName(remoteName, testMainBranch)), + AsNewBranch(plumbing.NewBranchReferenceName(testPRBranch)), + WithFile("VERSION", "v1.1.0"), + ), + WithCommit( + "chore: release v1.1.0", + OnBranch(plumbing.NewRemoteReferenceName(remoteName, testMainBranch)), + AsNewBranch(plumbing.NewRemoteReferenceName(remoteName, testPRBranch)), + WithFile("VERSION", "v1.1.0"), + ), + ), + want: false, + wantErr: assert.NoError, + }, + { + name: "remote pr only needs rebase", + repo: WithTestRepo( + WithCommit( + "chore: release v1.0.0", + OnBranch(plumbing.NewBranchReferenceName(testMainBranch)), + AsNewBranch(plumbing.NewRemoteReferenceName(remoteName, testMainBranch)), + WithFile("VERSION", "v1.0.0"), + ), + WithCommit( + "chore: release v1.1.0", + OnBranch(plumbing.NewRemoteReferenceName(remoteName, testMainBranch)), + AsNewBranch(plumbing.NewRemoteReferenceName(remoteName, testPRBranch)), + WithFile("VERSION", "v1.1.0"), + ), + WithCommit( + "feat: new feature on remote", + OnBranch(plumbing.NewRemoteReferenceName(remoteName, testMainBranch)), + WithFile("feature", "yes"), + ), + WithCommit( + "chore: release v1.1.0", + OnBranch(plumbing.NewRemoteReferenceName(remoteName, testMainBranch)), + AsNewBranch(plumbing.NewBranchReferenceName(testPRBranch)), + WithFile("VERSION", "v1.1.0"), + ), + ), + want: false, + wantErr: assert.NoError, + }, + { + name: "needs update", + repo: WithTestRepo( + WithCommit( + "chore: release v1.0.0", + OnBranch(plumbing.NewBranchReferenceName(testMainBranch)), + AsNewBranch(plumbing.NewRemoteReferenceName(remoteName, testMainBranch)), + WithFile("VERSION", "v1.0.0"), + ), + WithCommit( + "chore: release v1.1.0", + OnBranch(plumbing.NewRemoteReferenceName(remoteName, testMainBranch)), + AsNewBranch(plumbing.NewRemoteReferenceName(remoteName, testPRBranch)), + WithFile("VERSION", "v1.1.0"), + ), + WithCommit( + "chore: release v1.2.0", + OnBranch(plumbing.NewRemoteReferenceName(remoteName, testMainBranch)), + AsNewBranch(plumbing.NewBranchReferenceName(testPRBranch)), + 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(), testMainBranch, testPRBranch) + if !tt.wantErr(t, err) { + return + } + assert.Equal(t, tt.want, got) + }) + } +} diff --git a/releaserpleaser.go b/releaserpleaser.go index 88b2dbb..f3e13cb 100644 --- a/releaserpleaser.go +++ b/releaserpleaser.go @@ -274,7 +274,7 @@ func (rp *ReleaserPleaser) runReconcileReleasePR(ctx context.Context) error { logger.InfoContext(ctx, "created release commit", "commit.hash", releaseCommit.Hash, "commit.message", releaseCommit.Message) // 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 }