Move functions around

internal/git/git-http.go

@@ -19,8 +19,29 @@ import (
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/helper"
 )
 
-func PostRPC(a *api.API) http.Handler {
-	return repoPreAuthorizeHandler(a, handlePostRPC)
+func ReceivePack(a *api.API) http.Handler {
+	return postRPCHandler(a, "handleReceivePack", handleReceivePack)
+}
+
+func UploadPack(a *api.API) http.Handler {
+	return postRPCHandler(a, "handleUploadPack", handleUploadPack)
+}
+
+func postRPCHandler(a *api.API, name string, handler func(*GitHttpResponseWriter, *http.Request, *api.Response) (int64, error)) http.Handler {
+	return repoPreAuthorizeHandler(a, func(rw http.ResponseWriter, r *http.Request, ar *api.Response) {
+		var writtenIn int64
+		var err error
+
+		w := NewGitHttpResponseWriter(rw)
+		defer func() {
+			w.Log(r, writtenIn)
+		}()
+
+		writtenIn, err = handler(w, r, ar)
+		if err != nil {
+			helper.LogError(r, fmt.Errorf("%s: %v", name, err))
+		}
+	})
 }
 
 func looksLikeRepo(p string) bool {
@@ -49,28 +70,6 @@ func repoPreAuthorizeHandler(myAPI *api.API, handleFunc api.HandleFunc) http.Han
 	}, "")
 }
 
