Merge branch 'avar/user-branch-tag-merge-lint-fixes' into 'master'

User(Branch|Tag|Merge) tests: fix lint issues

See merge request gitlab-org/gitaly!3046

4 files changed, 16 insertions, 51 deletions

diff --git a/.golangci.yml b/.golangci.yml
index dfade16f5..ccea119ca 100644
--- a/.golangci.yml
+++ b/.golangci.yml
@@ -1046,18 +1046,6 @@ issues:
         - errcheck
       path: "internal/middleware/limithandler/concurrency_limiter_test.go"
       text: "Error return value of `limiter.Limit` is not checked"
-    - linters:
-        - errcheck
-      path: "internal/gitaly/service/operations/branches_test.go"
-      text: "Error return value of `\\(\\*os/exec\\.Cmd\\).Run` is not checked"
-    - linters:
-        - errcheck
-      path: "internal/gitaly/service/operations/merge_test.go"
-      text: "Error return value of `\\(\\*os/exec\\.Cmd\\).Run` is not checked"
-    - linters:
-        - errcheck
-      path: "internal/gitaly/service/operations/tags_test.go"
-      text: "Error return value of `\\(\\*os/exec\\.Cmd\\).Run` is not checked"
     ## END errcheck exclusions
     ##
     # govet checks all struct initializations must be keyed by field names
