From a44fd5e281d51195133c13f9605129d078c6bf91 Mon Sep 17 00:00:00 2001 From: "Jacob Vosmaer (GitLab)" Date: Fri, 6 Jul 2018 18:22:43 +0000 Subject: [PATCH 1/3] Remove unnecessary commit size calculations --- .../unreleased/fix-commit-size-nil-panic.yml | 5 +++++ internal/service/commit/commits_helper.go | 14 +++----------- internal/service/commit/server.go | 1 - 3 files changed, 8 insertions(+), 12 deletions(-) create mode 100644 changelogs/unreleased/fix-commit-size-nil-panic.yml diff --git a/changelogs/unreleased/fix-commit-size-nil-panic.yml b/changelogs/unreleased/fix-commit-size-nil-panic.yml new file mode 100644 index 00000000000..4b46a87bbf2 --- /dev/null +++ b/changelogs/unreleased/fix-commit-size-nil-panic.yml @@ -0,0 +1,5 @@ +--- +title: Remove unnecessary commit size calculations +merge_request: 791 +author: +type: fixed diff --git a/internal/service/commit/commits_helper.go b/internal/service/commit/commits_helper.go index e6771a9d830..ae1f0071fc9 100644 --- a/internal/service/commit/commits_helper.go +++ b/internal/service/commit/commits_helper.go @@ -14,6 +14,8 @@ type commitsSender interface { Send([]*pb.GitCommit) error } +const commitsPerChunk = 20 + func sendCommits(ctx context.Context, sender commitsSender, repo *pb.Repository, revisionRange []string, paths []string, extraArgs ...string) error { cmd, err := log.GitLogCommand(ctx, repo, revisionRange, paths, extraArgs...) if err != nil { @@ -26,18 +28,15 @@ func sendCommits(ctx context.Context, sender commitsSender, repo *pb.Repository, } var commits []*pb.GitCommit - commitsSize := 0 for logParser.Parse() { commit := logParser.Commit() - commitsSize += commitSize(commit) - if commitsSize >= maxMsgSize { + if len(commits) >= commitsPerChunk { if err := sender.Send(commits); err != nil { return err } commits = nil - commitsSize = 0 } commits = append(commits, commit) @@ -59,10 +58,3 @@ func sendCommits(ctx context.Context, sender commitsSender, repo *pb.Repository, return nil } - -func commitSize(commit *pb.GitCommit) int { - return len(commit.Id) + len(commit.Subject) + len(commit.Body) + - len(commit.Author.Name) + len(commit.Author.Email) + len(commit.Committer.Name) + len(commit.Committer.Email) + - 8 + 8 + // Author and Committer timestamps are int64 - len(commit.ParentIds)*40 -} diff --git a/internal/service/commit/server.go b/internal/service/commit/server.go index 7607d6f48a9..eb2f3046ddc 100644 --- a/internal/service/commit/server.go +++ b/internal/service/commit/server.go @@ -13,7 +13,6 @@ type server struct { var ( defaultBranchName = ref.DefaultBranchName - maxMsgSize = 1024 * 128 // 128 KiB ) // NewServer creates a new instance of a grpc CommitServiceServer -- GitLab From 9ca86cc4770659d7e5fe318adbf3ecb19fff603f Mon Sep 17 00:00:00 2001 From: "Jacob Vosmaer (GitLab)" Date: Wed, 11 Jul 2018 17:07:46 +0000 Subject: [PATCH 2/3] Fix nil commit author dereference --- .../unreleased/nil-committer-author.yml | 5 + internal/service/ref/util.go | 45 ++++--- internal/service/ref/util_test.go | 120 ++++++++++++++++++ 3 files changed, 149 insertions(+), 21 deletions(-) create mode 100644 changelogs/unreleased/nil-committer-author.yml create mode 100644 internal/service/ref/util_test.go diff --git a/changelogs/unreleased/nil-committer-author.yml b/changelogs/unreleased/nil-committer-author.yml new file mode 100644 index 00000000000..9d84a049e4b --- /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 562d5b27c88..6d328e61536 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 00000000000..f155ee7cc3f --- /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: ×tamp.Timestamp{Seconds: 1474987066}, + }, + Committer: &pb.CommitAuthor{ + Name: []byte("Job van der Voort"), + Email: []byte("job@gitlab.com"), + Date: ×tamp.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: ×tamp.Timestamp{Seconds: 1474987066}, + }, + CommitCommitter: &pb.FindLocalBranchCommitAuthor{ + Name: []byte("Job van der Voort"), + Email: []byte("job@gitlab.com"), + Date: ×tamp.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: ×tamp.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: ×tamp.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: ×tamp.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: ×tamp.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)) + }) + } +} -- GitLab From 493920d7ef2a0e9a5bceb2e84f6cae4e67479334 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 12 Jul 2018 13:45:36 +0200 Subject: [PATCH 3/3] Version 0.111.1 --- CHANGELOG.md | 8 ++++++++ VERSION | 2 +- changelogs/unreleased/fix-commit-size-nil-panic.yml | 5 ----- changelogs/unreleased/nil-committer-author.yml | 5 ----- 4 files changed, 9 insertions(+), 11 deletions(-) delete mode 100644 changelogs/unreleased/fix-commit-size-nil-panic.yml delete mode 100644 changelogs/unreleased/nil-committer-author.yml diff --git a/CHANGELOG.md b/CHANGELOG.md index c3c2a10fab1..06d740e4ad9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,13 @@ # Gitaly changelog +## v0.111.1 + +#### Fixed +- Fix nil commit author dereference + https://gitlab.com/gitlab-org/gitaly/merge_requests/800 +- Remove unnecessary commit size calculations + https://gitlab.com/gitlab-org/gitaly/merge_requests/791 + ## v0.111.0 #### Added diff --git a/VERSION b/VERSION index 5af665a17e0..3925f58df71 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.111.0 +0.111.1 diff --git a/changelogs/unreleased/fix-commit-size-nil-panic.yml b/changelogs/unreleased/fix-commit-size-nil-panic.yml deleted file mode 100644 index 4b46a87bbf2..00000000000 --- a/changelogs/unreleased/fix-commit-size-nil-panic.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Remove unnecessary commit size calculations -merge_request: 791 -author: -type: fixed diff --git a/changelogs/unreleased/nil-committer-author.yml b/changelogs/unreleased/nil-committer-author.yml deleted file mode 100644 index 9d84a049e4b..00000000000 --- a/changelogs/unreleased/nil-committer-author.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Fix nil commit author dereference -merge_request: 800 -author: -type: fixed -- GitLab