Skip to content

operations: Return detailed errors for hook failures in UserMergeBranch

Custom server-side hooks provide a feature where if they return an error, anything that was printed to stdout or stderr and is prefixed with "GL-HOOK-ERR":" will be displayed in the web interface. The interface to make this work between Rails and Gitaly is really fragile though: Rails relies on Gitaly passing up the hook error without any further decorations so that it can directly check whether the error has the specified prefix.

This calling convention is not documented in any public interface, but it has been like this since Gitaly has been born out of Rails. Of course, people repeatedly didn't know about this small detail and would improve error messages in a quest to make logging more useful in this context. And they cannot be blamed: the calling convention is simply bad.

Now that we have bought into gRPC's detailed error model though we can handle this problem in a much better way. Instead of relying on the exact error string returned by Gitaly, we use the new CustomHookError Protobuf message and directly tell Rails what the standard output and standard error streams contained. This will eventually free up the use of our errors so that we can make them meaningful, all while finally having an explicit and documented calling convention to bubble up hook errors to the caller.

Note that this change is not done behind a feature flag: the actual error message returned to Rails remains the same for now. We only use a proper error code now and embed the detailed error. And the way Rails inspects errors right now, nothing changes. Furthermore, it is currently broken anyway, so there doesn't seem to be much of a point to be careful and try not to break something that is already broken.

Part of #4167 (closed)

Edited by Patrick Steinhardt

Merge request reports