Re: [RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation

[Kaartic Sivaraam <kaartic.sivaraam@gmail.com>]

On Mon, 2017-11-06 at 11:24 +0900, Junio C Hamano wrote:
> Kaartic Sivaraam <kaartic.sivaraam@gmail.com> 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