From 5c107f34961e99bb80d6c8b09a6725499844b773 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 2 Dec 2020 12:48:39 +0100 Subject: [PATCH] operations: Fix wrong ordering of merge parents for UserMergeBranch While our tests do verify that generated merge commits from UserMergeBranch have expected merge parents, the order isn't verified at all. Unfortunately, ordering is important and not checking it has allowed us to sneak in a bug in the Go implementation which ordered parents the wrong way. Fix the test to verify order and fix the Go implementation to order merge parents correctly. --- .../unreleased/pks-go-user-merge-branch-reversed-parents.yml | 5 +++++ internal/gitaly/service/operations/merge.go | 4 ++-- internal/gitaly/service/operations/merge_test.go | 3 +-- 3 files changed, 8 insertions(+), 4 deletions(-) create mode 100644 changelogs/unreleased/pks-go-user-merge-branch-reversed-parents.yml diff --git a/changelogs/unreleased/pks-go-user-merge-branch-reversed-parents.yml b/changelogs/unreleased/pks-go-user-merge-branch-reversed-parents.yml new file mode 100644 index 00000000000..6a59a8ee947 --- /dev/null +++ b/changelogs/unreleased/pks-go-user-merge-branch-reversed-parents.yml @@ -0,0 +1,5 @@ +--- +title: 'operations: Fix wrong ordering of merge parents for UserMergeBranch' +merge_request: 2868 +author: +type: fixed diff --git a/internal/gitaly/service/operations/merge.go b/internal/gitaly/service/operations/merge.go index 64c6ad27644..93fd7782947 100644 --- a/internal/gitaly/service/operations/merge.go +++ b/internal/gitaly/service/operations/merge.go @@ -123,8 +123,8 @@ func (s *Server) userMergeBranch(stream gitalypb.OperationService_UserMergeBranc AuthorName: string(firstRequest.User.Name), AuthorMail: string(firstRequest.User.Email), Message: string(firstRequest.Message), - Ours: firstRequest.CommitId, - Theirs: revision, + Ours: revision, + Theirs: firstRequest.CommitId, }.Run(ctx, s.cfg) if err != nil { if errors.Is(err, git2go.ErrInvalidArgument) { diff --git a/internal/gitaly/service/operations/merge_test.go b/internal/gitaly/service/operations/merge_test.go index e1e0a775920..c9b467fb115 100644 --- a/internal/gitaly/service/operations/merge_test.go +++ b/internal/gitaly/service/operations/merge_test.go @@ -104,8 +104,7 @@ func testSuccessfulMerge(t *testing.T, ctx context.Context) { require.Equal(t, gitalypb.OperationBranchUpdate{CommitId: commit.Id}, *(secondResponse.BranchUpdate)) - require.Contains(t, commit.ParentIds, mergeBranchHeadBefore, "merge parents must include previous HEAD of branch") - require.Contains(t, commit.ParentIds, commitToMerge, "merge parents must include commit to merge") + require.Equal(t, commit.ParentIds, []string{mergeBranchHeadBefore, commitToMerge}) require.True(t, strings.HasPrefix(string(commit.Body), mergeCommitMessage), "expected %q to start with %q", commit.Body, mergeCommitMessage) -- GitLab