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.
This step makes sense, and nicely done.
We usually use the word "gently" to when we enhance an operation
that used to always die on failure. When there are tons of callsite
to the original operation F(), we introduce F_gently() variant and
do something like
F(...)
{
if (F_gently(...))
die(...);
}
so that the callers do not have to change. If there aren't that
many, it is OK to change the function signature of F() to tell it
not to die without adding a new F_gently() function, which is the
approach more appropriate for this change. The extra parameter used
for that purpose should be called "gently", perhaps.
> + if(ref_exists(ref->buf))
> + return BRANCH_EXISTS;
> + else
> + return BRANCH_DOESNT_EXIST;
Always have SP between "if" (and other keyword like "while") and its
condition.
For this one, however, this might be easier to follow:
return ref_exists(ref->buf) ? BRANCH_EXISTS : BRANCH_DOESNT_EXIST;
The names of the enum values may need further thought. They must
clearly convey two things, in addition to what kind of status they
represent; the current names only convey the status. From the names
of these values, it must be clear that they are part of the same
enum (e.g. by sharing a common prefix), and also from the names of
these values, it must be clear which ones are error conditions and
which are not, without knowing their actual values. A reader cannot
immediately tell from "BRANCH_EXISTS" if that is a good thing or
not.
Other than that, looks fairly cleanly done. I like what this step
wants to achieve.
Thanks.
