Merge branch 'git-commands' into 'master'

Standardize git command invocation

See merge request gitlab-org/gitaly!915

19 files changed, 55 insertions, 64 deletions

diff --git a/changelogs/unreleased/git-commands.yml b/changelogs/unreleased/git-commands.yml
new file mode 100644
index 000000000..6944d6d53
--- /dev/null
+++ b/changelogs/unreleased/git-commands.yml
@@ -0,0 +1,5 @@
+---
+title: Standardize git command invocation
+merge_request: 915
+author:
+type: other
diff --git a/internal/command/command.go b/internal/command/command.go
index 4a0ca8e34..bcd634ffe 100644
--- a/internal/command/command.go
+++ b/internal/command/command.go
@@ -16,6 +16,12 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/config"
 )
 
+// GitEnv contains the ENV variables for git commands
+var GitEnv = []string{
+	// Force english locale for consistency on the output messages
+	"LANG=en_US.UTF-8",
+}
+
 // exportedEnvVars contains a list of environment variables
 // that are always exported to child processes on spawn
 var exportedEnvVars = []string{
diff --git a/internal/git/catfile/batch.go b/internal/git/catfile/batch.go
index 779ac4049..c712b33a6 100644
--- a/internal/git/catfile/batch.go
+++ b/internal/git/catfile/batch.go
@@ -6,10 +6,9 @@ import (
 	"fmt"
 	"io"
 	"io/ioutil"
-	"os/exec"
 	"sync"
 
-	"gitlab.com/gitlab-org/gitaly/internal/command"
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 )
@@ -39,7 +38,7 @@ func newBatch(ctx context.Context, repoPath string, env []string) (*batch, error
 	var stdinReader io.Reader
 	stdinReader, b.w = io.Pipe()
 	batchCmdArgs := []string{"--git-dir", repoPath, "cat-file", "--batch"}
-	batchCmd, err := command.New(ctx, exec.Command(command.GitPath(), batchCmdArgs...), stdinReader, nil, nil, env...)
+	batchCmd, err := git.BareCommand(ctx, stdinReader, nil, nil, env, batchCmdArgs...)
 	if err != nil {
 		return nil, status.Errorf(codes.Internal, "CatFile: cmd: %v", err)
 	}
diff --git a/internal/git/catfile/batchcheck.go b/internal/git/catfile/batchcheck.go
index a0846802c..9dc3356ee 100644
--- a/internal/git/catfile/batchcheck.go
+++ b/internal/git/catfile/batchcheck.go
@@ -5,10 +5,9 @@ import (
 	"context"
 	"fmt"
 	"io"
-	"os/exec"
 	"sync"
 
-	"gitlab.com/gitlab-org/gitaly/internal/command"
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 )
@@ -26,7 +25,7 @@ func newBatchCheck(ctx context.Context, repoPath string, env []string) (*batchCh
 	var stdinReader io.Reader
 	stdinReader, bc.w = io.Pipe()
 	batchCmdArgs := []string{"--git-dir", repoPath, "cat-file", "--batch-check"}
-	batchCmd, err := command.New(ctx, exec.Command(command.GitPath(), batchCmdArgs...), stdinReader, nil, nil, env...)
+	batchCmd, err := git.BareCommand(ctx, stdinReader, nil, nil, env, batchCmdArgs...)
 	if err != nil {
 		return nil, status.Errorf(codes.Internal, "CatFile: cmd: %v", err)
 	}
diff --git a/internal/git/command.go b/internal/git/command.go
index 408221010..a552e5ddc 100644
--- a/internal/git/command.go
+++ b/internal/git/command.go
@@ -2,6 +2,7 @@ package git
 
 import (
 	"context"
+	"io"
 	"os/exec"
 
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
@@ -9,27 +10,27 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/git/alternates"
 )
 
-// GitEnv contains the ENV variables for git commands
-var GitEnv = []string{
-	// Force english locale for consistency on the output messages
-	"LANG=en_US.UTF-8",
-}
-
-// Command creates a git.Command with the given args
+// Command creates a git.Command with the given args and Repository
 func Command(ctx context.Context, repo *gitalypb.Repository, args ...string) (*command.Command, error) {
 	repoPath, env, err := alternates.PathAndEnv(repo)
 
-	env = append(env, GitEnv...)
-
 	if err != nil {
 		return nil, err
 	}
 
 	args = append([]string{"--git-dir", repoPath}, args...)
-	return command.New(ctx, exec.Command(command.GitPath(), args...), nil, nil, nil, env...)
+
+	return BareCommand(ctx, nil, nil, nil, env, args...)
+}
+
+// BareCommand creates a git.Command with the given args, stdin/stdout/stderr, and env
+func BareCommand(ctx context.Context, stdin io.Reader, stdout, stderr io.Writer, env []string, args ...string) (*command.Command, error) {
+	env = append(env, command.GitEnv...)
+
+	return command.New(ctx, exec.Command(command.GitPath(), args...), stdin, stdout, stderr, env...)
 }
 
 // CommandWithoutRepo works like Command but without a git repository
 func CommandWithoutRepo(ctx context.Context, args ...string) (*command.Command, error) {
-	return command.New(ctx, exec.Command(command.GitPath(), args...), nil, nil, nil, "")
+	return BareCommand(ctx, nil, nil, nil, nil, args...)
 }
diff --git a/internal/git/helper.go b/internal/git/helper.go
index 8af8511d9..85f607c2c 100644
--- a/internal/git/helper.go
+++ b/internal/git/helper.go
@@ -4,11 +4,8 @@ import (
 	"bytes"
 	"context"
 	"fmt"
-	"os/exec"
 	"strings"
 	"time"
-
-	"gitlab.com/gitlab-org/gitaly/internal/command"
 )
 
 // FallbackTimeValue is the value returned by `SafeTimeParse` in case it
@@ -42,7 +39,7 @@ func Version() (string, error) {
 	defer cancel()
 
 	var buf bytes.Buffer
-	cmd, err := command.New(ctx, exec.Command(command.GitPath(), "version"), nil, &buf, nil)
+	cmd, err := BareCommand(ctx, nil, &buf, nil, nil, "version")
 	if err != nil {
 		return "", err
 	}
diff --git a/internal/rubyserver/rubyserver.go b/internal/rubyserver/rubyserver.go
index 1e5112b16..46e935ca9 100644
--- a/internal/rubyserver/rubyserver.go
+++ b/internal/rubyserver/rubyserver.go
@@ -16,7 +16,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/config"
-	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/internal/rubyserver/balancer"
 	"gitlab.com/gitlab-org/gitaly/internal/supervisor"
@@ -110,7 +109,7 @@ func Start() (*Server, error) {
 		"GITALY_VERSION="+version.GetVersion(),
 	)
 
-	env = append(env, git.GitEnv...)
+	env = append(env, command.GitEnv...)
 
 	if dsn := cfg.Logging.RubySentryDSN; dsn != "" {
 		env = append(env, "SENTRY_DSN="+dsn)
diff --git a/internal/service/repository/calculate_checksum.go b/internal/service/repository/calculate_checksum.go
index b8a88cb24..d58a6627d 100644
--- a/internal/service/repository/calculate_checksum.go
+++ b/internal/service/repository/calculate_checksum.go
@@ -6,12 +6,10 @@ import (
 	"crypto/sha1"
 	"encoding/hex"
 	"math/big"
-	"os/exec"
 	"regexp"
 	"strings"
 
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
-	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/alternates"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
@@ -92,7 +90,7 @@ func isValidRepo(ctx context.Context, repo *gitalypb.Repository) bool {
 
 	args := []string{"-C", repoPath, "rev-parse", "--is-inside-git-dir"}
 	stdout := &bytes.Buffer{}
-	cmd, err := command.New(ctx, exec.Command(command.GitPath(), args...), nil, stdout, nil, env...)
+	cmd, err := git.BareCommand(ctx, nil, stdout, nil, env, args...)
 	if err != nil {
 		return false
 	}
diff --git a/internal/service/repository/create_from_bundle.go b/internal/service/repository/create_from_bundle.go
index 9ac287933..dffb329c1 100644
--- a/internal/service/repository/create_from_bundle.go
+++ b/internal/service/repository/create_from_bundle.go
@@ -4,12 +4,11 @@ import (
 	"fmt"
 	"io"
 	"os"
-	"os/exec"
 	"path"
 	"strings"
 
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
-	"gitlab.com/gitlab-org/gitaly/internal/command"
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/internal/tempdir"
 	"gitlab.com/gitlab-org/gitaly/streamio"
@@ -72,7 +71,7 @@ func (s *server) CreateRepositoryFromBundle(stream gitalypb.RepositoryService_Cr
 		bundlePath,
 		repoPath,
 	}
-	cmd, err := command.New(ctx, exec.Command(command.GitPath(), args...), nil, nil, nil)
+	cmd, err := git.CommandWithoutRepo(ctx, args...)
 	if err != nil {
 		cleanError := sanitizedError(repoPath, "CreateRepositoryFromBundle: cmd start failed: %v", err)
 		return status.Error(codes.Internal, cleanError)
@@ -91,7 +90,7 @@ func (s *server) CreateRepositoryFromBundle(stream gitalypb.RepositoryService_Cr
 		"refs/*:refs/*",
 	}
 
-	cmd, err = command.New(ctx, exec.Command(command.GitPath(), args...), nil, nil, nil)
+	cmd, err = git.CommandWithoutRepo(ctx, args...)
 	if err != nil {
 		cleanError := sanitizedError(repoPath, "CreateRepositoryFromBundle: cmd start failed fetching refs: %v", err)
 		return status.Error(codes.Internal, cleanError)
diff --git a/internal/service/repository/create_from_url.go b/internal/service/repository/create_from_url.go
index 3e4ef6cfb..7274bf7f0 100644
--- a/internal/service/repository/create_from_url.go
+++ b/internal/service/repository/create_from_url.go
@@ -3,10 +3,9 @@ package repository
 import (
 	"fmt"
 	"os"
-	"os/exec"
 
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
-	"gitlab.com/gitlab-org/gitaly/internal/command"
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"golang.org/x/net/context"
 	"google.golang.org/grpc/codes"
@@ -36,7 +35,7 @@ func (s *server) CreateRepositoryFromURL(ctx context.Context, req *gitalypb.Crea
 		req.Url,
 		repositoryFullPath,
 	}
-	cmd, err := command.New(ctx, exec.Command(command.GitPath(), args...), nil, nil, nil)
+	cmd, err := git.CommandWithoutRepo(ctx, args...)
 	if err != nil {
 		return nil, status.Errorf(codes.Internal, "CreateRepositoryFromURL: clone cmd start: %v", err)
 	}
diff --git a/internal/service/repository/fork.go b/internal/service/repository/fork.go
index 9e50c69dd..3cfef7738 100644
--- a/internal/service/repository/fork.go
+++ b/internal/service/repository/fork.go
@@ -3,13 +3,12 @@ package repository
 import (
 	"fmt"
 	"os"
-	"os/exec"
 	"path"
 
 	"github.com/golang/protobuf/jsonpb"
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
-	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/config"
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"golang.org/x/net/context"
 	"google.golang.org/grpc/codes"
@@ -82,7 +81,7 @@ func (s *server) CreateFork(ctx context.Context, req *gitalypb.CreateForkRequest
 		fmt.Sprintf("%s:%s", gitalyInternalURL, sourceRepository.RelativePath),
 		targetRepositoryFullPath,
 	}
-	cmd, err := command.New(ctx, exec.Command(command.GitPath(), args...), nil, nil, nil, env...)
+	cmd, err := git.BareCommand(ctx, nil, nil, nil, env, args...)
 	if err != nil {
 		return nil, status.Errorf(codes.Internal, "CreateFork: clone cmd start: %v", err)
 	}
diff --git a/internal/service/repository/fsck.go b/internal/service/repository/fsck.go
index d6ccb86ca..2e4f4195e 100644
--- a/internal/service/repository/fsck.go
+++ b/internal/service/repository/fsck.go
@@ -2,10 +2,9 @@ package repository
 
 import (
 	"bytes"
-	"os/exec"
 
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
-	"gitlab.com/gitlab-org/gitaly/internal/command"
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/alternates"
 	"golang.org/x/net/context"
 )
@@ -20,7 +19,7 @@ func (s *server) Fsck(ctx context.Context, req *gitalypb.FsckRequest) (*gitalypb
 
 	args := []string{"--git-dir", repoPath, "fsck"}
 
-	cmd, err := command.New(ctx, exec.Command(command.GitPath(), args...), nil, &stdout, &stderr, env...)
+	cmd, err := git.BareCommand(ctx, nil, &stdout, &stderr, env, args...)
 	if err != nil {
 		return nil, err
 	}
diff --git a/internal/service/smarthttp/inforefs.go b/internal/service/smarthttp/inforefs.go
index 7ebfd7b96..1992ac040 100644
--- a/internal/service/smarthttp/inforefs.go
+++ b/internal/service/smarthttp/inforefs.go
@@ -4,12 +4,10 @@ import (
 	"context"
 	"fmt"
 	"io"
-	"os/exec"
 
 	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus"
 	log "github.com/sirupsen/logrus"
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
-	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/pktline"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
@@ -51,8 +49,7 @@ func handleInfoRefs(ctx context.Context, service string, req *gitalypb.InfoRefsR
 
 	args = append(args, service, "--stateless-rpc", "--advertise-refs", repoPath)
 
-	osCommand := exec.Command(command.GitPath(), args...)
-	cmd, err := command.New(ctx, osCommand, nil, nil, nil, env...)
+	cmd, err := git.BareCommand(ctx, nil, nil, nil, env, args...)
 
 	if err != nil {
 		if _, ok := status.FromError(err); ok {
diff --git a/internal/service/smarthttp/receive_pack.go b/internal/service/smarthttp/receive_pack.go
index 74d7b0964..040193aaf 100644
--- a/internal/service/smarthttp/receive_pack.go
+++ b/internal/service/smarthttp/receive_pack.go
@@ -2,7 +2,6 @@ package smarthttp
 
 import (
 	"fmt"
-	"os/exec"
 
 	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus"
 	log "github.com/sirupsen/logrus"
@@ -52,6 +51,7 @@ func (s *server) PostReceivePack(stream gitalypb.SmartHTTPService_PostReceivePac
 	}
 
 	env = git.AddGitProtocolEnv(ctx, req, env)
+	env = append(env, command.GitEnv...)
 
 	repoPath, err := helper.GetRepoPath(req.Repository)
 	if err != nil {
@@ -59,8 +59,7 @@ func (s *server) PostReceivePack(stream gitalypb.SmartHTTPService_PostReceivePac
 	}
 
 	gitOptions := git.BuildGitOptions(req.GitConfigOptions, "receive-pack", "--stateless-rpc", repoPath)
-	osCommand := exec.Command(command.GitPath(), gitOptions...)
-	cmd, err := command.New(ctx, osCommand, stdin, stdout, nil, env...)
+	cmd, err := git.BareCommand(ctx, stdin, stdout, nil, env, gitOptions...)
 
 	if err != nil {
 		return status.Errorf(codes.Unavailable, "PostReceivePack: %v", err)
diff --git a/internal/service/smarthttp/upload_pack.go b/internal/service/smarthttp/upload_pack.go
index 0718df24d..60872fcbd 100644
--- a/internal/service/smarthttp/upload_pack.go
+++ b/internal/service/smarthttp/upload_pack.go
@@ -2,7 +2,6 @@ package smarthttp
 
 import (
 	"io"
-	"os/exec"
 
 	"github.com/prometheus/client_golang/prometheus"
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
@@ -55,7 +54,7 @@ func (s *server) PostUploadPack(stream gitalypb.SmartHTTPService_PostUploadPackS
 		return stream.Send(&gitalypb.PostUploadPackResponse{Data: p})
 	})
 
-	env := git.AddGitProtocolEnv(ctx, req, []string{})
+	env := git.AddGitProtocolEnv(ctx, req, command.GitEnv)
 
 	repoPath, err := helper.GetRepoPath(req.Repository)
 	if err != nil {
@@ -69,8 +68,7 @@ func (s *server) PostUploadPack(stream gitalypb.SmartHTTPService_PostUploadPackS
 
 	args = append(args, "upload-pack", "--stateless-rpc", repoPath)
 
-	osCommand := exec.Command(command.GitPath(), args...)
-	cmd, err := command.New(ctx, osCommand, stdin, stdout, nil, env...)
+	cmd, err := git.BareCommand(ctx, stdin, stdout, nil, env, args...)
 
 	if err != nil {
 		return status.Errorf(codes.Unavailable, "PostUploadPack: cmd: %v", err)
diff --git a/internal/service/ssh/receive_pack.go b/internal/service/ssh/receive_pack.go
index a54bc0ca3..84decfe9c 100644
--- a/internal/service/ssh/receive_pack.go
+++ b/internal/service/ssh/receive_pack.go
@@ -2,7 +2,6 @@ package ssh
 
 import (
 	"fmt"
-	"os/exec"
 
 	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus"
 	log "github.com/sirupsen/logrus"
@@ -52,6 +51,7 @@ func (s *server) SSHReceivePack(stream gitalypb.SSHService_SSHReceivePackServer)
 		env = append(env, fmt.Sprintf("GL_REPOSITORY=%s", req.GlRepository))
 	}
 	env = git.AddGitProtocolEnv(ctx, req, env)
+	env = append(env, command.GitEnv...)
 
 	repoPath, err := helper.GetRepoPath(req.Repository)
 	if err != nil {
@@ -59,8 +59,7 @@ func (s *server) SSHReceivePack(stream gitalypb.SSHService_SSHReceivePackServer)
 	}
 
 	gitOptions := git.BuildGitOptions(req.GitConfigOptions, "receive-pack", repoPath)
-	osCommand := exec.Command(command.GitPath(), gitOptions...)
-	cmd, err := command.New(ctx, osCommand, stdin, stdout, stderr, env...)
+	cmd, err := git.BareCommand(ctx, stdin, stdout, stderr, env, gitOptions...)
 
 	if err != nil {
 		return status.Errorf(codes.Unavailable, "SSHReceivePack: cmd: %v", err)
diff --git a/internal/service/ssh/upload_archive.go b/internal/service/ssh/upload_archive.go
index 682d87d6f..c29ebf07b 100644
--- a/internal/service/ssh/upload_archive.go
+++ b/internal/service/ssh/upload_archive.go
@@ -1,10 +1,9 @@
 package ssh
 
 import (
-	"os/exec"
-
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/internal/command"
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/streamio"
 	"google.golang.org/grpc/codes"
@@ -35,9 +34,7 @@ func (s *server) SSHUploadArchive(stream gitalypb.SSHService_SSHUploadArchiveSer
 		return stream.Send(&gitalypb.SSHUploadArchiveResponse{Stderr: p})
 	})
 
-	osCommand := exec.Command(command.GitPath(), "upload-archive", repoPath)
-
-	cmd, err := command.New(stream.Context(), osCommand, stdin, stdout, stderr)
+	cmd, err := git.BareCommand(stream.Context(), stdin, stdout, stderr, nil, "upload-archive", repoPath)
 
 	if err != nil {
 		return status.Errorf(codes.Unavailable, "SSHUploadArchive: cmd: %v", err)
diff --git a/internal/service/ssh/upload_pack.go b/internal/service/ssh/upload_pack.go
index a06b3caae..add35b0b3 100644
--- a/internal/service/ssh/upload_pack.go
+++ b/internal/service/ssh/upload_pack.go
@@ -1,8 +1,6 @@
 package ssh
 
 import (
-	"os/exec"
-
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
@@ -35,7 +33,7 @@ func (s *server) SSHUploadPack(stream gitalypb.SSHService_SSHUploadPackServer) e
 		return stream.Send(&gitalypb.SSHUploadPackResponse{Stderr: p})
 	})
 
-	env := git.AddGitProtocolEnv(ctx, req, []string{})
+	env := git.AddGitProtocolEnv(ctx, req, command.GitEnv)
 
 	repoPath, err := helper.GetRepoPath(req.Repository)
 	if err != nil {
@@ -50,9 +48,7 @@ func (s *server) SSHUploadPack(stream gitalypb.SSHService_SSHUploadPackServer) e
 
 	args = append(args, "upload-pack", repoPath)
 
-	osCommand := exec.Command(command.GitPath(), args...)
-
-	cmd, err := command.New(ctx, osCommand, stdin, stdout, stderr, env...)
+	cmd, err := git.BareCommand(ctx, stdin, stdout, stderr, env, args...)
 
 	if err != nil {
 		return status.Errorf(codes.Unavailable, "SSHUploadPack: cmd: %v", err)
diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go
index 7c5b0ebc8..1d470c61a 100644
--- a/internal/testhelper/testhelper.go
+++ b/internal/testhelper/testhelper.go
@@ -138,6 +138,11 @@ func RequireGrpcError(t *testing.T, err error, expectedCode codes.Code) {
 // MustRunCommand runs a command with an optional standard input and returns the standard output, or fails.
 func MustRunCommand(t *testing.T, stdin io.Reader, name string, args ...string) []byte {
 	cmd := exec.Command(name, args...)
+
+	if name == "git" {
+		cmd.Env = append(command.GitEnv, cmd.Env...)
+	}
+
 	if stdin != nil {
 		cmd.Stdin = stdin
 	}