Add commit parser tests

2 files changed, 109 insertions, 21 deletions

diff --git a/internal/git/log/commit.go b/internal/git/log/commit.go
index 91aaa539e..5b5601bec 100644
--- a/internal/git/log/commit.go
+++ b/internal/git/log/commit.go
@@ -67,11 +67,12 @@ func GetCommitCatfile(c *catfile.Batch, revision string) (*pb.GitCommit, error)
 
 func parseRawCommit(raw []byte, info *catfile.ObjectInfo) (*pb.GitCommit, error) {
 	split := bytes.SplitN(raw, []byte("\n\n"), 2)
-	if len(split) != 2 {
-		return nil, fmt.Errorf("commit %q has no message", info.Oid)
-	}
 
-	header, body := split[0], split[1]
+	header := split[0]
+	var body []byte
+	if len(split) == 2 {
+		body = split[1]
+	}
 
 	commit := &pb.GitCommit{
 		Id:       info.Oid,
@@ -95,20 +96,13 @@ func parseRawCommit(raw []byte, info *catfile.ObjectInfo) (*pb.GitCommit, error)
 			continue
 		}
 
-		var err error
 		switch headerSplit[0] {
 		case "parent":
 			commit.ParentIds = append(commit.ParentIds, headerSplit[1])
 		case "author":
-			commit.Author, err = parseCommitAuthor(headerSplit[1])
-			if err != nil {
-				return nil, err
-			}
+			commit.Author = parseCommitAuthor(headerSplit[1])
 		case "committer":
-			commit.Committer, err = parseCommitAuthor(headerSplit[1])
-			if err != nil {
-				return nil, err
-			}
+			commit.Committer = parseCommitAuthor(headerSplit[1])
 		}
 	}
 	if err := scanner.Err(); err != nil {
@@ -120,32 +114,37 @@ func parseRawCommit(raw []byte, info *catfile.ObjectInfo) (*pb.GitCommit, error)
 
 const maxUnixCommitDate = 1 << 53
 
-func parseCommitAuthor(line string) (*pb.CommitAuthor, error) {
+func parseCommitAuthor(line string) *pb.CommitAuthor {
 	author := &pb.CommitAuthor{}
 
 	splitName := strings.SplitN(line, "<", 2)
+	author.Name = []byte(strings.TrimSuffix(splitName[0], " "))
+
 	if len(splitName) < 2 {
-		return nil, fmt.Errorf("missing '<' in %q", line)
+		return author
 	}
 
-	author.Name = []byte(strings.TrimSuffix(splitName[0], " "))
-
 	line = splitName[1]
 	splitEmail := strings.SplitN(line, ">", 2)
-	if len(splitName) < 2 {
-		return nil, fmt.Errorf("missing '>' in %q", line)
+	if len(splitEmail) < 2 {
+		return author
 	}
 
 	author.Email = []byte(splitEmail[0])
 
-	sec, err := strconv.ParseInt(strings.Fields(splitEmail[1])[0], 10, 64)
+	secSplit := strings.Fields(splitEmail[1])
+	if len(secSplit) < 1 {
+		return author
+	}
+
+	sec, err := strconv.ParseInt(secSplit[0], 10, 64)
 	if err != nil || sec > maxUnixCommitDate {
 		sec = git.FallbackTimeValue.Unix()
 	}
 
 	author.Date = &timestamp.Timestamp{Seconds: sec}
 
-	return author, nil
+	return author
 }
 
 func subjectFromBody(body []byte) []byte {
diff --git a/internal/git/log/commit_test.go b/internal/git/log/commit_test.go
new file mode 100644
index 000000000..7592116b5
--- /dev/null
+++ b/internal/git/log/commit_test.go
@@ -0,0 +1,89 @@
+package log
+
+import (
+	"testing"
+
+	"github.com/golang/protobuf/ptypes/timestamp"
+	"github.com/stretchr/testify/require"
+	pb "gitlab.com/gitlab-org/gitaly-proto/go"
+	"gitlab.com/gitlab-org/gitaly/internal/git/catfile"
+)
+
+func TestParseRawCommit(t *testing.T) {
+	info := &catfile.ObjectInfo{
+		Oid:  "a984dfa4dee018c6d5f5f57ffec0d0e22763df16",
+		Type: "commit",
+	}
+
+	// Valid-but-interesting commits should be test at the FindCommit level.
+	// Invalid objects (that Git would complain about during fsck) can be
+	// tested here.
+	//
+	// Once a repository contains a pathological object it can be hard to get
+	// rid of it. Because of this I think it's nicer to ignore such objects
+	// than to throw hard errors.
+	testCases := []struct {
+		desc string
+		in   []byte
+		out  *pb.GitCommit
+	}{
+		{
+			desc: "empty commit object",
+			in:   []byte{},
+			out:  &pb.GitCommit{Id: info.Oid},
+		},
+		{
+			desc: "no email",
+			in:   []byte("author Jane Doe"),
+			out: &pb.GitCommit{
+				Id:     info.Oid,
+				Author: &pb.CommitAuthor{Name: []byte("Jane Doe")},
+			},
+		},
+		{
+			desc: "unmatched <",
+			in:   []byte("author Jane Doe <janedoe@example.com"),
+			out: &pb.GitCommit{
+				Id:     info.Oid,
+				Author: &pb.CommitAuthor{Name: []byte("Jane Doe")},
+			},
+		},
+		{
+			desc: "unmatched >",
+			in:   []byte("author Jane Doe janedoe@example.com>"),
+			out: &pb.GitCommit{
+				Id:     info.Oid,
+				Author: &pb.CommitAuthor{Name: []byte("Jane Doe janedoe@example.com>")},
+			},
+		},
+		{
+			desc: "missing date",
+			in:   []byte("author Jane Doe <janedoe@example.com> "),
+			out: &pb.GitCommit{
+				Id:     info.Oid,
+				Author: &pb.CommitAuthor{Name: []byte("Jane Doe"), Email: []byte("janedoe@example.com")},
+			},
+		},
+		{
+			desc: "date too high ",
+			in:   []byte("author Jane Doe <janedoe@example.com> 9007199254740993 +0200"),
+			out: &pb.GitCommit{
+				Id: info.Oid,
+				Author: &pb.CommitAuthor{
+					Name:  []byte("Jane Doe"),
+					Email: []byte("janedoe@example.com"),
+					Date:  &timestamp.Timestamp{Seconds: 9223371974719179007},
+				},
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			info.Size = int64(len(tc.in))
+			out, err := parseRawCommit(tc.in, info)
+			require.NoError(t, err, "parse error")
+			require.Equal(t, *tc.out, *out)
+		})
+	}
+}