On Mon, 2017-11-06 at 11:24 +0900, Junio C Hamano wrote:
> Kaartic Sivaraam <kaartic.sivaraam@xxxxxxxxx> writes:
>
> 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.
>
Good suggestion, wasn't aware of it before. Renamed it.
> > + 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;
>
Done.
> 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.
>
I've added the prefix of "VALIDATION_{FAIL,PASS}" as appropriate to the
enum values. This made the names to have the form,
VALIDATION_(kind of status)_(reason)
This made the names a bit long but I couldn't come up with a better
prefix for now. Any suggestions are welcome.
Thanks for the detailed review!
---
Kaartic