diff --git a/internal/gitaly/service/operations/branches_test.go b/internal/gitaly/service/operations/branches_test.go
index c63173abb..7d3917136 100644
--- a/internal/gitaly/service/operations/branches_test.go
+++ b/internal/gitaly/service/operations/branches_test.go
@@ -4,7 +4,6 @@ import (
 	"context"
 	"fmt"
 	"net"
-	"os/exec"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -109,7 +108,7 @@ func testSuccessfulCreateBranchRequest(t *testing.T, ctx context.Context) {
 
 			response, err := client.UserCreateBranch(ctx, request)
 			if testCase.expectedBranch != nil {
-				defer exec.Command(config.Config.Git.BinPath, "-C", testRepoPath, "branch", "-D", branchName).Run()
+				defer testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", "-D", branchName)
 			}
 
 			require.NoError(t, err)
@@ -186,7 +185,7 @@ func TestUserCreateBranchWithTransaction(t *testing.T) {
 
 	for _, tc := range testcases {
 		t.Run(tc.desc, func(t *testing.T) {
-			defer exec.Command(config.Config.Git.BinPath, "-C", testRepoPath, "branch", "-D", "new-branch").Run()
+			defer testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", "-D", "new-branch")
 
 			client, conn := newOperationClient(t, tc.address)
 			defer conn.Close()
@@ -242,7 +241,7 @@ func testSuccessfulGitHooksForUserCreateBranchRequest(t *testing.T, ctx context.
 
 	for _, hookName := range GitlabHooks {
 		t.Run(hookName, func(t *testing.T) {
-			defer exec.Command(config.Config.Git.BinPath, "-C", testRepoPath, "branch", "-D", branchName).Run()
+			defer testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", "-D", branchName)
 
 			hookOutputTempPath, cleanup := testhelper.WriteEnvToCustomHook(t, testRepoPath, hookName)
 			defer cleanup()
@@ -484,7 +483,6 @@ func testSuccessfulUserDeleteBranchRequest(t *testing.T, ctx context.Context) {
 	for _, testCase := range testCases {
 		t.Run(testCase.desc, func(t *testing.T) {
 			testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", testCase.branchNameInput, testCase.branchCommit)
-			defer exec.Command(config.Config.Git.BinPath, "-C", testRepoPath, "branch", "-d", testCase.branchNameInput).Run()
 
 			request := &gitalypb.UserDeleteBranchRequest{
 				Repository: testRepo,
@@ -513,7 +511,6 @@ func TestSuccessfulGitHooksForUserDeleteBranchRequest(t *testing.T) {
 	defer conn.Close()
 
 	branchNameInput := "to-be-deleted-soon-branch"
-	defer exec.Command(config.Config.Git.BinPath, "-C", testRepoPath, "branch", "-d", branchNameInput).Run()
 
 	request := &gitalypb.UserDeleteBranchRequest{
 		Repository: testRepo,
@@ -634,7 +631,7 @@ func TestUserDeleteBranchFailedDueToRefsHeadsPrefix(t *testing.T) {
 	for _, testCase := range testCases {
 		t.Run(testCase.desc, func(t *testing.T) {
 			testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", testCase.branchNameInput, testCase.branchCommit)
-			defer exec.Command(config.Config.Git.BinPath, "-C", testRepoPath, "branch", "-d", testCase.branchNameInput).Run()
+			defer testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", "-d", testCase.branchNameInput)
 
 			request := &gitalypb.UserDeleteBranchRequest{
 				Repository: testRepo,
@@ -667,7 +664,6 @@ func TestFailedUserDeleteBranchDueToHooks(t *testing.T) {
 
 	branchNameInput := "to-be-deleted-soon-branch"
 	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", branchNameInput)
-	defer exec.Command(config.Config.Git.BinPath, "-C", testRepoPath, "branch", "-d", branchNameInput).Run()
 
 	request := &gitalypb.UserDeleteBranchRequest{
 		Repository: testRepo,
@@ -767,7 +763,7 @@ func testBranchHookOutput(t *testing.T, ctx context.Context) {
 				require.Equal(t, testCase.output, createResponse.PreReceiveError)
 
 				testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", branchNameInput)
-				defer exec.Command(config.Config.Git.BinPath, "-C", testRepoPath, "branch", "-d", branchNameInput).Run()
+				defer testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", "-d", branchNameInput)
 
 				deleteResponse, err := client.UserDeleteBranch(ctx, deleteRequest)
 				require.NoError(t, err)
diff --git a/internal/gitaly/service/operations/merge_test.go b/internal/gitaly/service/operations/merge_test.go
index 91ce33c9b..d08ff4b85 100644
--- a/internal/gitaly/service/operations/merge_test.go
+++ b/internal/gitaly/service/operations/merge_test.go
@@ -56,7 +56,7 @@ func TestSuccessfulMerge(t *testing.T) {
 	mergeBidi, err := client.UserMergeBranch(ctx)
 	require.NoError(t, err)
 
-	prepareMergeBranch(t, testRepoPath)
+	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", mergeBranchName, mergeBranchHeadBefore)
 
 	hooks := GitlabHooks
 	hookTempfiles := make([]string, len(hooks))
@@ -144,7 +144,7 @@ func TestSuccessfulMerge_stableMergeIDs(t *testing.T) {
 	mergeBidi, err := client.UserMergeBranch(ctx)
 	require.NoError(t, err)
 
-	prepareMergeBranch(t, testRepoPath)
+	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", mergeBranchName, mergeBranchHeadBefore)
 
 	firstRequest := &gitalypb.UserMergeBranchRequest{
 		Repository: testRepo,
@@ -214,7 +214,7 @@ func TestAbortedMerge(t *testing.T) {
 	testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
 	defer cleanupFn()
 
-	prepareMergeBranch(t, testRepoPath)
+	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", mergeBranchName, mergeBranchHeadBefore)
 
 	firstRequest := &gitalypb.UserMergeBranchRequest{
 		Repository: testRepo,
@@ -290,7 +290,7 @@ func TestFailedMergeConcurrentUpdate(t *testing.T) {
 	mergeBidi, err := client.UserMergeBranch(ctx)
 	require.NoError(t, err)
 
-	prepareMergeBranch(t, testRepoPath)
+	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", mergeBranchName, mergeBranchHeadBefore)
 
 	mergeCommitMessage := "Merged by Gitaly"
 	firstRequest := &gitalypb.UserMergeBranchRequest{
@@ -337,7 +337,7 @@ func TestUserMergeBranch_ambiguousReference(t *testing.T) {
 	merge, err := client.UserMergeBranch(ctx)
 	require.NoError(t, err)
 
-	prepareMergeBranch(t, testRepoPath)
+	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", mergeBranchName, mergeBranchHeadBefore)
 
 	repo := git.NewRepository(testRepo, config.Config)
 
@@ -403,7 +403,7 @@ func TestFailedMergeDueToHooks(t *testing.T) {
 	client, conn := newOperationClient(t, serverSocketPath)
 	defer conn.Close()
 
-	prepareMergeBranch(t, testRepoPath)
+	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", mergeBranchName, mergeBranchHeadBefore)
 
 	hookContent := []byte("#!/bin/sh\necho 'failure'\nexit 1")
 
@@ -699,7 +699,7 @@ func TestSuccessfulUserMergeToRefRequest(t *testing.T) {
 	testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
 	defer cleanupFn()
 
-	prepareMergeBranch(t, testRepoPath)
+	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", mergeBranchName, mergeBranchHeadBefore)
 
 	existingTargetRef := []byte("refs/merge-requests/x/written")
 	emptyTargetRef := []byte("refs/merge-requests/x/merge")
@@ -808,7 +808,7 @@ func TestConflictsOnUserMergeToRefRequest(t *testing.T) {
 	testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
 	defer cleanupFn()
 
-	prepareMergeBranchWithHead(t, testRepoPath, "824be604a34828eb682305f0d963056cfac87b2d")
+	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", mergeBranchName, "824be604a34828eb682305f0d963056cfac87b2d")
 
 	request := &gitalypb.UserMergeToRefRequest{
 		Repository:     testRepo,
@@ -858,7 +858,7 @@ func TestUserMergeToRef_stableMergeID(t *testing.T) {
 	testRepo, testRepoPath, cleanup := testhelper.NewTestRepo(t)
 	defer cleanup()
 
-	prepareMergeBranch(t, testRepoPath)
+	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", mergeBranchName, mergeBranchHeadBefore)
 
 	response, err := client.UserMergeToRef(ctx, &gitalypb.UserMergeToRefRequest{
 		Repository:     testRepo,
@@ -914,7 +914,7 @@ func TestFailedUserMergeToRefRequest(t *testing.T) {
 	testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
 	defer cleanupFn()
 
-	prepareMergeBranch(t, testRepoPath)
+	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", mergeBranchName, mergeBranchHeadBefore)
 
 	validTargetRef := []byte("refs/merge-requests/x/merge")
 
@@ -1016,7 +1016,7 @@ func TestUserMergeToRefIgnoreHooksRequest(t *testing.T) {
 	testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
 	defer cleanupFn()
 
-	prepareMergeBranch(t, testRepoPath)
+	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", mergeBranchName, mergeBranchHeadBefore)
 
 	targetRef := []byte("refs/merge-requests/x/merge")
 	mergeCommitMessage := "Merged by Gitaly"
@@ -1044,20 +1044,6 @@ func TestUserMergeToRefIgnoreHooksRequest(t *testing.T) {
 	}
 }
 
-func prepareMergeBranch(t *testing.T, testRepoPath string) {
-	prepareMergeBranchWithHead(t, testRepoPath, mergeBranchHeadBefore)
-}
-
-func prepareMergeBranchWithHead(t *testing.T, testRepoPath, mergeBranchHead string) {
-	deleteBranch(testRepoPath, mergeBranchName)
-	out, err := exec.Command(config.Config.Git.BinPath, "-C", testRepoPath, "branch", mergeBranchName, mergeBranchHead).CombinedOutput()
-	require.NoError(t, err, "set up branch to merge into: %s", out)
-}
-
-func deleteBranch(testRepoPath, branchName string) {
-	exec.Command(config.Config.Git.BinPath, "-C", testRepoPath, "branch", "-D", branchName).Run()
-}
-
 // This error is used as a sentinel value
 var errRecvTimeout = fmt.Errorf("timeout waiting for response")
 
diff --git a/internal/gitaly/service/operations/tags_test.go b/internal/gitaly/service/operations/tags_test.go
index 6d01aab69..8e879667e 100644
--- a/internal/gitaly/service/operations/tags_test.go
+++ b/internal/gitaly/service/operations/tags_test.go
@@ -5,7 +5,6 @@ import (
 	"fmt"
 	"io/ioutil"
 	"os"
-	"os/exec"
 	"path/filepath"
 	"testing"
 
@@ -45,8 +44,6 @@ func testSuccessfulUserDeleteTagRequest(t *testing.T, ctx context.Context) {
 
 	tagNameInput := "to-be-deleted-soon-tag"
 
-	defer exec.Command(config.Config.Git.BinPath, "-C", testRepoPath, "tag", "-d", tagNameInput).Run()
-
 	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "tag", tagNameInput)
 
 	request := &gitalypb.UserDeleteTagRequest{
@@ -79,7 +76,6 @@ func testSuccessfulGitHooksForUserDeleteTagRequest(t *testing.T, ctx context.Con
 	defer cleanupFn()
 
 	tagNameInput := "to-be-déleted-soon-tag"
-	defer exec.Command(config.Config.Git.BinPath, "-C", testRepoPath, "tag", "-d", tagNameInput).Run()
 
 	request := &gitalypb.UserDeleteTagRequest{
 		Repository: testRepo,
@@ -964,7 +960,6 @@ func testUserDeleteTagsuccessfulDeletionOfPrefixedTag(t *testing.T, ctx context.
 	for _, testCase := range testCases {
 		t.Run(testCase.desc, func(t *testing.T) {
 			testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "tag", testCase.tagNameInput, testCase.tagCommit)
-			defer exec.Command(config.Config.Git.BinPath, "-C", testRepoPath, "tag", "-d", testCase.tagNameInput).Run()
 
 			request := &gitalypb.UserDeleteTagRequest{
 				Repository: testRepo,