Merge branch 'commitstats-go' into 'master'

Rewrite CommitStats in Go

Closes #1467

See merge request gitlab-org/gitaly!1048

4 files changed, 103 insertions, 15 deletions

diff --git a/changelogs/unreleased/commitstats-go.yml b/changelogs/unreleased/commitstats-go.yml
new file mode 100644
index 000000000..70e97d691
--- /dev/null
+++ b/changelogs/unreleased/commitstats-go.yml
@@ -0,0 +1,5 @@
+---
+title: Rewrite CommitStats in Go
+merge_request: 1048
+author:
+type: performance
diff --git a/internal/service/commit/stats.go b/internal/service/commit/stats.go
index b1b9628f1..c9b32b171 100644
--- a/internal/service/commit/stats.go
+++ b/internal/service/commit/stats.go
@@ -1,28 +1,85 @@
 package commit
 
 import (
+	"bufio"
 	"context"
+	"fmt"
+	"strconv"
+	"strings"
 
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
+	"gitlab.com/gitlab-org/gitaly/internal/git"
+	"gitlab.com/gitlab-org/gitaly/internal/git/log"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
-	"gitlab.com/gitlab-org/gitaly/internal/rubyserver"
 )
 
 func (s *server) CommitStats(ctx context.Context, in *gitalypb.CommitStatsRequest) (*gitalypb.CommitStatsResponse, error) {
-	client, err := s.CommitServiceClient(ctx)
+	if err := git.ValidateRevision(in.Revision); err != nil {
+		return nil, helper.ErrInvalidArgument(err)
+	}
+
+	resp, err := commitStats(ctx, in)
 	if err != nil {
-		return nil, err
+		return nil, helper.ErrInternal(err)
 	}
 
-	repo := in.GetRepository()
-	if _, err := helper.GetRepoPath(repo); err != nil {
+	return resp, nil
+}
+
+func commitStats(ctx context.Context, in *gitalypb.CommitStatsRequest) (*gitalypb.CommitStatsResponse, error) {
+	commit, err := log.GetCommit(ctx, in.Repository, string(in.Revision))
+	if err != nil {
 		return nil, err
 	}
+	if commit == nil {
+		return nil, fmt.Errorf("commit not found: %q", in.Revision)
+	}
 
-	clientCtx, err := rubyserver.SetHeaders(ctx, repo)
+	cmd, err := git.Command(ctx, in.Repository, "diff", "--numstat", commit.Id+"^", commit.Id)
 	if err != nil {
 		return nil, err
 	}
 
-	return client.CommitStats(clientCtx, in)
+	scanner := bufio.NewScanner(cmd)
+	var added, deleted int32
+
+	for scanner.Scan() {
+		split := strings.SplitN(scanner.Text(), "\t", 3)
+		if len(split) != 3 {
+			return nil, fmt.Errorf("invalid numstat line %q", scanner.Text())
+		}
+
+		if split[0] == "-" && split[1] == "-" {
+			// binary file
+			continue
+		}
+
+		add64, err := strconv.ParseInt(split[0], 10, 32)
+		if err != nil {
+			return nil, err
+		}
+
+		added += int32(add64)
+
+		del64, err := strconv.ParseInt(split[1], 10, 32)
+		if err != nil {
+			return nil, err
+		}
+
+		deleted += int32(del64)
+	}
+
+	if err := scanner.Err(); err != nil {
+		return nil, err
+	}
+
+	if err := cmd.Wait(); err != nil {
+		return nil, err
+	}
+
+	return &gitalypb.CommitStatsResponse{
+		Oid:       commit.Id,
+		Additions: added,
+		Deletions: deleted,
+	}, nil
 }
diff --git a/internal/service/commit/stats_test.go b/internal/service/commit/stats_test.go
index f9c7e847a..53af68217 100644
--- a/internal/service/commit/stats_test.go
+++ b/internal/service/commit/stats_test.go
@@ -5,6 +5,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"google.golang.org/grpc/codes"
@@ -24,30 +25,53 @@ func TestCommitStatsSuccess(t *testing.T) {
 	defer cleanupFn()
 
 	tests := []struct {
-		revision             []byte
+		desc                 string
+		revision             string
 		oid                  string
 		additions, deletions int32
 	}{
 		{
-			revision:  []byte("test-do-not-touch"),
+			desc:      "multiple changes, multiple files",
+			revision:  "test-do-not-touch",
 			oid:       "899d3d27b04690ac1cd9ef4d8a74fde0667c57f1",
 			additions: 27,
 			deletions: 59,
 		},
 		{
-			revision:  []byte("899d3d27b04690ac1cd9ef4d8a74fde0667c57f1"),
+			desc:      "multiple changes, multiple files, reference by commit ID",
+			revision:  "899d3d27b04690ac1cd9ef4d8a74fde0667c57f1",
 			oid:       "899d3d27b04690ac1cd9ef4d8a74fde0667c57f1",
 			additions: 27,
 			deletions: 59,
 		},
+		{
+			desc:      "merge commit",
+			revision:  "60ecb67",
+			oid:       "60ecb67744cb56576c30214ff52294f8ce2def98",
+			additions: 1,
+			deletions: 0,
+		},
+		{
+			desc:      "binary file",
+			revision:  "ae73cb0",
+			oid:       "ae73cb07c9eeaf35924a10f713b364d32b2dd34f",
+			additions: 0,
+			deletions: 0,
+		},
 	}
 
 	for _, tc := range tests {
-		resp, err := client.CommitStats(ctx, &gitalypb.CommitStatsRequest{Repository: testRepo, Revision: tc.revision})
-		assert.NoError(t, err)
-		assert.Equal(t, tc.oid, resp.GetOid())
-		assert.Equal(t, tc.additions, resp.GetAdditions())
-		assert.Equal(t, tc.deletions, resp.GetDeletions())
+		t.Run(tc.desc, func(t *testing.T) {
+			resp, err := client.CommitStats(ctx, &gitalypb.CommitStatsRequest{
+				Repository: testRepo,
+				Revision:   []byte(tc.revision),
+			})
+			require.NoError(t, err)
+
+			assert.Equal(t, tc.oid, resp.GetOid())
+			assert.Equal(t, tc.additions, resp.GetAdditions())
+			assert.Equal(t, tc.deletions, resp.GetDeletions())
+		})
 	}
 }
 
diff --git a/ruby/lib/gitaly_server/commit_service.rb b/ruby/lib/gitaly_server/commit_service.rb
index 0abf22f50..e4916ec70 100644
--- a/ruby/lib/gitaly_server/commit_service.rb
+++ b/ruby/lib/gitaly_server/commit_service.rb
@@ -3,6 +3,8 @@ module GitalyServer
     include Utils
     include Gitlab::EncodingHelper
 
+    # TODO remove in gitlab 12.0, this is implemented in Go now:
+    # https://gitlab.com/gitlab-org/gitaly/issues/1471
     def commit_stats(request, call)
       repo = Gitlab::Git::Repository.from_gitaly(request.repository, call)
       revision = request.revision unless request.revision.empty?