From 2778fb7a767f9da6e2fca4a0ebf2d98b667b8ddf Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Thu, 24 Mar 2022 16:22:22 +0000 Subject: Allow Commit.RawBlame to take a Range parameter Git supports a wide array of range options for this option, but for now we restrict the formats we support to the very simplest, which is of the form "1,100". We can use this to implement limits on how many lines of the blame we retrieve GitLab-side, which should help to reduce the resource usage of this particular RPC. Changelog: added --- internal/gitaly/service/commit/raw_blame.go | 16 ++++++++- internal/gitaly/service/commit/raw_blame_test.go | 44 ++++++++++++++++++------ 2 files changed, 49 insertions(+), 11 deletions(-) (limited to 'internal/gitaly/service') diff --git a/internal/gitaly/service/commit/raw_blame.go b/internal/gitaly/service/commit/raw_blame.go index 860e511b7..8bd76ef10 100644 --- a/internal/gitaly/service/commit/raw_blame.go +++ b/internal/gitaly/service/commit/raw_blame.go @@ -3,6 +3,7 @@ package commit import ( "fmt" "io" + "regexp" "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" "gitlab.com/gitlab-org/gitaly/v14/internal/git" @@ -12,6 +13,8 @@ import ( "google.golang.org/grpc/status" ) +var validBlameRange = regexp.MustCompile(`\A\d+,\d+\z`) + func (s *server) RawBlame(in *gitalypb.RawBlameRequest, stream gitalypb.CommitService_RawBlameServer) error { if err := validateRawBlameRequest(in); err != nil { return status.Errorf(codes.InvalidArgument, "RawBlame: %v", err) @@ -20,10 +23,16 @@ func (s *server) RawBlame(in *gitalypb.RawBlameRequest, stream gitalypb.CommitSe ctx := stream.Context() revision := string(in.GetRevision()) path := string(in.GetPath()) + blameRange := string(in.GetRange()) + + flags := []git.Option{git.Flag{Name: "-p"}} + if blameRange != "" { + flags = append(flags, git.ValueFlag{Name: "-L", Value: blameRange}) + } cmd, err := s.gitCmdFactory.New(ctx, in.Repository, git.SubCmd{ Name: "blame", - Flags: []git.Option{git.Flag{Name: "-p"}}, + Flags: flags, Args: []string{revision}, PostSepArgs: []string{path}, }) @@ -59,5 +68,10 @@ func validateRawBlameRequest(in *gitalypb.RawBlameRequest) error { return fmt.Errorf("empty Path") } + blameRange := in.GetRange() + if len(blameRange) > 0 && !validBlameRange.Match(blameRange) { + return fmt.Errorf("invalid Range") + } + return nil } diff --git a/internal/gitaly/service/commit/raw_blame_test.go b/internal/gitaly/service/commit/raw_blame_test.go index 5de17dd91..5de729b04 100644 --- a/internal/gitaly/service/commit/raw_blame_test.go +++ b/internal/gitaly/service/commit/raw_blame_test.go @@ -19,22 +19,31 @@ func TestSuccessfulRawBlameRequest(t *testing.T) { _, repo, _, client := setupCommitServiceWithRepo(ctx, t, true) testCases := []struct { - revision, path, data []byte + revision, path, data, blameRange []byte }{ { - revision: []byte("e63f41fe459e62e1228fcef60d7189127aeba95a"), - path: []byte("files/ruby/popen.rb"), - data: testhelper.MustReadFile(t, "testdata/files-ruby-popen-e63f41f-blame.txt"), + revision: []byte("e63f41fe459e62e1228fcef60d7189127aeba95a"), + path: []byte("files/ruby/popen.rb"), + data: testhelper.MustReadFile(t, "testdata/files-ruby-popen-e63f41f-blame.txt"), + blameRange: []byte{}, }, { - revision: []byte("e63f41fe459e62e1228fcef60d7189127aeba95a"), - path: []byte("files/ruby/../ruby/popen.rb"), - data: testhelper.MustReadFile(t, "testdata/files-ruby-popen-e63f41f-blame.txt"), + revision: []byte("e63f41fe459e62e1228fcef60d7189127aeba95a"), + path: []byte("files/ruby/popen.rb"), + data: testhelper.MustReadFile(t, "testdata/files-ruby-popen-e63f41f-blame.txt")[0:956], + blameRange: []byte("1,5"), }, { - revision: []byte("93dcf076a236c837dd47d61f86d95a6b3d71b586"), - path: []byte("gitaly/empty-file"), - data: []byte{}, + revision: []byte("e63f41fe459e62e1228fcef60d7189127aeba95a"), + path: []byte("files/ruby/../ruby/popen.rb"), + data: testhelper.MustReadFile(t, "testdata/files-ruby-popen-e63f41f-blame.txt"), + blameRange: []byte{}, + }, + { + revision: []byte("93dcf076a236c837dd47d61f86d95a6b3d71b586"), + path: []byte("gitaly/empty-file"), + data: []byte{}, + blameRange: []byte{}, }, } @@ -44,6 +53,7 @@ func TestSuccessfulRawBlameRequest(t *testing.T) { Repository: repo, Revision: testCase.revision, Path: testCase.path, + Range: testCase.blameRange, } c, err := client.RawBlame(ctx, request) require.NoError(t, err) @@ -73,6 +83,7 @@ func TestFailedRawBlameRequest(t *testing.T) { description string repo *gitalypb.Repository revision, path []byte + blameRange []byte code codes.Code }{ { @@ -80,6 +91,7 @@ func TestFailedRawBlameRequest(t *testing.T) { repo: invalidRepo, revision: []byte("master"), path: []byte("a/b/c"), + blameRange: []byte{}, code: codes.InvalidArgument, }, { @@ -87,6 +99,7 @@ func TestFailedRawBlameRequest(t *testing.T) { repo: repo, revision: []byte(""), path: []byte("a/b/c"), + blameRange: []byte{}, code: codes.InvalidArgument, }, { @@ -94,6 +107,7 @@ func TestFailedRawBlameRequest(t *testing.T) { repo: repo, revision: []byte("abcdef"), path: []byte(""), + blameRange: []byte{}, code: codes.InvalidArgument, }, { @@ -101,6 +115,15 @@ func TestFailedRawBlameRequest(t *testing.T) { repo: repo, revision: []byte("--output=/meow"), path: []byte("a/b/c"), + blameRange: []byte{}, + code: codes.InvalidArgument, + }, + { + description: "Invalid range", + repo: repo, + revision: []byte("abcdef"), + path: []byte("a/b/c"), + blameRange: []byte("foo"), code: codes.InvalidArgument, }, } @@ -111,6 +134,7 @@ func TestFailedRawBlameRequest(t *testing.T) { Repository: testCase.repo, Revision: testCase.revision, Path: testCase.path, + Range: testCase.blameRange, } c, err := client.RawBlame(ctx, &request) require.NoError(t, err) -- cgit v1.2.3