Merge branch '4580-reimplement-resolveconflicts-without-git2go' into 'master'

conflicts: Implement `ResolveConflicts` with Git

Closes #4580

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5954

Merged-by: Toon Claes <toon@gitlab.com>
Approved-by: John Cai <jcai@gitlab.com>
Reviewed-by: Toon Claes <toon@gitlab.com>
Reviewed-by: John Cai <jcai@gitlab.com>
Reviewed-by: karthik nayak <knayak@gitlab.com>
Co-authored-by: Karthik Nayak <knayak@gitlab.com>

5 files changed, 407 insertions, 30 deletions

diff --git a/internal/featureflag/ff_resolve_conflicts_via_git.go b/internal/featureflag/ff_resolve_conflicts_via_git.go
new file mode 100644
index 000000000..b26525e48
--- /dev/null
+++ b/internal/featureflag/ff_resolve_conflicts_via_git.go
@@ -0,0 +1,10 @@
+package featureflag
+
+// ResolveConflictsViaGit enables the usage of git-merge-tree(1) for
+// the ResolveConflicts RPC.
+var ResolveConflictsViaGit = NewFeatureFlag(
+	"resolve_conflicts_via_git",
+	"v16.2.0",
+	"https://gitlab.com/gitlab-org/gitaly/-/issues/5395",
+	false,
+)
diff --git a/internal/git/conflict/resolve.go b/internal/git/conflict/resolve.go
index 85e0f7096..644ed8c35 100644
--- a/internal/git/conflict/resolve.go
+++ b/internal/git/conflict/resolve.go
@@ -82,8 +82,9 @@ type Resolution struct {
 
 // Resolve is used to resolve conflicts for a given blob. It expects the blob
 // to be provided as an io.Reader along with the resolutions for the provided
-// blob.
-func Resolve(src io.Reader, ours, theirs git.ObjectID, path string, resolution Resolution) (io.Reader, error) {
+// blob. Clients can also use appendNewLine to have an additional new line appended
+// to the end of the resolved buffer.
+func Resolve(src io.Reader, ours, theirs git.ObjectID, path string, resolution Resolution, appendNewLine bool) (io.Reader, error) {
 	var (
 		// conflict markers, git-merge-tree(1) appends the tree OIDs to the markers
 		start  = "<<<<<<< " + ours.String()
@@ -236,5 +237,9 @@ func Resolve(src io.Reader, ours, theirs git.ObjectID, path string, resolution R
 		return &resolvedContent, fmt.Errorf("writing bytes: %w", err)
 	}
 
+	if appendNewLine {
+		resolvedContent.Write([]byte("\n"))
+	}
+
 	return &resolvedContent, nil
 }
diff --git a/internal/git/conflict/resolve_test.go b/internal/git/conflict/resolve_test.go
index d189d901a..4bc5cc81a 100644
--- a/internal/git/conflict/resolve_test.go
+++ b/internal/git/conflict/resolve_test.go
@@ -15,11 +15,12 @@ func TestResolve(t *testing.T) {
 	t.Parallel()
 
 	type args struct {
-		src        io.Reader
-		ours       git.ObjectID
-		theirs     git.ObjectID
-		path       string
-		resolution Resolution
+		src           io.Reader
+		ours          git.ObjectID
+		theirs        git.ObjectID
+		path          string
+		resolution    Resolution
+		appendNewLine bool
 	}
 	tests := []struct {
 		name            string
@@ -54,6 +55,34 @@ we want this line
 we can both agree on this line though`),
 		},
 		{
+			name: "select ours with newline",
+			args: args{
+				src: strings.NewReader(fmt.Sprintf(`# this file is very conflicted
+<<<<<<< %s
+we want this line
+=======
+but they want this line
+>>>>>>> %s
+we can both agree on this line though
+`, gittest.DefaultObjectHash.EmptyTreeOID, gittest.DefaultObjectHash.EmptyTreeOID)),
+				ours:   gittest.DefaultObjectHash.EmptyTreeOID,
+				theirs: gittest.DefaultObjectHash.EmptyTreeOID,
+				path:   "conflict.txt",
+				resolution: Resolution{
+					NewPath: "conflict.txt",
+					OldPath: "conflict.txt",
+					Sections: map[string]string{
+						"dc1c302824bab8da29f7c06fec1c77cf16b975e6_2_2": "head",
+					},
+				},
+				appendNewLine: true,
+			},
+			resolvedContent: []byte(`# this file is very conflicted
+we want this line
+we can both agree on this line though
+`),
+		},
+		{
 			name: "select theirs",
 			args: args{
 				src: strings.NewReader(fmt.Sprintf(`# this file is very conflicted
@@ -167,7 +196,7 @@ we can both agree on this line though
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
 
-			resolvedReader, err := Resolve(tt.args.src, tt.args.ours, tt.args.theirs, tt.args.path, tt.args.resolution)
+			resolvedReader, err := Resolve(tt.args.src, tt.args.ours, tt.args.theirs, tt.args.path, tt.args.resolution, tt.args.appendNewLine)
 			if err != nil || tt.expectedErr != nil {
 				require.Equal(t, tt.expectedErr, err)
 				return
diff --git a/internal/gitaly/service/conflicts/resolve_conflicts.go b/internal/gitaly/service/conflicts/resolve_conflicts.go
index 334fdd52a..d2b0ba85b 100644
--- a/internal/gitaly/service/conflicts/resolve_conflicts.go
+++ b/internal/gitaly/service/conflicts/resolve_conflicts.go
@@ -7,11 +7,13 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"path/filepath"
 	"sort"
 	"strings"
 	"time"
 
 	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
+	"gitlab.com/gitlab-org/gitaly/v16/internal/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/conflict"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo"
@@ -169,23 +171,40 @@ func (s *server) resolveConflicts(header *gitalypb.ResolveConflictsRequestHeader
 		return errors.New("Rugged::InvalidError: unable to parse OID - contains invalid characters")
 	}
 
-	result, err := s.git2goExecutor.Resolve(ctx, quarantineRepo, git2go.ResolveCommand{
-		MergeCommand: git2go.MergeCommand{
-			Repository: repoPath,
-			AuthorName: string(header.User.Name),
-			AuthorMail: string(header.User.Email),
-			AuthorDate: authorDate,
-			Message:    string(header.CommitMessage),
-			Ours:       header.GetOurCommitOid(),
-			Theirs:     header.GetTheirCommitOid(),
-		},
-		Resolutions: resolutions,
-	})
-	if err != nil {
-		if errors.Is(err, git2go.ErrInvalidArgument) {
-			return structerr.NewInvalidArgument("%w", err)
+	var result git2go.ResolveResult
+	if !featureflag.ResolveConflictsViaGit.IsEnabled(ctx) {
+		result, err = s.git2goExecutor.Resolve(ctx, quarantineRepo, git2go.ResolveCommand{
+			MergeCommand: git2go.MergeCommand{
+				Repository: repoPath,
+				AuthorName: string(header.User.Name),
+				AuthorMail: string(header.User.Email),
+				AuthorDate: authorDate,
+				Message:    string(header.CommitMessage),
+				Ours:       header.GetOurCommitOid(),
+				Theirs:     header.GetTheirCommitOid(),
+			},
+			Resolutions: resolutions,
+		})
+		if err != nil {
+			if errors.Is(err, git2go.ErrInvalidArgument) {
+				return structerr.NewInvalidArgument("%w", err)
+			}
+			return err
+		}
+	} else {
+		result, err = s.resolveConflictsWithGit(
+			ctx,
+			header.GetOurCommitOid(),
+			header.GetTheirCommitOid(),
+			quarantineRepo,
+			resolutions,
+			authorDate,
+			header.User,
+			header.GetCommitMessage(),
+		)
+		if err != nil {
+			return err
 		}
-		return err
 	}
 
 	commitOID, err := git.ObjectHashSHA1.FromHex(result.CommitID)
@@ -208,6 +227,170 @@ func (s *server) resolveConflicts(header *gitalypb.ResolveConflictsRequestHeader
 	return nil
 }
 
+func (s *server) resolveConflictsWithGit(
+	ctx context.Context,
+	ours, theirs string,
+	repo *localrepo.Repo,
+	resolutions []conflict.Resolution,
+	authorDate time.Time,
+	user *gitalypb.User,
+	commitMessage []byte,
+) (git2go.ResolveResult, error) {
+	var result git2go.ResolveResult
+
+	treeOID, err := repo.MergeTree(ctx, ours, theirs, localrepo.WithAllowUnrelatedHistories())
+
+	var mergeConflictErr *localrepo.MergeTreeConflictError
+	if errors.As(err, &mergeConflictErr) {
+		conflictedFiles := mergeConflictErr.ConflictedFiles()
+		checkedConflictedFiles := make(map[string]bool)
+		for _, conflictedFile := range conflictedFiles {
+			checkedConflictedFiles[conflictedFile] = false
+		}
+
+		tree, err := repo.ReadTree(ctx, treeOID.Revision(), localrepo.WithRecursive())
+		if err != nil {
+			return result, structerr.NewInternal("getting tree: %w", err)
+		}
+
+		objectReader, cancel, err := s.catfileCache.ObjectReader(ctx, repo)
+		if err != nil {
+			return result, structerr.NewInternal("getting objectreader: %w", err)
+		}
+		defer cancel()
+
+		for _, resolution := range resolutions {
+			path := resolution.OldPath
+
+			if _, ok := checkedConflictedFiles[path]; !ok {
+				// Note: this emulates the Ruby error that occurs when
+				// there are no conflicts for a resolution
+				return result, errors.New("NoMethodError: undefined method `resolve_lines' for nil:NilClass")
+			}
+
+			// We mark the file as checked, any remaining files, which don't have a resolution
+			// associated, will throw an error.
+			checkedConflictedFiles[path] = true
+
+			conflictedBlob, err := tree.Get(path)
+			if err != nil {
+				return result, structerr.NewInternal("path not found in merged-tree: %w", err)
+			}
+
+			if conflictedBlob.Type != localrepo.Blob {
+				return result, structerr.NewInternal("entry should be of type blob").
+					WithMetadataItems(
+						structerr.MetadataItem{Key: "path", Value: path},
+						structerr.MetadataItem{Key: "type", Value: conflictedBlob.Type},
+					)
+			}
+
+			// We first read the object completely to see if the content is similar
+			// to the content in the resolution.
+			if resolution.Content != "" {
+				object, err := objectReader.Object(ctx, conflictedBlob.OID.Revision())
+				if err != nil {
+					return result, structerr.NewInternal("retrieving object: %w", err)
+				}
+
+				content, err := io.ReadAll(object)
+				if err != nil {
+					return result, structerr.NewInternal("reading object: %w", err)
+				}
+
+				// Git2Go conflict markers have filenames and git-merge-tree(1) has commit OIDs.
+				// Rails uses the older form, so to check if the content is the same, we need to
+				// adhere to this.
+				//
+				// Should be fixed with: https://gitlab.com/gitlab-org/git/-/issues/168
+				content = bytes.ReplaceAll(content, []byte(ours), []byte(resolution.OldPath))
+				content = bytes.ReplaceAll(content, []byte(theirs), []byte(resolution.NewPath))
+
+				if bytes.Equal([]byte(resolution.Content), content) {
+					// This is to keep the error consistent with git2go implementation
+					return result, structerr.NewInvalidArgument("Resolved content has no changes for file %s", path)
+				}
+			}
+
+			object, err := objectReader.Object(ctx, git.Revision(fmt.Sprintf("%s:%s", ours, resolution.OldPath)))
+			if err != nil {
+				return result, structerr.NewInternal("retrieving object: %w", err)
+			}
+
+			// Rails expects files ending with newlines to retain them post conflict, but
+			// git swallows ending newlines. So we manually append them if necessary.
+			needsNewLine := false
+
+			oursContent, err := io.ReadAll(object)
+			if err != nil {
+				return result, structerr.NewInternal("reading object: %w", err)
+			}
+			if len(oursContent) > 0 {
+				needsNewLine = oursContent[len(oursContent)-1] == '\n'
+			}
+
+			object, err = objectReader.Object(ctx, conflictedBlob.OID.Revision())
+			if err != nil {
+				return result, structerr.NewInternal("retrieving object: %w", err)
+			}
+
+			resolvedContent, err := conflict.Resolve(object, git.ObjectID(ours), git.ObjectID(theirs), path, resolution, needsNewLine)
+			if err != nil {
+				return result, structerr.NewInternal("%w", err)
+			}
+
+			blobOID, err := repo.WriteBlob(ctx, filepath.Base(path), resolvedContent)
+			if err != nil {
+				return result, structerr.NewInternal("writing blob: %w", err)
+			}
+
+			err = tree.Add(path, localrepo.TreeEntry{
+				OID:  blobOID,
+				Mode: conflictedBlob.Mode,
+				Path: filepath.Base(path),
+				Type: localrepo.Blob,
+			}, localrepo.WithOverwriteFile())
+			if err != nil {
+				return result, structerr.NewInternal("add to tree: %w", err)
+			}
+		}
+
+		for conflictedFile, checked := range checkedConflictedFiles {
+			if !checked {
+				return result, fmt.Errorf("Missing resolutions for the following files: %s", conflictedFile) //nolint // this is to stay consistent with rugged-rails error
+			}
+		}
+
+		err = tree.Write(ctx, repo)
+		if err != nil {
+			return result, structerr.NewInternal("write tree: %w", err)
+		}
+
+		treeOID = tree.OID
+	} else if err != nil {
+		return result, structerr.NewInternal("merge-tree: %w", err)
+	}
+
+	commitOID, err := repo.WriteCommit(ctx, localrepo.WriteCommitConfig{
+		Parents:        []git.ObjectID{git.ObjectID(ours), git.ObjectID(theirs)},
+		CommitterDate:  authorDate,
+		CommitterEmail: string(user.GetEmail()),
+		CommitterName:  string(user.GetName()),
+		AuthorDate:     authorDate,
+		AuthorEmail:    string(user.GetEmail()),
+		AuthorName:     string(user.GetName()),
+		Message:        string(commitMessage),
+		TreeID:         treeOID,
+	})
+	if err != nil {
+		return result, structerr.NewInternal("writing commit: %w", err)
+	}
+
+	result.CommitID = commitOID.String()
+
+	return result, nil
+}
+
 func sameRepo(left, right storage.Repository) bool {
 	lgaod := left.GetGitAlternateObjectDirectories()
 	rgaod := right.GetGitAlternateObjectDirectories()
diff --git a/internal/gitaly/service/conflicts/resolve_conflicts_test.go b/internal/gitaly/service/conflicts/resolve_conflicts_test.go
index 96962f317..8e5ab6ceb 100644
--- a/internal/gitaly/service/conflicts/resolve_conflicts_test.go
+++ b/internal/gitaly/service/conflicts/resolve_conflicts_test.go
@@ -11,6 +11,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/v16/internal/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo"
@@ -35,6 +36,15 @@ var (
 )
 
 func TestResolveConflicts(t *testing.T) {
+	t.Parallel()
+
+	testhelper.NewFeatureSets(
+		featureflag.ResolveConflictsViaGit,
+		featureflag.GPGSigning,
+	).Run(t, testResolveConflicts)
+}
+
+func testResolveConflicts(t *testing.T, ctx context.Context) {
 	type setupData struct {
 		cfg               config.Cfg
 		requestHeader     *gitalypb.ResolveConflictsRequest_Header
@@ -217,6 +227,75 @@ func TestResolveConflicts(t *testing.T) {
 			},
 		},
 		{
+			"single file in directory conflict, pick ours",
+			func(tb testing.TB, ctx context.Context) setupData {
+				cfg, client := setupConflictsService(tb, nil)
+				repo, repoPath := gittest.CreateRepository(tb, ctx, cfg)
+
+				baseCommitID := gittest.WriteCommit(tb, cfg, repoPath, gittest.WithTreeEntries(
+					gittest.TreeEntry{Path: "subdir", Mode: "040000", OID: gittest.WriteTree(t, cfg, repoPath, []gittest.TreeEntry{
+						{Path: "a", Mode: "100644", Content: "apple"},
+					})}),
+				)
+
+				ourCommitID := gittest.WriteCommit(tb, cfg, repoPath, gittest.WithParents(baseCommitID), gittest.WithBranch("ours"),
+					gittest.WithTreeEntries(
+						gittest.TreeEntry{Path: "subdir", Mode: "040000", OID: gittest.WriteTree(t, cfg, repoPath, []gittest.TreeEntry{
+							{Path: "a", Mode: "100644", Content: "apricot"},
+						})}),
+				)
+				theirCommitID := gittest.WriteCommit(tb, cfg, repoPath, gittest.WithParents(baseCommitID), gittest.WithBranch("theirs"),
+					gittest.WithTreeEntries(
+						gittest.TreeEntry{Path: "subdir", Mode: "040000", OID: gittest.WriteTree(t, cfg, repoPath, []gittest.TreeEntry{
+							{Path: "a", Mode: "100644", Content: "acai"},
+						})}),
+				)
+
+				files := []map[string]interface{}{
+					{
+						"old_path": "subdir/a",
+						"new_path": "subdir/a",
+						"sections": map[string]string{
+							fmt.Sprintf("%x_%d_%d", sha1.Sum([]byte("subdir/a")), 1, 1): "head",
+						},
+					},
+				}
+
+				filesJSON, err := json.Marshal(files)
+				require.NoError(t, err)
+
+				return setupData{
+					cfg:      cfg,
+					client:   client,
+					repoPath: repoPath,
+					repo:     repo,
+					requestHeader: &gitalypb.ResolveConflictsRequest_Header{
+						Header: &gitalypb.ResolveConflictsRequestHeader{
+							Repository:       repo,
+							TargetRepository: repo,
+							OurCommitOid:     ourCommitID.String(),
+							TheirCommitOid:   theirCommitID.String(),
+							TargetBranch:     []byte("theirs"),
+							SourceBranch:     []byte("ours"),
+							CommitMessage:    []byte(conflictResolutionCommitMessage),
+							User:             user,
+							Timestamp:        &timestamppb.Timestamp{},
+						},
+					},
+					requestsFilesJSON: []*gitalypb.ResolveConflictsRequest_FilesJson{
+						{FilesJson: filesJSON[:50]},
+						{FilesJson: filesJSON[50:]},
+					},
+					expectedResponse: &gitalypb.ResolveConflictsResponse{},
+					expectedContent: map[string]map[string][]byte{
+						"refs/heads/ours": {
+							"subdir/a": []byte("apricot"),
+						},
+					},
+				}
+			},
+		},
+		{
 			"single file conflict without ancestor, pick ours",
 			func(tb testing.TB, ctx context.Context) setupData {
 				cfg, client := setupConflictsService(tb, nil)
@@ -413,6 +492,70 @@ func TestResolveConflicts(t *testing.T) {
 			},
 		},
 		{
+			"multi file conflict, but missing file resolution",
+			func(tb testing.TB, ctx context.Context) setupData {
+				cfg, client := setupConflictsService(tb, nil)
+				repo, repoPath := gittest.CreateRepository(tb, ctx, cfg)
+
+				baseCommitID := gittest.WriteCommit(tb, cfg, repoPath, gittest.WithTreeEntries(
+					gittest.TreeEntry{Path: "a", Mode: "100644", Content: "apple\n" + strings.Repeat("filler\n", 10) + "banana"},
+					gittest.TreeEntry{Path: "b", Mode: "100644", Content: "strawberry"},
+				))
+
+				ourCommitID := gittest.WriteCommit(tb, cfg, repoPath, gittest.WithParents(baseCommitID), gittest.WithBranch("ours"),
+					gittest.WithTreeEntries(gittest.TreeEntry{
+						Path: "a", Mode: "100644",
+						Content: "apricot\n" + strings.Repeat("filler\n", 10) + "berries",
+					}, gittest.TreeEntry{Path: "b", Mode: "100644", Content: "blueberry"}))
+				theirCommitID := gittest.WriteCommit(tb, cfg, repoPath, gittest.WithParents(baseCommitID), gittest.WithBranch("theirs"),
+					gittest.WithTreeEntries(gittest.TreeEntry{
+						Path: "a", Mode: "100644",
+						Content: "acai\n" + strings.Repeat("filler\n", 10) + "birne",
+					}, gittest.TreeEntry{Path: "b", Mode: "100644", Content: "raspberry"}))
+
+				files := []map[string]interface{}{
+					{
+						"old_path": "b",
+						"new_path": "b",
+						"sections": map[string]string{
+							fmt.Sprintf("%x_%d_%d", sha1.Sum([]byte("b")), 1, 1): "head",
+						},
+					},
+				}
+
+				filesJSON, err := json.Marshal(files)
+				require.NoError(t, err)
+
+				return setupData{
+					cfg:      cfg,
+					client:   client,
+					repoPath: repoPath,
+					repo:     repo,
+					requestHeader: &gitalypb.ResolveConflictsRequest_Header{
+						Header: &gitalypb.ResolveConflictsRequestHeader{
+							Repository:       repo,
+							TargetRepository: repo,
+							OurCommitOid:     ourCommitID.String(),
+							TheirCommitOid:   theirCommitID.String(),
+							TargetBranch:     []byte("theirs"),
+							SourceBranch:     []byte("ours"),
+							CommitMessage:    []byte(conflictResolutionCommitMessage),
+							User:             user,
+							Timestamp:        &timestamppb.Timestamp{},
+						},
+					},
+					requestsFilesJSON: []*gitalypb.ResolveConflictsRequest_FilesJson{
+						{FilesJson: filesJSON[:50]},
+						{FilesJson: filesJSON[50:]},
+					},
+					expectedResponse: &gitalypb.ResolveConflictsResponse{
+						ResolutionError: "Missing resolutions for the following files: a",
+					},
+					skipCommitCheck: true,
+				}
+			},
+		},
+		{
 			"single file conflict, remote repo",
 			func(tb testing.TB, ctx context.Context) setupData {
 				cfg, client := setupConflictsService(tb, nil)
@@ -594,6 +737,17 @@ func TestResolveConflicts(t *testing.T) {
 				filesJSON, err := json.Marshal(files)
 				require.NoError(t, err)
 
+				expectedContent := map[string]map[string][]byte{
+					"refs/heads/ours": {
+						"a": []byte("A\r\nB\r\nX\r\nD\r\nE\r\n"),
+					},
+				}
+
+				// git replaces crlf with newlines when storing to the database
+				if featureflag.ResolveConflictsViaGit.IsEnabled(ctx) {
+					expectedContent["refs/heads/ours"]["a"] = []byte("A\nB\nX\nD\nE\n")
+				}
+
 				return setupData{
 					cfg:      cfg,
 					client:   client,
@@ -616,11 +770,7 @@ func TestResolveConflicts(t *testing.T) {
 						{FilesJson: filesJSON},
 					},
 					expectedResponse: &gitalypb.ResolveConflictsResponse{},
-					expectedContent: map[string]map[string][]byte{
-						"refs/heads/ours": {
-							"a": []byte("A\r\nB\r\nX\r\nD\r\nE\r\n"),
-						},
-					},
+					expectedContent:  expectedContent,
 				}
 			},
 		},
@@ -1173,11 +1323,11 @@ func TestResolveConflicts(t *testing.T) {
 		},
 	} {
 		tc := tc
+		ctx := ctx
 
 		t.Run(tc.desc, func(t *testing.T) {
 			t.Parallel()
 
-			ctx := testhelper.Context(t)
 			setup := tc.setup(t, ctx)
 
 			mdGS := testcfg.GitalyServersMetadataFromCfg(t, setup.cfg)