Merge branch 'error-style' into 'master'

Make error style more uniform

Deprecate 'logContext' in favor of just passing Go error values.

Also adds to missing 'return' statements after a 'fail500' in archive.go.

See merge request !16

archive.go

@@ -5,7 +5,6 @@ In this file we handle 'git archive' downloads
 package main
 
 import (
-	"errors"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -20,7 +19,8 @@ import (
 
 func handleGetArchive(w http.ResponseWriter, r *gitRequest) {
 	var format string
-	switch filepath.Base(r.URL.Path) {
+	urlPath := r.URL.Path
+	switch filepath.Base(urlPath) {
 	case "archive.zip":
 		format = "zip"
 	case "archive.tar":
@@ -30,7 +30,8 @@ func handleGetArchive(w http.ResponseWriter, r *gitRequest) {
 	case "archive.tar.bz2":
 		format = "tar.bz2"
 	default:
-		fail500(w, "handleGetArchive", errors.New("invalid archive format"))
+		fail500(w, fmt.Errorf("handleGetArchive: invalid format: %s", urlPath))
+		return
 	}
 
 	archiveFilename := path.Base(r.ArchivePath)
@@ -52,7 +53,8 @@ func handleGetArchive(w http.ResponseWriter, r *gitRequest) {
 	// to finalize the cached archive.
 	tempFile, err := prepareArchiveTempfile(path.Dir(r.ArchivePath), archiveFilename)
 	if err != nil {
-		fail500(w, "handleGetArchive create tempfile for archive", err)
+		fail500(w, fmt.Errorf("handleGetArchive: create tempfile: %v", err))
+		return
 	}
 	defer tempFile.Close()
 	defer os.Remove(tempFile.Name())
@@ -62,12 +64,12 @@ func handleGetArchive(w http.ResponseWriter, r *gitRequest) {
 	archiveCmd := gitCommand("", "git", "--git-dir="+r.RepoPath, "archive", "--format="+archiveFormat, "--prefix="+r.ArchivePrefix+"/", r.CommitId)
 	archiveStdout, err := archiveCmd.StdoutPipe()
 	if err != nil {
-		fail500(w, "handleGetArchive", err)
+		fail500(w, fmt.Errorf("handleGetArchive: archive stdout: %v", err))
 		return
 	}
 	defer archiveStdout.Close()
 	if err := archiveCmd.Start(); err != nil {
-		fail500(w, "handleGetArchive", err)
+		fail500(w, fmt.Errorf("handleGetArchive: start %v: %v", archiveCmd.Args, err))
 		return
 	}
 	defer cleanUpProcessGroup(archiveCmd) // Ensure brute force subprocess clean-up
@@ -80,13 +82,13 @@ func handleGetArchive(w http.ResponseWriter, r *gitRequest) {
 
 		stdout, err = compressCmd.StdoutPipe()
 		if err != nil {
-			fail500(w, "handleGetArchive compressCmd stdout pipe", err)
+			fail500(w, fmt.Errorf("handleGetArchive: compress stdout: %v", err))
 			return
 		}
 		defer stdout.Close()
 
 		if err := compressCmd.Start(); err != nil {
-			fail500(w, "handleGetArchive start compressCmd process", err)
+			fail500(w, fmt.Errorf("handleGetArchive: start %v: %v", compressCmd.Args, err))
 			return
 		}
 		defer compressCmd.Wait()
@@ -101,22 +103,22 @@ func handleGetArchive(w http.ResponseWriter, r *gitRequest) {
 	setArchiveHeaders(w, format, archiveFilename)
 	w.WriteHeader(200) // Don't bother with HTTP 500 from this point on, just return
 	if _, err := io.Copy(w, archiveReader); err != nil {
-		logContext("handleGetArchive read from subprocess", err)
+		logError(fmt.Errorf("handleGetArchive: read: %v", err))
 		return
 	}
 	if err := archiveCmd.Wait(); err != nil {
-		logContext("handleGetArchive wait for archiveCmd", err)
+		logError(fmt.Errorf("handleGetArchive: archiveCmd: %v", err))
 		return
 	}
 	if compressCmd != nil {
 		if err := compressCmd.Wait(); err != nil {
-			logContext("handleGetArchive wait for compressCmd", err)
+			logError(fmt.Errorf("handleGetArchive: compressCmd: %v", err))
 			return
 		}
 	}
 
 	if err := finalizeCachedArchive(tempFile, r.ArchivePath); err != nil {
-		logContext("handleGetArchive finalize cached archive", err)
+		logError(fmt.Errorf("handleGetArchive: finalize cached archive: %v", err))
 		return
 	}
 }

authorization.go

@@ -3,6 +3,7 @@ package main
 import (
 	"encoding/json"
 	"errors"
+	"fmt"
 	"io"
 	"net/http"
 	"strings"
@@ -12,13 +13,13 @@ func preAuthorizeHandler(handleFunc serviceHandleFunc, suffix string) serviceHan
 	return func(w http.ResponseWriter, r *gitRequest) {
 		authReq, err := r.u.newUpstreamRequest(r.Request, nil, suffix)
 		if err != nil {
-			fail500(w, "newUpstreamRequest", err)
+			fail500(w, fmt.Errorf("preAuthorizeHandler: newUpstreamRequest: %v", err))
 			return
 		}
 
 		authResponse, err := r.u.httpClient.Do(authReq)
 		if err != nil {
-			fail500(w, "doAuthRequest", err)
+			fail500(w, fmt.Errorf("preAuthorizeHandler: do %v: %v", authReq.URL.Path, err))
 			return
 		}
 		defer authResponse.Body.Close()
@@ -46,7 +47,7 @@ func preAuthorizeHandler(handleFunc serviceHandleFunc, suffix string) serviceHan
 		// request metadata. We must extract this information from the auth
 		// response body.
 		if err := json.NewDecoder(authResponse.Body).Decode(&r.authorizationResponse); err != nil {
-			fail500(w, "decode authorization response", err)
+			fail500(w, fmt.Errorf("preAuthorizeHandler: decode authorization response: %v", err))
 			return
 		}
 		// Don't hog a TCP connection in CLOSE_WAIT, we can already close it now
@@ -68,7 +69,7 @@ func preAuthorizeHandler(handleFunc serviceHandleFunc, suffix string) serviceHan
 func repoPreAuthorizeHandler(handleFunc serviceHandleFunc) serviceHandleFunc {
 	return preAuthorizeHandler(func(w http.ResponseWriter, r *gitRequest) {
 		if r.RepoPath == "" {
-			fail500(w, "repoPreAuthorizeHandler", errors.New("missing authorization response"))
+			fail500(w, errors.New("repoPreAuthorizeHandler: RepoPath empty"))
 			return
 		}
 

git-http.go

@@ -24,12 +24,12 @@ func handleGetInfoRefs(w http.ResponseWriter, r *gitRequest) {
 	cmd := gitCommand(r.GL_ID, "git", subCommand(rpc), "--stateless-rpc", "--advertise-refs", r.RepoPath)
 	stdout, err := cmd.StdoutPipe()
 	if err != nil {
-		fail500(w, "handleGetInfoRefs", err)
+		fail500(w, fmt.Errorf("handleGetInfoRefs: stdout: %v", err))
 		return
 	}
 	defer stdout.Close()
 	if err := cmd.Start(); err != nil {
-		fail500(w, "handleGetInfoRefs", err)
+		fail500(w, fmt.Errorf("handleGetInfoRefs: start %v: %v", cmd.Args, err))
 		return
 	}
 	defer cleanUpProcessGroup(cmd) // Ensure brute force subprocess clean-up
@@ -39,19 +39,19 @@ func handleGetInfoRefs(w http.ResponseWriter, r *gitRequest) {
 	w.Header().Add("Cache-Control", "no-cache")
 	w.WriteHeader(200) // Don't bother with HTTP 500 from this point on, just return
 	if err := pktLine(w, fmt.Sprintf("# service=%s\n", rpc)); err != nil {
-		logContext("handleGetInfoRefs response", err)
+		logError(fmt.Errorf("handleGetInfoRefs: pktLine: %v", err))
 		return
 	}
 	if err := pktFlush(w); err != nil {
-		logContext("handleGetInfoRefs response", err)
+		logError(fmt.Errorf("handleGetInfoRefs: pktFlush: %v", err))
 		return
 	}
 	if _, err := io.Copy(w, stdout); err != nil {
-		logContext("handleGetInfoRefs read from subprocess", err)
+		logError(fmt.Errorf("handleGetInfoRefs: read from %v: %v", cmd.Args, err))
 		return
 	}
 	if err := cmd.Wait(); err != nil {
-		logContext("handleGetInfoRefs wait for subprocess", err)
+		logError(fmt.Errorf("handleGetInfoRefs: wait for %v: %v", cmd.Args, err))
 		return
 	}
 }
@@ -63,7 +63,7 @@ func handlePostRPC(w http.ResponseWriter, r *gitRequest) {
 	action := filepath.Base(r.URL.Path)
 	if !(action == "git-upload-pack" || action == "git-receive-pack") {
 		// The 'dumb' Git HTTP protocol is not supported
-		fail500(w, "handlePostRPC", err)
+		fail500(w, fmt.Errorf("handlePostRPC: unsupported action: %s", r.URL.Path))
 		return
 	}
 
@@ -71,25 +71,25 @@ func handlePostRPC(w http.ResponseWriter, r *gitRequest) {
 	cmd := gitCommand(r.GL_ID, "git", subCommand(action), "--stateless-rpc", r.RepoPath)
 	stdout, err := cmd.StdoutPipe()
 	if err != nil {
-		fail500(w, "handlePostRPC", err)
+		fail500(w, fmt.Errorf("handlePostRPC: stdout: %v", err))
 		return
 	}
 	defer stdout.Close()
 	stdin, err := cmd.StdinPipe()
 	if err != nil {
-		fail500(w, "handlePostRPC", err)
+		fail500(w, fmt.Errorf("handlePostRPC: stdin: %v", err))
 		return
 	}
 	defer stdin.Close()
 	if err := cmd.Start(); err != nil {
-		fail500(w, "handlePostRPC", err)
+		fail500(w, fmt.Errorf("handlePostRPC: start %v: %v", cmd.Args, err))
 		return
 	}
 	defer cleanUpProcessGroup(cmd) // Ensure brute force subprocess clean-up
 
 	// Write the client request body to Git's standard input
 	if _, err := io.Copy(stdin, r.Body); err != nil {
-		fail500(w, "handlePostRPC write to subprocess", err)
+		fail500(w, fmt.Errorf("handlePostRPC write to %v: %v", cmd.Args, err))
 		return
 	}
 	// Signal to the Git subprocess that no more data is coming
@@ -106,11 +106,11 @@ func handlePostRPC(w http.ResponseWriter, r *gitRequest) {
 
 	// This io.Copy may take a long time, both for Git push and pull.
 	if _, err := io.Copy(w, stdout); err != nil {
-		logContext("handlePostRPC read from subprocess", err)
+		logError(fmt.Errorf("handlePostRPC read from %v: %v", cmd.Args, err))
 		return
 	}
 	if err := cmd.Wait(); err != nil {
-		logContext("handlePostRPC wait for subprocess", err)
+		logError(fmt.Errorf("handlePostRPC wait for %v: %v", cmd.Args, err))
 		return
 	}
 }

handlers.go

@@ -24,7 +24,7 @@ func contentEncodingHandler(handleFunc serviceHandleFunc) serviceHandleFunc {
 		}
 
 		if err != nil {
-			fail500(w, "contentEncodingHandler", err)
+			fail500(w, fmt.Errorf("contentEncodingHandler: %v", err))
 			return
 		}
 		defer body.Close()

helpers.go

@@ -17,18 +17,18 @@ import (
 	"syscall"
 )
 
-func fail400(w http.ResponseWriter, context string, err error) {
+func fail400(w http.ResponseWriter, err error) {
 	http.Error(w, "Bad request", 400)
-	logContext(context, err)
+	logError(err)
 }
 
-func fail500(w http.ResponseWriter, context string, err error) {
+func fail500(w http.ResponseWriter, err error) {
 	http.Error(w, "Internal server error", 500)
-	logContext(context, err)
+	logError(err)
 }
 
-func logContext(context string, err error) {
-	log.Printf("%s: %v", context, err)
+func logError(err error) {
+	log.Printf("error: %v", err)
 }
 
 // Git subprocess helpers

lfs.go

@@ -8,6 +8,7 @@ import (
 	"crypto/sha256"
 	"encoding/hex"
 	"errors"
+	"fmt"
 	"io"
 	"io/ioutil"
 	"net/http"
@@ -15,26 +16,21 @@ import (
 	"path/filepath"
 )
 
-var (
-	errHashMismatch = errors.New("Content hash does not match OID")
-	errSizeMismatch = errors.New("Content size does not match")
-)
-
 func lfsAuthorizeHandler(handleFunc serviceHandleFunc) serviceHandleFunc {
 	return preAuthorizeHandler(func(w http.ResponseWriter, r *gitRequest) {
 
 		if r.StoreLFSPath == "" {
-			fail500(w, "lfsAuthorizeHandler", errors.New("Don't know where to store object, no store path specified."))
+			fail500(w, errors.New("lfsAuthorizeHandler: StoreLFSPath empty"))
 			return
 		}
 
 		if r.LfsOid == "" {
-			fail500(w, "lfsAuthorizeHandler", errors.New("Lfs object oid not specified."))
+			fail500(w, errors.New("lfsAuthorizeHandler: LfsOid empty"))
 			return
 		}
 
 		if err := os.MkdirAll(r.StoreLFSPath, 0700); err != nil {
-			fail500(w, "Couldn't create directory for storing LFS tmp objects.", err)
+			fail500(w, fmt.Errorf("lfsAuthorizeHandler: mkdir StoreLFSPath: %v", err))
 			return
 		}
 
@@ -45,7 +41,7 @@ func lfsAuthorizeHandler(handleFunc serviceHandleFunc) serviceHandleFunc {
 func handleStoreLfsObject(w http.ResponseWriter, r *gitRequest) {
 	file, err := ioutil.TempFile(r.StoreLFSPath, r.LfsOid)
 	if err != nil {
-		fail500(w, "Couldn't open tmp file for writing.", err)
+		fail500(w, fmt.Errorf("handleStoreLfsObject: create tempfile: %v", err))
 		return
 	}
 	defer os.Remove(file.Name())
@@ -56,32 +52,32 @@ func handleStoreLfsObject(w http.ResponseWriter, r *gitRequest) {
 
 	written, err := io.Copy(hw, r.Body)
 	if err != nil {
-		fail500(w, "Failed to save received LFS object.", err)
+		fail500(w, fmt.Errorf("handleStoreLfsObject: write tempfile: %v", err))
 		return
 	}
 	file.Close()
 
 	if written != r.LfsSize {
-		fail500(w, "Inconsistent size: ", errSizeMismatch)
+		fail500(w, fmt.Errorf("handleStoreLfsObject: expected size %d, wrote %d", r.LfsSize, written))
 		return
 	}
 
 	shaStr := hex.EncodeToString(hash.Sum(nil))
 	if shaStr != r.LfsOid {
-		fail500(w, "Inconsistent size: ", errSizeMismatch)
+		fail500(w, fmt.Errorf("handleStoreLfsObject: expected sha256 %s, got %s", r.LfsOid, shaStr))
 		return
 	}
 	r.Header.Set("X-GitLab-Lfs-Tmp", filepath.Base(file.Name()))
 
 	storeReq, err := r.u.newUpstreamRequest(r.Request, nil, "")
 	if err != nil {
-		fail500(w, "newUpstreamRequestLfsStoreCallback", err)
+		fail500(w, fmt.Errorf("handleStoreLfsObject: newUpstreamRequest: %v", err))
 		return
 	}
 
 	storeResponse, err := r.u.httpClient.Do(storeReq)
 	if err != nil {
-		fail500(w, "doRequestLfsStoreCallback", err)
+		fail500(w, fmt.Errorf("handleStoreLfsObject: do %v: %v", storeReq.URL.Path, err))
 		return
 	}
 	defer storeResponse.Body.Close()

proxy.go

 package main
 
 import (
+	"fmt"
 	"net/http"
 )
 
 func proxyRequest(w http.ResponseWriter, r *gitRequest) {
 	upRequest, err := r.u.newUpstreamRequest(r.Request, r.Body, "")
 	if err != nil {
-		fail500(w, "newUpstreamRequest", err)
+		fail500(w, fmt.Errorf("proxyRequest: newUpstreamRequest: %v", err))
 		return
 	}
 
 	upResponse, err := r.u.httpClient.Do(upRequest)
 	if err != nil {
-		fail500(w, "do upstream request", err)
+		fail500(w, fmt.Errorf("proxyRequest: do %v: %v", upRequest.URL.Path, err))
 		return
 	}
 	defer upResponse.Body.Close()

uploads.go

@@ -3,6 +3,7 @@ package main
 import (
 	"bytes"
 	"errors"
+	"fmt"
 	"io"
 	"io/ioutil"
 	"mime/multipart"
@@ -84,7 +85,7 @@ func rewriteFormFilesFromMultipart(r *gitRequest, writer *multipart.Writer) (cle
 
 func handleFileUploads(w http.ResponseWriter, r *gitRequest) {
 	if r.TempPath == "" {
-		fail500(w, "handleUploadFile", errors.New("missing temporary path"))
+		fail500(w, errors.New("handleFileUploads: TempPath empty"))
 		return
 	}
 
@@ -98,7 +99,7 @@ func handleFileUploads(w http.ResponseWriter, r *gitRequest) {
 		if err == http.ErrNotMultipart {
 			proxyRequest(w, r)
 		} else {
-			fail500(w, "Couldn't handle upload request.", err)
+			fail500(w, fmt.Errorf("handleFileUploads: extract files from multipart: %v", err))
 		}
 		return
 	}
@@ -113,7 +114,7 @@ func handleFileUploads(w http.ResponseWriter, r *gitRequest) {
 	// Create request
 	upstreamRequest, err := r.u.newUpstreamRequest(r.Request, nil, "")
 	if err != nil {
-		fail500(w, "Couldn't handle artifacts upload request.", err)
+		fail500(w, fmt.Errorf("handleFileUploads: newUpstreamRequest: %v", err))
 		return
 	}
 
@@ -125,7 +126,7 @@ func handleFileUploads(w http.ResponseWriter, r *gitRequest) {
 	// Forward request to backend
 	upstreamResponse, err := r.u.httpClient.Do(upstreamRequest)
 	if err != nil {
-		fail500(w, "do upstream request", err)
+		fail500(w, fmt.Errorf("handleFileUploads: do request %v: %v", upstreamRequest.URL.Path, err))
 		return
 	}
 	defer upstreamResponse.Body.Close()

xsendfile.go

@@ -7,6 +7,7 @@ via the X-Sendfile mechanism. All that is needed in the Rails code is the
 package main
 
 import (
+	"fmt"
 	"io"
 	"log"
 	"net/http"
@@ -16,7 +17,7 @@ import (
 func handleSendFile(w http.ResponseWriter, r *gitRequest) {
 	upRequest, err := r.u.newUpstreamRequest(r.Request, r.Body, "")
 	if err != nil {
-		fail500(w, "newUpstreamRequest", err)
+		fail500(w, fmt.Errorf("handleSendFile: newUpstreamRequest: %v", err))
 		return
 	}
 
@@ -24,7 +25,7 @@ func handleSendFile(w http.ResponseWriter, r *gitRequest) {
 	upResponse, err := r.u.httpClient.Do(upRequest)
 	r.Body.Close()
 	if err != nil {
-		fail500(w, "do upstream request", err)
+		fail500(w, fmt.Errorf("handleSendfile: do upstream request: %v", err))
 		return
 	}
 
@@ -45,7 +46,7 @@ func handleSendFile(w http.ResponseWriter, r *gitRequest) {
 
 		// Copy body from Rails upResponse
 		if _, err := io.Copy(w, upResponse.Body); err != nil {
-			fail500(w, "Couldn't finalize X-File download request.", err)
+			fail500(w, fmt.Errorf("handleSendFile: copy upstream response: %v", err))
 		}
 		return
 	}
@@ -54,14 +55,14 @@ func handleSendFile(w http.ResponseWriter, r *gitRequest) {
 	upResponse.Body.Close()
 	content, err := os.Open(sendfile)
 	if err != nil {
-		fail500(w, "open sendfile", err)
+		fail500(w, fmt.Errorf("handleSendile: open sendfile: %v", err))
 		return
 	}
 	defer content.Close()
 
 	fi, err := content.Stat()
 	if err != nil {
-		fail500(w, "xSendFile get mtime", err)
+		fail500(w, fmt.Errorf("handleSendfile: get mtime: %v", err))
 		return
 	}
 	http.ServeContent(w, r.Request, "", fi.ModTime(), content)