Merge branch 'kn-git-merge-tree-fixes' into 'master'

Better error handling for git-merge-tree(1)

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

Merged-by: karthik nayak <knayak@gitlab.com>
Approved-by: Christian Couder <chriscool@tuxfamily.org>

2 files changed, 50 insertions, 26 deletions

diff --git a/internal/git/localrepo/merge.go b/internal/git/localrepo/merge.go
index 4b09f1b01..7afbd6dc7 100644
--- a/internal/git/localrepo/merge.go
+++ b/internal/git/localrepo/merge.go
@@ -4,13 +4,13 @@ import (
 	"bytes"
 	"context"
 	"errors"
-	"fmt"
 	"strconv"
 	"strings"
 
 	"gitlab.com/gitlab-org/gitaly/v16/internal/command"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/helper/text"
+	"gitlab.com/gitlab-org/gitaly/v16/internal/structerr"
 )
 
 // MergeStage denotes the stage indicated by git-merge-tree(1) in the conflicting
@@ -84,7 +84,7 @@ func (repo *Repo) MergeTree(
 
 	objectHash, err := repo.ObjectHash(ctx)
 	if err != nil {
-		return "", fmt.Errorf("getting object hash %w", err)
+		return "", structerr.NewInternal("getting object hash %w", err)
 	}
 
 	var stdout, stderr bytes.Buffer
@@ -101,22 +101,23 @@ func (repo *Repo) MergeTree(
 	if err != nil {
 		exitCode, success := command.ExitStatus(err)
 		if !success {
-			return "", errors.New("could not parse exit status of merge-tree(1)")
+			return "", structerr.NewInternal("could not parse exit status of merge-tree(1)")
 		}
 
-		if exitCode > 1 {
-			if text.ChompBytes(stderr.Bytes()) == "fatal: refusing to merge unrelated histories" {
-				return "", ErrMergeTreeUnrelatedHistory
-			}
-			return "", fmt.Errorf("merge-tree: %w", err)
+		if exitCode == 1 {
+			return parseMergeTreeError(objectHash, config, stdout.String())
+		}
+
+		if text.ChompBytes(stderr.Bytes()) == "fatal: refusing to merge unrelated histories" {
+			return "", ErrMergeTreeUnrelatedHistory
 		}
 
-		return parseMergeTreeError(objectHash, config, stdout.String())
+		return "", structerr.NewInternal("merge-tree: %w", err).WithMetadata("exit_status", exitCode)
 	}
 
 	oid, err := objectHash.FromHex(strings.Split(stdout.String(), "\x00")[0])
 	if err != nil {
-		return "", fmt.Errorf("hex to oid: %w", err)
+		return "", structerr.NewInternal("hex to oid: %w", err)
 	}
 
 	return oid, nil
@@ -130,18 +131,18 @@ func parseMergeTreeError(objectHash git.ObjectHash, cfg mergeTreeConfig, output
 
 	oidAndConflictsBuf, infoMsg, ok := strings.Cut(output, "\x00\x00")
 	if !ok {
-		return "", fmt.Errorf("couldn't parse merge tree output: %s", output)
+		return "", structerr.NewInternal("couldn't parse merge tree output: %s", output).WithMetadata("stderr", output)
 	}
 
 	oidAndConflicts := strings.Split(oidAndConflictsBuf, "\x00")
 	// When the output is of unexpected length
 	if len(oidAndConflicts) < 2 {
-		return "", errors.New("couldn't split oid and file info")
+		return "", structerr.NewInternal("couldn't split oid and file info").WithMetadata("stderr", output)
 	}
 
 	oid, err := objectHash.FromHex(oidAndConflicts[0])
 	if err != nil {
-		return "", fmt.Errorf("hex to oid: %w", err)
+		return "", structerr.NewInternal("hex to oid: %w", err)
 	}
 
 	mergeTreeConflictError.ConflictingFileInfo = make([]ConflictingFileInfo, len(oidAndConflicts[1:]))
@@ -155,31 +156,31 @@ func parseMergeTreeError(objectHash git.ObjectHash, cfg mergeTreeConfig, output
 		} else {
 			infoAndFilename := strings.Split(infoLine, "\t")
 			if len(infoAndFilename) != 2 {
-				return "", fmt.Errorf("parsing conflicting file info: %s", infoLine)
+				return "", structerr.NewInternal("parsing conflicting file info: %s", infoLine)
 			}
 
 			info := strings.Fields(infoAndFilename[0])
 			if len(info) != 3 {
-				return "", fmt.Errorf("parsing conflicting file info: %s", infoLine)
+				return "", structerr.NewInternal("parsing conflicting file info: %s", infoLine)
 			}
 
 			mode, err := strconv.ParseInt(info[0], 8, 32)
 			if err != nil {
-				return "", fmt.Errorf("parsing mode: %w", err)
+				return "", structerr.NewInternal("parsing mode: %w", err)
 			}
 
 			mergeTreeConflictError.ConflictingFileInfo[i].OID, err = objectHash.FromHex(info[1])
 			if err != nil {
-				return "", fmt.Errorf("hex to oid: %w", err)
+				return "", structerr.NewInternal("hex to oid: %w", err)
 			}
 
 			stage, err := strconv.Atoi(info[2])
 			if err != nil {
-				return "", fmt.Errorf("converting stage to int: %w", err)
+				return "", structerr.NewInternal("converting stage to int: %w", err)
 			}
 
 			if stage < 1 || stage > 3 {
-				return "", fmt.Errorf("invalid value for stage: %d", stage)
+				return "", structerr.NewInternal("invalid value for stage: %d", stage)
 			}
 
 			mergeTreeConflictError.ConflictingFileInfo[i].Mode = int32(mode)
@@ -197,11 +198,11 @@ func parseMergeTreeError(objectHash git.ObjectHash, cfg mergeTreeConfig, output
 
 		numOfPaths, err := strconv.Atoi(fields[i])
 		if err != nil {
-			return "", fmt.Errorf("converting stage to int: %w", err)
+			return "", structerr.NewInternal("converting stage to int: %w", err)
 		}
 
 		if i+numOfPaths+2 > len(fields) {
-			return "", fmt.Errorf("incorrect number of fields: %s", infoMsg)
+			return "", structerr.NewInternal("incorrect number of fields: %s", infoMsg)
 		}
 
 		c.Paths = fields[i+1 : i+numOfPaths+1]
diff --git a/internal/git/localrepo/merge_test.go b/internal/git/localrepo/merge_test.go
index 3106e16d4..ca7d40344 100644
--- a/internal/git/localrepo/merge_test.go
+++ b/internal/git/localrepo/merge_test.go
@@ -10,6 +10,7 @@ import (
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git"
 	"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"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/testhelper/testcfg"
 )
@@ -743,7 +744,7 @@ func TestParseResult(t *testing.T) {
 				"",
 			}, "\x00"),
 			oid:         gittest.DefaultObjectHash.EmptyTreeOID,
-			expectedErr: fmt.Errorf("parsing conflicting file info: 100644 %s 1a", gittest.DefaultObjectHash.EmptyTreeOID),
+			expectedErr: structerr.NewInternal("parsing conflicting file info: 100644 %s 1a", gittest.DefaultObjectHash.EmptyTreeOID),
 		},
 		{
 			desc: "incorrect number of fields in conflicting file info",
@@ -760,7 +761,7 @@ func TestParseResult(t *testing.T) {
 				"",
 			}, "\x00"),
 			oid:         gittest.DefaultObjectHash.EmptyTreeOID,
-			expectedErr: fmt.Errorf("parsing conflicting file info: %s 2\ta", gittest.DefaultObjectHash.EmptyTreeOID),
+			expectedErr: structerr.NewInternal("parsing conflicting file info: %s 2\ta", gittest.DefaultObjectHash.EmptyTreeOID),
 		},
 		{
 			desc: "invalid OID in conflicting file info",
@@ -777,7 +778,7 @@ func TestParseResult(t *testing.T) {
 				"",
 			}, "\x00"),
 			oid: gittest.DefaultObjectHash.EmptyTreeOID,
-			expectedErr: fmt.Errorf("hex to oid: %w", git.InvalidObjectIDLengthError{
+			expectedErr: structerr.NewInternal("hex to oid: %w", git.InvalidObjectIDLengthError{
 				OID:           "$$$",
 				CorrectLength: gittest.DefaultObjectHash.EncodedLen(),
 				Length:        3,
@@ -798,7 +799,7 @@ func TestParseResult(t *testing.T) {
 				"",
 			}, "\x00"),
 			oid: gittest.DefaultObjectHash.EmptyTreeOID,
-			expectedErr: fmt.Errorf("converting stage to int: %w", &strconv.NumError{
+			expectedErr: structerr.NewInternal("converting stage to int: %w", &strconv.NumError{
 				Func: "Atoi",
 				Num:  "foo",
 				Err:  errors.New("invalid syntax"),
@@ -819,7 +820,29 @@ func TestParseResult(t *testing.T) {
 				"",
 			}, "\x00"),
 			oid:         gittest.DefaultObjectHash.EmptyTreeOID,
-			expectedErr: fmt.Errorf("invalid value for stage: 5"),
+			expectedErr: structerr.NewInternal("invalid value for stage: 5"),
+		},
+		{
+			desc: "missing conflicting file section",
+			output: strings.Join([]string{
+				gittest.DefaultObjectHash.EmptyTreeOID.String(),
+				"",
+				"1",
+				"a",
+				"Auto-merging",
+				"Auto-merging a\n",
+				"",
+			}, "\x00"),
+			oid: gittest.DefaultObjectHash.EmptyTreeOID,
+			expectedErr: structerr.NewInternal("couldn't split oid and file info").WithMetadata("stderr", strings.Join([]string{
+				gittest.DefaultObjectHash.EmptyTreeOID.String(),
+				"",
+				"1",
+				"a",
+				"Auto-merging",
+				"Auto-merging a\n",
+				"",
+			}, "\x00")),
 		},
 	}