DRY git commands

I've edited a few methods to DRY the code, plus some minor additions to
incorporate a review.

CHANGELOG

@@ -8,7 +8,6 @@ v1.0.0
   changes were made.
 - Add support for logging to file, and logfile rotation with SIGHUP.
 - Improve error messages.
-- Support to send diffs for single commits
 v0.8.5

internal/git/command.go

@@ -2,9 +2,13 @@ package git
 import (
 	"fmt"
+	"io"
+	"net/http"
 	"os"
 	"os/exec"
 	"syscall"
+
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/helper"
 )
 // Git subprocess helpers
@@ -24,3 +28,28 @@ func gitCommand(gl_id string, name string, args ...string) *exec.Cmd {
 	cmd.Stderr = os.Stderr
 	return cmd
 }
+
+func execGitCommand(w http.ResponseWriter, r *http.Request, cmd *exec.Cmd) {
+	stdout, err := cmd.StdoutPipe()
+	if err != nil {
+		helper.Fail500(w, r, fmt.Errorf("execGitCommand: create stdout pipe: %v", err))
+		return
+	}
+
+	if err := cmd.Start(); err != nil {
+		helper.Fail500(w, r, fmt.Errorf("execGitCommand: start %v: %v", cmd.Args, err))
+		return
+	}
+	defer helper.CleanUpProcessGroup(cmd)
+
+	w.Header().Del("Content-Length")
+	if _, err := io.Copy(w, stdout); err != nil {
+		helper.LogError(r, &copyError{fmt.Errorf("execGitCommand: copy %v stdout: %v", cmd.Args, err)})
+		return
+	}
+
+	if err := cmd.Wait(); err != nil {
+		helper.LogError(r, fmt.Errorf("execGitCommand: wait for %v: %v", cmd.Args, err))
+		return
+	}
+}

internal/git/diff.go

