diff options
author | Will Chandler <wchandler@gitlab.com> | 2023-05-26 00:21:13 +0300 |
---|---|---|
committer | Will Chandler <wchandler@gitlab.com> | 2023-05-26 00:21:13 +0300 |
commit | 2df9d7f637a21e0b90c875726fcbc2f21e0d43d3 (patch) | |
tree | 0ea7c21eeaec317a5af9804b2f3199e6fa2a1044 | |
parent | 75a157036114fea3c51eed343fa4cf482ccdb0ba (diff) | |
parent | 89303d204e92808295b5527640493a627c733943 (diff) |
Merge branch 'revert-git-merge-tree' into 'master'
conflicts: Introduce and use `conflictFilesWithGitMergeTree`
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5843
Merged-by: Will Chandler <wchandler@gitlab.com>
Approved-by: John Cai <jcai@gitlab.com>
Approved-by: Will Chandler <wchandler@gitlab.com>
Reviewed-by: karthik nayak <knayak@gitlab.com>
Co-authored-by: Karthik Nayak <knayak@gitlab.com>
-rw-r--r-- | internal/featureflag/ff_list_conflict_files_merge_tree.go | 10 | ||||
-rw-r--r-- | internal/git/localrepo/merge.go | 7 | ||||
-rw-r--r-- | internal/git/localrepo/merge_test.go | 20 | ||||
-rw-r--r-- | internal/gitaly/service/conflicts/list_conflict_files.go | 162 | ||||
-rw-r--r-- | internal/gitaly/service/conflicts/list_conflict_files_test.go | 97 |
5 files changed, 275 insertions, 21 deletions
diff --git a/internal/featureflag/ff_list_conflict_files_merge_tree.go b/internal/featureflag/ff_list_conflict_files_merge_tree.go new file mode 100644 index 000000000..c30713c70 --- /dev/null +++ b/internal/featureflag/ff_list_conflict_files_merge_tree.go @@ -0,0 +1,10 @@ +package featureflag + +// ListConflictFilesMergeTree enables the usage of git-merge-tree(1) for +// the ListConflictFiles RPC. +var ListConflictFilesMergeTree = NewFeatureFlag( + "list_conflict_files_merge_tree", + "v16.0.0", + "https://gitlab.com/gitlab-org/gitaly/-/issues/5098", + false, +) diff --git a/internal/git/localrepo/merge.go b/internal/git/localrepo/merge.go index 874a94d9b..4b09f1b01 100644 --- a/internal/git/localrepo/merge.go +++ b/internal/git/localrepo/merge.go @@ -163,6 +163,11 @@ func parseMergeTreeError(objectHash git.ObjectHash, cfg mergeTreeConfig, output return "", fmt.Errorf("parsing conflicting file info: %s", infoLine) } + mode, err := strconv.ParseInt(info[0], 8, 32) + if err != nil { + return "", fmt.Errorf("parsing mode: %w", err) + } + mergeTreeConflictError.ConflictingFileInfo[i].OID, err = objectHash.FromHex(info[1]) if err != nil { return "", fmt.Errorf("hex to oid: %w", err) @@ -177,6 +182,7 @@ func parseMergeTreeError(objectHash git.ObjectHash, cfg mergeTreeConfig, output return "", fmt.Errorf("invalid value for stage: %d", stage) } + mergeTreeConflictError.ConflictingFileInfo[i].Mode = int32(mode) mergeTreeConflictError.ConflictingFileInfo[i].Stage = MergeStage(stage) mergeTreeConflictError.ConflictingFileInfo[i].FileName = infoAndFilename[1] } @@ -213,6 +219,7 @@ func parseMergeTreeError(objectHash git.ObjectHash, cfg mergeTreeConfig, output // ConflictingFileInfo holds the conflicting file info output from git-merge-tree(1). type ConflictingFileInfo struct { FileName string + Mode int32 OID git.ObjectID Stage MergeStage } diff --git a/internal/git/localrepo/merge_test.go b/internal/git/localrepo/merge_test.go index 6e5dc5b70..3106e16d4 100644 --- a/internal/git/localrepo/merge_test.go +++ b/internal/git/localrepo/merge_test.go @@ -175,11 +175,13 @@ func TestMergeTree(t *testing.T) { FileName: "file2", OID: blob1, Stage: MergeStageOurs, + Mode: 0o100644, }, { FileName: "file2", OID: blob2, Stage: MergeStageTheirs, + Mode: 0o100644, }, }, ConflictInfoMessage: []ConflictInfoMessage{ @@ -251,16 +253,19 @@ func TestMergeTree(t *testing.T) { FileName: "file1", OID: blob1, Stage: MergeStageAncestor, + Mode: 0o100644, }, { FileName: "file1", OID: blob2, Stage: MergeStageOurs, + Mode: 0o100644, }, { FileName: "file1", OID: blob3, Stage: MergeStageTheirs, + Mode: 0o100644, }, }, ConflictInfoMessage: []ConflictInfoMessage{ @@ -321,11 +326,13 @@ func TestMergeTree(t *testing.T) { FileName: "file2", OID: blob2, Stage: MergeStageAncestor, + Mode: 0o100644, }, { FileName: "file2", OID: blob2, Stage: MergeStageOurs, + Mode: 0o100644, }, }, ConflictInfoMessage: []ConflictInfoMessage{ @@ -393,21 +400,25 @@ func TestMergeTree(t *testing.T) { FileName: "file2", OID: blob2a, Stage: MergeStageOurs, + Mode: 0o100644, }, { FileName: "file2", OID: blob3a, Stage: MergeStageTheirs, + Mode: 0o100644, }, { FileName: "file3", OID: blob2b, Stage: MergeStageOurs, + Mode: 0o100644, }, { FileName: "file3", OID: blob3b, Stage: MergeStageTheirs, + Mode: 0o100644, }, }, ConflictInfoMessage: []ConflictInfoMessage{ @@ -491,26 +502,31 @@ func TestMergeTree(t *testing.T) { FileName: "file1", OID: blob, Stage: MergeStageAncestor, + Mode: 0o100644, }, { FileName: "file1", OID: blob2a, Stage: MergeStageOurs, + Mode: 0o100644, }, { FileName: "file1", OID: blob3a, Stage: MergeStageTheirs, + Mode: 0o100644, }, { FileName: "file2", OID: blob2b, Stage: MergeStageOurs, + Mode: 0o100644, }, { FileName: "file2", OID: blob3b, Stage: MergeStageTheirs, + Mode: 0o100644, }, }, ConflictInfoMessage: []ConflictInfoMessage{ @@ -588,21 +604,25 @@ func TestMergeTree(t *testing.T) { FileName: "file1", OID: blob, Stage: MergeStageAncestor, + Mode: 0o100644, }, { FileName: "file1", OID: blob3a, Stage: MergeStageTheirs, + Mode: 0o100644, }, { FileName: "file2", OID: blob2b, Stage: MergeStageOurs, + Mode: 0o100644, }, { FileName: "file2", OID: blob3b, Stage: MergeStageTheirs, + Mode: 0o100644, }, }, ConflictInfoMessage: []ConflictInfoMessage{ diff --git a/internal/gitaly/service/conflicts/list_conflict_files.go b/internal/gitaly/service/conflicts/list_conflict_files.go index 823198c43..c629381b8 100644 --- a/internal/gitaly/service/conflicts/list_conflict_files.go +++ b/internal/gitaly/service/conflicts/list_conflict_files.go @@ -8,6 +8,7 @@ import ( "io" "unicode/utf8" + "gitlab.com/gitlab-org/gitaly/v16/internal/featureflag" "gitlab.com/gitlab-org/gitaly/v16/internal/git" "gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo" "gitlab.com/gitlab-org/gitaly/v16/internal/git2go" @@ -41,9 +42,170 @@ func (s *server) ListConflictFiles(request *gitalypb.ListConflictFilesRequest, s return err } + if featureflag.ListConflictFilesMergeTree.IsEnabled(ctx) { + return s.conflictFilesWithGitMergeTree(ctx, request, stream, ours, theirs, repo) + } + return s.conflictFilesWithGit2Go(ctx, request, stream, ours, theirs, repo, repoPath) } +func (s *server) conflictFilesWithGitMergeTree( + ctx context.Context, + request *gitalypb.ListConflictFilesRequest, + stream gitalypb.ConflictsService_ListConflictFilesServer, + ours, theirs git.ObjectID, + repo *localrepo.Repo, +) error { + var mergeConflictErr *localrepo.MergeTreeConflictError + + oid, err := repo.MergeTree(ctx, ours.String(), theirs.String(), localrepo.WithAllowUnrelatedHistories()) + if err == nil { + // When there are no errors, it denotes that there are no conflicts. + return nil + } else if !errors.As(err, &mergeConflictErr) { + // If its not a conflict, we return the error to the user. + return structerr.NewInternal("couldn't find conflict: %w", err) + } + + type conflictHeader struct { + theirPath string + ourPath string + ancestorPath string + ourMode int32 + } + + objectReader, cancel, err := s.catfileCache.ObjectReader(ctx, repo) + if err != nil { + return err + } + defer cancel() + + // We need to combine data with same path, but we also want to retain + // the ordering. We use a map to track the data with the same path, but + // retain ordering by using a slice. + pathToConflict := make(map[string]*conflictHeader) + var conflicts []*conflictHeader + + for _, conflictFile := range mergeConflictErr.ConflictingFileInfo { + val, ok := pathToConflict[conflictFile.FileName] + if !ok { + val = &conflictHeader{} + conflicts = append(conflicts, val) + } + + switch conflictFile.Stage { + case localrepo.MergeStageAncestor: + val.ancestorPath = conflictFile.FileName + case localrepo.MergeStageOurs: + val.ourPath = conflictFile.FileName + val.ourMode = conflictFile.Mode + case localrepo.MergeStageTheirs: + val.theirPath = conflictFile.FileName + } + + pathToConflict[conflictFile.FileName] = val + } + + var conflictFiles []*gitalypb.ConflictFile + msgSize := 0 + + // Git2Go conflict markers have filenames and git-merge-tree(1) has commit OIDs, + // to keep the content the same, we replace commit OID with filenames. + replaceOids := func(conflict *conflictHeader, chunk []byte) []byte { + chunk = bytes.ReplaceAll(chunk, []byte(request.OurCommitOid), []byte(conflict.ourPath)) + chunk = bytes.ReplaceAll(chunk, []byte(request.TheirCommitOid), []byte(conflict.theirPath)) + + return chunk + } + + for _, conflict := range conflicts { + if !request.AllowTreeConflicts && (conflict.theirPath == "" || conflict.ourPath == "") { + return structerr.NewFailedPrecondition("conflict side missing") + } + + conflictFiles = append(conflictFiles, &gitalypb.ConflictFile{ + ConflictFilePayload: &gitalypb.ConflictFile_Header{ + Header: &gitalypb.ConflictFileHeader{ + CommitOid: request.OurCommitOid, + TheirPath: []byte(conflict.theirPath), + OurPath: []byte(conflict.ourPath), + AncestorPath: []byte(conflict.ancestorPath), + OurMode: conflict.ourMode, + }, + }, + }) + + path := conflict.ourPath + if path == "" { + path = conflict.theirPath + } + + fileOID, err := repo.ResolveRevision(ctx, oid.Revision()+":"+git.Revision(path)) + if err != nil { + return fmt.Errorf("getting file revision: %w", err) + } + + object, err := objectReader.Object(ctx, fileOID.Revision()) + if err != nil { + return fmt.Errorf("getting objectreader: %w", err) + } + + var content bytes.Buffer + _, err = content.ReadFrom(object) + if err != nil && err != io.EOF { + return structerr.NewInternal("reading conflict object: %w", err) + } + + if !utf8.Valid(content.Bytes()) { + return structerr.NewFailedPrecondition("unsupported encoding") + } + + parsedContent := replaceOids(conflict, content.Bytes()) + contentLen := len(parsedContent) + + for i := 0; i < contentLen; i += streamio.WriteBufferSize { + end := i + streamio.WriteBufferSize + if contentLen < end { + end = contentLen + } + + conflictFiles = append(conflictFiles, &gitalypb.ConflictFile{ + ConflictFilePayload: &gitalypb.ConflictFile_Content{ + Content: parsedContent[i:end], + }, + }) + + // We don't send a message for each chunk because the content of + // a file may be smaller than the size limit, which means we can + // keep adding data to the message + msgSize += end - i + if msgSize < streamio.WriteBufferSize { + continue + } + + if err := stream.Send(&gitalypb.ListConflictFilesResponse{ + Files: conflictFiles, + }); err != nil { + return structerr.NewInternal("error streaming conflict files: %w", err) + } + + conflictFiles = conflictFiles[:0] + msgSize = 0 + } + } + + // Send leftover data, if any + if len(conflictFiles) > 0 { + if err := stream.Send(&gitalypb.ListConflictFilesResponse{ + Files: conflictFiles, + }); err != nil { + return structerr.NewInternal("error streaming conflict files: %w", err) + } + } + + return nil +} + func (s *server) conflictFilesWithGit2Go( ctx context.Context, request *gitalypb.ListConflictFilesRequest, diff --git a/internal/gitaly/service/conflicts/list_conflict_files_test.go b/internal/gitaly/service/conflicts/list_conflict_files_test.go index 07e92d0ab..a61b8b8e7 100644 --- a/internal/gitaly/service/conflicts/list_conflict_files_test.go +++ b/internal/gitaly/service/conflicts/list_conflict_files_test.go @@ -9,6 +9,7 @@ import ( "strings" "testing" + "gitlab.com/gitlab-org/gitaly/v16/internal/featureflag" "gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest" "gitlab.com/gitlab-org/gitaly/v16/internal/structerr" "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper" @@ -23,7 +24,11 @@ type conflictFile struct { func TestListConflictFiles(t *testing.T) { t.Parallel() - ctx := testhelper.Context(t) + testhelper.NewFeatureSets(featureflag.ListConflictFilesMergeTree).Run(t, testListConflictFiles) +} + +func testListConflictFiles(t *testing.T, ctx context.Context) { + t.Parallel() type setupData struct { request *gitalypb.ListConflictFilesRequest @@ -84,6 +89,45 @@ func TestListConflictFiles(t *testing.T) { }, }, { + "conflict in submodules commits are not handled", + func(tb testing.TB, ctx context.Context) setupData { + cfg, client := setupConflictsServiceWithoutRepo(tb, nil) + repo, repoPath := gittest.CreateRepository(tb, ctx, cfg) + _, subRepoPath := gittest.CreateRepository(tb, ctx, cfg) + + subCommitID := gittest.WriteCommit(t, cfg, subRepoPath) + ourCommitID := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("master"), gittest.WithTreeEntries( + gittest.TreeEntry{ + Mode: "100644", + Path: ".gitmodules", + Content: fmt.Sprintf(`[submodule %q]\n\tpath = %s\n\turl = file://%s`, "sub", "sub", subRepoPath), + }, + gittest.TreeEntry{OID: subCommitID, Mode: "160000", Path: "sub"}, + )) + + newSubCommitID := gittest.WriteCommit(t, cfg, subRepoPath) + theirCommitID := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("master"), gittest.WithTreeEntries( + gittest.TreeEntry{ + Mode: "100644", + Path: ".gitmodules", + Content: fmt.Sprintf(`[submodule %q]\n\tpath = %s\n\turl = file://%s`, "sub", "sub", subRepoPath), + }, + gittest.TreeEntry{OID: newSubCommitID, Mode: "160000", Path: "sub"}, + )) + + request := &gitalypb.ListConflictFilesRequest{ + Repository: repo, + OurCommitOid: ourCommitID.String(), + TheirCommitOid: theirCommitID.String(), + } + + return setupData{ + client: client, + request: request, + } + }, + }, + { "Lists the expected conflict files with ancestor path", func(tb testing.TB, ctx context.Context) setupData { cfg, client := setupConflictsServiceWithoutRepo(tb, nil) @@ -303,29 +347,38 @@ func TestListConflictFiles(t *testing.T) { AllowTreeConflicts: true, } - return setupData{ - client: client, - request: request, - expectedFiles: []*conflictFile{ - { - Header: &gitalypb.ConflictFileHeader{ - CommitOid: ourCommitID.String(), - OurPath: []byte("a"), - OurMode: int32(0o100644), - AncestorPath: []byte("a"), - }, - Content: []byte("<<<<<<< a\nmango\n=======\n>>>>>>> \n"), + expectedFiles := []*conflictFile{ + { + Header: &gitalypb.ConflictFileHeader{ + CommitOid: ourCommitID.String(), + OurPath: []byte("a"), + OurMode: int32(0o100644), + AncestorPath: []byte("a"), }, - { - Header: &gitalypb.ConflictFileHeader{ - CommitOid: ourCommitID.String(), - TheirPath: []byte("b"), - AncestorPath: []byte("b"), - }, - Content: []byte("<<<<<<< \n=======\npeach\n>>>>>>> b\n"), + Content: []byte("<<<<<<< a\nmango\n=======\n>>>>>>> \n"), + }, + { + Header: &gitalypb.ConflictFileHeader{ + CommitOid: ourCommitID.String(), + TheirPath: []byte("b"), + AncestorPath: []byte("b"), }, + Content: []byte("<<<<<<< \n=======\npeach\n>>>>>>> b\n"), }, } + + // When using git-merge-tree(1), deleted files don't contain conflict markers. + // Whereas if you see above, Git2Go contains conflict markers. + if featureflag.ListConflictFilesMergeTree.IsEnabled(ctx) { + expectedFiles[0].Content = []byte("mango") + expectedFiles[1].Content = []byte("peach") + } + + return setupData{ + client: client, + request: request, + expectedFiles: expectedFiles, + } }, }, { @@ -481,7 +534,9 @@ func getConflictFiles(t *testing.T, c gitalypb.ConflictsService_ListConflictFile } // Append leftover file - files = append(files, currentFile) + if currentFile != nil { + files = append(files, currentFile) + } return files, nil } |