Merge branch 'nil-committer-author' into 'master'

Fix nil commit author dereference

See merge request gitlab-org/gitaly!800

3 files changed, 149 insertions, 21 deletions

diff --git a/changelogs/unreleased/nil-committer-author.yml b/changelogs/unreleased/nil-committer-author.yml
new file mode 100644
index 000000000..9d84a049e
--- /dev/null
+++ b/changelogs/unreleased/nil-committer-author.yml
@@ -0,0 +1,5 @@
+---
+title: Fix nil commit author dereference
+merge_request: 800
+author:
+type: fixed
diff --git a/internal/service/ref/util.go b/internal/service/ref/util.go
index 562d5b27c..6d328e615 100644
--- a/internal/service/ref/util.go
+++ b/internal/service/ref/util.go
@@ -21,29 +21,30 @@ func parseRef(ref []byte) ([][]byte, error) {
 	return elements, nil
 }
 
-func buildLocalBranch(c *catfile.Batch, elements [][]byte) (*pb.FindLocalBranchResponse, error) {
-	target, err := log.GetCommitCatfile(c, string(elements[1]))
-	if err != nil {
-		return nil, err
+func buildLocalBranch(name []byte, target *pb.GitCommit) *pb.FindLocalBranchResponse {
+	response := &pb.FindLocalBranchResponse{
+		Name:          name,
+		CommitId:      target.Id,
+		CommitSubject: target.Subject,
 	}
-	author := pb.FindLocalBranchCommitAuthor{
-		Name:  target.Author.Name,
-		Email: target.Author.Email,
-		Date:  target.Author.Date,
+
+	if author := target.Author; author != nil {
+		response.CommitAuthor = &pb.FindLocalBranchCommitAuthor{
+			Name:  author.Name,
+			Email: author.Email,
+			Date:  author.Date,
+		}
 	}
-	committer := pb.FindLocalBranchCommitAuthor{
-		Name:  target.Committer.Name,
-		Email: target.Committer.Email,
-		Date:  target.Committer.Date,
+
+	if committer := target.Committer; committer != nil {
+		response.CommitCommitter = &pb.FindLocalBranchCommitAuthor{
+			Name:  committer.Name,
+			Email: committer.Email,
+			Date:  committer.Date,
+		}
 	}
 
-	return &pb.FindLocalBranchResponse{
-		Name:            elements[0],
-		CommitId:        target.Id,
-		CommitSubject:   target.Subject,
-		CommitAuthor:    &author,
-		CommitCommitter: &committer,
-	}, nil
+	return response
 }
 
 func buildBranch(c *catfile.Batch, elements [][]byte) (*pb.FindAllBranchesResponse_Branch, error) {
@@ -79,11 +80,13 @@ func newFindLocalBranchesWriter(stream pb.Ref_FindLocalBranchesServer, c *catfil
 			if err != nil {
 				return err
 			}
-			branch, err := buildLocalBranch(c, elements)
+
+			target, err := log.GetCommitCatfile(c, string(elements[1]))
 			if err != nil {
 				return err
 			}
-			branches = append(branches, branch)
+
+			branches = append(branches, buildLocalBranch(elements[0], target))
 		}
 		return stream.Send(&pb.FindLocalBranchesResponse{Branches: branches})
 	}
diff --git a/internal/service/ref/util_test.go b/internal/service/ref/util_test.go
new file mode 100644
index 000000000..f155ee7cc
--- /dev/null
+++ b/internal/service/ref/util_test.go
@@ -0,0 +1,120 @@
+package ref
+
+import (
+	"testing"
+
+	"github.com/golang/protobuf/ptypes/timestamp"
+	"github.com/stretchr/testify/require"
+	pb "gitlab.com/gitlab-org/gitaly-proto/go"
+)
+
+func TestBuildLocalBranch(t *testing.T) {
+	testCases := []struct {
+		desc string
+		in   *pb.GitCommit
+		out  *pb.FindLocalBranchResponse
+	}{
+		{
+			desc: "all required fields present",
+			in: &pb.GitCommit{
+				Id:      "b83d6e391c22777fca1ed3012fce84f633d7fed0",
+				Subject: []byte("Merge branch 'branch-merged' into 'master'"),
+				Body:    []byte("Merge branch 'branch-merged' into 'master'\r\n\r\nadds bar folder and branch-test text file to check Repository merged_to_root_ref method\r\n\r\n\r\n\r\nSee merge request !12"),
+				Author: &pb.CommitAuthor{
+					Name:  []byte("Job van der Voort"),
+					Email: []byte("job@gitlab.com"),
+					Date:  &timestamp.Timestamp{Seconds: 1474987066},
+				},
+				Committer: &pb.CommitAuthor{
+					Name:  []byte("Job van der Voort"),
+					Email: []byte("job@gitlab.com"),
+					Date:  &timestamp.Timestamp{Seconds: 1474987066},
+				},
+				ParentIds: []string{
+					"1b12f15a11fc6e62177bef08f47bc7b5ce50b141",
+					"498214de67004b1da3d820901307bed2a68a8ef6",
+				},
+				BodySize: 162,
+			},
+			out: &pb.FindLocalBranchResponse{
+				Name:          []byte("my-branch"),
+				CommitId:      "b83d6e391c22777fca1ed3012fce84f633d7fed0",
+				CommitSubject: []byte("Merge branch 'branch-merged' into 'master'"),
+				CommitAuthor: &pb.FindLocalBranchCommitAuthor{
+					Name:  []byte("Job van der Voort"),
+					Email: []byte("job@gitlab.com"),
+					Date:  &timestamp.Timestamp{Seconds: 1474987066},
+				},
+				CommitCommitter: &pb.FindLocalBranchCommitAuthor{
+					Name:  []byte("Job van der Voort"),
+					Email: []byte("job@gitlab.com"),
+					Date:  &timestamp.Timestamp{Seconds: 1474987066},
+				},
+			},
+		},
+		{
+			desc: "missing author",
+			in: &pb.GitCommit{
+				Id:      "b83d6e391c22777fca1ed3012fce84f633d7fed0",
+				Subject: []byte("Merge branch 'branch-merged' into 'master'"),
+				Body:    []byte("Merge branch 'branch-merged' into 'master'\r\n\r\nadds bar folder and branch-test text file to check Repository merged_to_root_ref method\r\n\r\n\r\n\r\nSee merge request !12"),
+				Committer: &pb.CommitAuthor{
+					Name:  []byte("Job van der Voort"),
+					Email: []byte("job@gitlab.com"),
+					Date:  &timestamp.Timestamp{Seconds: 1474987066},
+				},
+				ParentIds: []string{
+					"1b12f15a11fc6e62177bef08f47bc7b5ce50b141",
+					"498214de67004b1da3d820901307bed2a68a8ef6",
+				},
+				BodySize: 162,
+			},
+			out: &pb.FindLocalBranchResponse{
+				Name:          []byte("my-branch"),
+				CommitId:      "b83d6e391c22777fca1ed3012fce84f633d7fed0",
+				CommitSubject: []byte("Merge branch 'branch-merged' into 'master'"),
+				CommitAuthor:  nil,
+				CommitCommitter: &pb.FindLocalBranchCommitAuthor{
+					Name:  []byte("Job van der Voort"),
+					Email: []byte("job@gitlab.com"),
+					Date:  &timestamp.Timestamp{Seconds: 1474987066},
+				},
+			},
+		},
+		{
+			desc: "missing committer",
+			in: &pb.GitCommit{
+				Id:      "b83d6e391c22777fca1ed3012fce84f633d7fed0",
+				Subject: []byte("Merge branch 'branch-merged' into 'master'"),
+				Body:    []byte("Merge branch 'branch-merged' into 'master'\r\n\r\nadds bar folder and branch-test text file to check Repository merged_to_root_ref method\r\n\r\n\r\n\r\nSee merge request !12"),
+				Author: &pb.CommitAuthor{
+					Name:  []byte("Job van der Voort"),
+					Email: []byte("job@gitlab.com"),
+					Date:  &timestamp.Timestamp{Seconds: 1474987066},
+				},
+				ParentIds: []string{
+					"1b12f15a11fc6e62177bef08f47bc7b5ce50b141",
+					"498214de67004b1da3d820901307bed2a68a8ef6",
+				},
+				BodySize: 162,
+			},
+			out: &pb.FindLocalBranchResponse{
+				Name:          []byte("my-branch"),
+				CommitId:      "b83d6e391c22777fca1ed3012fce84f633d7fed0",
+				CommitSubject: []byte("Merge branch 'branch-merged' into 'master'"),
+				CommitAuthor: &pb.FindLocalBranchCommitAuthor{
+					Name:  []byte("Job van der Voort"),
+					Email: []byte("job@gitlab.com"),
+					Date:  &timestamp.Timestamp{Seconds: 1474987066},
+				},
+				CommitCommitter: nil,
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			require.Equal(t, *tc.out, *buildLocalBranch([]byte("my-branch"), tc.in))
+		})
+	}
+}