Skip to content
Snippets Groups Projects
Commit 25923c6b authored by Pavlo Strokov's avatar Pavlo Strokov
Browse files

Removal of the git.NewCommand

The change replaces all usages of the git.NewCommand
function with git.CommandFactory.New method.
The New method of the ExecCommandFactory changed to
use newCommand method to execute git commands.
The new parameter was added to some functions to pass
git.CommandFactory down to the place where git.NewCommand
was used before.

Part of: #2699
parent 5473e588
No related branches found
No related tags found
Loading
Showing
with 65 additions and 54 deletions
......@@ -28,7 +28,7 @@ func TestInfo(t *testing.T) {
testRepository, _, cleanup := testhelper.NewTestRepo(t)
defer cleanup()
c, err := New(ctx, git.NewExecCommandFactory(config.Cfg{}), testRepository)
c, err := New(ctx, git.NewExecCommandFactory(config.Config), testRepository)
require.NoError(t, err)
testCases := []struct {
......@@ -64,7 +64,7 @@ func TestBlob(t *testing.T) {
testRepository, _, cleanup := testhelper.NewTestRepo(t)
defer cleanup()
c, err := New(ctx, git.NewExecCommandFactory(config.Cfg{}), testRepository)
c, err := New(ctx, git.NewExecCommandFactory(config.Config), testRepository)
require.NoError(t, err)
gitignoreBytes, err := ioutil.ReadFile("testdata/blob-dfaa3f97ca337e20154a98ac9d0be76ddd1fcc82")
......@@ -131,7 +131,7 @@ func TestCommit(t *testing.T) {
testRepository, _, cleanup := testhelper.NewTestRepo(t)
defer cleanup()
c, err := New(ctx, git.NewExecCommandFactory(config.Cfg{}), testRepository)
c, err := New(ctx, git.NewExecCommandFactory(config.Config), testRepository)
require.NoError(t, err)
commitBytes, err := ioutil.ReadFile("testdata/commit-e63f41fe459e62e1228fcef60d7189127aeba95a")
......@@ -169,7 +169,7 @@ func TestTag(t *testing.T) {
testRepository, _, cleanup := testhelper.NewTestRepo(t)
defer cleanup()
c, err := New(ctx, git.NewExecCommandFactory(config.Cfg{}), testRepository)
c, err := New(ctx, git.NewExecCommandFactory(config.Config), testRepository)
require.NoError(t, err)
tagBytes, err := ioutil.ReadFile("testdata/tag-a509fa67c27202a2bc9dd5e014b4af7e6063ac76")
......@@ -236,7 +236,7 @@ func TestTree(t *testing.T) {
testRepository, _, cleanup := testhelper.NewTestRepo(t)
defer cleanup()
c, err := New(ctx, git.NewExecCommandFactory(config.Cfg{}), testRepository)
c, err := New(ctx, git.NewExecCommandFactory(config.Config), testRepository)
require.NoError(t, err)
treeBytes, err := ioutil.ReadFile("testdata/tree-7e2f26d033ee47cd0745649d1a28277c56197921")
......@@ -303,7 +303,7 @@ func TestRepeatedCalls(t *testing.T) {
testRepository, _, cleanup := testhelper.NewTestRepo(t)
defer cleanup()
c, err := New(ctx, git.NewExecCommandFactory(config.Cfg{}), testRepository)
c, err := New(ctx, git.NewExecCommandFactory(config.Config), testRepository)
require.NoError(t, err)
treeOid := git.Revision("7e2f26d033ee47cd0745649d1a28277c56197921")
......@@ -418,7 +418,7 @@ func catfileWithFreshSessionID(ctx context.Context, repo *gitalypb.Repository) (
SessionIDField: id,
})
return New(metadata.NewIncomingContext(ctx, md), git.NewExecCommandFactory(config.Cfg{}), repo)
return New(metadata.NewIncomingContext(ctx, md), git.NewExecCommandFactory(config.Config), repo)
}
func waitTrue(callback func() bool) bool {
......
......@@ -62,7 +62,7 @@ func NewExecCommandFactory(cfg config.Cfg) *ExecCommandFactory {
// New creates a new command for the repo repository.
func (cf *ExecCommandFactory) New(ctx context.Context, repo repository.GitRepo, globals []GlobalOption, sc Cmd, opts ...CmdOpt) (*command.Command, error) {
return NewCommand(ctx, repo, globals, sc, opts...)
return cf.newCommand(ctx, repo, "", globals, sc, opts...)
}
// NewWithoutRepo creates a command without a target repository.
......
......@@ -31,7 +31,7 @@ func TestWithRefHook(t *testing.T) {
{
name: "NewCommand",
fn: func() (*command.Command, error) {
return NewCommand(ctx, testRepo, nil, subCmd, opt)
return NewExecCommandFactory(config.Config).New(ctx, testRepo, nil, subCmd, opt)
},
},
} {
......
......@@ -212,7 +212,7 @@ func (repo *Repo) ReadCommit(ctx context.Context, revision git.Revision, opts ..
var commit *gitalypb.GitCommit
if cfg.withTrailers {
commit, err = log.GetCommitCatfileWithTrailers(ctx, repo, c, revision)
commit, err = log.GetCommitCatfileWithTrailers(ctx, repo.commandFactory, repo, c, revision)
} else {
commit, err = log.GetCommitCatfile(ctx, c, revision)
}
......
......@@ -199,7 +199,7 @@ func (repo *Repo) FetchRemote(ctx context.Context, remoteName string, opts Fetch
return err
}
cmd, err := git.NewCommand(ctx, repo, opts.Global,
cmd, err := repo.commandFactory.New(ctx, repo, opts.Global,
git.SubCmd{
Name: "fetch",
Flags: opts.buildFlags(),
......
......@@ -31,7 +31,7 @@ func GetCommitCatfile(ctx context.Context, c catfile.Batch, revision git.Revisio
// GetCommitCatfileWithTrailers looks up a commit by revision using an existing
// catfile.Batch instance, and includes Git trailers in the returned commit.
func GetCommitCatfileWithTrailers(ctx context.Context, repo repository.GitRepo, c catfile.Batch, revision git.Revision) (*gitalypb.GitCommit, error) {
func GetCommitCatfileWithTrailers(ctx context.Context, gitCmdFactory git.CommandFactory, repo repository.GitRepo, c catfile.Batch, revision git.Revision) (*gitalypb.GitCommit, error) {
commit, err := GetCommitCatfile(ctx, c, revision)
if err != nil {
......@@ -40,7 +40,7 @@ func GetCommitCatfileWithTrailers(ctx context.Context, repo repository.GitRepo,
// We use the commit ID here instead of revision. This way we still get
// trailers if the revision is not a SHA but e.g. a tag name.
showCmd, err := git.NewCommand(ctx, repo, nil, git.SubCmd{
showCmd, err := gitCmdFactory.New(ctx, repo, nil, git.SubCmd{
Name: "show",
Args: []string{commit.Id},
Flags: []git.Option{
......
......@@ -176,11 +176,12 @@ func TestGetCommitCatfileWithTrailers(t *testing.T) {
testRepo, _, cleanup := testhelper.NewTestRepo(t)
defer cleanup()
catfile, err := catfile.New(ctx, git.NewExecCommandFactory(config.Config), testRepo)
gitCmdFactory := git.NewExecCommandFactory(config.Config)
catfile, err := catfile.New(ctx, gitCmdFactory, testRepo)
require.NoError(t, err)
commit, err := GetCommitCatfileWithTrailers(ctx, testRepo, catfile, "5937ac0a7beb003549fc5fd26fc247adbce4a52e")
commit, err := GetCommitCatfileWithTrailers(ctx, gitCmdFactory, testRepo, catfile, "5937ac0a7beb003549fc5fd26fc247adbce4a52e")
require.NoError(t, err)
......
......@@ -13,8 +13,8 @@ import (
)
// LastCommitForPath returns the last commit which modified path.
func LastCommitForPath(ctx context.Context, batch catfile.Batch, repo repository.GitRepo, revision git.Revision, path string, options *gitalypb.GlobalOptions) (*gitalypb.GitCommit, error) {
cmd, err := git.NewCommand(ctx, repo, git.ConvertGlobalOptions(options), git.SubCmd{
func LastCommitForPath(ctx context.Context, gitCmdFactory git.CommandFactory, batch catfile.Batch, repo repository.GitRepo, revision git.Revision, path string, options *gitalypb.GlobalOptions) (*gitalypb.GitCommit, error) {
cmd, err := gitCmdFactory.New(ctx, repo, git.ConvertGlobalOptions(options), git.SubCmd{
Name: "log",
Flags: []git.Option{git.Flag{Name: "--format=%H"}, git.Flag{Name: "--max-count=1"}},
Args: []string{revision.String()},
......@@ -32,13 +32,13 @@ func LastCommitForPath(ctx context.Context, batch catfile.Batch, repo repository
}
// GitLogCommand returns a Command that executes git log with the given the arguments
func GitLogCommand(ctx context.Context, factory git.CommandFactory, repo repository.GitRepo, revisions []git.Revision, paths []string, options *gitalypb.GlobalOptions, extraArgs ...git.Option) (*command.Command, error) {
func GitLogCommand(ctx context.Context, gitCmdFactory git.CommandFactory, repo repository.GitRepo, revisions []git.Revision, paths []string, options *gitalypb.GlobalOptions, extraArgs ...git.Option) (*command.Command, error) {
args := make([]string, len(revisions))
for i, revision := range revisions {
args[i] = revision.String()
}
return git.NewCommand(ctx, repo, git.ConvertGlobalOptions(options), git.SubCmd{
return gitCmdFactory.New(ctx, repo, git.ConvertGlobalOptions(options), git.SubCmd{
Name: "log",
Flags: append([]git.Option{git.Flag{Name: "--pretty=%H"}}, extraArgs...),
Args: args,
......
......@@ -171,8 +171,8 @@ func (o *ObjectPool) Unlink(ctx context.Context, repo *gitalypb.Repository) erro
// We need to use removeRemote, and can't leverage `git config --remove-section`
// as the latter doesn't clean up refs
remoteName := repo.GetGlRepository()
if err := remote.Remove(ctx, o.cfg, o, remoteName); err != nil {
if present, err2 := remote.Exists(ctx, o.cfg, o, remoteName); err2 != nil || present {
if err := remote.Remove(ctx, o.gitCmdFactory, o.cfg, o, remoteName); err != nil {
if present, err2 := remote.Exists(ctx, o.gitCmdFactory, o.cfg, o, remoteName); err2 != nil || present {
return err
}
}
......
......@@ -10,8 +10,8 @@ import (
)
//Remove removes the remote from repository
func Remove(ctx context.Context, cfg config.Cfg, repo repository.GitRepo, name string) error {
cmd, err := git.NewCommand(ctx, repo, nil, git.SubSubCmd{
func Remove(ctx context.Context, gitCmdFactory git.CommandFactory, cfg config.Cfg, repo repository.GitRepo, name string) error {
cmd, err := gitCmdFactory.New(ctx, repo, nil, git.SubSubCmd{
Name: "remote",
Action: "remove",
Args: []string{name},
......@@ -25,8 +25,8 @@ func Remove(ctx context.Context, cfg config.Cfg, repo repository.GitRepo, name s
// Exists will always return a boolean value, but should only be depended on
// when the error value is nil
func Exists(ctx context.Context, cfg config.Cfg, repo repository.GitRepo, name string) (bool, error) {
cmd, err := git.NewCommand(ctx, repo, nil,
func Exists(ctx context.Context, gitCmdFactory git.CommandFactory, cfg config.Cfg, repo repository.GitRepo, name string) (bool, error) {
cmd, err := gitCmdFactory.New(ctx, repo, nil,
git.SubCmd{Name: "remote"},
git.WithRefTxHook(ctx, repo, cfg),
)
......
......@@ -6,6 +6,7 @@ import (
"testing"
"github.com/stretchr/testify/require"
"gitlab.com/gitlab-org/gitaly/internal/git"
"gitlab.com/gitlab-org/gitaly/internal/gitaly/config"
"gitlab.com/gitlab-org/gitaly/internal/testhelper"
)
......@@ -29,7 +30,8 @@ func TestRemoveRemote(t *testing.T) {
testRepo, _, cleanupFn := testhelper.NewTestRepo(t)
defer cleanupFn()
require.NoError(t, Remove(ctx, config.Config, testRepo, "origin"))
gitCmdFactory := git.NewExecCommandFactory(config.Config)
require.NoError(t, Remove(ctx, gitCmdFactory, config.Config, testRepo, "origin"))
repoPath := filepath.Join(testhelper.GitlabTestStoragePath(), testRepo.RelativePath)
......@@ -52,7 +54,8 @@ func TestRemoveRemoteDontRemoveLocalBranches(t *testing.T) {
masterBeforeRemove := testhelper.MustRunCommand(t, nil, "git", "-C", repoPath, "show-ref", "refs/heads/master")
require.NoError(t, Remove(ctx, config.Config, testRepo, "origin"))
gitCmdFactory := git.NewExecCommandFactory(config.Config)
require.NoError(t, Remove(ctx, gitCmdFactory, config.Config, testRepo, "origin"))
out := testhelper.MustRunCommand(t, nil, "git", "-C", repoPath, "remote")
require.Len(t, out, 0)
......@@ -68,11 +71,13 @@ func TestRemoteExists(t *testing.T) {
testRepo, _, cleanupFn := testhelper.NewTestRepo(t)
defer cleanupFn()
found, err := Exists(ctx, config.Config, testRepo, "origin")
gitCmdFactory := git.NewExecCommandFactory(config.Config)
found, err := Exists(ctx, gitCmdFactory, config.Config, testRepo, "origin")
require.NoError(t, err)
require.True(t, found)
found, err = Exists(ctx, config.Config, testRepo, "can-not-be-found")
found, err = Exists(ctx, gitCmdFactory, config.Config, testRepo, "can-not-be-found")
require.NoError(t, err)
require.False(t, found)
}
......@@ -14,10 +14,10 @@ import (
// LogObjectsInfo read statistics of the git repo objects
// and logs it under 'count-objects' key as structured entry.
func LogObjectsInfo(ctx context.Context, repo repository.GitRepo) {
func LogObjectsInfo(ctx context.Context, gitCmdFactory git.CommandFactory, repo repository.GitRepo) {
logger := ctxlogrus.Extract(ctx)
cmd, err := git.NewCommand(ctx, repo, nil, git.SubCmd{
cmd, err := gitCmdFactory.New(ctx, repo, nil, git.SubCmd{
Name: "count-objects",
Flags: []git.Option{git.Flag{Name: "--verbose"}},
})
......
......@@ -12,6 +12,7 @@ import (
"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
"github.com/sirupsen/logrus"
"github.com/stretchr/testify/require"
"gitlab.com/gitlab-org/gitaly/internal/git"
"gitlab.com/gitlab-org/gitaly/internal/gitaly/config"
"gitlab.com/gitlab-org/gitaly/internal/testhelper"
"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
......@@ -30,6 +31,7 @@ func TestLogObjectInfo(t *testing.T) {
logBuffer := &bytes.Buffer{}
log := &logrus.Logger{Out: logBuffer, Formatter: &logrus.JSONFormatter{}, Level: logrus.InfoLevel}
testCtx := ctxlogrus.ToContext(ctx, log.WithField("test", "logging"))
gitCmdFactory := git.NewExecCommandFactory(config.Config)
requireLog := func(msg string) map[string]interface{} {
var out map[string]interface{}
......@@ -60,7 +62,7 @@ func TestLogObjectInfo(t *testing.T) {
testhelper.MustRunCommand(t, nil, "git", "clone", "--shared", repoPath1, "--reference", repoPath1, "--reference", repoPath2, tmpDir)
logBuffer.Reset()
LogObjectsInfo(testCtx, &gitalypb.Repository{
LogObjectsInfo(testCtx, gitCmdFactory, &gitalypb.Repository{
StorageName: repo1.StorageName,
RelativePath: filepath.Join(strings.TrimPrefix(tmpDir, storagePath), ".git"),
})
......@@ -71,7 +73,7 @@ func TestLogObjectInfo(t *testing.T) {
t.Run("repo without alternates", func(t *testing.T) {
logBuffer.Reset()
LogObjectsInfo(testCtx, repo2)
LogObjectsInfo(testCtx, gitCmdFactory, repo2)
countObjects := requireLog(logBuffer.String())
require.Contains(t, countObjects, "prune-packable")
......
......@@ -61,8 +61,8 @@ func UnpackedObjects(repoPath string) (int64, error) {
}
// LooseObjects returns the number of loose objects that are not in a packfile.
func LooseObjects(ctx context.Context, repository repository.GitRepo) (int64, error) {
cmd, err := git.NewCommand(ctx, repository, nil, git.SubCmd{Name: "count-objects", Flags: []git.Option{git.Flag{Name: "--verbose"}}})
func LooseObjects(ctx context.Context, gitCmdFactory git.CommandFactory, repository repository.GitRepo) (int64, error) {
cmd, err := gitCmdFactory.New(ctx, repository, nil, git.SubCmd{Name: "count-objects", Flags: []git.Option{git.Flag{Name: "--verbose"}}})
if err != nil {
return 0, err
}
......
......@@ -7,6 +7,8 @@ import (
"time"
"github.com/stretchr/testify/require"
"gitlab.com/gitlab-org/gitaly/internal/git"
"gitlab.com/gitlab-org/gitaly/internal/gitaly/config"
"gitlab.com/gitlab-org/gitaly/internal/testhelper"
)
......@@ -37,7 +39,8 @@ func TestRepositoryProfile(t *testing.T) {
require.NoError(t, err)
require.Equal(t, int64(blobs), unpackedObjects)
looseObjects, err := LooseObjects(ctx, testRepo)
gitCmdFactory := git.NewExecCommandFactory(config.Config)
looseObjects, err := LooseObjects(ctx, gitCmdFactory, testRepo)
require.NoError(t, err)
require.Equal(t, int64(blobs), looseObjects)
......@@ -54,7 +57,7 @@ func TestRepositoryProfile(t *testing.T) {
unpackedObjects, err = UnpackedObjects(testRepoPath)
require.NoError(t, err)
require.Zero(t, unpackedObjects)
looseObjects, err = LooseObjects(ctx, testRepo)
looseObjects, err = LooseObjects(ctx, gitCmdFactory, testRepo)
require.NoError(t, err)
require.Equal(t, int64(1), looseObjects)
......@@ -72,7 +75,7 @@ func TestRepositoryProfile(t *testing.T) {
require.NoError(t, err)
require.Equal(t, int64(1), unpackedObjects)
looseObjects, err = LooseObjects(ctx, testRepo)
looseObjects, err = LooseObjects(ctx, gitCmdFactory, testRepo)
require.NoError(t, err)
require.Equal(t, int64(2), looseObjects)
}
......@@ -18,17 +18,16 @@ import (
)
type mockOptimizer struct {
t testing.TB
actual []*gitalypb.Repository
storages []config.Storage
t testing.TB
actual []*gitalypb.Repository
cfg config.Cfg
}
func (mo *mockOptimizer) OptimizeRepository(ctx context.Context, req *gitalypb.OptimizeRepositoryRequest, _ ...grpc.CallOption) (*gitalypb.OptimizeRepositoryResponse, error) {
mo.actual = append(mo.actual, req.Repository)
cfg := config.Cfg{Storages: mo.storages}
l := config.NewLocator(cfg)
gitCmdFactory := git.NewExecCommandFactory(cfg)
resp, err := repository.NewServer(cfg, nil, l, transaction.NewManager(cfg), gitCmdFactory).OptimizeRepository(ctx, req)
l := config.NewLocator(mo.cfg)
gitCmdFactory := git.NewExecCommandFactory(mo.cfg)
resp, err := repository.NewServer(mo.cfg, nil, l, transaction.NewManager(mo.cfg), gitCmdFactory).OptimizeRepository(ctx, req)
assert.NoError(mo.t, err)
return resp, err
}
......@@ -55,8 +54,8 @@ func TestOptimizeReposRandomly(t *testing.T) {
config.Config.Storages = storages
mo := &mockOptimizer{
t: t,
storages: storages,
t: t,
cfg: config.Config,
}
walker := OptimizeReposRandomly(storages, mo)
......@@ -79,11 +78,12 @@ func TestOptimizeReposRandomly(t *testing.T) {
Path: storages[0].Path,
})
config.Config.Storages = storages
mo = &mockOptimizer{
t: t,
storages: storages,
t: t,
cfg: config.Config,
}
config.Config.Storages = storages
walker = OptimizeReposRandomly(storages, mo)
require.NoError(t, walker(ctx, testhelper.DiscardTestEntry(t), []string{"0", "1", "duplicate"}))
......
......@@ -48,7 +48,7 @@ func (s *server) lastCommitForPath(ctx context.Context, in *gitalypb.LastCommitF
options.LiteralPathspecs = true
}
commit, err := log.LastCommitForPath(ctx, c, repo, git.Revision(in.GetRevision()), path, options)
commit, err := log.LastCommitForPath(ctx, s.gitCmdFactory, c, repo, git.Revision(in.GetRevision()), path, options)
if log.IsNotFound(err) {
return &gitalypb.LastCommitForPathResponse{}, nil
}
......
......@@ -68,7 +68,7 @@ func (s *server) listLastCommitsForTree(in *gitalypb.ListLastCommitsForTreeReque
}
for _, entry := range entries[offset:limit] {
commit, err := log.LastCommitForPath(ctx, c, repo, git.Revision(in.GetRevision()), entry.Path, in.GetGlobalOptions())
commit, err := log.LastCommitForPath(ctx, s.gitCmdFactory, c, repo, git.Revision(in.GetRevision()), entry.Path, in.GetGlobalOptions())
if err != nil {
return err
}
......
......@@ -27,7 +27,7 @@ func (s *server) FindChangedPaths(in *gitalypb.FindChangedPathsRequest, stream g
diffChunker := chunk.New(&findChangedPathsSender{stream: stream})
cmd, err := git.NewCommand(stream.Context(), in.Repository, nil, git.SubCmd{
cmd, err := s.gitCmdFactory.New(stream.Context(), in.Repository, nil, git.SubCmd{
Name: "diff-tree",
Flags: []git.Option{
git.Flag{Name: "-z"},
......
......@@ -20,7 +20,7 @@ func (s *server) DiffStats(in *gitalypb.DiffStatsRequest, stream gitalypb.DiffSe
}
var batch []*gitalypb.DiffStats
cmd, err := git.NewCommand(stream.Context(), in.Repository, nil, git.SubCmd{
cmd, err := s.gitCmdFactory.New(stream.Context(), in.Repository, nil, git.SubCmd{
Name: "diff",
Flags: []git.Option{git.Flag{Name: "--numstat"}, git.Flag{Name: "-z"}},
Args: []string{in.LeftCommitId, in.RightCommitId},
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment