diff options
author | Paul Okstad <pokstad@gitlab.com> | 2019-06-24 15:52:52 +0300 |
---|---|---|
committer | Jacob Vosmaer <jacob@gitlab.com> | 2019-06-24 15:52:52 +0300 |
commit | 18bf5ceacb0a361f056628dd1f76bfe3013c7d37 (patch) | |
tree | 1c562c08f7864f6d786ed832d2d2b051eefb95ba | |
parent | 6df618362e4019b841023c02bfbf5a8b86bd9f5a (diff) |
GetRawChanges RPC uses both string and byte path fields
-rw-r--r-- | changelogs/unreleased/po_raw_changes_encoding.yml | 5 | ||||
-rw-r--r-- | internal/service/commit/list_last_commits_for_tree_test.go | 13 | ||||
-rw-r--r-- | internal/service/repository/raw_changes.go | 26 | ||||
-rw-r--r-- | internal/service/repository/raw_changes_test.go | 165 | ||||
-rw-r--r-- | internal/testhelper/commit.go | 11 |
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)) +} |