diff options
author | Will Chandler <wchandler@gitlab.com> | 2023-05-31 22:21:14 +0300 |
---|---|---|
committer | Will Chandler <wchandler@gitlab.com> | 2023-05-31 22:21:14 +0300 |
commit | 288739b62dddec3e48ddc9cbab13410c8fc9d556 (patch) | |
tree | 36df3ae5ab614ebbf16b3a0d9dc936601fb13956 | |
parent | c482efa57405ee65b3e9c4bfd74a89607c53303f (diff) | |
parent | c339202583c20aede731d5c684003fdf2bc6ed92 (diff) |
Merge branch 'fix-merge-tree' into 'master'
conflicts: Introduce and use `conflictFilesWithGitMergeTree`
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5853
Merged-by: Will Chandler <wchandler@gitlab.com>
Approved-by: Will Chandler <wchandler@gitlab.com>
Reviewed-by: John Cai <jcai@gitlab.com>
Reviewed-by: karthik nayak <knayak@gitlab.com>
Co-authored-by: Karthik Nayak <knayak@gitlab.com>
-rw-r--r-- | cmd/gitaly-git2go/conflicts.go | 21 | ||||
-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 | 111 |
6 files changed, 301 insertions, 30 deletions
diff --git a/cmd/gitaly-git2go/conflicts.go b/cmd/gitaly-git2go/conflicts.go index adb77843c..ed52086f9 100644 --- a/cmd/gitaly-git2go/conflicts.go +++ b/cmd/gitaly-git2go/conflicts.go @@ -103,23 +103,26 @@ func (*conflictsSubcommand) conflicts(request git2go.ConflictsCommand) git2go.Co func Merge(repo *git.Repository, conflict git.IndexConflict) (*git.MergeFileResult, error) { var ancestor, our, their git.MergeFileInput - for entry, input := range map[*git.IndexEntry]*git.MergeFileInput{ - conflict.Ancestor: &ancestor, - conflict.Our: &our, - conflict.Their: &their, + for _, val := range []struct { + entry *git.IndexEntry + input *git.MergeFileInput + }{ + {conflict.Ancestor, &ancestor}, + {conflict.Our, &our}, + {conflict.Their, &their}, } { - if entry == nil { + if val.entry == nil { continue } - blob, err := repo.LookupBlob(entry.Id) + blob, err := repo.LookupBlob(val.entry.Id) if err != nil { return nil, structerr.NewFailedPrecondition("could not get conflicting blob: %w", err) } - input.Path = entry.Path - input.Mode = uint(entry.Mode) - input.Contents = blob.Contents() + val.input.Path = val.entry.Path + val.input.Mode = uint(val.entry.Mode) + val.input.Contents = blob.Contents() } merge, err := git.MergeFile(ancestor, our, their, nil) 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..92caf4263 --- /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.1.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..c10ee8c0f 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 structerr.NewFailedPrecondition("getting file revision: %w", err) + } + + object, err := objectReader.Object(ctx, fileOID.Revision()) + if err != nil { + return structerr.NewFailedPrecondition("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..b74d78a29 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,59 @@ 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, gittest.WithTreeEntries(gittest.TreeEntry{ + Mode: "100644", + Path: "abc", + Content: "foo", + })) + 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, gittest.WithTreeEntries(gittest.TreeEntry{ + Mode: "100644", + Path: "abc", + Content: "bar", + })) + 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(), + } + + expectedErr := structerr.NewFailedPrecondition("could not get conflicting blob: object not found - no match for id (%s)", subCommitID) + if featureflag.ListConflictFilesMergeTree.IsEnabled(ctx) { + expectedErr = structerr.NewFailedPrecondition("getting objectreader: object not found") + } + + return setupData{ + client: client, + request: request, + expectedError: expectedErr, + } + }, + }, + { "Lists the expected conflict files with ancestor path", func(tb testing.TB, ctx context.Context) setupData { cfg, client := setupConflictsServiceWithoutRepo(tb, nil) @@ -303,29 +361,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 +548,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 } |