diff options
author | Eric Ju <eju@gitlab.com> | 2024-01-18 03:37:07 +0300 |
---|---|---|
committer | Eric Ju <eju@gitlab.com> | 2024-01-18 03:37:07 +0300 |
commit | 7997900a820e07709f644145231cadc799f02c44 (patch) | |
tree | 43b0fc470c0a5bc69aeeded299530c6f0e9bb249 | |
parent | aa352ff89f9a2aca1496c62f6a6dc8935a2c48b3 (diff) |
repository: Make GetInfoAttributes read from HEAD:.gitattributeej-stop-using-info-atrributes-2
This is a solution for N+1 problem we come across, see
https://gitlab.com/groups/gitlab-org/-/epics/9006#note_1730745750 for
details.
In order to remove the reliance of info/attribute and avoid breaking
Rails with TooManyInvocationsError. GetInfoAttributes is set to read
from HEAD:.gitattribute.
It will also try to delete info/attribute before reading.
After proper refactor on Rails to solve the N+1 case.
GetInfoAttributes can be removed totally.
This is my commit message body
-rw-r--r-- | internal/gitaly/service/repository/info_attributes.go | 51 | ||||
-rw-r--r-- | internal/gitaly/service/repository/info_attributes_test.go | 12 |
2 files changed, 49 insertions, 14 deletions
diff --git a/internal/gitaly/service/repository/info_attributes.go b/internal/gitaly/service/repository/info_attributes.go index 9ddf04fdc..f5a9628b2 100644 --- a/internal/gitaly/service/repository/info_attributes.go +++ b/internal/gitaly/service/repository/info_attributes.go @@ -1,16 +1,17 @@ package repository import ( - "io" - "os" - "path/filepath" - + "bufio" + "gitlab.com/gitlab-org/gitaly/v16/internal/git" "gitlab.com/gitlab-org/gitaly/v16/internal/structerr" "gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb" "gitlab.com/gitlab-org/gitaly/v16/streamio" + "io" + "os" + "strings" ) -func (s *server) GetInfoAttributes(in *gitalypb.GetInfoAttributesRequest, stream gitalypb.RepositoryService_GetInfoAttributesServer) error { +func (s *server) GetInfoAttributes(in *gitalypb.GetInfoAttributesRequest, stream gitalypb.RepositoryService_GetInfoAttributesServer) (returnedErr error) { repository := in.GetRepository() if err := s.locator.ValidateRepository(repository); err != nil { return structerr.NewInvalidArgument("%w", err) @@ -20,14 +21,40 @@ func (s *server) GetInfoAttributes(in *gitalypb.GetInfoAttributesRequest, stream return err } - attrFile := filepath.Join(repoPath, "info", "attributes") - f, err := os.Open(attrFile) - if err != nil { - if os.IsNotExist(err) { - return stream.Send(&gitalypb.GetInfoAttributesResponse{}) + // In git 2.43.0+, gitattributes supports reading from HEAD:.gitattributes, + // so info/attributes is no longer needed. To make sure info/attributes file is cleaned up, + // we delete it if it exists when reading from HEAD:.gitattributes is called. + // This logic can be removed when ApplyGitattributes and GetInfoAttributes PRC are totally removed from + // the code base. + deletionErr := deleteInfoAttributesFile(repoPath) + if !os.IsNotExist(deletionErr) { + s.logger.WithError(deletionErr).Error("failed to delete info/gitattributes file at " + repoPath) + } + + repo := s.localrepo(in.GetRepository()) + ctx := stream.Context() + var stderr strings.Builder + // Call cat-file -p HEAD:.gitattributes instead of cat info/attributes + catFileCmd, err := repo.Exec(ctx, git.Command{ + Name: "cat-file", + Flags: []git.Option{ + git.Flag{Name: "-p"}, + }, + Args: []string{"HEAD:.gitattributes"}, + }, + git.WithSetupStdout(), + git.WithStderr(&stderr), + ) + defer func() { + if err := catFileCmd.Wait(); err != nil { + s.logger.Error("git cat-file command error: " + stderr.String()) } + }() - return structerr.NewInternal("failure to read info attributes: %w", err) + buf := bufio.NewReader(catFileCmd) + _, err = buf.Peek(1) + if err == io.EOF { + return stream.Send(&gitalypb.GetInfoAttributesResponse{}) } sw := streamio.NewWriter(func(p []byte) error { @@ -35,7 +62,7 @@ func (s *server) GetInfoAttributes(in *gitalypb.GetInfoAttributesRequest, stream Attributes: p, }) }) + _, err = io.Copy(sw, buf) - _, err = io.Copy(sw, f) return err } diff --git a/internal/gitaly/service/repository/info_attributes_test.go b/internal/gitaly/service/repository/info_attributes_test.go index 908761822..7cb1c3e90 100644 --- a/internal/gitaly/service/repository/info_attributes_test.go +++ b/internal/gitaly/service/repository/info_attributes_test.go @@ -2,6 +2,7 @@ package repository import ( "bytes" + "gitlab.com/gitlab-org/gitaly/v16/internal/helper/perm" "io" "os" "path/filepath" @@ -10,7 +11,6 @@ import ( "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" - "gitlab.com/gitlab-org/gitaly/v16/internal/helper/perm" "gitlab.com/gitlab-org/gitaly/v16/internal/structerr" "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb" @@ -33,6 +33,13 @@ func TestGetInfoAttributesExisting(t *testing.T) { err := os.WriteFile(attrsPath, data, perm.SharedFile) require.NoError(t, err) + gitattributesContent := "*.go diff=go text\n*.md text\n*.jpg -text" + gittest.WriteCommit(t, cfg, repoPath, + gittest.WithBranch("main"), + gittest.WithTreeEntries( + gittest.TreeEntry{Path: ".gitattributes", Mode: "100644", Content: gitattributesContent}, + )) + request := &gitalypb.GetInfoAttributesRequest{Repository: repo} //nolint:staticcheck @@ -45,7 +52,8 @@ func TestGetInfoAttributesExisting(t *testing.T) { })) require.NoError(t, err) - require.Equal(t, data, receivedData) + require.Equal(t, gitattributesContent, string(receivedData)) + require.NoFileExists(t, attrsPath) } func TestGetInfoAttributesNonExisting(t *testing.T) { |