-func handlePostRPC(rw http.ResponseWriter, r *http.Request, a *api.Response) {
-	var writtenIn int64
-
-	w := NewGitHttpResponseWriter(rw)
-	defer func() {
-		w.Log(r, writtenIn)
-	}()
-
-	action := getService(r)
-	if !(action == "git-upload-pack" || action == "git-receive-pack") {
-		// The 'dumb' Git HTTP protocol is not supported
-		helper.Fail500(w, r, fmt.Errorf("handlePostRPC: unsupported action: %s", r.URL.Path))
-		return
-	}
-
-	if action == "git-receive-pack" {
-		writtenIn, _ = handleReceivePack(action, w, r, a)
-	} else {
-		writtenIn, _ = handleUploadPack(action, w, r, a)
-	}
-}
-
 func setupGitCommand(action string, a *api.Response, w *GitHttpResponseWriter, r *http.Request) (cmd *exec.Cmd, stdin io.WriteCloser, stdout io.ReadCloser, err error) {
 	// Prepare our Git subprocess
 	cmd = gitCommand(a.GL_ID, "git", subCommand(action), "--stateless-rpc", a.RepoPath)

internal/git/git-http_test.go

@@ -32,14 +32,14 @@ func createTestPayload() []byte {
 }
 
 func TestHandleUploadPack(t *testing.T) {
-	testHandlePostRpc(t, "git-upload-pack")
+	testHandlePostRpc(t, "git-upload-pack", handleUploadPack)
 }
 
 func TestHandleReceivePack(t *testing.T) {
-	testHandlePostRpc(t, "git-receive-pack")
+	testHandlePostRpc(t, "git-receive-pack", handleReceivePack)
 }
 
-func testHandlePostRpc(t *testing.T, action string) {
+func testHandlePostRpc(t *testing.T, action string, handler func(*GitHttpResponseWriter, *http.Request, *api.Response) (int64, error)) {
 	execCommand = fakeExecCommand
 	defer func() { execCommand = exec.Command }()
 
@@ -55,7 +55,7 @@ func testHandlePostRpc(t *testing.T, action string) {
 	resp := &api.Response{GL_ID: GL_ID}
 
 	rr := httptest.NewRecorder()
-	handlePostRPC(rr, req, resp)
+	handler(NewGitHttpResponseWriter(rr), req, resp)
 
 	// Check HTTP status code
 	if status := rr.Code; status != http.StatusOK {

internal/git/receive-pack.go

@@ -9,12 +9,13 @@ import (
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/helper"
 )
 
-func handleReceivePack(action string, w *GitHttpResponseWriter, r *http.Request, a *api.Response) (writtenIn int64, err error) {
+func handleReceivePack(w *GitHttpResponseWriter, r *http.Request, a *api.Response) (writtenIn int64, err error) {
 	body := r.Body
+	action := getService(r)
 	cmd, stdin, stdout, err := setupGitCommand(action, a, w, r)
 
 	if err != nil {
-		return
+		return writtenIn, fmt.Errorf("setupGitCommand: %v", err)
 	}
 
 	defer stdout.Close()
@@ -25,8 +26,8 @@ func handleReceivePack(action string, w *GitHttpResponseWriter, r *http.Request,
 	writtenIn, err = io.Copy(stdin, body)
 
 	if err != nil {
-		helper.Fail500(w, r, fmt.Errorf("handleReceivePack: write to %v: %v", cmd.Args, err))
-		return
+		fail500(w)
+		return writtenIn, fmt.Errorf("write to %v: %v", cmd.Args, err)
 	}
 	// Signal to the Git subprocess that no more data is coming
 	stdin.Close()
@@ -41,18 +42,13 @@ func handleReceivePack(action string, w *GitHttpResponseWriter, r *http.Request,
 	_, err = io.Copy(w, stdout)
 
 	if err != nil {
-		helper.LogError(
-			r,
-			&copyError{fmt.Errorf("handleReceivePack: copy output of %v: %v", cmd.Args, err)},
-		)
-		return
+		return writtenIn, &copyError{fmt.Errorf("copy output of %v: %v", cmd.Args, err)}
 	}
 
 	err = cmd.Wait()
 
 	if err != nil {
-		helper.LogError(r, fmt.Errorf("handleReceivePack: wait for %v: %v", cmd.Args, err))
-		return
+		return writtenIn, fmt.Errorf("wait for %v: %v", cmd.Args, err)
 	}
 
 	return writtenIn, nil

internal/git/upload-pack.go

@@ -11,7 +11,7 @@ import (
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/helper"
 )
 
-func handleUploadPack(action string, w *GitHttpResponseWriter, r *http.Request, a *api.Response) (writtenIn int64, err error) {
+func handleUploadPack(w *GitHttpResponseWriter, r *http.Request, a *api.Response) (writtenIn int64, err error) {
 	var isShallowClone bool
 	var body io.Reader
 
@@ -21,8 +21,8 @@ func handleUploadPack(action string, w *GitHttpResponseWriter, r *http.Request,
 	// either.
 	_, err = io.Copy(buffer, io.LimitReader(r.Body, 4096))
 	if err != nil {
-		helper.Fail500(w, r, &copyError{fmt.Errorf("handleUploadPack: buffer git-upload-pack body: %v", err)})
-		return writtenIn, err
+		fail500(w)
+		return writtenIn, &copyError{fmt.Errorf("buffer git-upload-pack body: %v", err)}
 	}
 
 	isShallowClone = scanDeepen(bytes.NewReader(buffer.Bytes()))
@@ -32,17 +32,19 @@ func handleUploadPack(action string, w *GitHttpResponseWriter, r *http.Request,
 	buf, err := ioutil.ReadAll(body)
 
 	if err != nil {
-		helper.Fail500(w, r, &copyError{fmt.Errorf("handleUploadPack: full buffer git-upload-pack body: %v", err)})
-		return writtenIn, err
+		fail500(w)
+		return writtenIn, &copyError{fmt.Errorf("full buffer git-upload-pack body: %v", err)}
 	}
 
 	body = ioutil.NopCloser(bytes.NewBuffer(buf))
 	r.Body.Close()
 
+	action := getService(r)
 	cmd, stdin, stdout, err := setupGitCommand(action, a, w, r)
 
 	if err != nil {
-		return writtenIn, err
+		fail500(w)
+		return writtenIn, fmt.Errorf("setupGitCommand: %v", err)
 	}
 
 	defer stdout.Close()
@@ -56,34 +58,32 @@ func handleUploadPack(action string, w *GitHttpResponseWriter, r *http.Request,
 
 	go func() {
 		_, err := io.Copy(w, stdout)
-		if err != nil {
-			helper.LogError(
-				r,
-				&copyError{fmt.Errorf("handleUploadPack: copy output of %v: %v", cmd.Args, err)},
-			)
-		}
+		// This error may be lost if some other error prevents us from <-ing on this channel.
 		stdoutError <- err
 	}()
 
 	// Write the client request body to Git's standard input
 	if writtenIn, err = io.Copy(stdin, body); err != nil {
-		helper.Fail500(w, r, fmt.Errorf("handleUploadPack: write to %v: %v", cmd.Args, err))
-		return writtenIn, err
+		fail500(w)
+		return writtenIn, fmt.Errorf("write to %v: %v", cmd.Args, err)
 	}
 
 	// Signal to the Git subprocess that no more data is coming
 	stdin.Close()
 
 	if err := <-stdoutError; err != nil {
-		return writtenIn, err
+		return writtenIn, &copyError{fmt.Errorf("copy output of %v: %v", cmd.Args, err)}
 	}
 
 	err = cmd.Wait()
 
 	if err != nil && !(isExitError(err) && isShallowClone) {
-		helper.LogError(r, fmt.Errorf("handleUploadPack: wait for %v: %v", cmd.Args, err))
-		return writtenIn, err
+		return writtenIn, fmt.Errorf("wait for %v: %v", cmd.Args, err)
 	}
 
 	return writtenIn, nil
 }
+
+func fail500(w http.ResponseWriter) {
+	helper.Fail500(w, nil, nil)
+}

internal/upstream/routes.go

@@ -124,8 +124,8 @@ func (u *Upstream) configureRoutes() {
 	u.Routes = []routeEntry{
 		// Git Clone
 		route("GET", gitProjectPattern+`info/refs\z`, git.GetInfoRefsHandler(api, &u.Config)),
-		route("POST", gitProjectPattern+`git-upload-pack\z`, contentEncodingHandler(git.PostRPC(api)), isContentType("application/x-git-upload-pack-request")),
-		route("POST", gitProjectPattern+`git-receive-pack\z`, contentEncodingHandler(git.PostRPC(api)), isContentType("application/x-git-receive-pack-request")),
+		route("POST", gitProjectPattern+`git-upload-pack\z`, contentEncodingHandler(git.UploadPack(api)), isContentType("application/x-git-upload-pack-request")),
+		route("POST", gitProjectPattern+`git-receive-pack\z`, contentEncodingHandler(git.ReceivePack(api)), isContentType("application/x-git-receive-pack-request")),
 		route("PUT", gitProjectPattern+`gitlab-lfs/objects/([0-9a-f]{64})/([0-9]+)\z`, lfs.PutStore(api, proxy), isContentType("application/octet-stream")),
 
 		// CI Artifacts