From 71818d640829c6a13508c4e1cb1d347cc1196c9b Mon Sep 17 00:00:00 2001 From: John Cai Date: Wed, 19 Dec 2018 12:27:44 -0800 Subject: [PATCH 01/10] adding a GetCommitMessage method by splitting the helper method parseRawCommit into sub-helper functions --- internal/git/log/commit.go | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/internal/git/log/commit.go b/internal/git/log/commit.go index 0ded22414d9..d1da9836d71 100644 --- a/internal/git/log/commit.go +++ b/internal/git/log/commit.go @@ -50,7 +50,40 @@ func GetCommitCatfile(c *catfile.Batch, revision string) (*gitalypb.GitCommit, e return parseRawCommit(raw, info) } +// GetCommitMessage looks up a commit message and returns it in its entirety. +func GetCommitMessage(ctx context.Context, repo *gitalypb.Repository, revision string) ([]byte, error) { + c, err := catfile.New(ctx, repo) + if err != nil { + return nil, err + } + info, err := c.Info(revision + "^{commit}") + if err != nil { + if catfile.IsNotFound(err) { + return nil, nil + } + + return nil, err + } + + r, err := c.Commit(info.Oid) + if err != nil { + return nil, err + } + + raw, err := ioutil.ReadAll(r) + if err != nil { + return nil, err + } + _, body := splitRawCommit(raw) + return body, nil +} + func parseRawCommit(raw []byte, info *catfile.ObjectInfo) (*gitalypb.GitCommit, error) { + header, body := splitRawCommit(raw) + return buildCommit(header, body, info) +} + +func splitRawCommit(raw []byte) ([]byte, []byte) { split := bytes.SplitN(raw, []byte("\n\n"), 2) header := split[0] @@ -58,7 +91,10 @@ func parseRawCommit(raw []byte, info *catfile.ObjectInfo) (*gitalypb.GitCommit, if len(split) == 2 { body = split[1] } + return header, body +} +func buildCommit(header, body []byte, info *catfile.ObjectInfo) (*gitalypb.GitCommit, error) { commit := &gitalypb.GitCommit{ Id: info.Oid, Body: body, -- GitLab From 7745022f971ef37646518c1970a564272f2233d0 Mon Sep 17 00:00:00 2001 From: John Cai Date: Wed, 19 Dec 2018 12:27:56 -0800 Subject: [PATCH 02/10] use pure go implementation of get commit message --- internal/service/commit/commit_messages.go | 32 +++++----------------- 1 file changed, 7 insertions(+), 25 deletions(-) diff --git a/internal/service/commit/commit_messages.go b/internal/service/commit/commit_messages.go index 7d041ae941a..e82f31d830d 100644 --- a/internal/service/commit/commit_messages.go +++ b/internal/service/commit/commit_messages.go @@ -4,7 +4,7 @@ import ( "fmt" "gitlab.com/gitlab-org/gitaly-proto/go/gitalypb" - "gitlab.com/gitlab-org/gitaly/internal/rubyserver" + "gitlab.com/gitlab-org/gitaly/internal/git/log" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" ) @@ -13,33 +13,15 @@ func (s *server) GetCommitMessages(request *gitalypb.GetCommitMessagesRequest, s if err := validateGetCommitMessagesRequest(request); err != nil { return status.Errorf(codes.InvalidArgument, "GetCommitMessages: %v", err) } - ctx := stream.Context() - - client, err := s.CommitServiceClient(ctx) - if err != nil { - return err - } - - clientCtx, err := rubyserver.SetHeaders(ctx, request.GetRepository()) - if err != nil { - return err - } - - rubyStream, err := client.GetCommitMessages(clientCtx, request) - if err != nil { - return err - } - - return rubyserver.Proxy(func() error { - resp, err := rubyStream.Recv() + for _, commitID := range request.GetCommitIds() { + msg, err := log.GetCommitMessage(ctx, request.GetRepository(), commitID) if err != nil { - md := rubyStream.Trailer() - stream.SetTrailer(md) - return err + return status.Errorf(codes.Internal, "failed to get commit message: %v", err) } - return stream.Send(resp) - }) + stream.Send(&gitalypb.GetCommitMessagesResponse{CommitId: commitID, Message: msg}) + } + return nil } func validateGetCommitMessagesRequest(request *gitalypb.GetCommitMessagesRequest) error { -- GitLab From 985f22723a7075f35eae9769e092fdb72728242a Mon Sep 17 00:00:00 2001 From: John Cai Date: Fri, 21 Dec 2018 11:27:01 -0800 Subject: [PATCH 03/10] use readers and chunk the message --- internal/git/log/commit.go | 59 ++++++++++++---------- internal/git/log/commit_test.go | 18 ++++--- internal/service/commit/commit_messages.go | 19 ++++++- 3 files changed, 58 insertions(+), 38 deletions(-) diff --git a/internal/git/log/commit.go b/internal/git/log/commit.go index d1da9836d71..9253a7e1f66 100644 --- a/internal/git/log/commit.go +++ b/internal/git/log/commit.go @@ -4,6 +4,7 @@ import ( "bufio" "bytes" "context" + "io" "io/ioutil" "strconv" "strings" @@ -42,16 +43,11 @@ func GetCommitCatfile(c *catfile.Batch, revision string) (*gitalypb.GitCommit, e return nil, err } - raw, err := ioutil.ReadAll(r) - if err != nil { - return nil, err - } - - return parseRawCommit(raw, info) + return parseRawCommit(r, info) } // GetCommitMessage looks up a commit message and returns it in its entirety. -func GetCommitMessage(ctx context.Context, repo *gitalypb.Repository, revision string) ([]byte, error) { +func GetCommitMessage(ctx context.Context, repo *gitalypb.Repository, revision string) (io.Reader, error) { c, err := catfile.New(ctx, repo) if err != nil { return nil, err @@ -70,42 +66,49 @@ func GetCommitMessage(ctx context.Context, repo *gitalypb.Repository, revision s return nil, err } - raw, err := ioutil.ReadAll(r) - if err != nil { - return nil, err - } - _, body := splitRawCommit(raw) + _, body := splitRawCommit(r) return body, nil } -func parseRawCommit(raw []byte, info *catfile.ObjectInfo) (*gitalypb.GitCommit, error) { - header, body := splitRawCommit(raw) +func parseRawCommit(r io.Reader, info *catfile.ObjectInfo) (*gitalypb.GitCommit, error) { + header, body := splitRawCommit(r) return buildCommit(header, body, info) } -func splitRawCommit(raw []byte) ([]byte, []byte) { - split := bytes.SplitN(raw, []byte("\n\n"), 2) - - header := split[0] - var body []byte - if len(split) == 2 { - body = split[1] +func splitRawCommit(r io.Reader) (io.Reader, io.Reader) { + var header bytes.Buffer + bufReader := bufio.NewReader(r) + for b, err := bufReader.ReadBytes('\n'); err == nil; { + if string(b) == "\n" { + break + } + header.Write(b) + b, err = bufReader.ReadBytes('\n') } - return header, body + return &header, bufReader } -func buildCommit(header, body []byte, info *catfile.ObjectInfo) (*gitalypb.GitCommit, error) { +func buildCommit(header, body io.Reader, info *catfile.ObjectInfo) (*gitalypb.GitCommit, error) { + bodyBytes, err := ioutil.ReadAll(body) + if err != nil { + return &gitalypb.GitCommit{}, err + } + if len(bodyBytes) == 0 { + bodyBytes = nil + } + commit := &gitalypb.GitCommit{ Id: info.Oid, - Body: body, - Subject: subjectFromBody(body), - BodySize: int64(len(body)), + BodySize: int64(len(bodyBytes)), + Body: bodyBytes, + Subject: subjectFromBody(bodyBytes), } - if max := helper.MaxCommitOrTagMessageSize; len(commit.Body) > max { + + if max := helper.MaxCommitOrTagMessageSize; len(bodyBytes) > max { commit.Body = commit.Body[:max] } - scanner := bufio.NewScanner(bytes.NewReader(header)) + scanner := bufio.NewScanner(header) for scanner.Scan() { line := scanner.Text() if len(line) == 0 || line[0] == ' ' { diff --git a/internal/git/log/commit_test.go b/internal/git/log/commit_test.go index b3144aceddc..3a756b6a244 100644 --- a/internal/git/log/commit_test.go +++ b/internal/git/log/commit_test.go @@ -1,6 +1,7 @@ package log import ( + "bytes" "testing" "github.com/golang/protobuf/ptypes/timestamp" @@ -34,7 +35,7 @@ func TestParseRawCommit(t *testing.T) { }, { desc: "no email", - in: []byte("author Jane Doe"), + in: []byte("author Jane Doe\n"), out: &gitalypb.GitCommit{ Id: info.Oid, Author: &gitalypb.CommitAuthor{Name: []byte("Jane Doe")}, @@ -42,7 +43,7 @@ func TestParseRawCommit(t *testing.T) { }, { desc: "unmatched <", - in: []byte("author Jane Doe ", - in: []byte("author Jane Doe janedoe@example.com>"), + in: []byte("author Jane Doe janedoe@example.com>\n"), out: &gitalypb.GitCommit{ Id: info.Oid, Author: &gitalypb.CommitAuthor{Name: []byte("Jane Doe janedoe@example.com>")}, @@ -58,7 +59,7 @@ func TestParseRawCommit(t *testing.T) { }, { desc: "missing date", - in: []byte("author Jane Doe "), + in: []byte("author Jane Doe \n"), out: &gitalypb.GitCommit{ Id: info.Oid, Author: &gitalypb.CommitAuthor{Name: []byte("Jane Doe"), Email: []byte("janedoe@example.com")}, @@ -66,7 +67,7 @@ func TestParseRawCommit(t *testing.T) { }, { desc: "date too high", - in: []byte("author Jane Doe 9007199254740993 +0200"), + in: []byte("author Jane Doe 9007199254740993 +0200\n"), out: &gitalypb.GitCommit{ Id: info.Oid, Author: &gitalypb.CommitAuthor{ @@ -78,7 +79,7 @@ func TestParseRawCommit(t *testing.T) { }, { desc: "date negative", - in: []byte("author Jane Doe -1 +0200"), + in: []byte("author Jane Doe -1 +0200\n"), out: &gitalypb.GitCommit{ Id: info.Oid, Author: &gitalypb.CommitAuthor{ @@ -93,9 +94,10 @@ func TestParseRawCommit(t *testing.T) { for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { info.Size = int64(len(tc.in)) - out, err := parseRawCommit(tc.in, info) + out, err := parseRawCommit(bytes.NewBuffer(tc.in), info) require.NoError(t, err, "parse error") - require.Equal(t, *tc.out, *out) + require.Equal(t, tc.out, out) + }) } } diff --git a/internal/service/commit/commit_messages.go b/internal/service/commit/commit_messages.go index e82f31d830d..50c9b158e2f 100644 --- a/internal/service/commit/commit_messages.go +++ b/internal/service/commit/commit_messages.go @@ -5,6 +5,7 @@ import ( "gitlab.com/gitlab-org/gitaly-proto/go/gitalypb" "gitlab.com/gitlab-org/gitaly/internal/git/log" + "gitlab.com/gitlab-org/gitaly/internal/helper" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" ) @@ -15,11 +16,25 @@ func (s *server) GetCommitMessages(request *gitalypb.GetCommitMessagesRequest, s } ctx := stream.Context() for _, commitID := range request.GetCommitIds() { - msg, err := log.GetCommitMessage(ctx, request.GetRepository(), commitID) + resp := &gitalypb.GetCommitMessagesResponse{ + CommitId: commitID, + } + + msgReader, err := log.GetCommitMessage(ctx, request.GetRepository(), commitID) if err != nil { return status.Errorf(codes.Internal, "failed to get commit message: %v", err) } - stream.Send(&gitalypb.GetCommitMessagesResponse{CommitId: commitID, Message: msg}) + + chunk := make([]byte, helper.MaxCommitOrTagMessageSize) + for n, err := msgReader.Read(chunk); err == nil; { + resp.Message = chunk[:n] + stream.Send(resp) + + // read the next chunk + n, err = msgReader.Read(chunk) + // reset the response + resp = &gitalypb.GetCommitMessagesResponse{} + } } return nil } -- GitLab From accdf058cd5c0acb10d287b2f8271f767520ee66 Mon Sep 17 00:00:00 2001 From: John Cai Date: Fri, 21 Dec 2018 11:30:34 -0800 Subject: [PATCH 04/10] adding changelog --- changelogs/unreleased/jc-rewrite-get-commit-message.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/jc-rewrite-get-commit-message.yml diff --git a/changelogs/unreleased/jc-rewrite-get-commit-message.yml b/changelogs/unreleased/jc-rewrite-get-commit-message.yml new file mode 100644 index 00000000000..245a5537e9c --- /dev/null +++ b/changelogs/unreleased/jc-rewrite-get-commit-message.yml @@ -0,0 +1,5 @@ +--- +title: Rewrite get commit message +merge_request: 1012 +author: johncai +type: other -- GitLab From f40cd8c302fb615b2bed2dcb52394f0d6b5948a3 Mon Sep 17 00:00:00 2001 From: John Cai Date: Thu, 3 Jan 2019 09:46:33 -0800 Subject: [PATCH 05/10] use []byte --- internal/git/log/commit.go | 54 +++++++++++----------- internal/service/commit/commit_messages.go | 35 ++++++++------ 2 files changed, 48 insertions(+), 41 deletions(-) diff --git a/internal/git/log/commit.go b/internal/git/log/commit.go index 9253a7e1f66..916b12d000e 100644 --- a/internal/git/log/commit.go +++ b/internal/git/log/commit.go @@ -47,7 +47,7 @@ func GetCommitCatfile(c *catfile.Batch, revision string) (*gitalypb.GitCommit, e } // GetCommitMessage looks up a commit message and returns it in its entirety. -func GetCommitMessage(ctx context.Context, repo *gitalypb.Repository, revision string) (io.Reader, error) { +func GetCommitMessage(ctx context.Context, repo *gitalypb.Repository, revision string) ([]byte, error) { c, err := catfile.New(ctx, repo) if err != nil { return nil, err @@ -66,49 +66,51 @@ func GetCommitMessage(ctx context.Context, repo *gitalypb.Repository, revision s return nil, err } - _, body := splitRawCommit(r) + _, body, err := splitRawCommit(r) + if err != nil { + return nil, err + } return body, nil } func parseRawCommit(r io.Reader, info *catfile.ObjectInfo) (*gitalypb.GitCommit, error) { - header, body := splitRawCommit(r) - return buildCommit(header, body, info) -} - -func splitRawCommit(r io.Reader) (io.Reader, io.Reader) { - var header bytes.Buffer - bufReader := bufio.NewReader(r) - for b, err := bufReader.ReadBytes('\n'); err == nil; { - if string(b) == "\n" { - break - } - header.Write(b) - b, err = bufReader.ReadBytes('\n') + header, body, err := splitRawCommit(r) + if err != nil { + return nil, err } - return &header, bufReader + return buildCommit(header, body, info) } -func buildCommit(header, body io.Reader, info *catfile.ObjectInfo) (*gitalypb.GitCommit, error) { - bodyBytes, err := ioutil.ReadAll(body) +func splitRawCommit(r io.Reader) ([]byte, []byte, error) { + raw, err := ioutil.ReadAll(r) if err != nil { - return &gitalypb.GitCommit{}, err + return nil, nil, err } - if len(bodyBytes) == 0 { - bodyBytes = nil + + split := bytes.SplitN(raw, []byte("\n\n"), 2) + + header := split[0] + var body []byte + if len(split) == 2 { + body = split[1] } + return header, body, nil +} + +func buildCommit(header, body []byte, info *catfile.ObjectInfo) (*gitalypb.GitCommit, error) { commit := &gitalypb.GitCommit{ Id: info.Oid, - BodySize: int64(len(bodyBytes)), - Body: bodyBytes, - Subject: subjectFromBody(bodyBytes), + BodySize: int64(len(body)), + Body: body, + Subject: subjectFromBody(body), } - if max := helper.MaxCommitOrTagMessageSize; len(bodyBytes) > max { + if max := helper.MaxCommitOrTagMessageSize; len(body) > max { commit.Body = commit.Body[:max] } - scanner := bufio.NewScanner(header) + scanner := bufio.NewScanner(bytes.NewReader(header)) for scanner.Scan() { line := scanner.Text() if len(line) == 0 || line[0] == ' ' { diff --git a/internal/service/commit/commit_messages.go b/internal/service/commit/commit_messages.go index 50c9b158e2f..7e4f2f23f05 100644 --- a/internal/service/commit/commit_messages.go +++ b/internal/service/commit/commit_messages.go @@ -1,11 +1,13 @@ package commit import ( + "bytes" "fmt" + "io" "gitlab.com/gitlab-org/gitaly-proto/go/gitalypb" "gitlab.com/gitlab-org/gitaly/internal/git/log" - "gitlab.com/gitlab-org/gitaly/internal/helper" + "gitlab.com/gitlab-org/gitaly/streamio" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" ) @@ -16,24 +18,27 @@ func (s *server) GetCommitMessages(request *gitalypb.GetCommitMessagesRequest, s } ctx := stream.Context() for _, commitID := range request.GetCommitIds() { - resp := &gitalypb.GetCommitMessagesResponse{ - CommitId: commitID, - } - - msgReader, err := log.GetCommitMessage(ctx, request.GetRepository(), commitID) + msg, err := log.GetCommitMessage(ctx, request.GetRepository(), commitID) if err != nil { return status.Errorf(codes.Internal, "failed to get commit message: %v", err) } - chunk := make([]byte, helper.MaxCommitOrTagMessageSize) - for n, err := msgReader.Read(chunk); err == nil; { - resp.Message = chunk[:n] - stream.Send(resp) - - // read the next chunk - n, err = msgReader.Read(chunk) - // reset the response - resp = &gitalypb.GetCommitMessagesResponse{} + msgReader := bytes.NewReader(msg) + resp := &gitalypb.GetCommitMessagesResponse{ + CommitId: commitID, + } + sw := streamio.NewWriter(func(p []byte) error { + resp.Message = p + err := stream.Send(resp) + resp.CommitId = "" + if err != nil { + return err + } + return nil + }) + _, err = io.Copy(sw, msgReader) + if err != nil { + return status.Errorf(codes.Internal, "failed to send response: %v", err) } } return nil -- GitLab From 42580e950680187b58585b52e5b840029eacf04b Mon Sep 17 00:00:00 2001 From: John Cai Date: Fri, 21 Dec 2018 11:27:01 -0800 Subject: [PATCH 06/10] use readers and chunk the message --- internal/service/commit/commit_messages_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/service/commit/commit_messages_test.go b/internal/service/commit/commit_messages_test.go index 48bda100769..3a8cda04577 100644 --- a/internal/service/commit/commit_messages_test.go +++ b/internal/service/commit/commit_messages_test.go @@ -25,7 +25,8 @@ func TestSuccessfulGetCommitMessagesRequest(t *testing.T) { ctx, cancel := testhelper.Context() defer cancel() - message1 := strings.Repeat("a\n", helper.MaxCommitOrTagMessageSize*2) + // message1 := strings.Repeat("a\n", helper.MaxCommitOrTagMessageSize*2) + message1 := strings.Repeat("a\n", 3) message2 := strings.Repeat("b\n", helper.MaxCommitOrTagMessageSize*2) commit1ID := testhelper.CreateCommit(t, testRepoPath, "local-big-commits", &testhelper.CreateCommitOpts{Message: message1}) @@ -50,7 +51,7 @@ func TestSuccessfulGetCommitMessagesRequest(t *testing.T) { }, } fetchedMessages := readAllMessagesFromClient(t, c) - + require.Equal(t, len(expectedMessages), len(fetchedMessages)) require.Equal(t, expectedMessages, fetchedMessages) } -- GitLab From 2d8c184ff2588824d33fadbe75fff59fb59c6f9c Mon Sep 17 00:00:00 2001 From: John Cai Date: Thu, 20 Dec 2018 17:34:19 -0800 Subject: [PATCH 07/10] rewrite get all tags using pure go implementation --- internal/git/catfile/catfile.go | 8 ++ internal/git/log/tags.go | 121 ++++++++++++++++++++++++++++++ internal/service/ref/refs.go | 25 +----- internal/service/ref/refs_test.go | 55 +++++++++++--- internal/testhelper/tag.go | 11 +++ 5 files changed, 190 insertions(+), 30 deletions(-) create mode 100644 internal/git/log/tags.go diff --git a/internal/git/catfile/catfile.go b/internal/git/catfile/catfile.go index a100ed30685..3c5f59a20cf 100644 --- a/internal/git/catfile/catfile.go +++ b/internal/git/catfile/catfile.go @@ -40,6 +40,14 @@ func (c *Batch) Commit(revspec string) (io.Reader, error) { return c.batch.reader(revspec, "commit") } +// Tag returns a raw tag object. It is an error if revspec does not +// point to a commit. To prevent this first use Info to resolve the revspec +// and check the object type. Caller must consume the Reader before +// making another call on C. +func (c *Batch) Tag(revspec string) (io.Reader, error) { + return c.batch.reader(revspec, "tag") +} + // Blob returns a reader for the requested blob. The entire blob must be // read before any new objects can be requested from this Batch instance. // diff --git a/internal/git/log/tags.go b/internal/git/log/tags.go new file mode 100644 index 00000000000..a1b58898f4f --- /dev/null +++ b/internal/git/log/tags.go @@ -0,0 +1,121 @@ +package log + +import ( + "bufio" + "context" + "errors" + "io/ioutil" + "strings" + + "gitlab.com/gitlab-org/gitaly-proto/go/gitalypb" + "gitlab.com/gitlab-org/gitaly/internal/git" + "gitlab.com/gitlab-org/gitaly/internal/git/catfile" + "gitlab.com/gitlab-org/gitaly/internal/helper" +) + +func GetAllTags(ctx context.Context, repo *gitalypb.Repository) ([]*gitalypb.Tag, error) { + var tags []*gitalypb.Tag + + c, err := catfile.New(ctx, repo) + if err != nil { + return tags, nil + } + + tagsCmd, err := git.Command(ctx, repo, "for-each-ref", "--format", "%(objectname) %(refname:short)", "refs/tags/") + if err != nil { + return tags, err + } + s := bufio.NewScanner(tagsCmd) + for s.Scan() { + line := strings.SplitN(s.Text(), " ", 2) + oid := strings.TrimSpace(line[0]) + info, err := c.Info(oid) + if err != nil { + return tags, err + } + if info.IsBlob() { + continue + } + tag, err := buildTag(oid, info.Type, line[1], c) + tags = append(tags, tag) + if err != nil { + return tags, err + } + } + + return tags, nil +} + +func buildTag(oid, objectType, tagName string, b *catfile.Batch) (*gitalypb.Tag, error) { + var tag *gitalypb.Tag + var err error + switch objectType { + // annotated tag + case "tag": + tag, err = buildAnnotatedTag(b, oid) + if err != nil { + return nil, err + } + // lightweight tag + case "commit": + tag, err = buildLightweightTag(b, oid, tagName) + if err != nil { + return nil, err + } + default: + return nil, errors.New("unknown object type for tag") + } + return tag, nil +} + +func buildAnnotatedTag(b *catfile.Batch, oid string) (*gitalypb.Tag, error) { + r, err := b.Tag(oid) + if err != nil { + return nil, err + } + tagBytes, err := ioutil.ReadAll(r) + if err != nil { + return nil, err + } + + header, body := splitRawCommit(tagBytes) + headerLines := strings.Split(string(header), "\n") + if err != nil { + return nil, err + } + body = body[:len(body)-1] + + commit, err := GetCommitCatfile(b, strings.SplitN(headerLines[0], " ", 2)[1]) + if err != nil { + return nil, err + } + tag := &gitalypb.Tag{ + Id: oid, + Name: []byte(strings.SplitN(headerLines[2], " ", 2)[1]), + Message: body, + MessageSize: int64(len(body)), + Tagger: parseCommitAuthor(strings.SplitN(headerLines[3], " ", 2)[1]), + TargetCommit: commit, + } + + if max := helper.MaxCommitOrTagMessageSize; len(tag.Message) > max { + tag.Message = tag.Message[:max] + } + + return tag, nil +} + +func buildLightweightTag(b *catfile.Batch, oid, name string) (*gitalypb.Tag, error) { + commit, err := GetCommitCatfile(b, oid) + if err != nil { + return nil, err + } + + tag := &gitalypb.Tag{ + Id: oid, + Name: []byte(name), + TargetCommit: commit, + } + + return tag, nil +} diff --git a/internal/service/ref/refs.go b/internal/service/ref/refs.go index e986e0ede91..d9b0b40e440 100644 --- a/internal/service/ref/refs.go +++ b/internal/service/ref/refs.go @@ -12,8 +12,8 @@ import ( "gitlab.com/gitlab-org/gitaly-proto/go/gitalypb" "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/git/catfile" + gitlog "gitlab.com/gitlab-org/gitaly/internal/git/log" "gitlab.com/gitlab-org/gitaly/internal/helper/lines" - "gitlab.com/gitlab-org/gitaly/internal/rubyserver" "golang.org/x/net/context" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" @@ -73,29 +73,12 @@ func (s *server) FindAllTagNames(in *gitalypb.FindAllTagNamesRequest, stream git func (s *server) FindAllTags(in *gitalypb.FindAllTagsRequest, stream gitalypb.RefService_FindAllTagsServer) error { ctx := stream.Context() - client, err := s.RefServiceClient(ctx) + tags, err := gitlog.GetAllTags(ctx, in.GetRepository()) if err != nil { return err } - - clientCtx, err := rubyserver.SetHeaders(ctx, in.GetRepository()) - if err != nil { - return err - } - - rubyStream, err := client.FindAllTags(clientCtx, in) - if err != nil { - return err - } - - return rubyserver.Proxy(func() error { - resp, err := rubyStream.Recv() - if err != nil { - md := rubyStream.Trailer() - stream.SetTrailer(md) - return err - } - return stream.Send(resp) + return stream.Send(&gitalypb.FindAllTagsResponse{ + Tags: tags, }) } diff --git a/internal/service/ref/refs_test.go b/internal/service/ref/refs_test.go index f675c2c9a56..6dbeacd7034 100644 --- a/internal/service/ref/refs_test.go +++ b/internal/service/ref/refs_test.go @@ -410,7 +410,9 @@ func TestSuccessfulFindAllTagsRequest(t *testing.T) { bigCommit, err := log.GetCommit(ctx, testRepoCopy, bigCommitID) require.NoError(t, err) - annotatedTagID := testhelper.CreateTag(t, testRepoCopyPath, "v1.2.0", blobID, &testhelper.CreateTagOpts{Message: "Blob tag"}) + annotatedTagID := testhelper.CreateTag(t, testRepoCopyPath, "v1.2.0", commitID, &testhelper.CreateTagOpts{Message: "Annotated tag"}) + annotatedTagTimestamp, err := testhelper.GetTagDate(t, testRepoCopyPath, annotatedTagID) + require.NoError(t, err) testhelper.CreateTag(t, testRepoCopyPath, "v1.3.0", commitID, nil) testhelper.CreateTag(t, testRepoCopyPath, "v1.4.0", blobID, nil) @@ -424,6 +426,8 @@ func TestSuccessfulFindAllTagsRequest(t *testing.T) { // A tag with a big message bigMessage := strings.Repeat("a", 11*1024) bigMessageTag1ID := testhelper.CreateTag(t, testRepoCopyPath, "v1.7.0", commitID, &testhelper.CreateTagOpts{Message: bigMessage}) + bigMessageTagTimestamp, err := testhelper.GetTagDate(t, testRepoCopyPath, bigMessageTag1ID) + require.NoError(t, err) client, conn := newRefServiceClient(t, serverSocketPath) defer conn.Close() @@ -454,6 +458,11 @@ func TestSuccessfulFindAllTagsRequest(t *testing.T) { TargetCommit: gitCommit, Message: []byte("Release"), MessageSize: 7, + Tagger: &gitalypb.CommitAuthor{ + Name: []byte("Dmitriy Zaporozhets"), + Email: []byte("dmitriy.zaporozhets@gmail.com"), + Date: ×tamp.Timestamp{Seconds: 1393491299}, + }, }, { Name: []byte("v1.1.0"), @@ -477,22 +486,45 @@ func TestSuccessfulFindAllTagsRequest(t *testing.T) { }, Message: []byte("Version 1.1.0"), MessageSize: 13, + Tagger: &gitalypb.CommitAuthor{ + Name: []byte("Dmitriy Zaporozhets"), + Email: []byte("dmitriy.zaporozhets@gmail.com"), + Date: ×tamp.Timestamp{Seconds: 1393505709}, + }, }, { - Name: []byte("v1.2.0"), - Id: string(annotatedTagID), - Message: []byte("Blob tag"), - MessageSize: 8, + Name: []byte("v1.2.0"), + Id: string(annotatedTagID), + TargetCommit: &gitalypb.GitCommit{ + Id: "6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9", + Subject: []byte("More submodules"), + Body: []byte("More submodules\n\nSigned-off-by: Dmitriy Zaporozhets \n"), + Author: &gitalypb.CommitAuthor{ + Name: []byte("Dmitriy Zaporozhets"), + Email: []byte("dmitriy.zaporozhets@gmail.com"), + Date: ×tamp.Timestamp{Seconds: 1393491261}, + }, + Committer: &gitalypb.CommitAuthor{ + Name: []byte("Dmitriy Zaporozhets"), + Email: []byte("dmitriy.zaporozhets@gmail.com"), + Date: ×tamp.Timestamp{Seconds: 1393491261}, + }, + ParentIds: []string{"d14d6c0abdd253381df51a723d58691b2ee1ab08"}, + BodySize: 84, + }, + Message: []byte("Annotated tag"), + MessageSize: 13, + Tagger: &gitalypb.CommitAuthor{ + Name: []byte("Scrooge McDuck"), + Email: []byte("scrooge@mcduck.com"), + Date: ×tamp.Timestamp{Seconds: annotatedTagTimestamp}, + }, }, { Name: []byte("v1.3.0"), Id: string(commitID), TargetCommit: gitCommit, }, - { - Name: []byte("v1.4.0"), - Id: string(blobID), - }, { Name: []byte("v1.5.0"), Id: string(commitID), @@ -509,6 +541,11 @@ func TestSuccessfulFindAllTagsRequest(t *testing.T) { Message: []byte(bigMessage[:helper.MaxCommitOrTagMessageSize]), MessageSize: int64(len(bigMessage)), TargetCommit: gitCommit, + Tagger: &gitalypb.CommitAuthor{ + Name: []byte("Scrooge McDuck"), + Email: []byte("scrooge@mcduck.com"), + Date: ×tamp.Timestamp{Seconds: bigMessageTagTimestamp}, + }, }, } diff --git a/internal/testhelper/tag.go b/internal/testhelper/tag.go index 29e01e78cc0..42ee09c612a 100644 --- a/internal/testhelper/tag.go +++ b/internal/testhelper/tag.go @@ -3,6 +3,7 @@ package testhelper import ( "bytes" "fmt" + "strconv" "strings" "testing" ) @@ -43,3 +44,13 @@ func CreateTag(t *testing.T, repoPath, tagName, targetID string, opts *CreateTag tagID := MustRunCommand(t, nil, "git", "-C", repoPath, "rev-parse", tagName) return strings.TrimSpace(string(tagID)) } + +func GetTagDate(t *testing.T, repoPath, tagName string) (int64, error) { + tagInfoLines := strings.Split(string(MustRunCommand(t, nil, "git", "-C", repoPath, "show", "--date=unix", tagName)), "\n") + timestampString := strings.TrimSpace(strings.SplitN(tagInfoLines[2], "Date:", 2)[1]) + timestamp, err := strconv.ParseInt(strings.TrimSpace(string(timestampString)), 10, 64) + if err != nil { + return 0, nil + } + return timestamp, nil +} -- GitLab From a7fd8d8ea7704c1f70bee21a09e36f1351ba68f6 Mon Sep 17 00:00:00 2001 From: John Cai Date: Thu, 20 Dec 2018 18:09:36 -0800 Subject: [PATCH 08/10] adding validationg for find all tags --- .../service/commit/commit_messages_test.go | 2 +- internal/service/ref/refs.go | 20 ++++++++++++++++++- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/internal/service/commit/commit_messages_test.go b/internal/service/commit/commit_messages_test.go index 3a8cda04577..0eb832758cc 100644 --- a/internal/service/commit/commit_messages_test.go +++ b/internal/service/commit/commit_messages_test.go @@ -51,7 +51,7 @@ func TestSuccessfulGetCommitMessagesRequest(t *testing.T) { }, } fetchedMessages := readAllMessagesFromClient(t, c) - require.Equal(t, len(expectedMessages), len(fetchedMessages)) + require.Equal(t, expectedMessages, fetchedMessages) } diff --git a/internal/service/ref/refs.go b/internal/service/ref/refs.go index d9b0b40e440..4044e941d68 100644 --- a/internal/service/ref/refs.go +++ b/internal/service/ref/refs.go @@ -3,10 +3,13 @@ package ref import ( "bufio" "bytes" + "errors" "fmt" "regexp" "strings" + "gitlab.com/gitlab-org/gitaly/internal/helper" + grpc_logrus "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus" log "github.com/sirupsen/logrus" "gitlab.com/gitlab-org/gitaly-proto/go/gitalypb" @@ -73,15 +76,30 @@ func (s *server) FindAllTagNames(in *gitalypb.FindAllTagNamesRequest, stream git func (s *server) FindAllTags(in *gitalypb.FindAllTagsRequest, stream gitalypb.RefService_FindAllTagsServer) error { ctx := stream.Context() + if err := validateFindAllTagsRequest(in); err != nil { + return status.Errorf(codes.InvalidArgument, "FindAllTags: %v", err) + } + tags, err := gitlog.GetAllTags(ctx, in.GetRepository()) if err != nil { - return err + return status.Errorf(codes.Internal, "error when getting all tags: %v", err) } return stream.Send(&gitalypb.FindAllTagsResponse{ Tags: tags, }) } +func validateFindAllTagsRequest(request *gitalypb.FindAllTagsRequest) error { + if request.GetRepository() == nil { + return errors.New("empty Repository") + } + _, err := helper.GetRepoPath(request.GetRepository()) + if err != nil { + return errors.New("invalid git directory") + } + return nil +} + func _findBranchNames(ctx context.Context, repo *gitalypb.Repository) ([][]byte, error) { var names [][]byte -- GitLab From 57a47008f53da44fe84018edd5268385bce00d01 Mon Sep 17 00:00:00 2001 From: John Cai Date: Thu, 20 Dec 2018 18:22:09 -0800 Subject: [PATCH 09/10] adding comments for exported methods --- internal/git/log/tags.go | 1 + internal/testhelper/tag.go | 1 + 2 files changed, 2 insertions(+) diff --git a/internal/git/log/tags.go b/internal/git/log/tags.go index a1b58898f4f..6ad5f423f3b 100644 --- a/internal/git/log/tags.go +++ b/internal/git/log/tags.go @@ -13,6 +13,7 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/helper" ) +// GetAllTags retrieves all tag objects for a given repository func GetAllTags(ctx context.Context, repo *gitalypb.Repository) ([]*gitalypb.Tag, error) { var tags []*gitalypb.Tag diff --git a/internal/testhelper/tag.go b/internal/testhelper/tag.go index 42ee09c612a..a94d99685e5 100644 --- a/internal/testhelper/tag.go +++ b/internal/testhelper/tag.go @@ -45,6 +45,7 @@ func CreateTag(t *testing.T, repoPath, tagName, targetID string, opts *CreateTag return strings.TrimSpace(string(tagID)) } +// GetTagDate gets the tag date in a timestamp form func GetTagDate(t *testing.T, repoPath, tagName string) (int64, error) { tagInfoLines := strings.Split(string(MustRunCommand(t, nil, "git", "-C", repoPath, "show", "--date=unix", tagName)), "\n") timestampString := strings.TrimSpace(strings.SplitN(tagInfoLines[2], "Date:", 2)[1]) -- GitLab From cce583df8566e5fb5022c22029797f1ea48fbd68 Mon Sep 17 00:00:00 2001 From: John Cai Date: Fri, 21 Dec 2018 13:42:48 -0800 Subject: [PATCH 10/10] changes after rebasing against get commit messages branch --- .../jc-rewrite-get-commit-message.yml | 5 -- internal/git/log/commit_test.go | 12 ++--- internal/git/log/tags.go | 54 +++++++++++-------- 3 files changed, 37 insertions(+), 34 deletions(-) delete mode 100644 changelogs/unreleased/jc-rewrite-get-commit-message.yml diff --git a/changelogs/unreleased/jc-rewrite-get-commit-message.yml b/changelogs/unreleased/jc-rewrite-get-commit-message.yml deleted file mode 100644 index 245a5537e9c..00000000000 --- a/changelogs/unreleased/jc-rewrite-get-commit-message.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Rewrite get commit message -merge_request: 1012 -author: johncai -type: other diff --git a/internal/git/log/commit_test.go b/internal/git/log/commit_test.go index 3a756b6a244..7a73b72824f 100644 --- a/internal/git/log/commit_test.go +++ b/internal/git/log/commit_test.go @@ -35,7 +35,7 @@ func TestParseRawCommit(t *testing.T) { }, { desc: "no email", - in: []byte("author Jane Doe\n"), + in: []byte("author Jane Doe"), out: &gitalypb.GitCommit{ Id: info.Oid, Author: &gitalypb.CommitAuthor{Name: []byte("Jane Doe")}, @@ -43,7 +43,7 @@ func TestParseRawCommit(t *testing.T) { }, { desc: "unmatched <", - in: []byte("author Jane Doe ", - in: []byte("author Jane Doe janedoe@example.com>\n"), + in: []byte("author Jane Doe janedoe@example.com>"), out: &gitalypb.GitCommit{ Id: info.Oid, Author: &gitalypb.CommitAuthor{Name: []byte("Jane Doe janedoe@example.com>")}, @@ -59,7 +59,7 @@ func TestParseRawCommit(t *testing.T) { }, { desc: "missing date", - in: []byte("author Jane Doe \n"), + in: []byte("author Jane Doe "), out: &gitalypb.GitCommit{ Id: info.Oid, Author: &gitalypb.CommitAuthor{Name: []byte("Jane Doe"), Email: []byte("janedoe@example.com")}, @@ -67,7 +67,7 @@ func TestParseRawCommit(t *testing.T) { }, { desc: "date too high", - in: []byte("author Jane Doe 9007199254740993 +0200\n"), + in: []byte("author Jane Doe 9007199254740993 +0200"), out: &gitalypb.GitCommit{ Id: info.Oid, Author: &gitalypb.CommitAuthor{ @@ -79,7 +79,7 @@ func TestParseRawCommit(t *testing.T) { }, { desc: "date negative", - in: []byte("author Jane Doe -1 +0200\n"), + in: []byte("author Jane Doe -1 +0200"), out: &gitalypb.GitCommit{ Id: info.Oid, Author: &gitalypb.CommitAuthor{ diff --git a/internal/git/log/tags.go b/internal/git/log/tags.go index 6ad5f423f3b..0487d2f6503 100644 --- a/internal/git/log/tags.go +++ b/internal/git/log/tags.go @@ -2,9 +2,10 @@ package log import ( "bufio" + "bytes" "context" "errors" - "io/ioutil" + "fmt" "strings" "gitlab.com/gitlab-org/gitaly-proto/go/gitalypb" @@ -38,10 +39,10 @@ func GetAllTags(ctx context.Context, repo *gitalypb.Repository) ([]*gitalypb.Tag continue } tag, err := buildTag(oid, info.Type, line[1], c) - tags = append(tags, tag) if err != nil { return tags, err } + tags = append(tags, tag) } return tags, nil @@ -72,37 +73,44 @@ func buildTag(oid, objectType, tagName string, b *catfile.Batch) (*gitalypb.Tag, func buildAnnotatedTag(b *catfile.Batch, oid string) (*gitalypb.Tag, error) { r, err := b.Tag(oid) if err != nil { - return nil, err - } - tagBytes, err := ioutil.ReadAll(r) - if err != nil { - return nil, err + return nil, fmt.Errorf("buildAnnotatedTag error when getting tag reader: %v", err) } - - header, body := splitRawCommit(tagBytes) - headerLines := strings.Split(string(header), "\n") + header, body, err := splitRawCommit(r) if err != nil { - return nil, err + return nil, fmt.Errorf("buildAnnotatedTag error when splitting commit: %v", err) } - body = body[:len(body)-1] - commit, err := GetCommitCatfile(b, strings.SplitN(headerLines[0], " ", 2)[1]) - if err != nil { - return nil, err - } tag := &gitalypb.Tag{ - Id: oid, - Name: []byte(strings.SplitN(headerLines[2], " ", 2)[1]), - Message: body, - MessageSize: int64(len(body)), - Tagger: parseCommitAuthor(strings.SplitN(headerLines[3], " ", 2)[1]), - TargetCommit: commit, + Id: oid, + MessageSize: int64(len(body)), + Message: body, } - if max := helper.MaxCommitOrTagMessageSize; len(tag.Message) > max { + if max := helper.MaxCommitOrTagMessageSize; len(body) > max { tag.Message = tag.Message[:max] } + s := bufio.NewScanner(bytes.NewReader(header)) + for s.Scan() { + line := s.Text() + if len(line) == 0 || line[0] == ' ' { + continue + } + headerSplit := strings.SplitN(line, " ", 2) + switch headerSplit[0] { + case "object": + commit, err := GetCommitCatfile(b, headerSplit[1]) + if err != nil { + return nil, fmt.Errorf("buildAnnotatedTag error when getting target commit: %v", err) + } + tag.TargetCommit = commit + case "tag": + tag.Name = []byte(headerSplit[1]) + case "tagger": + tag.Tagger = parseCommitAuthor(headerSplit[1]) + } + } + return tag, nil } -- GitLab