Merge branch 'po_raw_changes_encoding' into 'master'

GetRawChanges RPC uses both string and byte path fields

See merge request gitlab-org/gitaly!1207

5 files changed, 166 insertions, 54 deletions

diff --git a/changelogs/unreleased/po_raw_changes_encoding.yml b/changelogs/unreleased/po_raw_changes_encoding.yml
new file mode 100644
index 000000000..3e4df31bb
--- /dev/null
+++ b/changelogs/unreleased/po_raw_changes_encoding.yml
@@ -0,0 +1,5 @@
+---
+title: GetRawChanges RPC uses both string and byte path fields
+merge_request: 1207
+author:
+type: fixed
diff --git a/internal/service/commit/list_last_commits_for_tree_test.go b/internal/service/commit/list_last_commits_for_tree_test.go
index 7ff1707de..4fc13a5f5 100644
--- a/internal/service/commit/list_last_commits_for_tree_test.go
+++ b/internal/service/commit/list_last_commits_for_tree_test.go
@@ -3,16 +3,13 @@ package commit
 import (
 	"bytes"
 	"context"
-	"fmt"
 	"io"
-	"strings"
 	"testing"
 	"unicode/utf8"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
-	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"google.golang.org/grpc/codes"
 )
@@ -316,10 +313,12 @@ func TestNonUtf8ListLastCommitsForTreeRequest(t *testing.T) {
 	nonUTF8Filename := "hello\x80world"
 	require.False(t, utf8.ValidString(nonUTF8Filename))
 
-	mktreeIn := strings.NewReader(fmt.Sprintf("100644 blob %s\t%s", blobID, nonUTF8Filename))
-	treeID := text.ChompBytes(testhelper.MustRunCommand(t, mktreeIn, "git", "-C", testRepoPath, "mktree"))
-
-	commitID := text.ChompBytes(testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "commit-tree", treeID, "-m", "commit for non-utf8 path"))
+	commitID := testhelper.CommitBlobWithName(t,
+		testRepoPath,
+		blobID,
+		nonUTF8Filename,
+		"commit for non-utf8 path",
+	)
 
 	request := &gitalypb.ListLastCommitsForTreeRequest{
 		Repository: testRepo,
diff --git a/internal/service/repository/raw_changes.go b/internal/service/repository/raw_changes.go
index 41b2faa05..6855ba38b 100644
--- a/internal/service/repository/raw_changes.go
+++ b/internal/service/repository/raw_changes.go
@@ -5,6 +5,7 @@ import (
 	"io"
 	"regexp"
 	"strconv"
+	"unicode/utf8"
 
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
@@ -155,34 +156,47 @@ func changeFromDiff(batch *catfile.Batch, d *rawdiff.Diff) (*gitalypb.GetRawChan
 	return resp, nil
 }
 
+// InvalidUTF8PathPlaceholder is a temporary placeholder that indicates the
+const InvalidUTF8PathPlaceholder = "ENCODING ERROR gitaly#1470"
+
 func setOperationAndPaths(d *rawdiff.Diff, resp *gitalypb.GetRawChangesResponse_RawChange) error {
 	if len(d.Status) == 0 {
 		return fmt.Errorf("empty diff status")
 	}
 
-	resp.NewPath = d.SrcPath
-	resp.OldPath = d.SrcPath
+	resp.NewPathBytes = []byte(d.SrcPath)
+	resp.OldPathBytes = []byte(d.SrcPath)
 
 	switch d.Status[0] {
 	case 'A':
 		resp.Operation = gitalypb.GetRawChangesResponse_RawChange_ADDED
-		resp.OldPath = ""
+		resp.OldPathBytes = nil
 	case 'C':
 		resp.Operation = gitalypb.GetRawChangesResponse_RawChange_COPIED
-		resp.NewPath = d.DstPath
+		resp.NewPathBytes = []byte(d.DstPath)
 	case 'D':
 		resp.Operation = gitalypb.GetRawChangesResponse_RawChange_DELETED
-		resp.NewPath = ""
+		resp.NewPathBytes = nil
 	case 'M':
 		resp.Operation = gitalypb.GetRawChangesResponse_RawChange_MODIFIED
 	case 'R':
 		resp.Operation = gitalypb.GetRawChangesResponse_RawChange_RENAMED
-		resp.NewPath = d.DstPath
+		resp.NewPathBytes = []byte(d.DstPath)
 	case 'T':
 		resp.Operation = gitalypb.GetRawChangesResponse_RawChange_TYPE_CHANGED
 	default:
 		resp.Operation = gitalypb.GetRawChangesResponse_RawChange_UNKNOWN
 	}
 
+	resp.OldPath = string(resp.OldPathBytes)
+	resp.NewPath = string(resp.NewPathBytes)
+
+	if !utf8.ValidString(resp.OldPath) {
+		resp.OldPath = InvalidUTF8PathPlaceholder
+	}
+	if !utf8.ValidString(resp.NewPath) {
+		resp.NewPath = InvalidUTF8PathPlaceholder
+	}
+
 	return nil
 }
diff --git a/internal/service/repository/raw_changes_test.go b/internal/service/repository/raw_changes_test.go
index d01dc8281..459aaffb3 100644
--- a/internal/service/repository/raw_changes_test.go
+++ b/internal/service/repository/raw_changes_test.go
@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"io"
 	"testing"
+	"unicode/utf8"
 
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
@@ -30,20 +31,23 @@ func TestGetRawChanges(t *testing.T) {
 			newRev: "7975be0116940bf2ad4321f79d02a55c5f7779aa",
 			changes: []*gitalypb.GetRawChangesResponse_RawChange{
 				{
-					BlobId:    "c60514b6d3d6bf4bec1030f70026e34dfbd69ad5",
-					Size:      824,
-					NewPath:   "README.md",
-					OldPath:   "README.md",
-					Operation: gitalypb.GetRawChangesResponse_RawChange_MODIFIED,
-					OldMode:   0100644,
-					NewMode:   0100644,
+					BlobId:       "c60514b6d3d6bf4bec1030f70026e34dfbd69ad5",
+					Size:         824,
+					NewPath:      "README.md",
+					NewPathBytes: []byte("README.md"),
+					OldPath:      "README.md",
+					OldPathBytes: []byte("README.md"),
+					Operation:    gitalypb.GetRawChangesResponse_RawChange_MODIFIED,
+					OldMode:      0100644,
+					NewMode:      0100644,
 				},
 				{
-					BlobId:    "723c2c3f4c8a2a1e957f878c8813acfc08cda2b6",
-					Size:      1219696,
-					NewPath:   "files/images/emoji.png",
-					Operation: gitalypb.GetRawChangesResponse_RawChange_ADDED,
-					NewMode:   0100644,
+					BlobId:       "723c2c3f4c8a2a1e957f878c8813acfc08cda2b6",
+					Size:         1219696,
+					NewPath:      "files/images/emoji.png",
+					NewPathBytes: []byte("files/images/emoji.png"),
+					Operation:    gitalypb.GetRawChangesResponse_RawChange_ADDED,
+					NewMode:      0100644,
 				},
 			},
 		},
@@ -52,25 +56,28 @@ func TestGetRawChanges(t *testing.T) {
 			newRev: "1a0b36b3cdad1d2ee32457c102a8c0b7056fa863",
 			changes: []*gitalypb.GetRawChangesResponse_RawChange{
 				{
-					BlobId:    "470ad2fcf1e33798f1afc5781d08e60c40f51e7a",
-					Size:      231,
-					NewPath:   ".gitignore",
-					Operation: gitalypb.GetRawChangesResponse_RawChange_ADDED,
-					NewMode:   0100644,
+					BlobId:       "470ad2fcf1e33798f1afc5781d08e60c40f51e7a",
+					Size:         231,
+					NewPath:      ".gitignore",
+					NewPathBytes: []byte(".gitignore"),
+					Operation:    gitalypb.GetRawChangesResponse_RawChange_ADDED,
+					NewMode:      0100644,
 				},
 				{
-					BlobId:    "50b27c6518be44c42c4d87966ae2481ce895624c",
-					Size:      1075,
-					NewPath:   "LICENSE",
-					Operation: gitalypb.GetRawChangesResponse_RawChange_ADDED,
-					NewMode:   0100644,
+					BlobId:       "50b27c6518be44c42c4d87966ae2481ce895624c",
+					Size:         1075,
+					NewPath:      "LICENSE",
+					NewPathBytes: []byte("LICENSE"),
+					Operation:    gitalypb.GetRawChangesResponse_RawChange_ADDED,
+					NewMode:      0100644,
 				},
 				{
-					BlobId:    "faaf198af3a36dbf41961466703cc1d47c61d051",
-					Size:      55,
-					NewPath:   "README.md",
-					Operation: gitalypb.GetRawChangesResponse_RawChange_ADDED,
-					NewMode:   0100644,
+					BlobId:       "faaf198af3a36dbf41961466703cc1d47c61d051",
+					Size:         55,
+					NewPath:      "README.md",
+					NewPathBytes: []byte("README.md"),
+					Operation:    gitalypb.GetRawChangesResponse_RawChange_ADDED,
+					NewMode:      0100644,
 				},
 			},
 		},
@@ -79,13 +86,15 @@ func TestGetRawChanges(t *testing.T) {
 			newRev: "06041ab2037429d243a38abb55957818dd9f948d",
 			changes: []*gitalypb.GetRawChangesResponse_RawChange{
 				{
-					BlobId:    "c84acd1ff0b844201312052f9bb3b7259eb2e177",
-					Size:      23,
-					NewPath:   "files/executables/ls",
-					OldPath:   "files/executables/ls",
-					Operation: gitalypb.GetRawChangesResponse_RawChange_MODIFIED,
-					OldMode:   0100755,
-					NewMode:   0100644,
+					BlobId:       "c84acd1ff0b844201312052f9bb3b7259eb2e177",
+					Size:         23,
+					NewPath:      "files/executables/ls",
+					NewPathBytes: []byte("files/executables/ls"),
+					OldPath:      "files/executables/ls",
+					OldPathBytes: []byte("files/executables/ls"),
+					Operation:    gitalypb.GetRawChangesResponse_RawChange_MODIFIED,
+					OldMode:      0100755,
+					NewMode:      0100644,
 				},
 			},
 		},
@@ -290,15 +299,89 @@ func TestGetRawChangesMappingOperations(t *testing.T) {
 	}
 
 	firstChange := &gitalypb.GetRawChangesResponse_RawChange{
-		BlobId:    "53855584db773c3df5b5f61f72974cb298822fbb",
-		Size:      22846,
-		NewPath:   "CHANGELOG.md",
-		OldPath:   "CHANGELOG",
-		Operation: gitalypb.GetRawChangesResponse_RawChange_RENAMED,
-		OldMode:   0100644,
-		NewMode:   0100644,
+		BlobId:       "53855584db773c3df5b5f61f72974cb298822fbb",
+		Size:         22846,
+		NewPath:      "CHANGELOG.md",
+		NewPathBytes: []byte("CHANGELOG.md"),
+		OldPath:      "CHANGELOG",
+		OldPathBytes: []byte("CHANGELOG"),
+		Operation:    gitalypb.GetRawChangesResponse_RawChange_RENAMED,
+		OldMode:      0100644,
+		NewMode:      0100644,
 	}
 
 	require.Equal(t, firstChange, msg.GetRawChanges()[0])
 	require.EqualValues(t, expected, ops)
 }
+
+func TestGetRawChangesInvalidUTF8Paths(t *testing.T) {
+	server, serverSocketPath := runRepoServer(t)
+	defer server.Stop()
+
+	client, conn := newRepositoryClient(t, serverSocketPath)
+	defer conn.Close()
+
+	testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
+	defer cleanupFn()
+
+	const (
+		// These are arbitrary blobs known to exist in the test repository
+		blobID1         = "c60514b6d3d6bf4bec1030f70026e34dfbd69ad5"
+		blobID2         = "50b27c6518be44c42c4d87966ae2481ce895624c"
+		nonUTF8Filename = "hello\x80world"
+	)
+	require.False(t, utf8.ValidString(nonUTF8Filename)) // sanity check
+
+	fromCommitID := testhelper.CommitBlobWithName(
+		t,
+		testRepoPath,
+		blobID1,
+		nonUTF8Filename,
+		"killer AI might use non-UTF filenames",
+	)
+	toCommitID := testhelper.CommitBlobWithName(
+		t,
+		testRepoPath,
+		blobID2,
+		nonUTF8Filename,
+		"hostile extraterrestials won't use UTF",
+	)
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	req := &gitalypb.GetRawChangesRequest{
+		Repository:   testRepo,
+		FromRevision: fromCommitID,
+		ToRevision:   toCommitID,
+	}
+
+	c, err := client.GetRawChanges(ctx, req)
+	require.NoError(t, err)
+
+	newPathFound := false
+	oldPathFound := false
+	for {
+		msg, err := c.Recv()
+		if err != nil {
+			require.Equal(t, io.EOF, err)
+			break
+		}
+
+		for _, rawChange := range msg.GetRawChanges() {
+			if string(rawChange.GetOldPathBytes()) == nonUTF8Filename {
+				oldPathFound = true
+				//lint:ignore SA1019 gitlab.com/gitlab-org/gitaly/issues/1746
+				require.Equal(t, rawChange.GetOldPath(), InvalidUTF8PathPlaceholder)
+			}
+
+			if string(rawChange.GetNewPathBytes()) == nonUTF8Filename {
+				newPathFound = true
+				//lint:ignore SA1019 gitlab.com/gitlab-org/gitaly/issues/1746
+				require.Equal(t, rawChange.GetNewPath(), InvalidUTF8PathPlaceholder)
+			}
+		}
+	}
+
+	require.True(t, newPathFound && oldPathFound)
+}
diff --git a/internal/testhelper/commit.go b/internal/testhelper/commit.go
index 81596352e..51aa9ca9e 100644
--- a/internal/testhelper/commit.go
+++ b/internal/testhelper/commit.go
@@ -6,6 +6,7 @@ import (
 	"os"
 	"os/exec"
 	"path"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -84,3 +85,13 @@ func CreateCommitInAlternateObjectDirectory(t *testing.T, repoPath, altObjectsDi
 
 	return currentHead[:len(currentHead)-1]
 }
+
+// CommitBlobWithName will create a commit for the specified blob with the
+// specified name. This enables testing situations where the filepath is not
+// possible due to filesystem constraints (e.g. non-UTF characters). The commit
+// ID is returned.
+func CommitBlobWithName(t *testing.T, testRepoPath, blobID, fileName, commitMessage string) string {
+	mktreeIn := strings.NewReader(fmt.Sprintf("100644 blob %s\t%s", blobID, fileName))
+	treeID := text.ChompBytes(MustRunCommand(t, mktreeIn, "git", "-C", testRepoPath, "mktree"))
+	return text.ChompBytes(MustRunCommand(t, nil, "git", "-C", testRepoPath, "commit-tree", treeID, "-m", commitMessage))
+}