@@ -2,7 +2,6 @@ package git
 import (
 	"fmt"
-	"io"
 	"log"
 	"net/http"
@@ -29,28 +28,5 @@ func (d *diff) Inject(w http.ResponseWriter, r *http.Request, sendData string) {
 	log.Printf("SendDiff: sending diff between %q and %q for %q", params.ShaFrom, params.ShaTo, r.URL.Path)
 	gitDiffCmd := gitCommand("", "git", "--git-dir="+params.RepoPath, "diff", params.ShaFrom, params.ShaTo)
-	stdout, err := gitDiffCmd.StdoutPipe()
-	if err != nil {
-		helper.Fail500(w, r, fmt.Errorf("SendDiff: create stdout pipe: %v", err))
-		return
-	}
-
-	if err := gitDiffCmd.Start(); err != nil {
-		helper.Fail500(w, r, fmt.Errorf("SendDiff: start %v: %v", gitDiffCmd.Args, err))
-		return
-	}
-	defer helper.CleanUpProcessGroup(gitDiffCmd)
-
-	w.Header().Del("Content-Length")
-	if _, err := io.Copy(w, stdout); err != nil {
-		helper.LogError(
-			r,
-			&copyError{fmt.Errorf("SendDiff: copy %v stdout: %v", gitDiffCmd.Args, err)},
-		)
-		return
-	}
-	if err := gitDiffCmd.Wait(); err != nil {
-		helper.LogError(r, fmt.Errorf("SendDiff: wait for %v: %v", gitDiffCmd.Args, err))
-		return
-	}
+	execGitCommand(w, r, gitDiffCmd)
 }

internal/git/format-patch.go

@@ -2,7 +2,6 @@ package git
 import (
 	"fmt"
-	"io"
 	"log"
 	"net/http"
@@ -30,26 +29,5 @@ func (p *patch) Inject(w http.ResponseWriter, r *http.Request, sendData string)
 	gitRange := fmt.Sprintf("%s..%s", params.ShaFrom, params.ShaTo)
 	gitPatchCmd := gitCommand("", "git", "--git-dir="+params.RepoPath, "format-patch", gitRange, "--stdout")
-
-	stdout, err := gitPatchCmd.StdoutPipe()
-	if err != nil {
-		helper.Fail500(w, r, fmt.Errorf("SendPatch: create stdout pipe: %v", err))
-		return
-	}
-
-	if err := gitPatchCmd.Start(); err != nil {
-		helper.Fail500(w, r, fmt.Errorf("SendPatch: start %v: %v", gitPatchCmd.Args, err))
-		return
-	}
-	defer helper.CleanUpProcessGroup(gitPatchCmd)
-
-	w.Header().Del("Content-Length")
-	if _, err := io.Copy(w, stdout); err != nil {
-		helper.LogError(r, &copyError{fmt.Errorf("SendPatch: copy %v stdout: %v", gitPatchCmd.Args, err)})
-		return
-	}
-	if err := gitPatchCmd.Wait(); err != nil {
-		helper.LogError(r, fmt.Errorf("SendPatch: wait for %v: %v", gitPatchCmd.Args, err))
-		return
-	}
+	execGitCommand(w, r, gitPatchCmd)
 }

internal/git/show.go

@@ -2,7 +2,6 @@ package git
 import (
 	"fmt"
-	"io"
 	"log"
 	"net/http"
@@ -28,35 +27,24 @@ func (s *show) Inject(w http.ResponseWriter, r *http.Request, sendData string) {
 	log.Printf("SendCommit: sending commit %q for %q", params.Sha, r.URL.Path)
-	gitShowCmd := gitCommand("", "git", "--git-dir="+params.RepoPath, "show", "-p", format(params), params.Sha)
-
-	stdout, err := gitShowCmd.StdoutPipe()
+	format, err := format(params)
 	if err != nil {
-		helper.Fail500(w, r, fmt.Errorf("SendCommit: create stdout pipe: %v", err))
+		helper.Fail500(w, r, fmt.Errorf("SendCommit: %v", err))
 		return
 	}
-	if err := gitShowCmd.Start(); err != nil {
-		helper.Fail500(w, r, fmt.Errorf("SendCommit: start %v: %v", gitShowCmd.Args, err))
-		return
-	}
-	defer helper.CleanUpProcessGroup(gitShowCmd)
-
-	w.Header().Del("Content-Length")
-	if _, err := io.Copy(w, stdout); err != nil {
-		helper.LogError(r, &copyError{fmt.Errorf("SendCommit: copy %v stdout: %v", gitShowCmd.Args, err)})
-		return
-	}
-	if err := gitShowCmd.Wait(); err != nil {
-		helper.LogError(r, fmt.Errorf("SendCommit: wait for %v: %v", gitShowCmd.Args, err))
-		return
-	}
+	gitShowCmd := gitCommand("", "git", "--git-dir="+params.RepoPath, "show", "-p", format, params.Sha)
+	execGitCommand(w, r, gitShowCmd)
 }
-func format(params showParams) string {
-	if params.Format == "diff" {
-		return "--format="
+func format(params showParams) (string, error) {
+	switch params.Format {
+	case "diff":
+		// An empty format will only show the raw diff, nothing else
+		return "--format=", nil
+	case "email":
+		return "--format=email", nil
+	default:
+		return "", fmt.Errorf("format: %q is unsupported", params.Format)
 	}
-
-	return "--format=email"
 }

main_test.go

@@ -699,7 +699,7 @@ func TestGetGitPatch(t *testing.T) {
 	// HEAD of master branch against HEAD of fix branch
 	fromSha := "6907208d755b60ebeacb2e9dfea74c92c3449a1f"
 	toSha := "48f0be4bd10c1decee6fae52f9ae6d10f77b60f4"
-	jsonParams := fmt.Sprintf(`{"RepoPath":"%s","ShaFrom":"%s","ShaTo":"%s"}`, path.Join(testRepoRoot, testRepo), fromSha, toSha)
+	jsonParams := fmt.Sprintf(`{"RepoPath":%q,"ShaFrom":%q,"ShaTo":%q}`, path.Join(testRepoRoot, testRepo), fromSha, toSha)
 	resp, body, err := doSendDataRequest("/something", "git-format-patch", jsonParams)
 	if err != nil {
@@ -718,7 +718,7 @@ func TestGetGitCommitDiff(t *testing.T) {
 	sha := "498214de67004b1da3d820901307bed2a68a8ef6"
 	repoPath := path.Join(testRepoRoot, testRepo)
 	format := "diff"
-	jsonParams := fmt.Sprintf(`{"RepoPath":"%s","Sha":"%s","Format":"%s"}`, repoPath, sha, format)
+	jsonParams := fmt.Sprintf(`{"RepoPath":%q,"Sha":%q,"Format":%q}`, repoPath, sha, format)
 	resp, body, err := doSendDataRequest("/git-show-commit", "git-show-commit", jsonParams)
 	if err != nil {
@@ -740,7 +740,7 @@ func TestGetGitCommitEmail(t *testing.T) {
 	sha := "498214de67004b1da3d820901307bed2a68a8ef6"
 	repoPath := path.Join(testRepoRoot, testRepo)
 	format := "email"
-	jsonParams := fmt.Sprintf(`{"RepoPath":"%s","Sha":"%s","Format":"%s"}`, repoPath, sha, format)
+	jsonParams := fmt.Sprintf(`{"RepoPath":%q,"Sha":%q,"Format":%q}`, repoPath, sha, format)
 	resp, body, err := doSendDataRequest("/git-show-commit", "git-show-commit", jsonParams)
 	if err != nil {
@@ -751,7 +751,7 @@ func TestGetGitCommitEmail(t *testing.T) {
 		t.Errorf("GET %q: expected HTTP 200, got %d", resp.Request.URL, resp.StatusCode)
 	}
-	prefix := "From 498214de67004b1da3d820901307bed2a68a8ef6 Mon Sep 17 00:00:00 2001"
+	prefix := fmt.Sprintf("From %s Mon Sep 17 00:00:00 2001", sha)
 	bodyText := string(body)
 	if !strings.HasPrefix(bodyText, prefix) {
 		t.Fatalf("Expected: %v, got: %v", prefix, bodyText)