Merge branch 'chomp-bytes' into 'master'

Introduce text.ChompBytes helper

See merge request gitlab-org/gitaly!1144

17 files changed, 54 insertions, 37 deletions

diff --git a/changelogs/unreleased/chomp-bytes.yml b/changelogs/unreleased/chomp-bytes.yml
new file mode 100644
index 000000000..3cc448493
--- /dev/null
+++ b/changelogs/unreleased/chomp-bytes.yml
@@ -0,0 +1,5 @@
+---
+title: Introduce text.ChompBytes helper
+merge_request: 1144
+author:
+type: other
diff --git a/internal/git/log/last_commit.go b/internal/git/log/last_commit.go
index 4f08e0acb..0f356d570 100644
--- a/internal/git/log/last_commit.go
+++ b/internal/git/log/last_commit.go
@@ -3,13 +3,13 @@ package log
 import (
 	"context"
 	"io/ioutil"
-	"strings"
 
 	grpc_logrus "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/helper/text"
 )
 
 // LastCommitForPath returns the last commit which modified path.
@@ -24,7 +24,7 @@ func LastCommitForPath(ctx context.Context, repo *gitalypb.Repository, revision
 		return nil, err
 	}
 
-	return GetCommit(ctx, repo, strings.TrimSpace(string(commitID)))
+	return GetCommit(ctx, repo, text.ChompBytes(commitID))
 }
 
 // GitLogCommand returns a Command that executes git log with the given the arguments
diff --git a/internal/git/objectpool/link.go b/internal/git/objectpool/link.go
index 1b5614c1b..389b24534 100644
--- a/internal/git/objectpool/link.go
+++ b/internal/git/objectpool/link.go
@@ -8,9 +8,9 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
-	"strings"
 
 	"gitlab.com/gitlab-org/gitaly/internal/git/remote"
+	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
 
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
@@ -46,7 +46,7 @@ func (o *ObjectPool) Link(ctx context.Context, repo *gitalypb.Repository) error
 
 	actualContent, err := ioutil.ReadFile(altPath)
 	if err == nil {
-		if strings.TrimSuffix(string(actualContent), "\n") == expectedContent {
+		if text.ChompBytes(actualContent) == expectedContent {
 			return nil
 		}
 
diff --git a/internal/helper/text/chomp.go b/internal/helper/text/chomp.go
new file mode 100644
index 000000000..6ce64cafa
--- /dev/null
+++ b/internal/helper/text/chomp.go
@@ -0,0 +1,8 @@
+package text
+
+import "strings"
+
+// ChompBytes converts b to a string with its trailing newline, if present, removed.
+func ChompBytes(b []byte) string {
+	return strings.TrimSuffix(string(b), "\n")
+}
diff --git a/internal/service/commit/languages.go b/internal/service/commit/languages.go
index 166ff5b8c..73ef24d6e 100644
--- a/internal/service/commit/languages.go
+++ b/internal/service/commit/languages.go
@@ -4,11 +4,11 @@ import (
 	"context"
 	"io/ioutil"
 	"sort"
-	"strings"
 
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
+	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
 	"gitlab.com/gitlab-org/gitaly/internal/linguist"
 	"gitlab.com/gitlab-org/gitaly/internal/service/ref"
 	"google.golang.org/grpc/codes"
@@ -86,5 +86,5 @@ func lookupRevision(ctx context.Context, repo *gitalypb.Repository, revision str
 		return "", err
 	}
 
-	return strings.TrimSpace(string(revParseBytes)), nil
+	return text.ChompBytes(revParseBytes), nil
 }
diff --git a/internal/service/operations/commit_files_test.go b/internal/service/operations/commit_files_test.go
index e26a05946..19b8eeedd 100644
--- a/internal/service/operations/commit_files_test.go
+++ b/internal/service/operations/commit_files_test.go
@@ -3,12 +3,12 @@ package operations_test
 import (
 	"fmt"
 	"strconv"
-	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/internal/git/log"
+	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
 	"gitlab.com/gitlab-org/gitaly/internal/service/operations"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"google.golang.org/grpc/codes"
@@ -223,7 +223,7 @@ func TestSuccessfulUserCommitFilesRequestForceCommit(t *testing.T) {
 	require.NoError(t, err)
 
 	mergeBaseOut := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "merge-base", targetBranchCommit.Id, startBranchCommit.Id)
-	mergeBaseID := strings.TrimSuffix(string(mergeBaseOut), "\n")
+	mergeBaseID := text.ChompBytes(mergeBaseOut)
 	require.NotEqual(t, mergeBaseID, targetBranchCommit.Id, "expected %s not to be an ancestor of %s", targetBranchCommit.Id, startBranchCommit.Id)
 
 	headerRequest := headerRequest(testRepo, user, targetBranchName, commitFilesMessage)
diff --git a/internal/service/operations/merge_test.go b/internal/service/operations/merge_test.go
index aab11c5d9..a682e4ff3 100644
--- a/internal/service/operations/merge_test.go
+++ b/internal/service/operations/merge_test.go
@@ -13,6 +13,7 @@ import (
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
 	gitlog "gitlab.com/gitlab-org/gitaly/internal/git/log"
+	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"google.golang.org/grpc/codes"
 )
@@ -273,7 +274,7 @@ func TestFailedMergeDueToHooks(t *testing.T) {
 			require.NoError(t, err, "consume EOF")
 
 			currentBranchHead := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "rev-parse", mergeBranchName)
-			require.Equal(t, mergeBranchHeadBefore, strings.TrimSpace(string(currentBranchHead)), "branch head updated")
+			require.Equal(t, mergeBranchHeadBefore, text.ChompBytes(currentBranchHead), "branch head updated")
 		})
 	}
 
