Gitaly PostUploadPack does not support shallow clone

Tested using Gitaly gitaly@3c80885cc4cbf6b86582e82834b8da65ca25a4b6

This looks like a subtle problem in workhorse. From a gRPC standpoint, we have a failing PostUploadPack RPC. We handle this error in workhorse by calling helper.Fail500. Helper.Fail500 writes Internal server error' to the response. This corrupts the response cause git clone` to fail.

Could this be the cause of https://gitlab.com/gitlab-org/gitlab-workhorse/issues/125 ?

Does the timing add up? This RPC was only enabled for about 30 minutes on gitlab.com last Thursday.

Either way I'm convinced there is a problem worth solving here.

One way to 'fix' it in workhorse is:

diff --git a/internal/helper/helpers.go b/internal/helper/helpers.go
index c9ce622..a83b2c7 100644
--- a/internal/helper/helpers.go
+++ b/internal/helper/helpers.go
@@ -18,7 +18,7 @@ import (
 const NginxResponseBufferHeader = "X-Accel-Buffering"
 
 func Fail500(w http.ResponseWriter, r *http.Request, err error) {
-	http.Error(w, "Internal server error", 500)
+	w.WriteHeader(500)
 	if err != nil {
 		captureRavenError(r, err)
 	}

If we want to be able to print that message perhaps we should use a http.ResponseWriter that triggers on WriteHeader(500) and writes that error into the response body only if nothing has been written yet.

On the other hand we are talking about a high-impact change to fix something very specific to Git. Maybe we shouldn't muck with helper.Fail500.

Then there is the question if Gitaly should return an error in the first place. It's a bug in Git that causes the command to 'fail'.

WriteHeader() has no effect if the response body has already been written to. This is pointing to a bug further up in the workhorse code, I think. We shouldn't be using Fail500 in cases where we may have already written to w.