Fix commit message encoding and support alternates in CatFile

6 files changed, 206 insertions, 38 deletions

diff --git a/internal/git/alternates/alternates.go b/internal/git/alternates/alternates.go
new file mode 100644
index 000000000..5d2de1fc4
--- /dev/null
+++ b/internal/git/alternates/alternates.go
@@ -0,0 +1,37 @@
+package alternates
+
+import (
+	"fmt"
+	"path"
+	"strings"
+
+	pb "gitlab.com/gitlab-org/gitaly-proto/go"
+	"gitlab.com/gitlab-org/gitaly/internal/helper"
+)
+
+// PathAndEnv finds the disk path to a repository, and returns the
+// alternate object directory environment variables encoded in the
+// pb.Repository instance.
+func PathAndEnv(repo *pb.Repository) (string, []string, error) {
+	repoPath, err := helper.GetRepoPath(repo)
+	if err != nil {
+		return "", nil, err
+	}
+
+	var env []string
+	if dir := repo.GetGitObjectDirectory(); dir != "" {
+		env = append(env, fmt.Sprintf("GIT_OBJECT_DIRECTORY=%s", path.Join(repoPath, dir)))
+	}
+
+	if dirs := repo.GetGitAlternateObjectDirectories(); len(dirs) > 0 {
+		var dirsList []string
+
+		for _, dir := range dirs {
+			dirsList = append(dirsList, path.Join(repoPath, dir))
+		}
+
+		env = append(env, fmt.Sprintf("GIT_ALTERNATE_OBJECT_DIRECTORIES=%s", strings.Join(dirsList, ":")))
+	}
+
+	return repoPath, env, nil
+}
diff --git a/internal/git/catfile/catfile.go b/internal/git/catfile/catfile.go
index 144595eda..7a41acb80 100644
--- a/internal/git/catfile/catfile.go
+++ b/internal/git/catfile/catfile.go
@@ -9,7 +9,9 @@ import (
 	"strconv"
 	"strings"
 
+	pb "gitlab.com/gitlab-org/gitaly-proto/go"
 	"gitlab.com/gitlab-org/gitaly/internal/command"
+	"gitlab.com/gitlab-org/gitaly/internal/git/alternates"
 
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/codes"
@@ -29,10 +31,15 @@ type Handler func(io.Writer, *bufio.Reader) error
 // CatFile fetches the tree entries information using git cat-file. It
 // calls the handler with the TreeEntry slice, and an stdin reader and a stdout
 // writer in case the handler wants to perform addition cat-file operations.
-func CatFile(ctx context.Context, repoPath string, handler Handler) error {
+func CatFile(ctx context.Context, repo *pb.Repository, handler Handler) error {
+	repoPath, env, err := alternates.PathAndEnv(repo)
+	if err != nil {
+		return err
+	}
+
 	stdinReader, stdinWriter := io.Pipe()
 	cmdArgs := []string{"--git-dir", repoPath, "cat-file", "--batch"}
-	cmd, err := command.New(ctx, exec.Command(command.GitPath(), cmdArgs...), stdinReader, nil, nil)
+	cmd, err := command.New(ctx, exec.Command(command.GitPath(), cmdArgs...), stdinReader, nil, nil, env...)
 	if err != nil {
 		return grpc.Errorf(codes.Internal, "CatFile: cmd: %v", err)
 	}
diff --git a/internal/git/command.go b/internal/git/command.go
index 256bed223..22b23e7d5 100644
--- a/internal/git/command.go
+++ b/internal/git/command.go
@@ -2,39 +2,21 @@ package git
 
 import (
 	"context"
-	"fmt"
 	"os/exec"
-	"path"
-	"strings"
 
 	"gitlab.com/gitlab-org/gitaly/internal/command"
-	"gitlab.com/gitlab-org/gitaly/internal/helper"
+	"gitlab.com/gitlab-org/gitaly/internal/git/alternates"
 
 	pb "gitlab.com/gitlab-org/gitaly-proto/go"
 )
 
 // Command creates a git.Command with the given args
 func Command(ctx context.Context, repo *pb.Repository, args ...string) (*command.Command, error) {
-	repoPath, err := helper.GetRepoPath(repo)
+	repoPath, env, err := alternates.PathAndEnv(repo)
 	if err != nil {
 		return nil, err
 	}
-	args = append([]string{"--git-dir", repoPath}, args...)
-
-	var env []string
-	if dir := repo.GetGitObjectDirectory(); dir != "" {
-		env = append(env, fmt.Sprintf("GIT_OBJECT_DIRECTORY=%s", path.Join(repoPath, dir)))
-	}
-
-	if dirs := repo.GetGitAlternateObjectDirectories(); len(dirs) > 0 {
-		var dirsList []string
-
-		for _, dir := range dirs {
-			dirsList = append(dirsList, path.Join(repoPath, dir))
-		}
-
-		env = append(env, fmt.Sprintf("GIT_ALTERNATE_OBJECT_DIRECTORIES=%s", strings.Join(dirsList, ":")))
-	}
 
+	args = append([]string{"--git-dir", repoPath}, args...)
 	return command.New(ctx, exec.Command(command.GitPath(), args...), nil, nil, nil, env...)
 }
diff --git a/internal/git/log/commit.go b/internal/git/log/commit.go
index 95bcfa6fb..eea40cfc2 100644
--- a/internal/git/log/commit.go
+++ b/internal/git/log/commit.go
@@ -15,8 +15,6 @@ import (
 
 var commitLogFormatFields = []string{
 	"%H",  // commit hash
-	"%s",  // subject
-	"%B",  // raw body (subject + body)
 	"%an", // author name
 	"%ae", // author email
 	"%aI", // author date, strict ISO 8601 format
@@ -40,7 +38,11 @@ func GetCommit(ctx context.Context, repo *pb.Repository, revision string, path s
 		return nil, err
 	}
 
-	logParser := NewLogParser(cmd)
+	logParser, err := NewLogParser(ctx, repo, cmd)
+	if err != nil {
+		return nil, err
+	}
+
 	if ok := logParser.Parse(); !ok {
 		return nil, logParser.Err()
 	}
diff --git a/internal/git/log/commitmessage.go b/internal/git/log/commitmessage.go
new file mode 100644
index 000000000..36edac8b5
--- /dev/null
+++ b/internal/git/log/commitmessage.go
@@ -0,0 +1,126 @@
+package log
+
+import (
+	"bufio"
+	"context"
+	"fmt"
+	"io"
+	"io/ioutil"
+	"strings"
+
+	pb "gitlab.com/gitlab-org/gitaly-proto/go"
+	"gitlab.com/gitlab-org/gitaly/internal/git/catfile"
+)
+
+type commitMessageResponse struct {
+	subject string
+	body    string
+	err     error
+}
+
+type commitMessageRequest struct {
+	commitID string
+	response chan commitMessageResponse
+}
+
+type commitMessageHelper struct {
+	ctx        context.Context
+	requests   chan commitMessageRequest
+	catFileErr chan error
+}
+
+func newCommitMessageHelper(ctx context.Context, repo *pb.Repository) (*commitMessageHelper, error) {
+	cmh := &commitMessageHelper{
+		ctx:        ctx,
+		requests:   make(chan commitMessageRequest),
+		catFileErr: make(chan error),
+	}
+
+	go func() {
+		select {
+		case cmh.catFileErr <- catfile.CatFile(ctx, repo, cmh.handleCatFile):
+		case <-ctx.Done():
+			// This case is here to ensure this goroutine won't leak. We can't assume
+			// someone is listening on the cmh.catFileErr channel.
+		}
+
+		close(cmh.catFileErr)
+	}()
+
+	return cmh, nil
+}
+
+// commitMessage returns the raw binary subject and body for the given commitID.
+func (cmh *commitMessageHelper) commitMessage(commitID string) (string, string, error) {
+	response := make(chan commitMessageResponse)
+
+	select {
+	case cmh.requests <- commitMessageRequest{commitID: commitID, response: response}:
+		result := <-response
+		return result.subject, result.body, result.err
+	case err := <-cmh.catFileErr:
+		return "", "", fmt.Errorf("git cat-file is not running: %v", err)
+	}
+}
+
+// handleCatFile gets the raw commit message for a sequence of commit
+// ID's from a git-cat-file process.
+func (cmh *commitMessageHelper) handleCatFile(stdin io.Writer, stdout *bufio.Reader) error {
+	for {
+		select {
+		case messageRequest := <-cmh.requests:
+			subject, body, err := getCommitMessage(messageRequest.commitID, stdin, stdout)
+
+			// Always return a response, because a client is blocked waiting for it.
+			messageRequest.response <- commitMessageResponse{
+				subject: subject,
+				body:    body,
+				err:     err,
+			}
+
+			if err != nil {
+				// Shut down the current goroutine.
+				return err
+			}
+		case <-cmh.ctx.Done():
+			// We need this case because we cannot count on the client to close the
+			// requests channel.
+			return cmh.ctx.Err()
+		}
+	}
+}
+
+// getCommitMessage returns subject, body, error by querying git cat-file via stdin and stdout.
+func getCommitMessage(commitID string, stdin io.Writer, stdout *bufio.Reader) (string, string, error) {
+	if _, err := fmt.Fprintln(stdin, commitID); err != nil {
+		return "", "", err
+	}
+
+	objInfo, err := catfile.ParseObjectInfo(stdout)
+	if err != nil {
+		return "", "", err
+	}
+
+	if objInfo.Oid == "" || objInfo.Type != "commit" {
+		return "", "", fmt.Errorf("invalid ObjectInfo for %q: %v", commitID, objInfo)
+	}
+
+	rawCommit, err := ioutil.ReadAll(io.LimitReader(stdout, objInfo.Size))
+	if err != nil {
+		return "", "", err
+	}
+
+	if _, err := stdout.Discard(1); err != nil {
+		return "", "", fmt.Errorf("error discarding newline: %v", err)
+	}
+
+	commitString := string(rawCommit)
+
+	var body string
+	if split := strings.SplitN(commitString, "\n\n", 2); len(split) == 2 {
+		body = split[1]
+	}
+	subject := strings.TrimRight(strings.SplitN(body, "\n", 2)[0], "\r\n")
+
+	return subject, body, nil
+}
diff --git a/internal/git/log/log.go b/internal/git/log/log.go
index 32811fcd4..62efabfda 100644
--- a/internal/git/log/log.go
+++ b/internal/git/log/log.go
@@ -3,13 +3,13 @@ package log
 import (
 	"bufio"
 	"bytes"
+	"context"
 	"fmt"
 	"io"
 	"strings"
 
-	"gitlab.com/gitlab-org/gitaly/internal/git"
-
 	pb "gitlab.com/gitlab-org/gitaly-proto/go"
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 )
 
 // Parser holds necessary state for parsing a git log stream
@@ -18,16 +18,24 @@ type Parser struct {
 	currentCommit *pb.GitCommit
 	finished      bool
 	err           error
+	*commitMessageHelper
 }
 
 const fieldDelimiter = "\x1f"
 
 // NewLogParser returns a new Parser
-func NewLogParser(src io.Reader) *Parser {
-	parser := &Parser{}
-	parser.reader = bufio.NewReader(src)
+func NewLogParser(ctx context.Context, repo *pb.Repository, src io.Reader) (*Parser, error) {
+	cmh, err := newCommitMessageHelper(ctx, repo)
+	if err != nil {
+		return nil, err
+	}
+
+	parser := &Parser{
+		reader:              bufio.NewReader(src),
+		commitMessageHelper: cmh,
+	}
 
-	return parser
+	return parser, nil
 }
 
 // Parse parses a single git log line. It returns true if successful, false if it finished
@@ -54,19 +62,25 @@ func (parser *Parser) Parse() bool {
 	}
 
 	elements := bytes.Split(line, []byte(fieldDelimiter))
-	if len(elements) != 10 {
+	if len(elements) != len(commitLogFormatFields) {
 		parser.err = fmt.Errorf("error parsing ref: %q", line)
 		return false
 	}
 
+	subject, body, err := parser.commitMessage(string(elements[0]))
+	if err != nil {
+		parser.err = err
+		return false
+	}
+
 	var parentIds []string
-	if len(elements[9]) > 0 {
-		parentIds = strings.Split(string(elements[9]), " ")
+	if parentFieldIndex := len(commitLogFormatFields) - 1; len(elements[parentFieldIndex]) > 0 {
+		parentIds = strings.Split(string(elements[parentFieldIndex]), " ")
 	}
 
-	commit, err := git.NewCommit(elements[0], elements[1], elements[2],
-		elements[3], elements[4], elements[5], elements[6], elements[7],
-		elements[8], parentIds...)
+	commit, err := git.NewCommit(elements[0], []byte(subject), []byte(body),
+		elements[1], elements[2], elements[3], elements[4], elements[5],
+		elements[6], parentIds...)
 	if err != nil {
 		parser.err = err
 		return false