diff --git a/internal/git/localrepo/merge.go b/internal/git/localrepo/merge.go index 60a2608cbc8fc1608e59e061b406b9ff7022bdf6..79f3bd60bb1256710b98f0c0a263f80f9b8362f5 100644 --- a/internal/git/localrepo/merge.go +++ b/internal/git/localrepo/merge.go @@ -5,6 +5,7 @@ import ( "context" "errors" "fmt" + "strconv" "strings" "gitlab.com/gitlab-org/gitaly/v15/internal/command" @@ -12,8 +13,23 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/helper/text" ) +// MergeStage denotes the stage indicated by git-merge-tree(1) in the conflicting +// files information section. The man page for git-merge(1) holds more information +// regarding the values of the stages and what they indicate. +type MergeStage uint + +const ( + // MergeStageAncestor denotes a conflicting file version from the common ancestor. + MergeStageAncestor = MergeStage(1) + // MergeStageOurs denotes a conflicting file version from our commit. + MergeStageOurs = MergeStage(2) + // MergeStageTheirs denotes a conflicting file version from their commit. + MergeStageTheirs = MergeStage(3) +) + type mergeTreeConfig struct { - allowUnrelatedHistories bool + allowUnrelatedHistories bool + conflictingFileNamesOnly bool } // MergeTreeOption is a function that sets a config in mergeTreeConfig. @@ -27,6 +43,14 @@ func WithAllowUnrelatedHistories() MergeTreeOption { } } +// WithConflictingFileNamesOnly lets MergeTree only parse the conflicting filenames and +// not the additional information. +func WithConflictingFileNamesOnly() MergeTreeOption { + return func(options *mergeTreeConfig) { + options.conflictingFileNamesOnly = true + } +} + // MergeTree calls git-merge-tree(1) with arguments, and parses the results from // stdout. func (repo *Repo) MergeTree( @@ -42,15 +66,23 @@ func (repo *Repo) MergeTree( flags := []git.Option{ git.Flag{Name: "--write-tree"}, - git.Flag{Name: "--name-only"}, } if config.allowUnrelatedHistories { flags = append(flags, git.Flag{Name: "--allow-unrelated-histories"}) } + if config.conflictingFileNamesOnly { + flags = append(flags, git.Flag{Name: "--name-only"}) + } + + objectHash, err := repo.ObjectHash(ctx) + if err != nil { + return "", fmt.Errorf("getting object hash %w", err) + } + var stdout, stderr bytes.Buffer - err := repo.ExecAndWait( + err = repo.ExecAndWait( ctx, git.Command{ Name: "merge-tree", @@ -75,12 +107,7 @@ func (repo *Repo) MergeTree( return "", fmt.Errorf("merge-tree: %w", err) } - return "", parseMergeTreeError(stdout.String()) - } - - objectHash, err := repo.ObjectHash(ctx) - if err != nil { - return "", fmt.Errorf("getting object hash %w", err) + return parseMergeTreeError(objectHash, config, stdout.String()) } oid, err := objectHash.FromHex(text.ChompBytes(stdout.Bytes())) @@ -94,33 +121,81 @@ func (repo *Repo) MergeTree( // parseMergeTreeError parses the output from git-merge-tree(1)'s stdout into // a MergeTreeResult struct. The format for the output can be found at // https://git-scm.com/docs/git-merge-tree#OUTPUT. -func parseMergeTreeError(output string) error { +func parseMergeTreeError(objectHash git.ObjectHash, cfg mergeTreeConfig, output string) (git.ObjectID, error) { var mergeTreeError MergeTreeError lines := strings.SplitN(output, "\n\n", 2) // When the output is of unexpected length if len(lines) < 2 { - return errors.New("error parsing merge tree result") + return "", errors.New("error parsing merge tree result") } mergeTreeError.InfoMessage = strings.TrimSuffix(lines[1], "\n") oidAndConflicts := strings.Split(lines[0], "\n") if len(oidAndConflicts) < 2 { - return &mergeTreeError + return "", &mergeTreeError } - mergeTreeError.ConflictingFiles = oidAndConflicts[1:] + oid, err := objectHash.FromHex(text.ChompBytes([]byte(oidAndConflicts[0]))) + if err != nil { + return "", fmt.Errorf("hex to oid: %w", err) + } + + mergeTreeError.ConflictingFileInfo = make([]ConflictingFileInfo, len(oidAndConflicts[1:])) + + // From git-merge-tree(1), the information is of the format ` ` + // unless the `--name-only` option is used, in which case only the filename is output. + // Note: that there is \t before the filename (https://gitlab.com/gitlab-org/git/blob/v2.40.0/builtin/merge-tree.c#L481) + for i, infoLine := range oidAndConflicts[1:] { + if cfg.conflictingFileNamesOnly { + mergeTreeError.ConflictingFileInfo[i].FileName = infoLine + } else { + infoAndFilename := strings.Split(infoLine, "\t") + if len(infoAndFilename) != 2 { + return "", fmt.Errorf("parsing conflicting file info: %s", infoLine) + } + + info := strings.Fields(infoAndFilename[0]) + if len(info) != 3 { + return "", fmt.Errorf("parsing conflicting file info: %s", infoLine) + } + + mergeTreeError.ConflictingFileInfo[i].OID, err = objectHash.FromHex(info[1]) + if err != nil { + return "", fmt.Errorf("hex to oid: %w", err) + } + + stage, err := strconv.Atoi(info[2]) + if err != nil { + return "", fmt.Errorf("converting stage to int: %w", err) + } + + if stage < 1 || stage > 3 { + return "", fmt.Errorf("invalid value for stage: %d", stage) + } + + mergeTreeError.ConflictingFileInfo[i].Stage = MergeStage(stage) + mergeTreeError.ConflictingFileInfo[i].FileName = infoAndFilename[1] + } + } + + return oid, &mergeTreeError +} - return &mergeTreeError +// ConflictingFileInfo holds the conflicting file info output from git-merge-tree(1). +type ConflictingFileInfo struct { + FileName string + OID git.ObjectID + Stage MergeStage } -// MergeTreeError encapsulates any conflicting files and messages that occur +// MergeTreeError encapsulates any conflicting file info and messages that occur // when a merge-tree(1) command fails. type MergeTreeError struct { - ConflictingFiles []string - InfoMessage string + ConflictingFileInfo []ConflictingFileInfo + InfoMessage string } // Error returns the error string for a conflict error. diff --git a/internal/git/localrepo/merge_test.go b/internal/git/localrepo/merge_test.go index 11112d72d0b12d9cf6568fba210c0c7ecf857343..39426f229ef8a41dcfa54956da99aac5dfaead9d 100644 --- a/internal/git/localrepo/merge_test.go +++ b/internal/git/localrepo/merge_test.go @@ -2,6 +2,9 @@ package localrepo import ( "context" + "errors" + "fmt" + "strconv" "testing" "github.com/stretchr/testify/require" @@ -127,8 +130,14 @@ func TestMergeTree(t *testing.T) { { desc: "with conflicts", expectedErr: &MergeTreeError{ - ConflictingFiles: []string{"file2"}, - InfoMessage: "Auto-merging file2\nCONFLICT (add/add): Merge conflict in file2", + ConflictingFileInfo: []ConflictingFileInfo{ + { + FileName: "file2", + OID: "", + Stage: 0, + }, + }, + InfoMessage: "Auto-merging file2\nCONFLICT (add/add): Merge conflict in file2", }, setupFunc: func(t *testing.T, ctx context.Context, repoPath string) (git.ObjectID, git.ObjectID, []gittest.TreeEntry) { tree1 := gittest.WriteTree(t, cfg, repoPath, []gittest.TreeEntry{ @@ -196,7 +205,7 @@ func TestMergeTree(t *testing.T) { ours, theirs, treeEntries := tc.setupFunc(t, ctx, repoPath) - mergeTreeResult, err := repo.MergeTree(ctx, string(ours), string(theirs)) + mergeTreeResult, err := repo.MergeTree(ctx, string(ours), string(theirs), WithConflictingFileNamesOnly()) require.Equal(t, tc.expectedErr, err) if tc.expectedErr == nil { @@ -277,52 +286,135 @@ func TestParseResult(t *testing.T) { testCases := []struct { desc string output string - oid string - expectedErr *MergeTreeError + oid git.ObjectID + expectedErr error }{ { desc: "single file conflict", - output: `4f18fcb9bc62a3a6a4f81236fd57eeb95bfb06b4 -file + output: fmt.Sprintf(`%s +100644 %s 2%sfile +100644 %s 3%sfile Auto-merging file CONFLICT (content): Merge conflict in file -`, - oid: "4f18fcb9bc62a3a6a4f81236fd57eeb95bfb06b4", +`, gittest.DefaultObjectHash.EmptyTreeOID, gittest.DefaultObjectHash.EmptyTreeOID, "\t", gittest.DefaultObjectHash.EmptyTreeOID, "\t"), + oid: gittest.DefaultObjectHash.EmptyTreeOID, expectedErr: &MergeTreeError{ - ConflictingFiles: []string{ - "file", + ConflictingFileInfo: []ConflictingFileInfo{ + { + FileName: "file", + OID: gittest.DefaultObjectHash.EmptyTreeOID, + Stage: MergeStageOurs, + }, + { + FileName: "file", + OID: gittest.DefaultObjectHash.EmptyTreeOID, + Stage: MergeStageTheirs, + }, }, InfoMessage: "Auto-merging file\nCONFLICT (content): Merge conflict in file", }, }, { desc: "multiple files conflict", - output: `4f18fcb9bc62a3a6a4f81236fd57eeb95bfb06b4 -file1 -file2 + output: fmt.Sprintf(`%s +100644 %s 2%sfile1 +100644 %s 3%sfile2 Auto-merging file CONFLICT (content): Merge conflict in file1 -`, - oid: "4f18fcb9bc62a3a6a4f81236fd57eeb95bfb06b4", +`, gittest.DefaultObjectHash.EmptyTreeOID, gittest.DefaultObjectHash.EmptyTreeOID, "\t", gittest.DefaultObjectHash.EmptyTreeOID, "\t"), + oid: gittest.DefaultObjectHash.EmptyTreeOID, expectedErr: &MergeTreeError{ - ConflictingFiles: []string{ - "file1", - "file2", + ConflictingFileInfo: []ConflictingFileInfo{ + { + FileName: "file1", + OID: gittest.DefaultObjectHash.EmptyTreeOID, + Stage: MergeStageOurs, + }, + { + FileName: "file2", + OID: gittest.DefaultObjectHash.EmptyTreeOID, + Stage: MergeStageTheirs, + }, }, InfoMessage: "Auto-merging file\nCONFLICT (content): Merge conflict in file1", }, }, + { + desc: "no tab in conflicting file info", + output: fmt.Sprintf(`%s +100644 %s 2 file1 + +Auto-merging file +CONFLICT (content): Merge conflict in file1 +`, gittest.DefaultObjectHash.EmptyTreeOID, gittest.DefaultObjectHash.EmptyTreeOID), + oid: gittest.DefaultObjectHash.EmptyTreeOID, + expectedErr: fmt.Errorf("parsing conflicting file info: 100644 %s 2 file1", gittest.DefaultObjectHash.EmptyTreeOID), + }, + { + desc: "incorrect number of fields in conflicting file info", + output: fmt.Sprintf(`%s +%s 2%sfile1 + +Auto-merging file +CONFLICT (content): Merge conflict in file1 +`, gittest.DefaultObjectHash.EmptyTreeOID, gittest.DefaultObjectHash.EmptyTreeOID, "\t"), + oid: gittest.DefaultObjectHash.EmptyTreeOID, + expectedErr: fmt.Errorf("parsing conflicting file info: %s 2\tfile1", gittest.DefaultObjectHash.EmptyTreeOID), + }, + { + desc: "invalid OID in conflicting file info", + output: fmt.Sprintf(`%s +100644 23 2%sfile1 + +Auto-merging file +CONFLICT (content): Merge conflict in file1 +`, gittest.DefaultObjectHash.EmptyTreeOID, "\t"), + oid: gittest.DefaultObjectHash.EmptyTreeOID, + expectedErr: fmt.Errorf("hex to oid: %w", git.InvalidObjectIDLengthError{ + OID: "23", + CorrectLength: gittest.DefaultObjectHash.EncodedLen(), + Length: 2, + }), + }, + { + desc: "invalid stage type in conflicting file info", + output: fmt.Sprintf(`%s +100644 %s foo%sfile1 + +Auto-merging file +CONFLICT (content): Merge conflict in file1 +`, gittest.DefaultObjectHash.EmptyTreeOID, gittest.DefaultObjectHash.EmptyTreeOID, "\t"), + oid: gittest.DefaultObjectHash.EmptyTreeOID, + expectedErr: fmt.Errorf("converting stage to int: %w", &strconv.NumError{ + Func: "Atoi", + Num: "foo", + Err: errors.New("invalid syntax"), + }), + }, + { + desc: "invalid stage value in conflicting file info", + output: fmt.Sprintf(`%s +100644 %s 5%sfile1 + +Auto-merging file +CONFLICT (content): Merge conflict in file1 +`, gittest.DefaultObjectHash.EmptyTreeOID, gittest.DefaultObjectHash.EmptyTreeOID, "\t"), + oid: gittest.DefaultObjectHash.EmptyTreeOID, + expectedErr: fmt.Errorf("invalid value for stage: 5"), + }, } for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { - err := parseMergeTreeError(tc.output) + oid, err := parseMergeTreeError(gittest.DefaultObjectHash, mergeTreeConfig{}, tc.output) if tc.expectedErr != nil { require.Equal(t, tc.expectedErr, err) return } + + require.Equal(t, tc.oid, oid) require.NoError(t, err) }) } diff --git a/internal/gitaly/service/conflicts/list_conflict_files.go b/internal/gitaly/service/conflicts/list_conflict_files.go index f7cf317ac5f5615b6d9d805c470bd89daf354864..9d00077575872353b422c2a542d520b496731be7 100644 --- a/internal/gitaly/service/conflicts/list_conflict_files.go +++ b/internal/gitaly/service/conflicts/list_conflict_files.go @@ -2,12 +2,14 @@ package conflicts import ( "bytes" + "context" "errors" "fmt" "io" "unicode/utf8" "gitlab.com/gitlab-org/gitaly/v15/internal/git" + "gitlab.com/gitlab-org/gitaly/v15/internal/git/localrepo" "gitlab.com/gitlab-org/gitaly/v15/internal/git2go" "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/service" "gitlab.com/gitlab-org/gitaly/v15/internal/structerr" @@ -39,6 +41,17 @@ func (s *server) ListConflictFiles(request *gitalypb.ListConflictFilesRequest, s return err } + return s.conflictFilesWithGit2Go(ctx, request, stream, ours, theirs, repo, repoPath) +} + +func (s *server) conflictFilesWithGit2Go( + ctx context.Context, + request *gitalypb.ListConflictFilesRequest, + stream gitalypb.ConflictsService_ListConflictFilesServer, + ours, theirs git.ObjectID, + repo *localrepo.Repo, + repoPath string, +) error { conflicts, err := s.git2goExecutor.Conflicts(ctx, repo, git2go.ConflictsCommand{ Repository: repoPath, Ours: ours.String(), diff --git a/internal/gitaly/service/operations/merge.go b/internal/gitaly/service/operations/merge.go index 0438826ea39a1070584d9633c438633e3c815dce..a35392b9af8366797e3c35b27008fa33e0765d06 100644 --- a/internal/gitaly/service/operations/merge.go +++ b/internal/gitaly/service/operations/merge.go @@ -62,7 +62,7 @@ func (s *Server) merge( ours string, theirs string, ) (string, error) { - treeOID, err := quarantineRepo.MergeTree(ctx, ours, theirs, localrepo.WithAllowUnrelatedHistories()) + treeOID, err := quarantineRepo.MergeTree(ctx, ours, theirs, localrepo.WithAllowUnrelatedHistories(), localrepo.WithConflictingFileNamesOnly()) if err != nil { return "", err } @@ -156,9 +156,9 @@ func (s *Server) UserMergeBranch(stream gitalypb.OperationService_UserMergeBranc if mergeErr != nil { var conflictErr *localrepo.MergeTreeError if errors.As(mergeErr, &conflictErr) { - conflictingFiles := make([][]byte, 0, len(conflictErr.ConflictingFiles)) - for _, conflictingFile := range conflictErr.ConflictingFiles { - conflictingFiles = append(conflictingFiles, []byte(conflictingFile)) + conflictingFiles := make([][]byte, 0, len(conflictErr.ConflictingFileInfo)) + for _, conflictingFileInfo := range conflictErr.ConflictingFileInfo { + conflictingFiles = append(conflictingFiles, []byte(conflictingFileInfo.FileName)) } return structerr.NewFailedPrecondition("merging commits: %w", mergeErr).