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 902bc77600..e804ff6afa 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 5f8b502bc4..afa7a689f7 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 2bcd3a5991..0e4a30c4c6 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 41e83b1877..7a3934986d 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 a8a23099bd..b3b65adc0b 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 10d9901caa..20c3c74987 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 cca79b9853..b48aaa1fd6 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 4b0baa11e5..3545b6d01f 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 97ddd57964..d439a06e00 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 fe0d7bdd55..20c24a7576 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 23dd0c03c5..c3ef6a9788 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 e8e8897b88..931c5c18e1 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 61abdee28e..bce628c893 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 00b7d49c82..39a6d1b136 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 6e3447cdfc..f3bd6bfc4b 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 845bf3785b..5b2a07156c 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 a40488d10b..b430b95658 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 0807f47a53..c73216d627 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 4990aac937..9bdc059df7 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 7d46acc66d..802d5a5c53 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 4c2d5c3cc7..ea58621068 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 78980e9a5e..d95b0952cf 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 e663085a0c..f2019deed6 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 3a89a1e521..ff7058d919 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 f82abaa056..353467b370 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 16839fa44b..c0662ac569 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 bbaf239cba..51a38962a4 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 31e0128baa..f1187a2b3b 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 87c185bafb..41b034aebd 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 3545b6d01f..6444c5743a 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 e804ff6afa..c122ae1db8 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 8d7309c474..161c1b71f0 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 39a6d1b136..752b89c8ee 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 0629d052d3..97099b859a 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 d88c295134..d7f011a517 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 c122ae1db8..f93163c573 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 0000000000..4ba7b338f1 --- /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 0000000000..2f63765882 --- /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 4d62adaf0c..d949eeb874 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 2f63765882..d7abadea05 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 dd4b78a242..71034daaa4 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 cb10fd01d0..53abcbbab4 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 0e4a30c4c6..4ac559962d 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 4998d2b3a3..187ec42dc3 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 9cef003942..835490538b 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 2a2dd2c540..e6da8db91f 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 1e69f5a50c..f033e12179 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 21eef190a0..20dd12e23e 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 f9ff064e23..a281ce3e69 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 407e56287c..7cd4f86862 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 69bfeb0f13..848520fb0c 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 e289dccd82..6c309447b4 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 b3503381e1..f1da33c6c3 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 c409ee8693..06788e52bb 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 7294daa534..11dfffeea2 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 1a71288bba..a5f0fa7dbe 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 c5b1971260..b3e018e651 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 6f07fb0f25..2e7186a3dd 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 f3bd6bfc4b..7f9dbdad18 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 719f8b6b2a..942d4feb6e 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 d7abadea05..bcfe53ab69 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 692f7f1321..d97f84c712 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 3dc2049751..f0ebd39e58 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 6eb2132ca4..fb9da92223 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 f93163c573..0a837723b0 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 2e9cf0b119..372bd577d7 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 b3b65adc0b..81a4331928 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 9c60ab1cf6..735b34d61d 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 d439a06e00..c318188de6 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 ea58621068..4c2d5c3cc7 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 bcfe53ab69..0000000000 --- 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 c318188de6..e649869c27 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 72c5070b29..112dad54ff 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 20c24a7576..144d83fe51 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 c73216d627..636d35e4fb 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 97099b859a..5d3004f6aa 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 d7f011a517..bb7f34b815 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