Merge branch 'jc-list-last-commits-for-tree-fix' into 'master'

Return path data in ListLastCommitsForTree

See merge request gitlab-org/gitaly!1168

4 files changed, 82 insertions, 7 deletions

diff --git a/internal/service/commit/list_last_commits_for_tree.go b/internal/service/commit/list_last_commits_for_tree.go
index 60d9c0952..fa906fa63 100644
--- a/internal/service/commit/list_last_commits_for_tree.go
+++ b/internal/service/commit/list_last_commits_for_tree.go
@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"io"
 	"sort"
+	"unicode/utf8"
 
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/internal/command"
@@ -14,6 +15,13 @@ import (
 	"google.golang.org/grpc/status"
 )
 
+const (
+	// InvalidUTF8PathPlaceholder is a placeholder we return in the Path field since
+	// returning non utf8 data will result in a marshalling error
+	// Once we deprecate the Path field, we can remove this
+	InvalidUTF8PathPlaceholder = "ENCODING ERROR gitaly#1547"
+)
+
 var (
 	maxNumStatBatchSize = 10
 )
@@ -56,8 +64,14 @@ func (s *server) ListLastCommitsForTree(in *gitalypb.ListLastCommitsForTreeReque
 		}
 
 		commitForTree := &gitalypb.ListLastCommitsForTreeResponse_CommitForTree{
-			Path:   entry.Path,
-			Commit: commit,
+			Path:      entry.Path,
+			PathBytes: []byte(entry.Path),
+			Commit:    commit,
+		}
+
+		// for non-utf8 encoded paths, return placeholder until we can deprecate Path
+		if !utf8.ValidString(entry.Path) {
+			commitForTree.Path = InvalidUTF8PathPlaceholder
 		}
 
 		batch = append(batch, commitForTree)
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 d3d2cd3e0..9d2ac116c 100644
--- a/internal/service/commit/list_last_commits_for_tree_test.go
+++ b/internal/service/commit/list_last_commits_for_tree_test.go
@@ -1,12 +1,18 @@
 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"
 )
@@ -196,7 +202,8 @@ func TestSuccessfulListLastCommitsForTreeRequest(t *testing.T) {
 				for _, fetchedCommit := range commits {
 					expectedInfo := testCase.info[counter]
 
-					require.Equal(t, string(expectedInfo.path), string(fetchedCommit.Path))
+					require.Equal(t, string(expectedInfo.path), fetchedCommit.Path)
+					require.Equal(t, expectedInfo.path, fetchedCommit.PathBytes)
 					require.Equal(t, expectedInfo.id, fetchedCommit.Commit.Id)
 
 					counter++
@@ -291,3 +298,58 @@ func TestFailedListLastCommitsForTreeRequest(t *testing.T) {
 		})
 	}
 }
+
+func TestNonUtf8ListLastCommitsForTreeRequest(t *testing.T) {
+	server, serverSockerPath := startTestServices(t)
+	defer server.Stop()
+
+	client, conn := newCommitServiceClient(t, serverSockerPath)
+	defer conn.Close()
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
+	defer cleanupFn()
+
+	// This is an arbitrary blob known to exist in the test repository
+	const blobID = "c60514b6d3d6bf4bec1030f70026e34dfbd69ad5"
+
+	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"))
+
+	request := &gitalypb.ListLastCommitsForTreeRequest{
+		Repository: testRepo,
+		Revision:   string(commitID),
+		Limit:      100,
+		Offset:     0,
+	}
+
+	stream, err := client.ListLastCommitsForTree(ctx, request)
+	require.NoError(t, err)
+
+	var nonUTF8FilenameFound bool
+	for {
+		fetchedCommits, err := stream.Recv()
+		if err == io.EOF {
+			break
+		}
+
+		require.NoError(t, err)
+
+		commits := fetchedCommits.GetCommits()
+
+		for _, fetchedCommit := range commits {
+			if bytes.Equal(fetchedCommit.PathBytes, []byte(nonUTF8Filename)) {
+				nonUTF8FilenameFound = true
+			}
+			assert.Equal(t, InvalidUTF8PathPlaceholder, fetchedCommit.Path)
+		}
+	}
+
+	assert.True(t, nonUTF8FilenameFound)
+}
diff --git a/internal/service/operations/submodules_test.go b/internal/service/operations/submodules_test.go
index 57d32cbed..ec1b1ba5b 100644
--- a/internal/service/operations/submodules_test.go
+++ b/internal/service/operations/submodules_test.go
@@ -74,8 +74,8 @@ func TestSuccessfulUserUpdateSubmoduleRequest(t *testing.T) {
 			parser := lstree.NewParser(bytes.NewReader(entry))
 			parsedEntry, err := parser.NextEntry()
 			require.NoError(t, err)
-			require.Equal(t, parsedEntry.Path, testCase.submodule)
-			require.Equal(t, parsedEntry.Oid, testCase.commitSha)
+			require.Equal(t, testCase.submodule, string(parsedEntry.Path))
+			require.Equal(t, testCase.commitSha, parsedEntry.Oid)
 		})
 	}
 }
diff --git a/internal/service/ref/pack_refs.go b/internal/service/ref/pack_refs.go
index 224d0411c..66de8ca32 100644
--- a/internal/service/ref/pack_refs.go
+++ b/internal/service/ref/pack_refs.go
@@ -5,11 +5,10 @@ import (
 	"errors"
 	"fmt"
 
+	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/repository"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
-
-	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
 )
 
 func (server) PackRefs(ctx context.Context, in *gitalypb.PackRefsRequest) (*gitalypb.PackRefsResponse, error) {