Respond with 500 when external commands failed
There is a bug in that, when the git-receive-pack --stateless-rpc failed, the client still get a successful message saying that the push is successful where actually the push is a failure, leading to data being lost.
I consider this as a serious bug and thus submitted this patch.
Merge request reports
Activity
@pmq20 do you know a way to trigger this failure so that we can test the effect of this change?
Reading https://golang.org/pkg/net/http/#ResponseWriter this MR makes no sense to me.
If WriteHeader has not yet been called, Write calls WriteHeader(http.StatusOK) before writing the data.
The very first write should force HTTP 200 already (remember that in the HTTP response the status code must come before the data in the response body).
77 77 // Start writing the response 78 78 w.Header().Add("Content-Type", fmt.Sprintf("application/x-%s-advertisement", rpc)) 79 79 w.Header().Add("Cache-Control", "no-cache") 80 w.WriteHeader(200) // Don't bother with HTTP 500 from this point on, just return 80 81 81 if err := pktLine(w, fmt.Sprintf("# service=%s\n", rpc)); err != nil { @jacobvosmaer Indeed. Let me think of another way to adjust this logic.
@jacobvosmaer What I have changed partially works when the external command failed with no output at all, in which situation the header has not yet been written out so the 500 header got a chance. However like what you have said when the external program did output something then 200 would not have a chance to be changed. It seems that we need to introduce a buffer or something to hold the body output temporarily.
@pmq20 do you know a specific error this would catch? What is the error message from Git?
@jacobvosmaer We were using a custom git implementation on our server. Sometimes it spits the correct output from git-receive-pack and then raised an error about some ref-updating logic. When workhorse always returns 200 and ignores the exit status of the command the client git command would treat this as a success since correct output has been seen where it actually failed.
@pmq20 so this is only about Git pushes?
mentioned in merge request !40 (closed)
@pmq20 what do you think about !40 (closed)? Could you test it?
mentioned in merge request !42 (merged)
mentioned in commit 285f47a7
Closing in favor of !42 (merged)
Thanks @pmq20 . FWIW this is not in GitLab 8.6, it should go into 8.7.
