diff options
| -rw-r--r-- | cmd/gitaly-git2go/conflicts/conflicts.go | 12 | ||||
| -rw-r--r-- | cmd/gitaly-git2go/merge.go | 80 | ||||
| -rw-r--r-- | internal/gitaly/service/operations/merge_test.go | 119 | ||||
| -rw-r--r-- | proto/go/gitalypb/operations.pb.go | 4 | ||||
| -rw-r--r-- | proto/operations.proto | 4 |
5 files changed, 179 insertions, 40 deletions
diff --git a/cmd/gitaly-git2go/conflicts/conflicts.go b/cmd/gitaly-git2go/conflicts/conflicts.go index 4a39931cd..96da74958 100644 --- a/cmd/gitaly-git2go/conflicts/conflicts.go +++ b/cmd/gitaly-git2go/conflicts/conflicts.go @@ -67,6 +67,18 @@ func Merge(repo *git.Repository, conflict git.IndexConflict) (*git.MergeFileResu return nil, fmt.Errorf("could not compute conflicts: %w", err) } + // In a case of tree-based conflicts (e.g. no ancestor), fallback to `Path` + // of `their` side. If that's also blank, fallback to `Path` of `our` side. + // This is to ensure that there's always a `Path` when we try to merge + // conflicts. + if merge.Path == "" { + if their.Path != "" { + merge.Path = their.Path + } else { + merge.Path = our.Path + } + } + return merge, nil } diff --git a/cmd/gitaly-git2go/merge.go b/cmd/gitaly-git2go/merge.go index aa4ba8701..23a26fe49 100644 --- a/cmd/gitaly-git2go/merge.go +++ b/cmd/gitaly-git2go/merge.go @@ -105,28 +105,54 @@ func resolveConflicts(repo *git.Repository, index *git.Index) error { } for _, conflict := range indexConflicts { - merge, err := conflicts.Merge(repo, conflict) - if err != nil { - return err - } + if isConflictMergeable(conflict) { + merge, err := conflicts.Merge(repo, conflict) + if err != nil { + return err + } - mergedBlob, err := repo.CreateBlobFromBuffer(merge.Contents) - if err != nil { - return err - } + mergedBlob, err := repo.CreateBlobFromBuffer(merge.Contents) + if err != nil { + return err + } - mergedIndexEntry := git.IndexEntry{ - Path: merge.Path, - Mode: git.Filemode(merge.Mode), - Id: mergedBlob, - } + mergedIndexEntry := git.IndexEntry{ + Path: merge.Path, + Mode: git.Filemode(merge.Mode), + Id: mergedBlob, + } - if err := index.Add(&mergedIndexEntry); err != nil { - return err - } + if err := index.Add(&mergedIndexEntry); err != nil { + return err + } - if err := index.RemoveConflict(merge.Path); err != nil { - return err + if err := index.RemoveConflict(merge.Path); err != nil { + return err + } + } else { + if conflict.Their != nil { + // If a conflict has `Their` present, we add it back to the index + // as we want those changes to be part of the merge. + if err := index.Add(conflict.Their); err != nil { + return err + } + + if err := index.RemoveConflict(conflict.Their.Path); err != nil { + return err + } + } else if conflict.Our != nil { + // If a conflict has `Our` present, remove its conflict as we + // don't want to include those changes. + if err := index.RemoveConflict(conflict.Our.Path); err != nil { + return err + } + } else { + // If conflict has no `Their` and `Our`, remove the conflict to + // mark it as resolved. + if err := index.RemoveConflict(conflict.Ancestor.Path); err != nil { + return err + } + } } } @@ -137,6 +163,24 @@ func resolveConflicts(repo *git.Repository, index *git.Index) error { return nil } +func isConflictMergeable(conflict git.IndexConflict) bool { + conflictIndexEntriesCount := 0 + + if conflict.Their != nil { + conflictIndexEntriesCount++ + } + + if conflict.Our != nil { + conflictIndexEntriesCount++ + } + + if conflict.Ancestor != nil { + conflictIndexEntriesCount++ + } + + return conflictIndexEntriesCount >= 2 +} + func getConflicts(index *git.Index) ([]git.IndexConflict, error) { var conflicts []git.IndexConflict diff --git a/internal/gitaly/service/operations/merge_test.go b/internal/gitaly/service/operations/merge_test.go index fb8562562..8ac2b81e9 100644 --- a/internal/gitaly/service/operations/merge_test.go +++ b/internal/gitaly/service/operations/merge_test.go @@ -1,11 +1,11 @@ package operations import ( - "bytes" "fmt" "io/ioutil" "os" "os/exec" + "regexp" "strings" "testing" "time" @@ -15,6 +15,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v14/internal/git" "gitlab.com/gitlab-org/gitaly/v14/internal/git/gittest" "gitlab.com/gitlab-org/gitaly/v14/internal/git/localrepo" + "gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/v14/internal/helper/text" "gitlab.com/gitlab-org/gitaly/v14/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb" @@ -698,36 +699,101 @@ func TestConflictsOnUserMergeToRefRequest(t *testing.T) { ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx) - gittest.Exec(t, cfg, "-C", repoPath, "branch", mergeBranchName, "824be604a34828eb682305f0d963056cfac87b2d") + t.Run("allow conflicts to be merged with markers when modified on both sides", func(t *testing.T) { + request := buildUserMergeToRefRequest(t, cfg, repo, repoPath, "1450cd639e0bc6721eb02800169e464f212cde06", "824be604a34828eb682305f0d963056cfac87b2d", "modified-both-sides-conflict") + request.AllowConflicts = true - request := &gitalypb.UserMergeToRefRequest{ - Repository: repo, - User: gittest.TestUser, - TargetRef: []byte("refs/merge-requests/x/written"), - SourceSha: "1450cd639e0bc6721eb02800169e464f212cde06", - Message: []byte("message1"), - FirstParentRef: []byte("refs/heads/" + mergeBranchName), - } + resp, err := client.UserMergeToRef(ctx, request) + require.NoError(t, err) + + output := text.ChompBytes(gittest.Exec(t, cfg, "-C", repoPath, "show", resp.CommitId)) + + markersRegexp := regexp.MustCompile(`(?s)\+<<<<<<< files\/ruby\/popen.rb.*?\+>>>>>>> files\/ruby\/popen.rb.*?\+<<<<<<< files\/ruby\/regex.rb.*?\+>>>>>>> files\/ruby\/regex.rb`) + require.Regexp(t, markersRegexp, output) + }) + + t.Run("allow conflicts to be merged with markers when modified on source and removed on target", func(t *testing.T) { + request := buildUserMergeToRefRequest(t, cfg, repo, repoPath, "eb227b3e214624708c474bdab7bde7afc17cefcc", "92417abf83b75e67b8ace920bc8e83e1986da4ac", "modified-source-removed-target-conflict") + request.AllowConflicts = true - t.Run("allow conflicts to be merged with markers", func(t *testing.T) { + resp, err := client.UserMergeToRef(ctx, request) + require.NoError(t, err) + + output := text.ChompBytes(gittest.Exec(t, cfg, "-C", repoPath, "show", resp.CommitId)) + + markersRegexp := regexp.MustCompile(`(?s)\+<<<<<<< \n.*?\+=======\n.*?\+>>>>>>> files/ruby/version_info.rb`) + require.Regexp(t, markersRegexp, output) + }) + + t.Run("allow conflicts to be merged with markers when removed on source and modified on target", func(t *testing.T) { + request := buildUserMergeToRefRequest(t, cfg, repo, repoPath, "92417abf83b75e67b8ace920bc8e83e1986da4ac", "eb227b3e214624708c474bdab7bde7afc17cefcc", "removed-source-modified-target-conflict") + request.AllowConflicts = true + + resp, err := client.UserMergeToRef(ctx, request) + require.NoError(t, err) + + output := text.ChompBytes(gittest.Exec(t, cfg, "-C", repoPath, "show", resp.CommitId)) + + markersRegexp := regexp.MustCompile(`(?s)\+<<<<<<< files/ruby/version_info.rb.*?\+=======\n.*?\+>>>>>>> \z`) + require.Regexp(t, markersRegexp, output) + }) + + t.Run("allow conflicts to be merged with markers when both source and target added the same file", func(t *testing.T) { + request := buildUserMergeToRefRequest(t, cfg, repo, repoPath, "f0f390655872bb2772c85a0128b2fbc2d88670cb", "5b4bb08538b9249995b94aa69121365ba9d28082", "source-target-added-same-file-conflict") request.AllowConflicts = true resp, err := client.UserMergeToRef(ctx, request) require.NoError(t, err) - var buf bytes.Buffer - cmd := exec.Command(cfg.Git.BinPath, "-C", repoPath, "show", resp.CommitId) - cmd.Stdout = &buf - require.NoError(t, cmd.Run()) + output := text.ChompBytes(gittest.Exec(t, cfg, "-C", repoPath, "show", resp.CommitId)) - bufStr := buf.String() - require.Contains(t, bufStr, "+<<<<<<< files/ruby/popen.rb") - require.Contains(t, bufStr, "+>>>>>>> files/ruby/popen.rb") - require.Contains(t, bufStr, "+<<<<<<< files/ruby/regex.rb") - require.Contains(t, bufStr, "+>>>>>>> files/ruby/regex.rb") + markersRegexp := regexp.MustCompile(`(?s)\+<<<<<<< NEW_FILE.md.*?\+=======\n.*?\+>>>>>>> NEW_FILE.md`) + require.Regexp(t, markersRegexp, output) + }) + + // Test cases below do not show any conflict markers because we don't try + // to merge the conflicts for these cases. We keep `Their` side of the + // conflict and ignore `Our` and `Ancestor` instead. This is because we want + // to show the `Their` side when we present the merge commit on the merge + // request diff. + t.Run("allow conflicts to be merged without markers when renamed on source and removed on target", func(t *testing.T) { + request := buildUserMergeToRefRequest(t, cfg, repo, repoPath, "aafecf84d791ec43dfa16e55eb0a0fbd9c72d3fb", "3ac7abfb7621914e596d5bf369be8234b9086052", "renamed-source-removed-target") + request.AllowConflicts = true + + resp, err := client.UserMergeToRef(ctx, request) + require.NoError(t, err) + + output := text.ChompBytes(gittest.Exec(t, cfg, "-C", repoPath, "show", resp.CommitId)) + + require.NotContains(t, output, "=======") + }) + + t.Run("allow conflicts to be merged without markers when removed on source and renamed on target", func(t *testing.T) { + request := buildUserMergeToRefRequest(t, cfg, repo, repoPath, "3ac7abfb7621914e596d5bf369be8234b9086052", "aafecf84d791ec43dfa16e55eb0a0fbd9c72d3fb", "removed-source-renamed-target") + request.AllowConflicts = true + + resp, err := client.UserMergeToRef(ctx, request) + require.NoError(t, err) + + output := text.ChompBytes(gittest.Exec(t, cfg, "-C", repoPath, "show", resp.CommitId)) + + require.NotContains(t, output, "=======") + }) + + t.Run("allow conflicts to be merged without markers when both source and target renamed the same file", func(t *testing.T) { + request := buildUserMergeToRefRequest(t, cfg, repo, repoPath, "aafecf84d791ec43dfa16e55eb0a0fbd9c72d3fb", "fe6d6ff5812e7fb292168851dc0edfc6a0171909", "source-target-renamed-same-file") + request.AllowConflicts = true + + resp, err := client.UserMergeToRef(ctx, request) + require.NoError(t, err) + + output := text.ChompBytes(gittest.Exec(t, cfg, "-C", repoPath, "show", resp.CommitId)) + + require.NotContains(t, output, "=======") }) t.Run("disallow conflicts to be merged", func(t *testing.T) { + request := buildUserMergeToRefRequest(t, cfg, repo, repoPath, "1450cd639e0bc6721eb02800169e464f212cde06", "824be604a34828eb682305f0d963056cfac87b2d", "disallowed-conflicts") request.AllowConflicts = false _, err := client.UserMergeToRef(ctx, request) @@ -735,6 +801,19 @@ func TestConflictsOnUserMergeToRefRequest(t *testing.T) { }) } +func buildUserMergeToRefRequest(t testing.TB, cfg config.Cfg, repo *gitalypb.Repository, repoPath string, sourceSha string, targetSha string, mergeBranchName string) *gitalypb.UserMergeToRefRequest { + gittest.Exec(t, cfg, "-C", repoPath, "branch", mergeBranchName, targetSha) + + return &gitalypb.UserMergeToRefRequest{ + Repository: repo, + User: gittest.TestUser, + TargetRef: []byte("refs/merge-requests/x/written"), + SourceSha: sourceSha, + Message: []byte("message1"), + FirstParentRef: []byte("refs/heads/" + mergeBranchName), + } +} + func TestUserMergeToRef_stableMergeID(t *testing.T) { ctx, cleanup := testhelper.Context() defer cleanup() diff --git a/proto/go/gitalypb/operations.pb.go b/proto/go/gitalypb/operations.pb.go index f40f084f2..5ef16f66e 100644 --- a/proto/go/gitalypb/operations.pb.go +++ b/proto/go/gitalypb/operations.pb.go @@ -872,7 +872,9 @@ type UserMergeToRefRequest struct { // first parent of the computed merge. Overrides `branch`. FirstParentRef []byte `protobuf:"bytes,7,opt,name=first_parent_ref,json=firstParentRef,proto3" json:"first_parent_ref,omitempty"` // Allow conflicts to occur. Any conflict markers will be part of the merge commit. - // Only text conflicts are handled, tree-based conflicts are not supported. + // When tree-based conflicts occur, no conflict markers will be added to the + // file on the merge commit. The `Their` side of the conflict will be kept and + // `Our` and `Ancestor` will be ignored. AllowConflicts bool `protobuf:"varint,8,opt,name=allow_conflicts,json=allowConflicts,proto3" json:"allow_conflicts,omitempty"` // timestamp is the optional timestamp to use for the merge commit. If it's // not set, the current time will be used. diff --git a/proto/operations.proto b/proto/operations.proto index d1cf73ec3..1ead16681 100644 --- a/proto/operations.proto +++ b/proto/operations.proto @@ -270,7 +270,9 @@ message UserMergeToRefRequest { // first parent of the computed merge. Overrides `branch`. bytes first_parent_ref = 7; // Allow conflicts to occur. Any conflict markers will be part of the merge commit. - // Only text conflicts are handled, tree-based conflicts are not supported. + // When tree-based conflicts occur, no conflict markers will be added to the + // file on the merge commit. The `Their` side of the conflict will be kept and + // `Our` and `Ancestor` will be ignored. bool allow_conflicts = 8; // timestamp is the optional timestamp to use for the merge commit. If it's // not set, the current time will be used. |