Draft: Make Gitaly INVALID_ARGUMENT error its own class
What does this MR do and why?
Gitaly errors are commonly rescued and translated further in downstream code.
ArgumentError is one of the most common exceptions encountered when
writing Ruby code, and should arguably never be rescued because it
implies a very serious programming mistake, such as passing the wrong
number of arguments to a method. Resuing it will certainly make future
maintainer's life more challenging, and can cause buggy code to go
undetected.
On the other hand, rescuing INVALID_ARGUMENT RPC responses can be
acceptable, as as the problem may be in user input (e.g. user provides
an empty branch name), and we may want to translate it to a more
human-friendly message.
Backstory
As I was working on !127531 (merged), with MergeRequests::MergeToRefService as my starting point, I kept getting bit by the rescue ArgumentError swallowing minor mistakes that I apparently make and fix all the time during development. The intent of that rescue is presumably to gracefully handle a common a user error in the Gitaly call (or some benign data race). But it is broadly scoped enough that it can mask all kinds of issues in the scoped code.
Given that all of the tests remain passing, I'm hoping that there was just this one reliance on this particular error conversion, but I'm open to making this change behind a feature flag if people are concerned about unintended side effects
Screenshots or screen recordings
Screenshots are required for UI changes, and strongly recommended for all other merge requests.
| Before | After |
|---|---|
How to set up and validate locally
Numbered steps to set up and validate the change are strongly suggested.
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.