Kaartic Sivaraam <kaartic.sivaraam@xxxxxxxxx> writes:
> This parameter allows the branchname validation functions to
> optionally return a flag specifying the reason for failure, when
> requested. This allows the caller to know why it was about to die.
> This allows more useful error messages to be given to the user when
> trying to rename a branch.
>
> The flags are specified in the form of an enum and values for success
> flags have been assigned explicitly to clearly express that certain
> callers rely on those values and they cannot be arbitrary.
>
> Only the logic has been added but no caller has been made to use
> it, yet. So, no functional changes.
>
> Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@xxxxxxxxx>
> ---
So... I am not finding dont_fail that was mentioned on the title
anywhere else in the patch. Such lack of attention to detail is
a bit off-putting.
The change itself overall looks OK. One minor thing that made me
wonder was this bit:
> +enum branch_validation_result {
> + /* Flags that convey there are fatal errors */
> + VALIDATION_FATAL_BRANCH_EXISTS_NO_FORCE = -3,
> + VALIDATION_FATAL_CANNOT_FORCE_UPDATE_CURRENT_BRANCH,
> + VALIDATION_FATAL_INVALID_BRANCH_NAME,
> + /* Flags that convey there are no fatal errors */
> + VALIDATION_PASS_BRANCH_DOESNT_EXIST = 0,
> + VALIDATION_PASS_BRANCH_EXISTS = 1,
> + VALIDATION_WARN_BRANCH_EXISTS = 2
> +};
where adding new error types will force us to touch _two_ lines
(i.e. either you add a new error before NO_FORCE with value -4 and
then remove the "= -3" from NO_FORCE, or you add a new error after
INVALID, and update NO_FORCE to -4), which can easily be screwed up
by a careless developer. The current code is not wrong per-se, but
I wonder if it can be made less error prone.