@@ -315,7 +316,7 @@ func TestSuccessfulUserFFBranchRequest(t *testing.T) {
 	require.NoError(t, err)
 	require.Equal(t, expectedResponse, resp)
 	newBranchHead := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "rev-parse", branchName)
-	require.Equal(t, commitID, strings.TrimSpace(string(newBranchHead)), "branch head not updated")
+	require.Equal(t, commitID, text.ChompBytes(newBranchHead), "branch head not updated")
 }
 
 func TestFailedUserFFBranchRequest(t *testing.T) {
@@ -481,7 +482,7 @@ func TestSuccessfulUserMergeToRefRequest(t *testing.T) {
 		targetRef []byte
 		emptyRef  bool
 		sourceSha string
-		message   []byte
+		message   string
 	}{
 		{
 			desc:      "empty target ref merge",
@@ -490,7 +491,7 @@ func TestSuccessfulUserMergeToRefRequest(t *testing.T) {
 			targetRef: emptyTargetRef,
 			emptyRef:  true,
 			sourceSha: commitToMerge,
-			message:   []byte(mergeCommitMessage),
+			message:   mergeCommitMessage,
 		},
 		{
 			desc:      "existing target ref",
@@ -499,7 +500,7 @@ func TestSuccessfulUserMergeToRefRequest(t *testing.T) {
 			targetRef: existingTargetRef,
 			emptyRef:  false,
 			sourceSha: commitToMerge,
-			message:   []byte(mergeCommitMessage),
+			message:   mergeCommitMessage,
 		},
 	}
 
@@ -514,7 +515,7 @@ func TestSuccessfulUserMergeToRefRequest(t *testing.T) {
 				Branch:     testCase.branch,
 				TargetRef:  testCase.targetRef,
 				SourceSha:  testCase.sourceSha,
-				Message:    testCase.message,
+				Message:    []byte(testCase.message),
 			}
 
 			commitBeforeRefMerge, fetchRefBeforeMergeErr := gitlog.GetCommit(ctx, testRepo, string(testCase.targetRef))
@@ -533,7 +534,7 @@ func TestSuccessfulUserMergeToRefRequest(t *testing.T) {
 			// Asserts commit parent SHAs
 			require.Equal(t, []string{mergeBranchHeadBefore, testCase.sourceSha}, commit.ParentIds, "merge commit parents must be the sha before HEAD and source sha")
 
-			require.True(t, strings.HasPrefix(string(commit.Body), string(testCase.message)), "expected %q to start with %q", commit.Body, string(testCase.message))
+			require.True(t, strings.HasPrefix(string(commit.Body), testCase.message), "expected %q to start with %q", commit.Body, testCase.message)
 
 			// Asserts author
 			author := commit.Author
diff --git a/internal/service/operations/tags_test.go b/internal/service/operations/tags_test.go
index b458fb000..e8d10d1d4 100644
--- a/internal/service/operations/tags_test.go
+++ b/internal/service/operations/tags_test.go
@@ -3,12 +3,12 @@ package operations
 import (
 	"os"
 	"os/exec"
-	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/internal/git/log"
+	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"google.golang.org/grpc/codes"
 )
@@ -168,7 +168,7 @@ func TestSuccessfulUserCreateTagRequest(t *testing.T) {
 			defer exec.Command("git", "-C", testRepoPath, "tag", "-d", inputTagName).Run()
 
 			id := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "rev-parse", inputTagName)
-			testCase.expectedTag.Id = strings.TrimSpace(string(id))
+			testCase.expectedTag.Id = text.ChompBytes(id)
 
 			require.NoError(t, err)
 			require.Equal(t, testCase.expectedTag, response.Tag)
diff --git a/internal/service/repository/gc_test.go b/internal/service/repository/gc_test.go
index 651a5dfea..b8f258ddd 100644
--- a/internal/service/repository/gc_test.go
+++ b/internal/service/repository/gc_test.go
@@ -7,13 +7,13 @@ import (
 	"os"
 	"path"
 	"path/filepath"
-	"strings"
 	"testing"
 	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
+	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"google.golang.org/grpc/codes"
 )
@@ -235,7 +235,7 @@ func TestCleanupInvalidKeepAroundRefs(t *testing.T) {
 
 			// The existing keeparound still exists
 			commitSha := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "rev-parse", existingRefName)
-			require.Equal(t, existingSha, strings.TrimSpace(string(commitSha)))
+			require.Equal(t, existingSha, text.ChompBytes(commitSha))
 
 			//The invalid one was removed
 			_, err = os.Stat(bogusPath)
@@ -244,7 +244,7 @@ func TestCleanupInvalidKeepAroundRefs(t *testing.T) {
 			if testcase.shouldExist {
 				keepAroundName := fmt.Sprintf("refs/keep-around/%s", testcase.refName)
 				commitSha := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "rev-parse", keepAroundName)
-				require.Equal(t, testcase.refName, strings.TrimSpace(string(commitSha)))
+				require.Equal(t, testcase.refName, text.ChompBytes(commitSha))
 			} else {
 				_, err := os.Stat(refPath)
 				require.True(t, os.IsNotExist(err), "expected 'does not exist' error, got %v", err)
diff --git a/internal/service/repository/merge_base.go b/internal/service/repository/merge_base.go
index 68850ce70..890779ebd 100644
--- a/internal/service/repository/merge_base.go
+++ b/internal/service/repository/merge_base.go
@@ -3,10 +3,10 @@ package repository
 import (
 	"context"
 	"io/ioutil"
-	"strings"
 
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
+	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 )
@@ -35,7 +35,7 @@ func (s *server) FindMergeBase(ctx context.Context, req *gitalypb.FindMergeBaseR
 		return nil, err
 	}
 
-	mergeBaseStr := strings.TrimSpace(string(mergeBase))
+	mergeBaseStr := text.ChompBytes(mergeBase)
 
 	if err := cmd.Wait(); err != nil {
 		// On error just return an empty merge base
diff --git a/internal/service/repository/write_config_test.go b/internal/service/repository/write_config_test.go
index 135025b50..fd8770cb6 100644
--- a/internal/service/repository/write_config_test.go
+++ b/internal/service/repository/write_config_test.go
@@ -1,11 +1,11 @@
 package repository
 
 import (
-	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
+	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"google.golang.org/grpc/codes"
 )
@@ -50,7 +50,7 @@ func TestWriteConfigSuccessful(t *testing.T) {
 			require.Empty(t, string(c.GetError()))
 
 			actualConfig := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "config", "gitlab.fullpath")
-			require.Equal(t, tc.setPath, strings.TrimRight(string(actualConfig), "\n"))
+			require.Equal(t, tc.setPath, text.ChompBytes(actualConfig))
 		})
 	}
 }
diff --git a/internal/service/smarthttp/receive_pack_test.go b/internal/service/smarthttp/receive_pack_test.go
index 6525aa72e..8223b0ae8 100644
--- a/internal/service/smarthttp/receive_pack_test.go
+++ b/internal/service/smarthttp/receive_pack_test.go
@@ -17,6 +17,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/config"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/hooks"
+	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/streamio"
 	"google.golang.org/grpc/codes"
@@ -224,7 +225,7 @@ func createCommit(t *testing.T, repoPath string, fileContents []byte) (oldHead s
 	committerEmail := "scrooge@mcduck.com"
 
 	// The latest commit ID on the remote repo
-	oldHead = strings.TrimSpace(string(testhelper.MustRunCommand(t, nil, "git", "-C", repoPath, "rev-parse", "master")))
+	oldHead = text.ChompBytes(testhelper.MustRunCommand(t, nil, "git", "-C", repoPath, "rev-parse", "master"))
 
 	changedFile := "README.md"
 	require.NoError(t, ioutil.WriteFile(path.Join(repoPath, changedFile), fileContents, 0644))
@@ -236,7 +237,7 @@ func createCommit(t *testing.T, repoPath string, fileContents []byte) (oldHead s
 		"commit", "-m", commitMsg)
 
 	// The commit ID we want to push to the remote repo
-	newHead = strings.TrimSpace(string(testhelper.MustRunCommand(t, nil, "git", "-C", repoPath, "rev-parse", "master")))
+	newHead = text.ChompBytes(testhelper.MustRunCommand(t, nil, "git", "-C", repoPath, "rev-parse", "master"))
 
 	return oldHead, newHead
 }
diff --git a/internal/service/wiki/get_page_versions_test.go b/internal/service/wiki/get_page_versions_test.go
index aafdf6e69..6da221901 100644
--- a/internal/service/wiki/get_page_versions_test.go
+++ b/internal/service/wiki/get_page_versions_test.go
@@ -3,12 +3,12 @@ package wiki
 import (
 	"bytes"
 	"io"
-	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
+	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 )
 
@@ -59,19 +59,19 @@ func TestWikiGetPageVersionsRequest(t *testing.T) {
 			versions: []*gitalypb.WikiPageVersion{
 				{
 					Commit: &gitalypb.GitCommit{
-						Id:        strings.TrimRight(string(v2cid), "\n"),
+						Id:        text.ChompBytes(v2cid),
 						Body:      []byte("Update WikiGétPageVersions"),
 						Subject:   []byte("Update WikiGétPageVersions"),
 						Author:    gitAuthor,
 						Committer: gitAuthor,
-						ParentIds: []string{strings.TrimRight(string(v1cid), "\n")},
+						ParentIds: []string{text.ChompBytes(v1cid)},
 						BodySize:  26,
 					},
 					Format: "markdown",
 				},
 				{
 					Commit: &gitalypb.GitCommit{
-						Id:        strings.TrimRight(string(v1cid), "\n"),
+						Id:        text.ChompBytes(v1cid),
 						Body:      []byte("Add WikiGétPageVersions"),
 						Subject:   []byte("Add WikiGétPageVersions"),
 						Author:    gitAuthor,
diff --git a/internal/supervisor/monitor.go b/internal/supervisor/monitor.go
index 1be009638..4544e259a 100644
--- a/internal/supervisor/monitor.go
+++ b/internal/supervisor/monitor.go
@@ -3,11 +3,11 @@ package supervisor
 import (
 	"os/exec"
 	"strconv"
-	"strings"
 	"time"
 
 	"github.com/prometheus/client_golang/prometheus"
 	log "github.com/sirupsen/logrus"
+	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
 )
 
 var (
@@ -82,7 +82,7 @@ func getRss(pid int) int {
 		return 0
 	}
 
-	rss, err := strconv.Atoi(strings.TrimSpace(string(psRss)))
+	rss, err := strconv.Atoi(text.ChompBytes(psRss))
 	if err != nil {
 		return 0
 	}
diff --git a/internal/testhelper/commit.go b/internal/testhelper/commit.go
index fe3b48d1d..81596352e 100644
--- a/internal/testhelper/commit.go
+++ b/internal/testhelper/commit.go
@@ -6,10 +6,10 @@ import (
 	"os"
 	"os/exec"
 	"path"
-	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
 )
 
 // CreateCommitOpts holds extra options for CreateCommit.
@@ -50,7 +50,7 @@ func CreateCommit(t *testing.T, repoPath, branchName string, opts *CreateCommitO
 		"commit-tree", "-F", "-", "-p", parentID, parentID + "^{tree}",
 	}
 	newCommit := MustRunCommand(t, stdin, "git", commitArgs...)
-	newCommitID := strings.TrimSpace(string(newCommit))
+	newCommitID := text.ChompBytes(newCommit)
 
 	MustRunCommand(t, nil, "git", "-C", repoPath, "update-ref", "refs/heads/"+branchName, newCommitID)
 	return newCommitID
diff --git a/internal/testhelper/tag.go b/internal/testhelper/tag.go
index 21e2e9a21..8ab0e64df 100644
--- a/internal/testhelper/tag.go
+++ b/internal/testhelper/tag.go
@@ -3,8 +3,9 @@ package testhelper
 import (
 	"bytes"
 	"fmt"
-	"strings"
 	"testing"
+
+	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
 )
 
 // CreateTagOpts holds extra options for CreateTag.
@@ -41,5 +42,5 @@ func CreateTag(t *testing.T, repoPath, tagName, targetID string, opts *CreateTag
 	MustRunCommand(t, stdin, "git", args...)
 
 	tagID := MustRunCommand(t, nil, "git", "-C", repoPath, "show-ref", "-s", tagName)
-	return strings.TrimSpace(string(tagID))
+	return text.ChompBytes(tagID)
 }
diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go
index ea679e795..8ceb75413 100644
--- a/internal/testhelper/testhelper.go
+++ b/internal/testhelper/testhelper.go
@@ -29,6 +29,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/config"
 	"gitlab.com/gitlab-org/gitaly/internal/helper/fieldextractors"
+	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
 	gitalylog "gitlab.com/gitlab-org/gitaly/internal/log"
 	"gitlab.com/gitlab-org/gitaly/internal/storage"
 	"google.golang.org/grpc"
@@ -332,7 +333,7 @@ func mustFindNoRunningChildProcess() {
 
 	out, err := pgrep.Output()
 	if err == nil {
-		pidsComma := strings.Replace(strings.TrimSpace(string(out)), ",", "\n", -1)
+		pidsComma := strings.Replace(text.ChompBytes(out), "\n", ",", -1)
 		psOut, _ := exec.Command("ps", "-o", "pid,args", "-p", pidsComma).Output()
 		panic(fmt.Errorf("found running child processes %s:\n%s", pidsComma, psOut))
 	}