From dee696d31b35fc0682b77388148076617d1c60c5 Mon Sep 17 00:00:00 2001 From: Paul Okstad Date: Wed, 28 Oct 2020 09:25:25 -0700 Subject: [PATCH 01/16] Add commands causing test failures --- internal/git/subcommand.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/internal/git/subcommand.go b/internal/git/subcommand.go index 902bc776002..e804ff6afa9 100644 --- a/internal/git/subcommand.go +++ b/internal/git/subcommand.go @@ -13,6 +13,9 @@ const ( scDiff = "diff" scPackRefs = "pack-refs" scMergeBase = "merge-base" + scWorktree = "worktree" + scHashObject = "hash-object" + scShowRef = "show-ref" ) var knownReadOnlyCmds = map[string]struct{}{ @@ -23,6 +26,7 @@ var knownReadOnlyCmds = map[string]struct{}{ scCountObjects: struct{}{}, scDiff: struct{}{}, scMergeBase: struct{}{}, + scShowRef: struct{}{}, } // knownNoRefUpdates indicates all repo mutating commands where it is known @@ -32,6 +36,8 @@ var knownNoRefUpdates = map[string]struct{}{ scMultiPackIndex: struct{}{}, scRepack: struct{}{}, scPackRefs: struct{}{}, + scWorktree: struct{}{}, + scHashObject: struct{}{}, } // mayUpdateRef indicates if a subcommand is known to update references. -- GitLab From 229d4284eb6c1df567db508ae1fa8dc875ef26b3 Mon Sep 17 00:00:00 2001 From: Paul Okstad Date: Wed, 4 Nov 2020 21:42:02 -0800 Subject: [PATCH 02/16] Repo converter helper --- internal/helper/repo.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/internal/helper/repo.go b/internal/helper/repo.go index 5f8b502bc4e..afa7a689f73 100644 --- a/internal/helper/repo.go +++ b/internal/helper/repo.go @@ -7,6 +7,7 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/git/repository" "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/internal/storage" + "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" ) @@ -74,3 +75,14 @@ func GetStorageByName(storageName string) (string, error) { return storagePath, nil } + +// ProtoRepoFromRepo allows object pools and repository abstractions to be used +// in places that require a concrete type +func ProtoRepoFromRepo(repo repository.GitRepo) *gitalypb.Repository { + return &gitalypb.Repository{ + StorageName: repo.GetStorageName(), + GitAlternateObjectDirectories: repo.GetGitAlternateObjectDirectories(), + GitObjectDirectory: repo.GetGitObjectDirectory(), + RelativePath: repo.GetRelativePath(), + } +} -- GitLab From 14a36c4d573c7ba94c187264feba6e8228c9aa96 Mon Sep 17 00:00:00 2001 From: Paul Okstad Date: Wed, 28 Oct 2020 15:40:33 -0700 Subject: [PATCH 03/16] Add ref hooks where needed --- internal/git/objectpool/fetch.go | 6 +++-- internal/git/remote/remote.go | 9 ++++++-- internal/git/repository.go | 14 ++++++++---- internal/git/updateref/updateref.go | 13 +++++++---- .../gitaly/service/operations/commit_files.go | 1 + internal/gitaly/service/ref/delete_refs.go | 1 + internal/gitaly/service/ref/refs.go | 3 ++- .../service/remote/fetch_internal_remote.go | 2 ++ .../service/remote/find_remote_root_ref.go | 5 ++++- internal/gitaly/service/remote/remotes.go | 5 ++++- internal/gitaly/service/repository/archive.go | 2 +- internal/gitaly/service/repository/cleanup.go | 8 +++---- .../repository/clone_from_pool_internal.go | 18 ++++++++++----- .../gitaly/service/repository/commit_graph.go | 7 +++--- .../service/repository/create_from_url.go | 19 +++++++++------- .../repository/create_from_url_test.go | 8 ++++++- internal/gitaly/service/repository/fetch.go | 1 + internal/gitaly/service/repository/fork.go | 22 ++++++++++++------- internal/gitaly/service/repository/fsck.go | 1 + internal/gitaly/service/repository/gc.go | 22 +++++++++++-------- .../gitaly/service/repository/merge_base.go | 10 +++++---- internal/gitaly/service/repository/remove.go | 3 +-- internal/gitaly/service/repository/server.go | 4 ++-- internal/gitaly/service/repository/util.go | 4 ++-- .../gitaly/service/repository/write_ref.go | 16 +++++++++----- .../gitaly/service/smarthttp/receive_pack.go | 16 +++++++++----- 26 files changed, 146 insertions(+), 74 deletions(-) diff --git a/internal/git/objectpool/fetch.go b/internal/git/objectpool/fetch.go index 2bcd3a59918..0e4a30c4c6d 100644 --- a/internal/git/objectpool/fetch.go +++ b/internal/git/objectpool/fetch.go @@ -17,6 +17,8 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/git/repository" "gitlab.com/gitlab-org/gitaly/internal/git/updateref" + "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" + "gitlab.com/gitlab-org/gitaly/internal/helper" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" ) @@ -59,7 +61,7 @@ func (o *ObjectPool) FetchFromOrigin(ctx context.Context, origin *gitalypb.Repos setOriginCmd, err = git.SafeCmd(ctx, o, nil, git.SubCmd{ Name: "remote", Args: []string{"set-url", sourceRemote, originPath}, - }) + }, git.WithRefTxHook(ctx, helper.ProtoRepoFromRepo(o), config.Config)) if err != nil { return err } @@ -67,7 +69,7 @@ func (o *ObjectPool) FetchFromOrigin(ctx context.Context, origin *gitalypb.Repos setOriginCmd, err = git.SafeCmd(ctx, o, nil, git.SubCmd{ Name: "remote", Args: []string{"add", sourceRemote, originPath}, - }) + }, git.WithRefTxHook(ctx, helper.ProtoRepoFromRepo(o), config.Config)) if err != nil { return err } diff --git a/internal/git/remote/remote.go b/internal/git/remote/remote.go index 41e83b18779..7a3934986d8 100644 --- a/internal/git/remote/remote.go +++ b/internal/git/remote/remote.go @@ -6,6 +6,8 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/git/repository" + "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" + "gitlab.com/gitlab-org/gitaly/internal/helper" ) //Remove removes the remote from repository @@ -14,7 +16,7 @@ func Remove(ctx context.Context, repo repository.GitRepo, name string) error { Name: "remote", Flags: []git.Option{git.SubSubCmd{Name: "remove"}}, Args: []string{name}, - }) + }, git.WithRefTxHook(ctx, helper.ProtoRepoFromRepo(repo), config.Config)) if err != nil { return err } @@ -25,7 +27,10 @@ func Remove(ctx context.Context, repo repository.GitRepo, name string) error { // Exists will always return a boolean value, but should only be depended on // when the error value is nil func Exists(ctx context.Context, repo repository.GitRepo, name string) (bool, error) { - cmd, err := git.SafeCmd(ctx, repo, nil, git.SubCmd{Name: "remote"}) + cmd, err := git.SafeCmd(ctx, repo, nil, + git.SubCmd{Name: "remote"}, + git.WithRefTxHook(ctx, helper.ProtoRepoFromRepo(repo), config.Config), + ) if err != nil { return false, err } diff --git a/internal/git/repository.go b/internal/git/repository.go index a8a23099bdb..b3b65adc0ba 100644 --- a/internal/git/repository.go +++ b/internal/git/repository.go @@ -12,6 +12,8 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/command" "gitlab.com/gitlab-org/gitaly/internal/git/repository" + "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" + "gitlab.com/gitlab-org/gitaly/internal/helper" "gitlab.com/gitlab-org/gitaly/internal/helper/text" ) @@ -388,10 +390,14 @@ func (repo *localRepository) GetBranches(ctx context.Context) ([]Reference, erro } func (repo *localRepository) UpdateRef(ctx context.Context, reference, newrev, oldrev string) error { - cmd, err := repo.command(ctx, nil, SubCmd{ - Name: "update-ref", - Flags: []Option{Flag{Name: "-z"}, Flag{Name: "--stdin"}}, - }, WithStdin(strings.NewReader(fmt.Sprintf("update %s\x00%s\x00%s\x00", reference, newrev, oldrev)))) + cmd, err := repo.command(ctx, nil, + SubCmd{ + Name: "update-ref", + Flags: []Option{Flag{Name: "-z"}, Flag{Name: "--stdin"}}, + }, + WithStdin(strings.NewReader(fmt.Sprintf("update %s\x00%s\x00%s\x00", reference, newrev, oldrev))), + WithRefTxHook(ctx, helper.ProtoRepoFromRepo(repo.repo), config.Config), + ) if err != nil { return err } diff --git a/internal/git/updateref/updateref.go b/internal/git/updateref/updateref.go index 10d9901caab..20c3c749872 100644 --- a/internal/git/updateref/updateref.go +++ b/internal/git/updateref/updateref.go @@ -7,6 +7,8 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/command" "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/git/repository" + "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" + "gitlab.com/gitlab-org/gitaly/internal/helper" ) // Updater wraps a `git update-ref --stdin` process, presenting an interface @@ -24,10 +26,13 @@ type Updater struct { // It is important that ctx gets canceled somewhere. If it doesn't, the process // spawned by New() may never terminate. func New(ctx context.Context, repo repository.GitRepo) (*Updater, error) { - cmd, err := git.SafeStdinCmd(ctx, repo, nil, git.SubCmd{ - Name: "update-ref", - Flags: []git.Option{git.Flag{Name: "-z"}, git.Flag{Name: "--stdin"}}, - }) + cmd, err := git.SafeStdinCmd(ctx, repo, nil, + git.SubCmd{ + Name: "update-ref", + Flags: []git.Option{git.Flag{Name: "-z"}, git.Flag{Name: "--stdin"}}, + }, + git.WithRefTxHook(ctx, helper.ProtoRepoFromRepo(repo), config.Config), + ) if err != nil { return nil, err } diff --git a/internal/gitaly/service/operations/commit_files.go b/internal/gitaly/service/operations/commit_files.go index cca79b98531..b48aaa1fd61 100644 --- a/internal/gitaly/service/operations/commit_files.go +++ b/internal/gitaly/service/operations/commit_files.go @@ -385,6 +385,7 @@ func (s *server) fetchRemoteObject(ctx context.Context, local, remote *gitalypb. Args: []string{"ssh://gitaly/internal.git", sha}, }, git.WithStderr(stderr), + git.WithRefTxHook(ctx, local, s.cfg), ) if err != nil { return err diff --git a/internal/gitaly/service/ref/delete_refs.go b/internal/gitaly/service/ref/delete_refs.go index 4b0baa11e54..3545b6d01f3 100644 --- a/internal/gitaly/service/ref/delete_refs.go +++ b/internal/gitaly/service/ref/delete_refs.go @@ -3,6 +3,7 @@ package ref import ( "bufio" "context" + "errors" "fmt" "strings" diff --git a/internal/gitaly/service/ref/refs.go b/internal/gitaly/service/ref/refs.go index 97ddd579644..d439a06e003 100644 --- a/internal/gitaly/service/ref/refs.go +++ b/internal/gitaly/service/ref/refs.go @@ -13,6 +13,7 @@ import ( "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/gitaly/config" "gitlab.com/gitlab-org/gitaly/internal/helper" "gitlab.com/gitlab-org/gitaly/internal/helper/chunk" "gitlab.com/gitlab-org/gitaly/internal/helper/lines" @@ -217,7 +218,7 @@ func SetDefaultBranchRef(ctx context.Context, repo *gitalypb.Repository, ref str cmd, err := git.SafeCmd(ctx, repo, nil, git.SubCmd{ Name: "symbolic-ref", Args: []string{"HEAD", ref}, - }) + }, git.WithRefTxHook(ctx, repo, config.Config)) if err != nil { return err } diff --git a/internal/gitaly/service/remote/fetch_internal_remote.go b/internal/gitaly/service/remote/fetch_internal_remote.go index fe0d7bdd559..20c24a7576d 100644 --- a/internal/gitaly/service/remote/fetch_internal_remote.go +++ b/internal/gitaly/service/remote/fetch_internal_remote.go @@ -8,6 +8,7 @@ import ( "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" "gitlab.com/gitlab-org/gitaly/internal/git" + "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/internal/gitaly/service/ref" "gitlab.com/gitlab-org/gitaly/internal/gitalyssh" "gitlab.com/gitlab-org/gitaly/internal/helper" @@ -37,6 +38,7 @@ func (s *server) FetchInternalRemote(ctx context.Context, req *gitalypb.FetchInt Flags: []git.Option{git.Flag{Name: "--prune"}}, Args: []string{gitalyssh.GitalyInternalURL, mirrorRefSpec}, }, + git.WithRefTxHook(ctx, req.Repository, config.Config), ) if err != nil { return nil, err diff --git a/internal/gitaly/service/remote/find_remote_root_ref.go b/internal/gitaly/service/remote/find_remote_root_ref.go index 23dd0c03c5a..c3ef6a97887 100644 --- a/internal/gitaly/service/remote/find_remote_root_ref.go +++ b/internal/gitaly/service/remote/find_remote_root_ref.go @@ -6,6 +6,7 @@ import ( "strings" "gitlab.com/gitlab-org/gitaly/internal/git" + "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" @@ -15,7 +16,9 @@ const headPrefix = "HEAD branch: " func findRemoteRootRef(ctx context.Context, repo *gitalypb.Repository, remote string) (string, error) { cmd, err := git.SafeCmd(ctx, repo, nil, - git.SubCmd{Name: "remote", Flags: []git.Option{git.SubSubCmd{Name: "show"}}, Args: []string{remote}}) + git.SubCmd{Name: "remote", Flags: []git.Option{git.SubSubCmd{Name: "show"}}, Args: []string{remote}}, + git.WithRefTxHook(ctx, repo, config.Config), + ) if err != nil { return "", err } diff --git a/internal/gitaly/service/remote/remotes.go b/internal/gitaly/service/remote/remotes.go index e8e8897b88f..931c5c18e1f 100644 --- a/internal/gitaly/service/remote/remotes.go +++ b/internal/gitaly/service/remote/remotes.go @@ -11,6 +11,7 @@ import ( "github.com/golang/protobuf/proto" "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/git/remote" + "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/internal/gitaly/rubyserver" "gitlab.com/gitlab-org/gitaly/internal/helper/chunk" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" @@ -117,7 +118,9 @@ func (s *server) ListRemotes(req *gitalypb.ListRemotesRequest, stream gitalypb.R repo := req.GetRepository() ctx := stream.Context() - cmd, err := git.SafeCmd(ctx, repo, nil, git.SubCmd{Name: "remote", Flags: []git.Option{git.Flag{Name: "-v"}}}) + cmd, err := git.SafeCmd(ctx, repo, nil, git.SubCmd{Name: "remote", Flags: []git.Option{git.Flag{Name: "-v"}}}, + git.WithRefTxHook(ctx, repo, config.Config), + ) if err != nil { return err } diff --git a/internal/gitaly/service/repository/archive.go b/internal/gitaly/service/repository/archive.go index 61abdee28ea..bce628c8937 100644 --- a/internal/gitaly/service/repository/archive.go +++ b/internal/gitaly/service/repository/archive.go @@ -81,7 +81,7 @@ func (s *server) GetArchive(in *gitalypb.GetArchiveRequest, stream gitalypb.Repo return stream.Send(&gitalypb.GetArchiveResponse{Data: p}) }) - gitlabConfig, err := json.Marshal(s.cfg) + gitlabConfig, err := json.Marshal(s.cfg.Gitlab) if err != nil { return err } diff --git a/internal/gitaly/service/repository/cleanup.go b/internal/gitaly/service/repository/cleanup.go index 00b7d49c825..39a6d1b1361 100644 --- a/internal/gitaly/service/repository/cleanup.go +++ b/internal/gitaly/service/repository/cleanup.go @@ -39,11 +39,11 @@ func (s *server) cleanupRepo(ctx context.Context, repo *gitalypb.Repository) err } worktreeThreshold := time.Now().Add(-6 * time.Hour) - if err := cleanStaleWorktrees(ctx, repo, repoPath, worktreeThreshold); err != nil { + if err := s.cleanStaleWorktrees(ctx, repo, repoPath, worktreeThreshold); err != nil { return status.Errorf(codes.Internal, "Cleanup: cleanStaleWorktrees: %v", err) } - if err := cleanDisconnectedWorktrees(ctx, repo); err != nil { + if err := s.cleanDisconnectedWorktrees(ctx, repo); err != nil { return status.Errorf(codes.Internal, "Cleanup: cleanDisconnectedWorktrees: %v", err) } @@ -104,7 +104,7 @@ func cleanPackedRefsLock(repoPath string, threshold time.Time) error { return nil } -func cleanStaleWorktrees(ctx context.Context, repo *gitalypb.Repository, repoPath string, threshold time.Time) error { +func (s *server) cleanStaleWorktrees(ctx context.Context, repo *gitalypb.Repository, repoPath string, threshold time.Time) error { worktreePath := filepath.Join(repoPath, worktreePrefix) dirInfo, err := os.Stat(worktreePath) @@ -143,7 +143,7 @@ func cleanStaleWorktrees(ctx context.Context, repo *gitalypb.Repository, repoPat return nil } -func cleanDisconnectedWorktrees(ctx context.Context, repo *gitalypb.Repository) error { +func (s *server) cleanDisconnectedWorktrees(ctx context.Context, repo *gitalypb.Repository) error { cmd, err := git.SafeCmd(ctx, repo, nil, git.SubCmd{ Name: "worktree", Flags: []git.Option{git.SubSubCmd{"prune"}}, diff --git a/internal/gitaly/service/repository/clone_from_pool_internal.go b/internal/gitaly/service/repository/clone_from_pool_internal.go index 6e3447cdfca..f3bd6bfc4be 100644 --- a/internal/gitaly/service/repository/clone_from_pool_internal.go +++ b/internal/gitaly/service/repository/clone_from_pool_internal.go @@ -119,11 +119,19 @@ func (s *server) cloneFromPool(ctx context.Context, objectPoolRepo *gitalypb.Obj return fmt.Errorf("could not get object pool path: %v", err) } - cmd, err := git.SafeBareCmd(ctx, git.CmdStream{}, nil, nil, git.SubCmd{ - Name: "clone", - Flags: []git.Option{git.Flag{Name: "--bare"}, git.Flag{Name: "--shared"}}, - PostSepArgs: []string{objectPoolPath, repositoryPath}, - }) + pbRepo, ok := repo.(*gitalypb.Repository) + if !ok { + return fmt.Errorf("expected *gitlaypb.Repository but got %T", repo) + } + + cmd, err := git.SafeBareCmd(ctx, git.CmdStream{}, nil, nil, + git.SubCmd{ + Name: "clone", + Flags: []git.Option{git.Flag{Name: "--bare"}, git.Flag{Name: "--shared"}}, + PostSepArgs: []string{objectPoolPath, repositoryPath}, + }, + git.WithRefTxHook(ctx, pbRepo, s.cfg), + ) if err != nil { return fmt.Errorf("clone with object pool start: %v", err) } diff --git a/internal/gitaly/service/repository/commit_graph.go b/internal/gitaly/service/repository/commit_graph.go index 845bf3785b0..5b2a07156c3 100644 --- a/internal/gitaly/service/repository/commit_graph.go +++ b/internal/gitaly/service/repository/commit_graph.go @@ -14,15 +14,15 @@ const ( ) // WriteCommitGraph write or update commit-graph file in a repository -func (*server) WriteCommitGraph(ctx context.Context, in *gitalypb.WriteCommitGraphRequest) (*gitalypb.WriteCommitGraphResponse, error) { - if err := writeCommitGraph(ctx, in); err != nil { +func (s *server) WriteCommitGraph(ctx context.Context, in *gitalypb.WriteCommitGraphRequest) (*gitalypb.WriteCommitGraphResponse, error) { + if err := s.writeCommitGraph(ctx, in); err != nil { return nil, helper.ErrInternal(fmt.Errorf("WriteCommitGraph: gitCommand: %v", err)) } return &gitalypb.WriteCommitGraphResponse{}, nil } -func writeCommitGraph(ctx context.Context, in *gitalypb.WriteCommitGraphRequest) error { +func (s *server) writeCommitGraph(ctx context.Context, in *gitalypb.WriteCommitGraphRequest) error { cmd, err := git.SafeCmd(ctx, in.GetRepository(), nil, git.SubCmd{ Name: "commit-graph", @@ -31,6 +31,7 @@ func writeCommitGraph(ctx context.Context, in *gitalypb.WriteCommitGraphRequest) git.Flag{Name: "--reachable"}, }, }, + git.WithRefTxHook(ctx, in.Repository, s.cfg), ) if err != nil { return err diff --git a/internal/gitaly/service/repository/create_from_url.go b/internal/gitaly/service/repository/create_from_url.go index a40488d10b9..b430b95658b 100644 --- a/internal/gitaly/service/repository/create_from_url.go +++ b/internal/gitaly/service/repository/create_from_url.go @@ -17,7 +17,7 @@ import ( "google.golang.org/grpc/status" ) -func cloneFromURLCommand(ctx context.Context, repoURL, repositoryFullPath string, stderr io.Writer) (*command.Command, error) { +func (s *server) cloneFromURLCommand(ctx context.Context, repo *gitalypb.Repository, repoURL, repositoryFullPath string, stderr io.Writer) (*command.Command, error) { u, err := url.Parse(repoURL) if err != nil { return nil, helper.ErrInternal(err) @@ -47,11 +47,14 @@ func cloneFromURLCommand(ctx context.Context, repoURL, repositoryFullPath string globalFlags = append(globalFlags, git.ValueFlag{Name: "-c", Value: fmt.Sprintf("http.extraHeader=%s", authHeader)}) } - return git.SafeBareCmd(ctx, git.CmdStream{Err: stderr}, nil, globalFlags, git.SubCmd{ - Name: "clone", - Flags: cloneFlags, - PostSepArgs: []string{u.String(), repositoryFullPath}, - }) + return git.SafeBareCmd(ctx, git.CmdStream{Err: stderr}, nil, globalFlags, + git.SubCmd{ + Name: "clone", + Flags: cloneFlags, + PostSepArgs: []string{u.String(), repositoryFullPath}, + }, + git.WithRefTxHook(ctx, repo, s.cfg), + ) } func (s *server) CreateRepositoryFromURL(ctx context.Context, req *gitalypb.CreateRepositoryFromURLRequest) (*gitalypb.CreateRepositoryFromURLResponse, error) { @@ -71,7 +74,7 @@ func (s *server) CreateRepositoryFromURL(ctx context.Context, req *gitalypb.Crea } stderr := bytes.Buffer{} - cmd, err := cloneFromURLCommand(ctx, req.GetUrl(), repositoryFullPath, &stderr) + cmd, err := s.cloneFromURLCommand(ctx, repository, req.GetUrl(), repositoryFullPath, &stderr) if err != nil { return nil, helper.ErrInternal(err) } @@ -86,7 +89,7 @@ func (s *server) CreateRepositoryFromURL(ctx context.Context, req *gitalypb.Crea return nil, status.Errorf(codes.Internal, "CreateRepositoryFromURL: create hooks failed: %v", err) } - if err := removeOriginInRepo(ctx, repository); err != nil { + if err := s.removeOriginInRepo(ctx, repository); err != nil { return nil, status.Errorf(codes.Internal, "CreateRepositoryFromURL: %v", err) } diff --git a/internal/gitaly/service/repository/create_from_url_test.go b/internal/gitaly/service/repository/create_from_url_test.go index 0807f47a539..c73216d627c 100644 --- a/internal/gitaly/service/repository/create_from_url_test.go +++ b/internal/gitaly/service/repository/create_from_url_test.go @@ -10,6 +10,7 @@ import ( "testing" "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitaly/client" "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" @@ -72,7 +73,12 @@ func TestCloneRepositoryFromUrlCommand(t *testing.T) { repositoryFullPath := "full/path/to/repository" url := fmt.Sprintf("https://%s@www.example.com/secretrepo.git", userInfo) - cmd, err := cloneFromURLCommand(ctx, url, repositoryFullPath, nil) + s := &server{ + conns: client.NewPool(), + locator: config.NewLocator(config.Config), + cfg: config.Config, + } + cmd, err := s.cloneFromURLCommand(ctx, &gitalypb.Repository{}, url, repositoryFullPath, nil) require.NoError(t, err) expectedScrubbedURL := "https://www.example.com/secretrepo.git" diff --git a/internal/gitaly/service/repository/fetch.go b/internal/gitaly/service/repository/fetch.go index 4990aac937c..9bdc059df72 100644 --- a/internal/gitaly/service/repository/fetch.go +++ b/internal/gitaly/service/repository/fetch.go @@ -52,6 +52,7 @@ func (s *server) FetchSourceBranch(ctx context.Context, req *gitalypb.FetchSourc Flags: []git.Option{git.Flag{Name: "--prune"}}, Args: []string{remote, refspec}, }, + git.WithRefTxHook(ctx, req.Repository, s.cfg), ) if err != nil { return nil, err diff --git a/internal/gitaly/service/repository/fork.go b/internal/gitaly/service/repository/fork.go index 7d46acc66dd..802d5a5c536 100644 --- a/internal/gitaly/service/repository/fork.go +++ b/internal/gitaly/service/repository/fork.go @@ -41,14 +41,20 @@ func (s *server) CreateFork(ctx context.Context, req *gitalypb.CreateForkRequest return nil, err } - cmd, err := git.SafeBareCmd(ctx, git.CmdStream{}, env, nil, git.SubCmd{ - Name: "clone", - Flags: []git.Option{git.Flag{Name: "--bare"}, git.Flag{Name: "--no-local"}}, - PostSepArgs: []string{ - fmt.Sprintf("%s:%s", gitalyssh.GitalyInternalURL, sourceRepository.RelativePath), - targetRepositoryFullPath, + cmd, err := git.SafeBareCmd(ctx, git.CmdStream{}, env, nil, + git.SubCmd{ + Name: "clone", + Flags: []git.Option{ + git.Flag{Name: "--bare"}, + git.Flag{Name: "--no-local"}, + }, + PostSepArgs: []string{ + fmt.Sprintf("%s:%s", gitalyssh.GitalyInternalURL, sourceRepository.RelativePath), + targetRepositoryFullPath, + }, }, - }) + git.WithRefTxHook(ctx, req.Repository, s.cfg), + ) if err != nil { return nil, status.Errorf(codes.Internal, "CreateFork: clone cmd start: %v", err) } @@ -56,7 +62,7 @@ func (s *server) CreateFork(ctx context.Context, req *gitalypb.CreateForkRequest return nil, status.Errorf(codes.Internal, "CreateFork: clone cmd wait: %v", err) } - if err := removeOriginInRepo(ctx, targetRepository); err != nil { + if err := s.removeOriginInRepo(ctx, targetRepository); err != nil { return nil, status.Errorf(codes.Internal, "CreateFork: %v", err) } diff --git a/internal/gitaly/service/repository/fsck.go b/internal/gitaly/service/repository/fsck.go index 4c2d5c3cc7c..ea58621068f 100644 --- a/internal/gitaly/service/repository/fsck.go +++ b/internal/gitaly/service/repository/fsck.go @@ -20,6 +20,7 @@ func (s *server) Fsck(ctx context.Context, req *gitalypb.FsckRequest) (*gitalypb cmd, err := git.SafeBareCmd(ctx, git.CmdStream{Out: &stdout, Err: &stderr}, env, []git.Option{git.ValueFlag{"--git-dir", repoPath}}, git.SubCmd{Name: "fsck"}, + git.WithRefTxHook(ctx, req.GetRepository(), s.cfg), ) if err != nil { return nil, err diff --git a/internal/gitaly/service/repository/gc.go b/internal/gitaly/service/repository/gc.go index 78980e9a5eb..d95b0952cf7 100644 --- a/internal/gitaly/service/repository/gc.go +++ b/internal/gitaly/service/repository/gc.go @@ -39,7 +39,7 @@ func (s *server) GarbageCollect(ctx context.Context, in *gitalypb.GarbageCollect return nil, err } - if err := gc(ctx, in); err != nil { + if err := s.gc(ctx, in); err != nil { return nil, err } @@ -47,7 +47,7 @@ func (s *server) GarbageCollect(ctx context.Context, in *gitalypb.GarbageCollect return nil, err } - if err := writeCommitGraph(ctx, &gitalypb.WriteCommitGraphRequest{Repository: repo}); err != nil { + if err := s.writeCommitGraph(ctx, &gitalypb.WriteCommitGraphRequest{Repository: repo}); err != nil { return nil, err } @@ -62,7 +62,7 @@ func (s *server) GarbageCollect(ctx context.Context, in *gitalypb.GarbageCollect return &gitalypb.GarbageCollectResponse{}, nil } -func gc(ctx context.Context, in *gitalypb.GarbageCollectRequest) error { +func (s *server) gc(ctx context.Context, in *gitalypb.GarbageCollectRequest) error { args := repackConfig(ctx, in.CreateBitmap) var flags []git.Option @@ -72,6 +72,7 @@ func gc(ctx context.Context, in *gitalypb.GarbageCollectRequest) error { cmd, err := git.SafeCmd(ctx, in.GetRepository(), args, git.SubCmd{Name: "gc", Flags: flags}, + git.WithRefTxHook(ctx, in.GetRepository(), s.cfg), ) if err != nil { @@ -148,7 +149,7 @@ func (s *server) cleanupKeepArounds(ctx context.Context, repo *gitalypb.Reposito continue } - if err := fixRef(ctx, repo, batch, path, refName, info.Name()); err != nil { + if err := s.fixRef(ctx, repo, batch, path, refName, info.Name()); err != nil { return err } } @@ -165,7 +166,7 @@ func checkRef(batch *catfile.Batch, refName string, info os.FileInfo) error { return err } -func fixRef(ctx context.Context, repo *gitalypb.Repository, batch *catfile.Batch, refPath string, name string, sha string) error { +func (s *server) fixRef(ctx context.Context, repo *gitalypb.Repository, batch *catfile.Batch, refPath string, name string, sha string) error { // So the ref is broken, let's get rid of it if err := os.RemoveAll(refPath); err != nil { return err @@ -177,10 +178,13 @@ func fixRef(ctx context.Context, repo *gitalypb.Repository, batch *catfile.Batch } // The name is a valid sha, recreate the ref - cmd, err := git.SafeCmd(ctx, repo, nil, git.SubCmd{ - Name: "update-ref", - Args: []string{name, sha}, - }) + cmd, err := git.SafeCmd(ctx, repo, nil, + git.SubCmd{ + Name: "update-ref", + Args: []string{name, sha}, + }, + git.WithRefTxHook(ctx, repo, s.cfg), + ) if err != nil { return err } diff --git a/internal/gitaly/service/repository/merge_base.go b/internal/gitaly/service/repository/merge_base.go index e663085a0c0..f2019deed65 100644 --- a/internal/gitaly/service/repository/merge_base.go +++ b/internal/gitaly/service/repository/merge_base.go @@ -21,10 +21,12 @@ func (s *server) FindMergeBase(ctx context.Context, req *gitalypb.FindMergeBaseR return nil, status.Errorf(codes.InvalidArgument, "FindMergeBase: at least 2 revisions are required") } - cmd, err := git.SafeCmd(ctx, req.GetRepository(), nil, git.SubCmd{ - Name: "merge-base", - Args: revisions, - }) + cmd, err := git.SafeCmd(ctx, req.GetRepository(), nil, + git.SubCmd{ + Name: "merge-base", + Args: revisions, + }, + ) if err != nil { if _, ok := status.FromError(err); ok { return nil, err diff --git a/internal/gitaly/service/repository/remove.go b/internal/gitaly/service/repository/remove.go index 3a89a1e521c..ff7058d9199 100644 --- a/internal/gitaly/service/repository/remove.go +++ b/internal/gitaly/service/repository/remove.go @@ -5,7 +5,6 @@ import ( "os" "path/filepath" - "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/internal/helper" "gitlab.com/gitlab-org/gitaly/internal/tempdir" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" @@ -17,7 +16,7 @@ func (s *server) RemoveRepository(ctx context.Context, in *gitalypb.RemoveReposi return nil, helper.ErrInternal(err) } - storage, ok := config.Config.Storage(in.GetRepository().GetStorageName()) + storage, ok := s.cfg.Storage(in.GetRepository().GetStorageName()) if !ok { return nil, helper.ErrInvalidArgumentf("storage %v not found", in.GetRepository().GetStorageName()) } diff --git a/internal/gitaly/service/repository/server.go b/internal/gitaly/service/repository/server.go index f82abaa056b..353467b3702 100644 --- a/internal/gitaly/service/repository/server.go +++ b/internal/gitaly/service/repository/server.go @@ -13,7 +13,7 @@ type server struct { conns *client.Pool internalGitalySocket string locator storage.Locator - cfg config.Gitlab + cfg config.Cfg binDir string loggingCfg config.Logging } @@ -25,7 +25,7 @@ func NewServer(cfg config.Cfg, rs *rubyserver.Server, locator storage.Locator, i locator: locator, conns: client.NewPool(), internalGitalySocket: internalGitalySocket, - cfg: cfg.Gitlab, + cfg: cfg, binDir: cfg.BinDir, loggingCfg: cfg.Logging, } diff --git a/internal/gitaly/service/repository/util.go b/internal/gitaly/service/repository/util.go index 16839fa44b9..c0662ac569b 100644 --- a/internal/gitaly/service/repository/util.go +++ b/internal/gitaly/service/repository/util.go @@ -8,8 +8,8 @@ import ( "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" ) -func removeOriginInRepo(ctx context.Context, repository *gitalypb.Repository) error { - cmd, err := git.SafeCmd(ctx, repository, nil, git.SubCmd{Name: "remote", Args: []string{"remove", "origin"}}) +func (s *server) removeOriginInRepo(ctx context.Context, repository *gitalypb.Repository) error { + cmd, err := git.SafeCmd(ctx, repository, nil, git.SubCmd{Name: "remote", Args: []string{"remove", "origin"}}, git.WithRefTxHook(ctx, repository, s.cfg)) if err != nil { return fmt.Errorf("remote cmd start: %v", err) diff --git a/internal/gitaly/service/repository/write_ref.go b/internal/gitaly/service/repository/write_ref.go index bbaf239cbab..51a38962a4e 100644 --- a/internal/gitaly/service/repository/write_ref.go +++ b/internal/gitaly/service/repository/write_ref.go @@ -15,22 +15,28 @@ func (s *server) WriteRef(ctx context.Context, req *gitalypb.WriteRefRequest) (* if err := validateWriteRefRequest(req); err != nil { return nil, helper.ErrInvalidArgument(err) } - if err := writeRef(ctx, req); err != nil { + if err := s.writeRef(ctx, req); err != nil { return nil, helper.ErrInternal(err) } return &gitalypb.WriteRefResponse{}, nil } -func writeRef(ctx context.Context, req *gitalypb.WriteRefRequest) error { +func (s *server) writeRef(ctx context.Context, req *gitalypb.WriteRefRequest) error { if string(req.Ref) == "HEAD" { - return updateSymbolicRef(ctx, req) + return s.updateSymbolicRef(ctx, req) } return updateRef(ctx, req) } -func updateSymbolicRef(ctx context.Context, req *gitalypb.WriteRefRequest) error { - cmd, err := git.SafeCmd(ctx, req.GetRepository(), nil, git.SubCmd{Name: "symbolic-ref", Args: []string{string(req.GetRef()), string(req.GetRevision())}}) +func (s *server) updateSymbolicRef(ctx context.Context, req *gitalypb.WriteRefRequest) error { + cmd, err := git.SafeCmd(ctx, req.GetRepository(), nil, + git.SubCmd{ + Name: "symbolic-ref", + Args: []string{string(req.GetRef()), string(req.GetRevision())}, + }, + git.WithRefTxHook(ctx, req.GetRepository(), s.cfg), + ) if err != nil { return fmt.Errorf("error when creating symbolic-ref command: %v", err) } diff --git a/internal/gitaly/service/smarthttp/receive_pack.go b/internal/gitaly/service/smarthttp/receive_pack.go index 31e0128baa2..f1187a2b3b4 100644 --- a/internal/gitaly/service/smarthttp/receive_pack.go +++ b/internal/gitaly/service/smarthttp/receive_pack.go @@ -4,6 +4,7 @@ import ( "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" log "github.com/sirupsen/logrus" "gitlab.com/gitlab-org/gitaly/internal/git" + "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/internal/helper" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" "gitlab.com/gitlab-org/gitaly/streamio" @@ -47,11 +48,16 @@ func (s *server) PostReceivePack(stream gitalypb.SmartHTTPService_PostReceivePac globalOpts = append(globalOpts, git.ValueFlag{"-c", o}) } - cmd, err := git.SafeBareCmd(ctx, git.CmdStream{In: stdin, Out: stdout}, nil, globalOpts, git.SubCmd{ - Name: "receive-pack", - Flags: []git.Option{git.Flag{Name: "--stateless-rpc"}}, - Args: []string{repoPath}, - }, git.WithReceivePackHooks(ctx, req, "http"), git.WithGitProtocol(ctx, req)) + cmd, err := git.SafeBareCmd(ctx, git.CmdStream{In: stdin, Out: stdout}, nil, globalOpts, + git.SubCmd{ + Name: "receive-pack", + Flags: []git.Option{git.Flag{Name: "--stateless-rpc"}}, + Args: []string{repoPath}, + }, + git.WithReceivePackHooks(ctx, req, "http"), + git.WithGitProtocol(ctx, req), + git.WithRefTxHook(ctx, req.Repository, config.Config), + ) if err != nil { return status.Errorf(codes.Unavailable, "PostReceivePack: %v", err) -- GitLab From 3b7289d39cefbefd1b9ad243ff97afd92068bbc7 Mon Sep 17 00:00:00 2001 From: Paul Okstad Date: Wed, 28 Oct 2020 15:17:42 -0700 Subject: [PATCH 04/16] SSH invoke helper whitespace trim --- internal/testhelper/testhelper.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go index 87c185bafb8..41b034aebde 100644 --- a/internal/testhelper/testhelper.go +++ b/internal/testhelper/testhelper.go @@ -660,7 +660,7 @@ func ListenGitalySSHCalls(t *testing.T, conf config.Cfg) (config.Cfg, func() []G return nil } - idx, err := strconv.Atoi(strings.TrimPrefix(filename, prefix)) + idx, err := strconv.Atoi(strings.TrimSpace(strings.TrimPrefix(filename, prefix))) if err != nil { return err } -- GitLab From a6c56ce9f3e9736244bd96071e266d0e39d41e44 Mon Sep 17 00:00:00 2001 From: Paul Okstad Date: Wed, 4 Nov 2020 21:43:52 -0800 Subject: [PATCH 05/16] DeleteRef RPC unit test fix --- internal/gitaly/service/ref/delete_refs.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal/gitaly/service/ref/delete_refs.go b/internal/gitaly/service/ref/delete_refs.go index 3545b6d01f3..6444c5743a0 100644 --- a/internal/gitaly/service/ref/delete_refs.go +++ b/internal/gitaly/service/ref/delete_refs.go @@ -22,6 +22,9 @@ func (s *server) DeleteRefs(ctx context.Context, in *gitalypb.DeleteRefsRequest) updater, err := updateref.New(ctx, in.GetRepository()) if err != nil { + if errors.Is(err, git.ErrInvalidArg) { + return nil, helper.ErrInvalidArgument(err) + } return nil, helper.ErrInternal(err) } -- GitLab From c99d915cf299a9fa4a3cce8a582313cfa44c2220 Mon Sep 17 00:00:00 2001 From: Paul Okstad Date: Thu, 5 Nov 2020 08:50:09 -0800 Subject: [PATCH 06/16] Worktree requires ref hook --- internal/git/subcommand.go | 2 -- internal/gitaly/service/operations/squash.go | 19 +++++++++++----- internal/gitaly/service/repository/cleanup.go | 22 ++++++++++++------- 3 files changed, 27 insertions(+), 16 deletions(-) diff --git a/internal/git/subcommand.go b/internal/git/subcommand.go index e804ff6afa9..c122ae1db80 100644 --- a/internal/git/subcommand.go +++ b/internal/git/subcommand.go @@ -13,7 +13,6 @@ const ( scDiff = "diff" scPackRefs = "pack-refs" scMergeBase = "merge-base" - scWorktree = "worktree" scHashObject = "hash-object" scShowRef = "show-ref" ) @@ -36,7 +35,6 @@ var knownNoRefUpdates = map[string]struct{}{ scMultiPackIndex: struct{}{}, scRepack: struct{}{}, scPackRefs: struct{}{}, - scWorktree: struct{}{}, scHashObject: struct{}{}, } diff --git a/internal/gitaly/service/operations/squash.go b/internal/gitaly/service/operations/squash.go index 8d7309c4741..161c1b71f0c 100644 --- a/internal/gitaly/service/operations/squash.go +++ b/internal/gitaly/service/operations/squash.go @@ -313,7 +313,11 @@ func (s *server) addWorktree(ctx context.Context, repo *gitalypb.Repository, wor } var stderr bytes.Buffer - cmd, err := git.SafeCmd(ctx, repo, nil, git.SubCmd{Name: "worktree", Flags: flags, Args: args}, git.WithStderr(&stderr)) + cmd, err := git.SafeCmd(ctx, repo, nil, + git.SubCmd{Name: "worktree", Flags: flags, Args: args}, + git.WithStderr(&stderr), + git.WithRefTxHook(ctx, repo, s.cfg), + ) if err != nil { return fmt.Errorf("creation of 'git worktree add': %w", gitError{ErrMsg: stderr.String(), Err: err}) } @@ -326,11 +330,14 @@ func (s *server) addWorktree(ctx context.Context, repo *gitalypb.Repository, wor } func (s *server) removeWorktree(ctx context.Context, repo *gitalypb.Repository, worktreeName string) error { - cmd, err := git.SafeCmd(ctx, repo, nil, git.SubCmd{ - Name: "worktree", - Flags: []git.Option{git.SubSubCmd{Name: "remove"}, git.Flag{Name: "--force"}}, - Args: []string{worktreeName}, - }) + cmd, err := git.SafeCmd(ctx, repo, nil, + git.SubCmd{ + Name: "worktree", + Flags: []git.Option{git.SubSubCmd{Name: "remove"}, git.Flag{Name: "--force"}}, + Args: []string{worktreeName}, + }, + git.WithRefTxHook(ctx, repo, s.cfg), + ) if err != nil { return fmt.Errorf("creation of 'worktree remove': %w", err) } diff --git a/internal/gitaly/service/repository/cleanup.go b/internal/gitaly/service/repository/cleanup.go index 39a6d1b1361..752b89c8ee7 100644 --- a/internal/gitaly/service/repository/cleanup.go +++ b/internal/gitaly/service/repository/cleanup.go @@ -126,10 +126,13 @@ func (s *server) cleanStaleWorktrees(ctx context.Context, repo *gitalypb.Reposit } if info.ModTime().Before(threshold) { - cmd, err := git.SafeCmd(ctx, repo, nil, git.SubCmd{ - Name: "worktree", - Flags: []git.Option{git.SubSubCmd{"remove"}, git.Flag{Name: "--force"}, git.SubSubCmd{info.Name()}}, - }) + cmd, err := git.SafeCmd(ctx, repo, nil, + git.SubCmd{ + Name: "worktree", + Flags: []git.Option{git.SubSubCmd{"remove"}, git.Flag{Name: "--force"}, git.SubSubCmd{info.Name()}}, + }, + git.WithRefTxHook(ctx, repo, s.cfg), + ) if err != nil { return err } @@ -144,10 +147,13 @@ func (s *server) cleanStaleWorktrees(ctx context.Context, repo *gitalypb.Reposit } func (s *server) cleanDisconnectedWorktrees(ctx context.Context, repo *gitalypb.Repository) error { - cmd, err := git.SafeCmd(ctx, repo, nil, git.SubCmd{ - Name: "worktree", - Flags: []git.Option{git.SubSubCmd{"prune"}}, - }) + cmd, err := git.SafeCmd(ctx, repo, nil, + git.SubCmd{ + Name: "worktree", + Flags: []git.Option{git.SubSubCmd{"prune"}}, + }, + git.WithRefTxHook(ctx, repo, s.cfg), + ) if err != nil { return err } -- GitLab From 4a7f257359e4da5c648184f260955da8601b4a70 Mon Sep 17 00:00:00 2001 From: Paul Okstad Date: Thu, 5 Nov 2020 09:17:19 -0800 Subject: [PATCH 07/16] Remove ref hook context marker --- internal/git/hooks.go | 14 -------------- internal/git/safecmd.go | 2 +- 2 files changed, 1 insertion(+), 15 deletions(-) diff --git a/internal/git/hooks.go b/internal/git/hooks.go index 0629d052d31..97099b859a7 100644 --- a/internal/git/hooks.go +++ b/internal/git/hooks.go @@ -51,20 +51,6 @@ func refHookEnv(ctx context.Context, repo *gitalypb.Repository, cfg config.Cfg) }, nil } -type refHookRequired struct{} - -// RequireRefHook updates the context to indicate a ref hook is required for -// the current operation -func RequireRefHook(ctx context.Context) context.Context { - return context.WithValue(ctx, refHookRequired{}, true) -} - -// IsRefHookRequired returns true if the context has been marked to indicate a -// ref hook may be required -func IsRefHookRequired(ctx context.Context) bool { - return ctx.Value(refHookRequired{}) != nil -} - // ReceivePackRequest abstracts away the different requests that end up // spawning git-receive-pack. type ReceivePackRequest interface { diff --git a/internal/git/safecmd.go b/internal/git/safecmd.go index d88c2951345..d7f011a5177 100644 --- a/internal/git/safecmd.go +++ b/internal/git/safecmd.go @@ -274,7 +274,7 @@ func handleOpts(ctx context.Context, sc Cmd, cc *cmdCfg, opts []CmdOpt) error { } } - if IsRefHookRequired(ctx) && !cc.refHookConfigured && mayUpdateRef(sc.Subcommand()) { + if !cc.refHookConfigured && mayUpdateRef(sc.Subcommand()) { return fmt.Errorf("subcommand %q: %w", sc.Subcommand(), ErrRefHookRequired) } if cc.refHookConfigured && !mayUpdateRef(sc.Subcommand()) { -- GitLab From 97119621a83292e10060785c43bd00ceffdef53b Mon Sep 17 00:00:00 2001 From: Paul Okstad Date: Thu, 5 Nov 2020 09:17:49 -0800 Subject: [PATCH 08/16] Recognize upload subcommands as read-only --- internal/git/subcommand.go | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/internal/git/subcommand.go b/internal/git/subcommand.go index c122ae1db80..f93163c5738 100644 --- a/internal/git/subcommand.go +++ b/internal/git/subcommand.go @@ -15,17 +15,21 @@ const ( scMergeBase = "merge-base" scHashObject = "hash-object" scShowRef = "show-ref" + scUploadPack = "upload-pack" + scUploadArchive = "upload-archive" ) var knownReadOnlyCmds = map[string]struct{}{ - scCatFile: struct{}{}, - scLog: struct{}{}, - scForEachRef: struct{}{}, - scRevParse: struct{}{}, - scCountObjects: struct{}{}, - scDiff: struct{}{}, - scMergeBase: struct{}{}, - scShowRef: struct{}{}, + scCatFile: struct{}{}, + scLog: struct{}{}, + scForEachRef: struct{}{}, + scRevParse: struct{}{}, + scCountObjects: struct{}{}, + scDiff: struct{}{}, + scMergeBase: struct{}{}, + scShowRef: struct{}{}, + scUploadPack: struct{}{}, + scUploadArchive: struct{}{}, } // knownNoRefUpdates indicates all repo mutating commands where it is known -- GitLab From 979310dc58d6bfeb93a7ec40b6ab2e9bcd17efcd Mon Sep 17 00:00:00 2001 From: Paul Okstad Date: Thu, 5 Nov 2020 09:21:25 -0800 Subject: [PATCH 09/16] Add ref hooks to SSH receive-pack --- .../testdata/gitlab-shell/.gitlab_shell_secret | 1 + .../repository/testdata/gitlab-shell/config.yml | 5 +++++ internal/gitaly/service/ssh/receive_pack.go | 14 ++++++++++---- 3 files changed, 16 insertions(+), 4 deletions(-) create mode 100644 internal/gitaly/service/repository/testdata/gitlab-shell/.gitlab_shell_secret create mode 100644 internal/gitaly/service/repository/testdata/gitlab-shell/config.yml diff --git a/internal/gitaly/service/repository/testdata/gitlab-shell/.gitlab_shell_secret b/internal/gitaly/service/repository/testdata/gitlab-shell/.gitlab_shell_secret new file mode 100644 index 00000000000..4ba7b338f17 --- /dev/null +++ b/internal/gitaly/service/repository/testdata/gitlab-shell/.gitlab_shell_secret @@ -0,0 +1 @@ +topsecret \ No newline at end of file diff --git a/internal/gitaly/service/repository/testdata/gitlab-shell/config.yml b/internal/gitaly/service/repository/testdata/gitlab-shell/config.yml new file mode 100644 index 00000000000..2f63765882d --- /dev/null +++ b/internal/gitaly/service/repository/testdata/gitlab-shell/config.yml @@ -0,0 +1,5 @@ +gitlab_url: http://127.0.0.1:51256 +http_settings: + user: "" + password: "" +custom_hooks_dir: "" diff --git a/internal/gitaly/service/ssh/receive_pack.go b/internal/gitaly/service/ssh/receive_pack.go index 4d62adaf0c3..d949eeb8748 100644 --- a/internal/gitaly/service/ssh/receive_pack.go +++ b/internal/gitaly/service/ssh/receive_pack.go @@ -9,6 +9,7 @@ import ( log "github.com/sirupsen/logrus" "gitlab.com/gitlab-org/gitaly/internal/command" "gitlab.com/gitlab-org/gitaly/internal/git" + "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/internal/helper" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" "gitlab.com/gitlab-org/gitaly/streamio" @@ -65,10 +66,15 @@ func (s *server) sshReceivePack(stream gitalypb.SSHService_SSHReceivePackServer, globalOpts = append(globalOpts, git.ValueFlag{"-c", o}) } - cmd, err := git.SafeBareCmd(ctx, git.CmdStream{In: stdin, Out: stdout, Err: stderr}, nil, globalOpts, git.SubCmd{ - Name: "receive-pack", - Args: []string{repoPath}, - }, git.WithReceivePackHooks(ctx, req, "ssh"), git.WithGitProtocol(ctx, req)) + cmd, err := git.SafeBareCmd(ctx, git.CmdStream{In: stdin, Out: stdout, Err: stderr}, nil, globalOpts, + git.SubCmd{ + Name: "receive-pack", + Args: []string{repoPath}, + }, + git.WithReceivePackHooks(ctx, req, "ssh"), + git.WithGitProtocol(ctx, req), + git.WithRefTxHook(ctx, req.Repository, config.Config), + ) if err != nil { return fmt.Errorf("start cmd: %v", err) -- GitLab From db0b171b4a310bfb12cfb54c0470296ed77c0cd8 Mon Sep 17 00:00:00 2001 From: Paul Okstad Date: Thu, 5 Nov 2020 09:54:20 -0800 Subject: [PATCH 10/16] Add ref hook option to SmartHTTP receive-pack --- .../service/repository/testdata/gitlab-shell/config.yml | 2 +- internal/gitaly/service/smarthttp/inforefs.go | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/internal/gitaly/service/repository/testdata/gitlab-shell/config.yml b/internal/gitaly/service/repository/testdata/gitlab-shell/config.yml index 2f63765882d..d7abadea052 100644 --- a/internal/gitaly/service/repository/testdata/gitlab-shell/config.yml +++ b/internal/gitaly/service/repository/testdata/gitlab-shell/config.yml @@ -1,4 +1,4 @@ -gitlab_url: http://127.0.0.1:51256 +gitlab_url: http://127.0.0.1:53058 http_settings: user: "" password: "" diff --git a/internal/gitaly/service/smarthttp/inforefs.go b/internal/gitaly/service/smarthttp/inforefs.go index dd4b78a242d..71034daaa41 100644 --- a/internal/gitaly/service/smarthttp/inforefs.go +++ b/internal/gitaly/service/smarthttp/inforefs.go @@ -9,6 +9,7 @@ import ( log "github.com/sirupsen/logrus" "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/git/pktline" + "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" "gitlab.com/gitlab-org/gitaly/streamio" "google.golang.org/grpc/codes" @@ -48,8 +49,10 @@ func (s *server) handleInfoRefs(ctx context.Context, service string, req *gitaly } var globalOpts []git.Option + cmdOpts := []git.CmdOpt{git.WithGitProtocol(ctx, req)} if service == "receive-pack" { globalOpts = append(globalOpts, git.ReceivePackConfig()...) + cmdOpts = append(cmdOpts, git.WithRefTxHook(ctx, req.Repository, config.Config)) } if service == "upload-pack" { @@ -64,7 +67,7 @@ func (s *server) handleInfoRefs(ctx context.Context, service string, req *gitaly Name: service, Flags: []git.Option{git.Flag{Name: "--stateless-rpc"}, git.Flag{Name: "--advertise-refs"}}, Args: []string{repoPath}, - }, git.WithGitProtocol(ctx, req)) + }, cmdOpts...) if err != nil { if _, ok := status.FromError(err); ok { -- GitLab From 3588c9cb9fbffad39af63547210578c455c9840d Mon Sep 17 00:00:00 2001 From: Paul Okstad Date: Tue, 10 Nov 2020 00:08:09 -0800 Subject: [PATCH 11/16] Inject config into object pool --- internal/git/objectpool/clone_test.go | 4 ++-- internal/git/objectpool/fetch.go | 24 ++++++++++++------- internal/git/objectpool/fetch_test.go | 8 +++---- internal/git/objectpool/pool.go | 12 ++++++---- internal/git/objectpool/pool_test.go | 21 +++++++--------- internal/git/objectpool/proto.go | 6 ++--- internal/git/objectpool/testhelper_test.go | 2 +- .../service/objectpool/alternates_test.go | 3 ++- internal/gitaly/service/objectpool/create.go | 3 ++- .../gitaly/service/objectpool/create_test.go | 6 ++--- .../objectpool/fetch_into_object_pool.go | 3 ++- .../objectpool/fetch_into_object_pool_test.go | 7 +++--- internal/gitaly/service/objectpool/get.go | 3 ++- .../gitaly/service/objectpool/get_test.go | 3 ++- .../gitaly/service/objectpool/link_test.go | 15 ++++++------ .../service/objectpool/reduplicate_test.go | 3 ++- .../service/repository/clone_from_pool.go | 4 ++-- .../repository/clone_from_pool_internal.go | 4 ++-- .../clone_from_pool_internal_test.go | 2 +- .../testdata/gitlab-shell/config.yml | 2 +- .../gitaly/service/smarthttp/inforefs_test.go | 2 +- .../gitaly/service/ssh/receive_pack_test.go | 2 +- internal/praefect/replicator_test.go | 2 +- 23 files changed, 78 insertions(+), 63 deletions(-) diff --git a/internal/git/objectpool/clone_test.go b/internal/git/objectpool/clone_test.go index cb10fd01d0a..53abcbbab4d 100644 --- a/internal/git/objectpool/clone_test.go +++ b/internal/git/objectpool/clone_test.go @@ -16,7 +16,7 @@ func TestClone(t *testing.T) { testRepo, _, cleanupFn := testhelper.NewTestRepo(t) defer cleanupFn() - pool, err := NewObjectPool(config.NewLocator(config.Config), testRepo.GetStorageName(), testhelper.NewTestObjectPoolName(t)) + pool, err := NewObjectPool(config.Config, testRepo.GetStorageName(), testhelper.NewTestObjectPoolName(t)) require.NoError(t, err) err = pool.clone(ctx, testRepo) @@ -34,7 +34,7 @@ func TestCloneExistingPool(t *testing.T) { testRepo, _, cleanupFn := testhelper.NewTestRepo(t) defer cleanupFn() - pool, err := NewObjectPool(config.NewLocator(config.Config), testRepo.GetStorageName(), testhelper.NewTestObjectPoolName(t)) + pool, err := NewObjectPool(config.Config, testRepo.GetStorageName(), testhelper.NewTestObjectPoolName(t)) require.NoError(t, err) err = pool.clone(ctx, testRepo) diff --git a/internal/git/objectpool/fetch.go b/internal/git/objectpool/fetch.go index 0e4a30c4c6d..4ac559962d3 100644 --- a/internal/git/objectpool/fetch.go +++ b/internal/git/objectpool/fetch.go @@ -17,7 +17,6 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/git/repository" "gitlab.com/gitlab-org/gitaly/internal/git/updateref" - "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/internal/helper" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" ) @@ -39,7 +38,11 @@ func (o *ObjectPool) FetchFromOrigin(ctx context.Context, origin *gitalypb.Repos return err } - getRemotes, err := git.SafeCmd(ctx, o, nil, git.SubCmd{Name: "remote"}) + opts := []git.CmdOpt{ + git.WithRefTxHook(ctx, helper.ProtoRepoFromRepo(o), o.cfg), + } + + getRemotes, err := git.SafeCmd(ctx, o, nil, git.SubCmd{Name: "remote"}, opts...) if err != nil { return err } @@ -61,7 +64,7 @@ func (o *ObjectPool) FetchFromOrigin(ctx context.Context, origin *gitalypb.Repos setOriginCmd, err = git.SafeCmd(ctx, o, nil, git.SubCmd{ Name: "remote", Args: []string{"set-url", sourceRemote, originPath}, - }, git.WithRefTxHook(ctx, helper.ProtoRepoFromRepo(o), config.Config)) + }, opts...) if err != nil { return err } @@ -69,7 +72,7 @@ func (o *ObjectPool) FetchFromOrigin(ctx context.Context, origin *gitalypb.Repos setOriginCmd, err = git.SafeCmd(ctx, o, nil, git.SubCmd{ Name: "remote", Args: []string{"add", sourceRemote, originPath}, - }, git.WithRefTxHook(ctx, helper.ProtoRepoFromRepo(o), config.Config)) + }, opts...) if err != nil { return err } @@ -84,11 +87,14 @@ func (o *ObjectPool) FetchFromOrigin(ctx context.Context, origin *gitalypb.Repos } refSpec := fmt.Sprintf("+refs/*:%s/*", sourceRefNamespace) - fetchCmd, err := git.SafeCmd(ctx, o, nil, git.SubCmd{ - Name: "fetch", - Flags: []git.Option{git.Flag{Name: "--quiet"}}, - Args: []string{sourceRemote, refSpec}, - }) + fetchCmd, err := git.SafeCmd(ctx, o, nil, + git.SubCmd{ + Name: "fetch", + Flags: []git.Option{git.Flag{Name: "--quiet"}}, + Args: []string{sourceRemote, refSpec}, + }, + opts..., + ) if err != nil { return err } diff --git a/internal/git/objectpool/fetch_test.go b/internal/git/objectpool/fetch_test.go index 4998d2b3a38..187ec42dc3c 100644 --- a/internal/git/objectpool/fetch_test.go +++ b/internal/git/objectpool/fetch_test.go @@ -18,7 +18,7 @@ func TestFetchFromOriginDangling(t *testing.T) { source, _, cleanup := testhelper.NewTestRepo(t) defer cleanup() - pool, err := NewObjectPool(config.NewLocator(config.Config), source.StorageName, testhelper.NewTestObjectPoolName(t)) + pool, err := NewObjectPool(config.Config, source.StorageName, testhelper.NewTestObjectPoolName(t)) require.NoError(t, err) ctx, cancel := testhelper.Context() @@ -90,7 +90,7 @@ func TestFetchFromOriginDeltaIslands(t *testing.T) { source, sourcePath, cleanup := testhelper.NewTestRepo(t) defer cleanup() - pool, err := NewObjectPool(config.NewLocator(config.Config), source.StorageName, testhelper.NewTestObjectPoolName(t)) + pool, err := NewObjectPool(config.Config, source.StorageName, testhelper.NewTestObjectPoolName(t)) require.NoError(t, err) ctx, cancel := testhelper.Context() @@ -116,7 +116,7 @@ func TestFetchFromOriginBitmapHashCache(t *testing.T) { source, _, cleanup := testhelper.NewTestRepo(t) defer cleanup() - pool, err := NewObjectPool(config.NewLocator(config.Config), source.StorageName, testhelper.NewTestObjectPoolName(t)) + pool, err := NewObjectPool(config.Config, source.StorageName, testhelper.NewTestObjectPoolName(t)) require.NoError(t, err) ctx, cancel := testhelper.Context() @@ -145,7 +145,7 @@ func TestFetchFromOriginRefUpdates(t *testing.T) { source, sourcePath, cleanup := testhelper.NewTestRepo(t) defer cleanup() - pool, err := NewObjectPool(config.NewLocator(config.Config), source.StorageName, testhelper.NewTestObjectPoolName(t)) + pool, err := NewObjectPool(config.Config, source.StorageName, testhelper.NewTestObjectPoolName(t)) require.NoError(t, err) poolPath := pool.FullPath() diff --git a/internal/git/objectpool/pool.go b/internal/git/objectpool/pool.go index 9cef0039420..835490538ba 100644 --- a/internal/git/objectpool/pool.go +++ b/internal/git/objectpool/pool.go @@ -13,6 +13,7 @@ import ( "strings" "gitlab.com/gitlab-org/gitaly/internal/git" + "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/internal/helper" "gitlab.com/gitlab-org/gitaly/internal/storage" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" @@ -32,6 +33,7 @@ const ErrInvalidPoolDir errString = "invalid object pool directory" // live in a pool in a distinct repository which is used as an alternate object // store for other repositories. type ObjectPool struct { + cfg config.Cfg locator storage.Locator storageName string storagePath string @@ -42,7 +44,7 @@ type ObjectPool struct { // NewObjectPool will initialize the object with the required data on the storage // shard. Relative path is validated to match the expected naming and directory // structure. If the shard cannot be found, this function returns an error. -func NewObjectPool(locator storage.Locator, storageName, relativePath string) (pool *ObjectPool, err error) { +func NewObjectPool(cfg config.Cfg, storageName, relativePath string) (pool *ObjectPool, err error) { storagePath, err := helper.GetStorageByName(storageName) if err != nil { return nil, err @@ -53,7 +55,7 @@ func NewObjectPool(locator storage.Locator, storageName, relativePath string) (p return nil, ErrInvalidPoolDir } - return &ObjectPool{locator: locator, storageName: storageName, storagePath: storagePath, relativePath: relativePath}, nil + return &ObjectPool{cfg: cfg, locator: config.NewLocator(cfg), storageName: storageName, storagePath: storagePath, relativePath: relativePath}, nil } // GetGitAlternateObjectDirectories for object pools are empty, given pools are @@ -140,7 +142,9 @@ func (o *ObjectPool) Init(ctx context.Context) (err error) { } // FromRepo returns an instance of ObjectPool that the repository points to -func FromRepo(locator storage.Locator, repo *gitalypb.Repository) (*ObjectPool, error) { +func FromRepo(cfg config.Cfg, repo *gitalypb.Repository) (*ObjectPool, error) { + locator := config.NewLocator(cfg) + dir, err := getAlternateObjectDir(locator, repo) if err != nil { return nil, err @@ -160,7 +164,7 @@ func FromRepo(locator storage.Locator, repo *gitalypb.Repository) (*ObjectPool, return nil, err } - return NewObjectPool(locator, repo.GetStorageName(), filepath.Dir(altPathRelativeToStorage)) + return NewObjectPool(cfg, repo.GetStorageName(), filepath.Dir(altPathRelativeToStorage)) } var ( diff --git a/internal/git/objectpool/pool_test.go b/internal/git/objectpool/pool_test.go index 2a2dd2c5400..e6da8db91f3 100644 --- a/internal/git/objectpool/pool_test.go +++ b/internal/git/objectpool/pool_test.go @@ -14,10 +14,10 @@ import ( ) func TestNewObjectPool(t *testing.T) { - _, err := NewObjectPool(nil, "default", testhelper.NewTestObjectPoolName(t)) + _, err := NewObjectPool(config.Config, "default", testhelper.NewTestObjectPoolName(t)) require.NoError(t, err) - _, err = NewObjectPool(nil, "mepmep", testhelper.NewTestObjectPoolName(t)) + _, err = NewObjectPool(config.Config, "mepmep", testhelper.NewTestObjectPoolName(t)) require.Error(t, err, "creating pool in storage that does not exist should fail") } @@ -27,9 +27,8 @@ func TestNewFromRepoSuccess(t *testing.T) { defer cleanup() relativePoolPath := testhelper.NewTestObjectPoolName(t) - locator := config.NewLocator(config.Config) - pool, err := NewObjectPool(locator, testRepo.GetStorageName(), relativePoolPath) + pool, err := NewObjectPool(config.Config, testRepo.GetStorageName(), relativePoolPath) require.NoError(t, err) defer pool.Remove(ctx) @@ -37,7 +36,7 @@ func TestNewFromRepoSuccess(t *testing.T) { require.NoError(t, pool.Create(ctx, testRepo)) require.NoError(t, pool.Link(ctx, testRepo)) - poolFromRepo, err := FromRepo(locator, testRepo) + poolFromRepo, err := FromRepo(config.Config, testRepo) require.NoError(t, err) require.Equal(t, relativePoolPath, poolFromRepo.relativePath) require.Equal(t, pool.storageName, poolFromRepo.storageName) @@ -47,10 +46,8 @@ func TestNewFromRepoNoObjectPool(t *testing.T) { testRepo, testRepoPath, cleanup := testhelper.NewTestRepo(t) defer cleanup() - locator := config.NewLocator(config.Config) - // no alternates file - poolFromRepo, err := FromRepo(locator, testRepo) + poolFromRepo, err := FromRepo(config.Config, testRepo) require.Equal(t, ErrAlternateObjectDirNotExist, err) require.Nil(t, poolFromRepo) @@ -83,7 +80,7 @@ func TestNewFromRepoNoObjectPool(t *testing.T) { t.Run(tc.desc, func(t *testing.T) { alternateFilePath := filepath.Join(testRepoPath, "objects", "info", "alternates") require.NoError(t, ioutil.WriteFile(alternateFilePath, tc.fileContent, 0644)) - poolFromRepo, err := FromRepo(locator, testRepo) + poolFromRepo, err := FromRepo(config.Config, testRepo) require.Equal(t, tc.expectedErr, err) require.Nil(t, poolFromRepo) @@ -101,7 +98,7 @@ func TestCreate(t *testing.T) { masterSha := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "show-ref", "master") - pool, err := NewObjectPool(config.NewLocator(config.Config), testRepo.GetStorageName(), testhelper.NewTestObjectPoolName(t)) + pool, err := NewObjectPool(config.Config, testRepo.GetStorageName(), testhelper.NewTestObjectPoolName(t)) require.NoError(t, err) err = pool.Create(ctx, testRepo) @@ -134,7 +131,7 @@ func TestCreateSubDirsExist(t *testing.T) { testRepo, _, cleanupFn := testhelper.NewTestRepo(t) defer cleanupFn() - pool, err := NewObjectPool(config.NewLocator(config.Config), testRepo.GetStorageName(), testhelper.NewTestObjectPoolName(t)) + pool, err := NewObjectPool(config.Config, testRepo.GetStorageName(), testhelper.NewTestObjectPoolName(t)) defer pool.Remove(ctx) require.NoError(t, err) @@ -155,7 +152,7 @@ func TestRemove(t *testing.T) { testRepo, _, cleanupFn := testhelper.NewTestRepo(t) defer cleanupFn() - pool, err := NewObjectPool(config.NewLocator(config.Config), testRepo.GetStorageName(), testhelper.NewTestObjectPoolName(t)) + pool, err := NewObjectPool(config.Config, testRepo.GetStorageName(), testhelper.NewTestObjectPoolName(t)) require.NoError(t, err) err = pool.Create(ctx, testRepo) diff --git a/internal/git/objectpool/proto.go b/internal/git/objectpool/proto.go index 1e69f5a50cb..f033e121790 100644 --- a/internal/git/objectpool/proto.go +++ b/internal/git/objectpool/proto.go @@ -1,13 +1,13 @@ package objectpool import ( - "gitlab.com/gitlab-org/gitaly/internal/storage" + "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" ) // FromProto returns an object pool object from a git repository object -func FromProto(locator storage.Locator, o *gitalypb.ObjectPool) (*ObjectPool, error) { - return NewObjectPool(locator, o.GetRepository().GetStorageName(), o.GetRepository().GetRelativePath()) +func FromProto(cfg config.Cfg, o *gitalypb.ObjectPool) (*ObjectPool, error) { + return NewObjectPool(cfg, o.GetRepository().GetStorageName(), o.GetRepository().GetRelativePath()) } // ToProto returns a new struct that is the protobuf definition of the ObjectPool diff --git a/internal/git/objectpool/testhelper_test.go b/internal/git/objectpool/testhelper_test.go index 21eef190a0d..20dd12e23e8 100644 --- a/internal/git/objectpool/testhelper_test.go +++ b/internal/git/objectpool/testhelper_test.go @@ -16,7 +16,7 @@ func TestMain(m *testing.M) { } func NewTestObjectPool(ctx context.Context, t *testing.T, storageName string) (*ObjectPool, func()) { - pool, err := NewObjectPool(config.NewLocator(config.Config), storageName, testhelper.NewTestObjectPoolName(t)) + pool, err := NewObjectPool(config.Config, storageName, testhelper.NewTestObjectPoolName(t)) require.NoError(t, err) return pool, func() { require.NoError(t, pool.Remove(ctx)) diff --git a/internal/gitaly/service/objectpool/alternates_test.go b/internal/gitaly/service/objectpool/alternates_test.go index f9ff064e231..a281ce3e699 100644 --- a/internal/gitaly/service/objectpool/alternates_test.go +++ b/internal/gitaly/service/objectpool/alternates_test.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/git/objectpool" + "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" gconfig "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" @@ -28,7 +29,7 @@ func TestDisconnectGitAlternates(t *testing.T) { testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t) defer cleanupFn() - pool, err := objectpool.NewObjectPool(locator, testRepo.GetStorageName(), testhelper.NewTestObjectPoolName(t)) + pool, err := objectpool.NewObjectPool(config.Config, testRepo.GetStorageName(), testhelper.NewTestObjectPoolName(t)) require.NoError(t, err) defer pool.Remove(ctx) diff --git a/internal/gitaly/service/objectpool/create.go b/internal/gitaly/service/objectpool/create.go index 407e56287cf..7cd4f868625 100644 --- a/internal/gitaly/service/objectpool/create.go +++ b/internal/gitaly/service/objectpool/create.go @@ -4,6 +4,7 @@ import ( "context" "gitlab.com/gitlab-org/gitaly/internal/git/objectpool" + "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/internal/helper" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" "google.golang.org/grpc/codes" @@ -67,7 +68,7 @@ func (s *server) poolForRequest(req poolRequest) (*objectpool.ObjectPool, error) return nil, errMissingPool } - pool, err := objectpool.NewObjectPool(s.locator, poolRepo.GetStorageName(), poolRepo.GetRelativePath()) + pool, err := objectpool.NewObjectPool(config.Config, poolRepo.GetStorageName(), poolRepo.GetRelativePath()) if err != nil { if err == objectpool.ErrInvalidPoolDir { return nil, errInvalidPoolDir diff --git a/internal/gitaly/service/objectpool/create_test.go b/internal/gitaly/service/objectpool/create_test.go index 69bfeb0f138..848520fb0c2 100644 --- a/internal/gitaly/service/objectpool/create_test.go +++ b/internal/gitaly/service/objectpool/create_test.go @@ -29,7 +29,7 @@ func TestCreate(t *testing.T) { testRepo, _, cleanupFn := testhelper.NewTestRepo(t) defer cleanupFn() - pool, err := objectpool.NewObjectPool(locator, "default", testhelper.NewTestObjectPoolName(t)) + pool, err := objectpool.NewObjectPool(config.Config, "default", testhelper.NewTestObjectPoolName(t)) require.NoError(t, err) poolReq := &gitalypb.CreateObjectPoolRequest{ @@ -77,7 +77,7 @@ func TestUnsuccessfulCreate(t *testing.T) { defer cleanupFn() validPoolPath := testhelper.NewTestObjectPoolName(t) - pool, err := objectpool.NewObjectPool(locator, "default", validPoolPath) + pool, err := objectpool.NewObjectPool(config.Config, "default", validPoolPath) require.NoError(t, err) defer pool.Remove(ctx) @@ -177,7 +177,7 @@ func TestDelete(t *testing.T) { defer cleanupFn() validPoolPath := testhelper.NewTestObjectPoolName(t) - pool, err := objectpool.NewObjectPool(locator, "default", validPoolPath) + pool, err := objectpool.NewObjectPool(config.Config, "default", validPoolPath) require.NoError(t, err) require.NoError(t, pool.Create(ctx, testRepo)) diff --git a/internal/gitaly/service/objectpool/fetch_into_object_pool.go b/internal/gitaly/service/objectpool/fetch_into_object_pool.go index e289dccd828..6c309447b47 100644 --- a/internal/gitaly/service/objectpool/fetch_into_object_pool.go +++ b/internal/gitaly/service/objectpool/fetch_into_object_pool.go @@ -7,6 +7,7 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/git/objectpool" "gitlab.com/gitlab-org/gitaly/internal/git/stats" + "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/internal/helper" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" ) @@ -16,7 +17,7 @@ func (s *server) FetchIntoObjectPool(ctx context.Context, req *gitalypb.FetchInt return nil, helper.ErrInvalidArgument(err) } - objectPool, err := objectpool.FromProto(s.locator, req.GetObjectPool()) + objectPool, err := objectpool.FromProto(config.Config, req.GetObjectPool()) if err != nil { return nil, helper.ErrInvalidArgument(fmt.Errorf("object pool invalid: %v", err)) } diff --git a/internal/gitaly/service/objectpool/fetch_into_object_pool_test.go b/internal/gitaly/service/objectpool/fetch_into_object_pool_test.go index b3503381e18..f1da33c6c32 100644 --- a/internal/gitaly/service/objectpool/fetch_into_object_pool_test.go +++ b/internal/gitaly/service/objectpool/fetch_into_object_pool_test.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/internal/git/objectpool" + "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" gconfig "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" @@ -35,7 +36,7 @@ func TestFetchIntoObjectPool_Success(t *testing.T) { repoCommit := testhelper.CreateCommit(t, testRepoPath, t.Name(), &testhelper.CreateCommitOpts{Message: t.Name()}) - pool, err := objectpool.NewObjectPool(locator, "default", testhelper.NewTestObjectPoolName(t)) + pool, err := objectpool.NewObjectPool(config.Config, "default", testhelper.NewTestObjectPoolName(t)) require.NoError(t, err) defer pool.Remove(ctx) @@ -89,7 +90,7 @@ func TestFetchIntoObjectPool_CollectLogStatistics(t *testing.T) { testRepo, _, cleanupFn := testhelper.NewTestRepo(t) defer cleanupFn() - pool, err := objectpool.NewObjectPool(locator, "default", testhelper.NewTestObjectPoolName(t)) + pool, err := objectpool.NewObjectPool(config.Config, "default", testhelper.NewTestObjectPoolName(t)) require.NoError(t, err) defer pool.Remove(ctx) @@ -127,7 +128,7 @@ func TestFetchIntoObjectPool_Failure(t *testing.T) { testRepo, _, cleanupFn := testhelper.NewTestRepo(t) defer cleanupFn() - pool, err := objectpool.NewObjectPool(locator, "default", testhelper.NewTestObjectPoolName(t)) + pool, err := objectpool.NewObjectPool(config.Config, "default", testhelper.NewTestObjectPoolName(t)) require.NoError(t, err) defer pool.Remove(ctx) diff --git a/internal/gitaly/service/objectpool/get.go b/internal/gitaly/service/objectpool/get.go index c409ee8693c..06788e52bbf 100644 --- a/internal/gitaly/service/objectpool/get.go +++ b/internal/gitaly/service/objectpool/get.go @@ -6,6 +6,7 @@ import ( "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" "gitlab.com/gitlab-org/gitaly/internal/git/objectpool" + "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/internal/helper" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" ) @@ -15,7 +16,7 @@ func (s *server) GetObjectPool(ctx context.Context, in *gitalypb.GetObjectPoolRe return nil, helper.ErrInternal(errors.New("repository is empty")) } - objectPool, err := objectpool.FromRepo(s.locator, in.GetRepository()) + objectPool, err := objectpool.FromRepo(config.Config, in.GetRepository()) if err != nil { ctxlogrus.Extract(ctx). diff --git a/internal/gitaly/service/objectpool/get_test.go b/internal/gitaly/service/objectpool/get_test.go index 7294daa5340..11dfffeea2b 100644 --- a/internal/gitaly/service/objectpool/get_test.go +++ b/internal/gitaly/service/objectpool/get_test.go @@ -8,6 +8,7 @@ import ( "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/internal/git/objectpool" + "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" gconfig "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" @@ -26,7 +27,7 @@ func TestGetObjectPoolSuccess(t *testing.T) { relativePoolPath := testhelper.NewTestObjectPoolName(t) - pool, err := objectpool.NewObjectPool(locator, testRepo.GetStorageName(), relativePoolPath) + pool, err := objectpool.NewObjectPool(config.Config, testRepo.GetStorageName(), relativePoolPath) require.NoError(t, err) poolCtx, cancel := testhelper.Context() diff --git a/internal/gitaly/service/objectpool/link_test.go b/internal/gitaly/service/objectpool/link_test.go index 1a71288bba8..a5f0fa7dbe9 100644 --- a/internal/gitaly/service/objectpool/link_test.go +++ b/internal/gitaly/service/objectpool/link_test.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/internal/git/log" "gitlab.com/gitlab-org/gitaly/internal/git/objectpool" + "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" gconfig "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/internal/storage" "gitlab.com/gitlab-org/gitaly/internal/testhelper" @@ -30,7 +31,7 @@ func TestLink(t *testing.T) { testRepo, _, cleanupFn := testhelper.NewTestRepo(t) defer cleanupFn() - pool, err := objectpool.NewObjectPool(locator, testRepo.GetStorageName(), testhelper.NewTestObjectPoolName(t)) + pool, err := objectpool.NewObjectPool(config.Config, testRepo.GetStorageName(), testhelper.NewTestObjectPoolName(t)) require.NoError(t, err) require.NoError(t, pool.Remove(ctx), "make sure pool does not exist at start of test") @@ -104,7 +105,7 @@ func TestLinkIdempotent(t *testing.T) { testRepo, _, cleanupFn := testhelper.NewTestRepo(t) defer cleanupFn() - pool, err := objectpool.NewObjectPool(locator, testRepo.GetStorageName(), testhelper.NewTestObjectPoolName(t)) + pool, err := objectpool.NewObjectPool(config.Config, testRepo.GetStorageName(), testhelper.NewTestObjectPoolName(t)) require.NoError(t, err) defer pool.Remove(ctx) require.NoError(t, pool.Create(ctx, testRepo)) @@ -135,7 +136,7 @@ func TestLinkNoClobber(t *testing.T) { testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t) defer cleanupFn() - pool, err := objectpool.NewObjectPool(locator, testRepo.GetStorageName(), testhelper.NewTestObjectPoolName(t)) + pool, err := objectpool.NewObjectPool(config.Config, testRepo.GetStorageName(), testhelper.NewTestObjectPoolName(t)) require.NoError(t, err) defer pool.Remove(ctx) @@ -175,7 +176,7 @@ func TestLinkNoPool(t *testing.T) { testRepo, _, cleanupFn := testhelper.NewTestRepo(t) defer cleanupFn() - pool, err := objectpool.NewObjectPool(locator, testRepo.GetStorageName(), testhelper.NewTestObjectPoolName(t)) + pool, err := objectpool.NewObjectPool(config.Config, testRepo.GetStorageName(), testhelper.NewTestObjectPoolName(t)) require.NoError(t, err) // intentionally do not call pool.Create defer pool.Remove(ctx) @@ -211,7 +212,7 @@ func TestUnlink(t *testing.T) { deletedRepo, deletedRepoPath, removeDeletedRepo := testhelper.NewTestRepo(t) defer removeDeletedRepo() - pool, err := objectpool.NewObjectPool(locator, testRepo.GetStorageName(), testhelper.NewTestObjectPoolName(t)) + pool, err := objectpool.NewObjectPool(config.Config, testRepo.GetStorageName(), testhelper.NewTestObjectPoolName(t)) require.NoError(t, err) defer pool.Remove(ctx) @@ -222,7 +223,7 @@ func TestUnlink(t *testing.T) { removeDeletedRepo() testhelper.AssertPathNotExists(t, deletedRepoPath) - pool2, err := objectpool.NewObjectPool(locator, testRepo.GetStorageName(), testhelper.NewTestObjectPoolName(t)) + pool2, err := objectpool.NewObjectPool(config.Config, testRepo.GetStorageName(), testhelper.NewTestObjectPoolName(t)) require.NoError(t, err) require.NoError(t, pool2.Create(ctx, testRepo), "create pool 2") defer pool2.Remove(ctx) @@ -321,7 +322,7 @@ func TestUnlinkIdempotent(t *testing.T) { testRepo, _, cleanupFn := testhelper.NewTestRepo(t) defer cleanupFn() - pool, err := objectpool.NewObjectPool(locator, testRepo.GetStorageName(), testhelper.NewTestObjectPoolName(t)) + pool, err := objectpool.NewObjectPool(config.Config, testRepo.GetStorageName(), testhelper.NewTestObjectPoolName(t)) require.NoError(t, err) defer pool.Remove(ctx) require.NoError(t, pool.Create(ctx, testRepo)) diff --git a/internal/gitaly/service/objectpool/reduplicate_test.go b/internal/gitaly/service/objectpool/reduplicate_test.go index c5b1971260c..b3e018e6514 100644 --- a/internal/gitaly/service/objectpool/reduplicate_test.go +++ b/internal/gitaly/service/objectpool/reduplicate_test.go @@ -7,6 +7,7 @@ import ( "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/git/objectpool" + "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" gconfig "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" @@ -26,7 +27,7 @@ func TestReduplicate(t *testing.T) { testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t) defer cleanupFn() - pool, err := objectpool.NewObjectPool(locator, testRepo.GetStorageName(), testhelper.NewTestObjectPoolName(t)) + pool, err := objectpool.NewObjectPool(config.Config, testRepo.GetStorageName(), testhelper.NewTestObjectPoolName(t)) require.NoError(t, err) defer pool.Remove(ctx) require.NoError(t, pool.Create(ctx, testRepo)) diff --git a/internal/gitaly/service/repository/clone_from_pool.go b/internal/gitaly/service/repository/clone_from_pool.go index 6f07fb0f258..2e7186a3ddf 100644 --- a/internal/gitaly/service/repository/clone_from_pool.go +++ b/internal/gitaly/service/repository/clone_from_pool.go @@ -32,7 +32,7 @@ func (s *server) CloneFromPool(ctx context.Context, req *gitalypb.CloneFromPoolR return nil, helper.ErrInternalf("fetch http remote: %v", err) } - objectPool, err := objectpool.FromProto(s.locator, req.GetPool()) + objectPool, err := objectpool.FromProto(s.cfg, req.GetPool()) if err != nil { return nil, helper.ErrInternalf("get object pool from request: %v", err) } @@ -54,7 +54,7 @@ func (s *server) validateCloneFromPoolRequestRepositoryState(req *gitalypb.Clone return errors.New("target reopsitory already exists") } - objectPool, err := objectpool.FromProto(s.locator, req.GetPool()) + objectPool, err := objectpool.FromProto(s.cfg, req.GetPool()) if err != nil { return fmt.Errorf("getting object pool from repository: %v", err) } diff --git a/internal/gitaly/service/repository/clone_from_pool_internal.go b/internal/gitaly/service/repository/clone_from_pool_internal.go index f3bd6bfc4be..7f9dbdad186 100644 --- a/internal/gitaly/service/repository/clone_from_pool_internal.go +++ b/internal/gitaly/service/repository/clone_from_pool_internal.go @@ -46,7 +46,7 @@ func (s *server) CloneFromPoolInternal(ctx context.Context, req *gitalypb.CloneF return nil, helper.ErrInternalf("fetch internal remote failed") } - objectPool, err := objectpool.FromProto(s.locator, req.GetPool()) + objectPool, err := objectpool.FromProto(s.cfg, req.GetPool()) if err != nil { return nil, helper.ErrInternalf("get object pool from request: %v", err) } @@ -68,7 +68,7 @@ func (s *server) validateCloneFromPoolInternalRequestRepositoryState(req *gitaly return errors.New("target reopsitory already exists") } - objectPool, err := objectpool.FromProto(s.locator, req.GetPool()) + objectPool, err := objectpool.FromProto(s.cfg, req.GetPool()) if err != nil { return fmt.Errorf("getting object pool from repository: %v", err) } diff --git a/internal/gitaly/service/repository/clone_from_pool_internal_test.go b/internal/gitaly/service/repository/clone_from_pool_internal_test.go index 719f8b6b2a3..942d4feb6e3 100644 --- a/internal/gitaly/service/repository/clone_from_pool_internal_test.go +++ b/internal/gitaly/service/repository/clone_from_pool_internal_test.go @@ -22,7 +22,7 @@ func NewTestObjectPool(t *testing.T) (*objectpool.ObjectPool, *gitalypb.Reposito relativePath := testhelper.NewTestObjectPoolName(t) repo := testhelper.CreateRepo(t, storagePath, relativePath) - pool, err := objectpool.NewObjectPool(config.NewLocator(config.Config), repo.GetStorageName(), relativePath) + pool, err := objectpool.NewObjectPool(config.Config, repo.GetStorageName(), relativePath) require.NoError(t, err) return pool, repo diff --git a/internal/gitaly/service/repository/testdata/gitlab-shell/config.yml b/internal/gitaly/service/repository/testdata/gitlab-shell/config.yml index d7abadea052..bcfe53ab69d 100644 --- a/internal/gitaly/service/repository/testdata/gitlab-shell/config.yml +++ b/internal/gitaly/service/repository/testdata/gitlab-shell/config.yml @@ -1,4 +1,4 @@ -gitlab_url: http://127.0.0.1:53058 +gitlab_url: http://127.0.0.1:51981 http_settings: user: "" password: "" diff --git a/internal/gitaly/service/smarthttp/inforefs_test.go b/internal/gitaly/service/smarthttp/inforefs_test.go index 692f7f13215..d97f84c7125 100644 --- a/internal/gitaly/service/smarthttp/inforefs_test.go +++ b/internal/gitaly/service/smarthttp/inforefs_test.go @@ -193,7 +193,7 @@ func TestObjectPoolRefAdvertisementHiding(t *testing.T) { ctx, cancel := testhelper.Context() defer cancel() - pool, err := objectpool.NewObjectPool(config.NewLocator(config.Config), repo.GetStorageName(), testhelper.NewTestObjectPoolName(t)) + pool, err := objectpool.NewObjectPool(config.Config, repo.GetStorageName(), testhelper.NewTestObjectPoolName(t)) require.NoError(t, err) require.NoError(t, pool.Create(ctx, repo)) diff --git a/internal/gitaly/service/ssh/receive_pack_test.go b/internal/gitaly/service/ssh/receive_pack_test.go index 3dc2049751f..f0ebd39e58f 100644 --- a/internal/gitaly/service/ssh/receive_pack_test.go +++ b/internal/gitaly/service/ssh/receive_pack_test.go @@ -177,7 +177,7 @@ func TestObjectPoolRefAdvertisementHidingSSH(t *testing.T) { repo, _, cleanupFn := testhelper.NewTestRepo(t) defer cleanupFn() - pool, err := objectpool.NewObjectPool(config.NewLocator(config.Config), repo.GetStorageName(), testhelper.NewTestObjectPoolName(t)) + pool, err := objectpool.NewObjectPool(config.Config, repo.GetStorageName(), testhelper.NewTestObjectPoolName(t)) require.NoError(t, err) require.NoError(t, pool.Create(ctx, repo)) diff --git a/internal/praefect/replicator_test.go b/internal/praefect/replicator_test.go index 6eb2132ca4f..fb9da92223b 100644 --- a/internal/praefect/replicator_test.go +++ b/internal/praefect/replicator_test.go @@ -96,7 +96,7 @@ func TestReplMgr_ProcessBacklog(t *testing.T) { // create object pool on the source objectPoolPath := testhelper.NewTestObjectPoolName(t) - pool, err := objectpool.NewObjectPool(gitaly_config.NewLocator(gitaly_config.Config), testRepo.GetStorageName(), objectPoolPath) + pool, err := objectpool.NewObjectPool(gitaly_config.Config, testRepo.GetStorageName(), objectPoolPath) require.NoError(t, err) poolCtx, cancel := testhelper.Context() -- GitLab From 926905d73ad19dfdb9d1d5a905200252227ac09e Mon Sep 17 00:00:00 2001 From: Paul Okstad Date: Tue, 10 Nov 2020 13:16:28 -0800 Subject: [PATCH 12/16] More read-only subcommands --- internal/git/subcommand.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/internal/git/subcommand.go b/internal/git/subcommand.go index f93163c5738..0a837723b0d 100644 --- a/internal/git/subcommand.go +++ b/internal/git/subcommand.go @@ -17,6 +17,15 @@ const ( scShowRef = "show-ref" scUploadPack = "upload-pack" scUploadArchive = "upload-archive" + scBlame = "blame" + scLsTree = "ls-tree" + scRevList = "rev-list" + scLsRemote = "ls-remote" + scFsck = "fsck" + scGrep = "grep" + scBundle = "bundle" + scArchive = "archive" + scFormatPatch = "format-patch" ) var knownReadOnlyCmds = map[string]struct{}{ @@ -30,6 +39,15 @@ var knownReadOnlyCmds = map[string]struct{}{ scShowRef: struct{}{}, scUploadPack: struct{}{}, scUploadArchive: struct{}{}, + scBlame: struct{}{}, + scLsTree: struct{}{}, + scRevList: struct{}{}, + scLsRemote: struct{}{}, + scFsck: struct{}{}, + scGrep: struct{}{}, + scBundle: struct{}{}, + scArchive: struct{}{}, + scFormatPatch: struct{}{}, } // knownNoRefUpdates indicates all repo mutating commands where it is known -- GitLab From e474be18328a9b4199fb1cae36461ef64d926b19 Mon Sep 17 00:00:00 2001 From: Paul Okstad Date: Tue, 10 Nov 2020 00:18:35 -0800 Subject: [PATCH 13/16] Add and remove ref hooks as needed Once the cache middleware was removed, it created new failures in CI that alert us to where ref hook options are needed. This updates those places and removes places where it is no longer needed due to updates in internal/git/subcommand.go --- internal/git/remote.go | 4 ++++ internal/git/repository.go | 1 + internal/git/safecmd_test.go | 13 ++++++++----- internal/gitaly/service/ref/refs.go | 15 +++++++++------ internal/gitaly/service/repository/fsck.go | 1 - .../repository/testdata/gitlab-shell/config.yml | 5 ----- 6 files changed, 22 insertions(+), 17 deletions(-) delete mode 100644 internal/gitaly/service/repository/testdata/gitlab-shell/config.yml diff --git a/internal/git/remote.go b/internal/git/remote.go index 2e9cf0b119a..372bd577d77 100644 --- a/internal/git/remote.go +++ b/internal/git/remote.go @@ -8,6 +8,7 @@ import ( "gitlab.com/gitlab-org/gitaly/client" "gitlab.com/gitlab-org/gitaly/internal/git/repository" + "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/internal/helper" "gitlab.com/gitlab-org/gitaly/internal/storage" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" @@ -191,6 +192,7 @@ func (repo RepositoryRemote) Add(ctx context.Context, name, url string, opts Rem Args: []string{name, url}, }, WithStderr(&stderr), + WithRefTxHook(ctx, helper.ProtoRepoFromRepo(repo.repo), config.Config), ) if err != nil { return err @@ -220,6 +222,7 @@ func (repo RepositoryRemote) Remove(ctx context.Context, name string) error { Args: []string{name}, }, WithStderr(&stderr), + WithRefTxHook(ctx, helper.ProtoRepoFromRepo(repo.repo), config.Config), ) if err != nil { return err @@ -264,6 +267,7 @@ func (repo RepositoryRemote) SetURL(ctx context.Context, name, url string, opts Args: []string{name, url}, }, WithStderr(&stderr), + WithRefTxHook(ctx, helper.ProtoRepoFromRepo(repo.repo), config.Config), ) if err != nil { return err diff --git a/internal/git/repository.go b/internal/git/repository.go index b3b65adc0ba..81a4331928a 100644 --- a/internal/git/repository.go +++ b/internal/git/repository.go @@ -421,6 +421,7 @@ func (repo *localRepository) FetchRemote(ctx context.Context, remoteName string, Args: []string{remoteName}, }, WithStderr(opts.Stderr), + WithRefTxHook(ctx, helper.ProtoRepoFromRepo(repo.repo), config.Config), ) if err != nil { return err diff --git a/internal/git/safecmd_test.go b/internal/git/safecmd_test.go index 9c60ab1cf68..735b34d61d1 100644 --- a/internal/git/safecmd_test.go +++ b/internal/git/safecmd_test.go @@ -112,6 +112,7 @@ func TestSafeCmdInvalidArg(t *testing.T) { &gitalypb.Repository{}, tt.globals, tt.subCmd, + git.WithRefTxHook(context.Background(), &gitalypb.Repository{}, config.Config), ) require.EqualError(t, err, tt.errMsg) require.True(t, git.IsInvalidArgErr(err)) @@ -198,21 +199,22 @@ func TestSafeCmdValid(t *testing.T) { expectArgs: []string{"--contributing", "--author", "a-gopher", "accept", "--is-important", "--why", "looking-for-first-contribution", "mr", endOfOptions}, }, } { - cmd, err := git.SafeCmd(ctx, testRepo, tt.globals, tt.subCmd) + opts := []git.CmdOpt{git.WithRefTxHook(ctx, &gitalypb.Repository{}, config.Config)} + cmd, err := git.SafeCmd(ctx, testRepo, tt.globals, tt.subCmd, opts...) require.NoError(t, err) // ignore first 3 indeterministic args (executable path and repo args) require.Equal(t, tt.expectArgs, cmd.Args()[3:]) - cmd, err = git.SafeCmdWithEnv(ctx, nil, testRepo, tt.globals, tt.subCmd) + cmd, err = git.SafeCmdWithEnv(ctx, nil, testRepo, tt.globals, tt.subCmd, opts...) require.NoError(t, err) // ignore first 3 indeterministic args (executable path and repo args) require.Equal(t, tt.expectArgs, cmd.Args()[3:]) - cmd, err = git.SafeStdinCmd(ctx, testRepo, tt.globals, tt.subCmd) + cmd, err = git.SafeStdinCmd(ctx, testRepo, tt.globals, tt.subCmd, opts...) require.NoError(t, err) require.Equal(t, tt.expectArgs, cmd.Args()[3:]) - cmd, err = git.SafeBareCmd(ctx, git.CmdStream{}, nil, tt.globals, tt.subCmd) + cmd, err = git.SafeBareCmd(ctx, git.CmdStream{}, nil, tt.globals, tt.subCmd, opts...) require.NoError(t, err) // ignore first indeterministic arg (executable path) require.Equal(t, tt.expectArgs, cmd.Args()[1:]) @@ -243,7 +245,8 @@ func TestSafeCmdWithEnv(t *testing.T) { env := []string{"TEST_VAR1=1", "TEST_VAR2=2"} - cmd, err := git.SafeCmdWithEnv(ctx, env, testRepo, globals, subCmd) + opts := []git.CmdOpt{git.WithRefTxHook(ctx, &gitalypb.Repository{}, config.Config)} + cmd, err := git.SafeCmdWithEnv(ctx, env, testRepo, globals, subCmd, opts...) require.NoError(t, err) // ignore first 3 indeterministic args (executable path and repo args) require.Equal(t, expectArgs, cmd.Args()[3:]) diff --git a/internal/gitaly/service/ref/refs.go b/internal/gitaly/service/ref/refs.go index d439a06e003..c318188de69 100644 --- a/internal/gitaly/service/ref/refs.go +++ b/internal/gitaly/service/ref/refs.go @@ -413,13 +413,16 @@ func parseTagLine(c *catfile.Batch, tagLine string) (*gitalypb.Tag, error) { } func findTag(ctx context.Context, repository *gitalypb.Repository, tagName []byte) (*gitalypb.Tag, error) { - tagCmd, err := git.SafeCmd(ctx, repository, nil, git.SubCmd{ - Name: "tag", - Flags: []git.Option{ - git.Flag{Name: "-l"}, git.ValueFlag{"--format", tagFormat}, + tagCmd, err := git.SafeCmd(ctx, repository, nil, + git.SubCmd{ + Name: "tag", + Flags: []git.Option{ + git.Flag{Name: "-l"}, git.ValueFlag{"--format", tagFormat}, + }, + Args: []string{string(tagName)}, }, - Args: []string{string(tagName)}, - }) + git.WithRefTxHook(ctx, repository, config.Config), + ) if err != nil { return nil, fmt.Errorf("for-each-ref error: %v", err) } diff --git a/internal/gitaly/service/repository/fsck.go b/internal/gitaly/service/repository/fsck.go index ea58621068f..4c2d5c3cc7c 100644 --- a/internal/gitaly/service/repository/fsck.go +++ b/internal/gitaly/service/repository/fsck.go @@ -20,7 +20,6 @@ func (s *server) Fsck(ctx context.Context, req *gitalypb.FsckRequest) (*gitalypb cmd, err := git.SafeBareCmd(ctx, git.CmdStream{Out: &stdout, Err: &stderr}, env, []git.Option{git.ValueFlag{"--git-dir", repoPath}}, git.SubCmd{Name: "fsck"}, - git.WithRefTxHook(ctx, req.GetRepository(), s.cfg), ) if err != nil { return nil, err diff --git a/internal/gitaly/service/repository/testdata/gitlab-shell/config.yml b/internal/gitaly/service/repository/testdata/gitlab-shell/config.yml deleted file mode 100644 index bcfe53ab69d..00000000000 --- a/internal/gitaly/service/repository/testdata/gitlab-shell/config.yml +++ /dev/null @@ -1,5 +0,0 @@ -gitlab_url: http://127.0.0.1:51981 -http_settings: - user: "" - password: "" -custom_hooks_dir: "" -- GitLab From 0629fb92d399e921766439f63d983cccbce910e8 Mon Sep 17 00:00:00 2001 From: Paul Okstad Date: Tue, 10 Nov 2020 23:00:02 -0800 Subject: [PATCH 14/16] Inject config in FetchInternalRemote --- internal/gitaly/service/ref/refs.go | 4 ++-- internal/gitaly/service/ref/refs_test.go | 3 ++- internal/gitaly/service/remote/fetch_internal_remote.go | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/internal/gitaly/service/ref/refs.go b/internal/gitaly/service/ref/refs.go index c318188de69..e649869c274 100644 --- a/internal/gitaly/service/ref/refs.go +++ b/internal/gitaly/service/ref/refs.go @@ -214,11 +214,11 @@ func _headReference(ctx context.Context, repo *gitalypb.Repository) ([]byte, err } // SetDefaultBranchRef overwrites the default branch ref for the repository -func SetDefaultBranchRef(ctx context.Context, repo *gitalypb.Repository, ref string) error { +func SetDefaultBranchRef(ctx context.Context, repo *gitalypb.Repository, ref string, cfg config.Cfg) error { cmd, err := git.SafeCmd(ctx, repo, nil, git.SubCmd{ Name: "symbolic-ref", Args: []string{"HEAD", ref}, - }, git.WithRefTxHook(ctx, repo, config.Config)) + }, git.WithRefTxHook(ctx, repo, cfg)) if err != nil { return err } diff --git a/internal/gitaly/service/ref/refs_test.go b/internal/gitaly/service/ref/refs_test.go index 72c5070b294..112dad54ff3 100644 --- a/internal/gitaly/service/ref/refs_test.go +++ b/internal/gitaly/service/ref/refs_test.go @@ -17,6 +17,7 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/git/catfile" "gitlab.com/gitlab-org/gitaly/internal/git/log" "gitlab.com/gitlab-org/gitaly/internal/git/updateref" + "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/internal/helper" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" @@ -328,7 +329,7 @@ func TestSetDefaultBranchRef(t *testing.T) { testRepo, _, cleanupFn := testhelper.NewTestRepo(t) defer cleanupFn() - err := SetDefaultBranchRef(ctx, testRepo, tc.ref) + err := SetDefaultBranchRef(ctx, testRepo, tc.ref, config.Config) require.NoError(t, err) newRef, err := DefaultBranchName(ctx, testRepo) diff --git a/internal/gitaly/service/remote/fetch_internal_remote.go b/internal/gitaly/service/remote/fetch_internal_remote.go index 20c24a7576d..144d83fe516 100644 --- a/internal/gitaly/service/remote/fetch_internal_remote.go +++ b/internal/gitaly/service/remote/fetch_internal_remote.go @@ -60,7 +60,7 @@ func (s *server) FetchInternalRemote(ctx context.Context, req *gitalypb.FetchInt } if !bytes.Equal(defaultBranch, remoteDefaultBranch) { - if err := ref.SetDefaultBranchRef(ctx, req.Repository, string(remoteDefaultBranch)); err != nil { + if err := ref.SetDefaultBranchRef(ctx, req.Repository, string(remoteDefaultBranch), config.Config); err != nil { return nil, status.Errorf(codes.Internal, "FetchInternalRemote: set default branch: %v", err) } } -- GitLab From fe5b51db143800377cffbf2f64ef5b137ae5cfe3 Mon Sep 17 00:00:00 2001 From: Paul Okstad Date: Tue, 10 Nov 2020 23:18:09 -0800 Subject: [PATCH 15/16] Remove redundant server struct fields --- internal/gitaly/service/repository/create_from_url_test.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/internal/gitaly/service/repository/create_from_url_test.go b/internal/gitaly/service/repository/create_from_url_test.go index c73216d627c..636d35e4fbb 100644 --- a/internal/gitaly/service/repository/create_from_url_test.go +++ b/internal/gitaly/service/repository/create_from_url_test.go @@ -10,7 +10,6 @@ import ( "testing" "github.com/stretchr/testify/require" - "gitlab.com/gitlab-org/gitaly/client" "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" @@ -73,11 +72,7 @@ func TestCloneRepositoryFromUrlCommand(t *testing.T) { repositoryFullPath := "full/path/to/repository" url := fmt.Sprintf("https://%s@www.example.com/secretrepo.git", userInfo) - s := &server{ - conns: client.NewPool(), - locator: config.NewLocator(config.Config), - cfg: config.Config, - } + s := &server{cfg: config.Config} cmd, err := s.cloneFromURLCommand(ctx, &gitalypb.Repository{}, url, repositoryFullPath, nil) require.NoError(t, err) -- GitLab From b1a60e685cfc16ac1f5fe40723a06f3b5edccb05 Mon Sep 17 00:00:00 2001 From: Paul Okstad Date: Tue, 10 Nov 2020 23:45:46 -0800 Subject: [PATCH 16/16] Ref tx hook override --- internal/git/hooks.go | 17 +++++++++++++++++ internal/git/safecmd.go | 5 +++-- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/internal/git/hooks.go b/internal/git/hooks.go index 97099b859a7..5d3004f6aa0 100644 --- a/internal/git/hooks.go +++ b/internal/git/hooks.go @@ -20,6 +20,10 @@ var jsonpbMarshaller = &jsonpb.Marshaler{} // repository changes that may possibly update references func WithRefTxHook(ctx context.Context, repo *gitalypb.Repository, cfg config.Cfg) CmdOpt { return func(cc *cmdCfg) error { + if cc.refHookOverriden { + return errors.New("ref tx hook provided, but already overriden") + } + if repo == nil { return fmt.Errorf("missing repo: %w", ErrInvalidArg) } @@ -36,6 +40,19 @@ func WithRefTxHook(ctx context.Context, repo *gitalypb.Repository, cfg config.Cf } } +// WithOverrideRefTxHook returns an option that indicates the command should not +// require a ref hook. This option should only be used in cases where the git +// command does not alter references. +func WithOverrideRefTxHook() CmdOpt { + return func(cc *cmdCfg) error { + if cc.refHookConfigured { + return errors.New("ref tx hook override provided, but already configured") + } + cc.refHookOverriden = true + return nil + } +} + // refHookEnv returns all env vars required by the reference transaction hook func refHookEnv(ctx context.Context, repo *gitalypb.Repository, cfg config.Cfg) ([]string, error) { repoJSON, err := jsonpbMarshaller.MarshalToString(repo) diff --git a/internal/git/safecmd.go b/internal/git/safecmd.go index d7f011a5177..bb7f34b8154 100644 --- a/internal/git/safecmd.go +++ b/internal/git/safecmd.go @@ -229,6 +229,7 @@ type cmdCfg struct { stdout io.Writer stderr io.Writer refHookConfigured bool + refHookOverridden bool } // CmdOpt is an option for running a command @@ -274,10 +275,10 @@ func handleOpts(ctx context.Context, sc Cmd, cc *cmdCfg, opts []CmdOpt) error { } } - if !cc.refHookConfigured && mayUpdateRef(sc.Subcommand()) { + if !cc.refHookConfigured && !cc.refHookOverridden && mayUpdateRef(sc.Subcommand()) { return fmt.Errorf("subcommand %q: %w", sc.Subcommand(), ErrRefHookRequired) } - if cc.refHookConfigured && !mayUpdateRef(sc.Subcommand()) { + if cc.refHookConfigured && (!mayUpdateRef(sc.Subcommand()) || cc.refHookOverridden) { return fmt.Errorf("subcommand %q: %w", sc.Subcommand(), ErrRefHookNotRequired) } -- GitLab