Re: [PATCH 2/3] branch: split validate_new_branchname() into two

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

On Fri, 2017-10-13 at 14:11 +0900, Junio C Hamano wrote:
> 
> diff --git a/branch.c b/branch.c
> index 7404597678..2c3a364a0b 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -178,19 +178,31 @@ int read_branch_desc(struct strbuf *buf, const char *branch_name)
>  	return 0;
>  }
>  
> -int validate_new_branchname(const char *name, struct strbuf *ref,
> -			    int force, int attr_only)
> +/*
> + * Check if 'name' can be a valid name for a branch; die otherwise.
> + * Return 1 if the named branch already exists; return 0 otherwise.
> + * Fill ref with the full refname for the branch.
> + */
> +int validate_branchname(const char *name, struct strbuf *ref)
>  {
> -	const char *head;
> -
>  	if (strbuf_check_branch_ref(ref, name))
>  		die(_("'%s' is not a valid branch name."), name);
>  
> -	if (!ref_exists(ref->buf))
> -		return 0;
> +	return ref_exists(ref->buf);
> +}
>  
> -	if (attr_only)
> -		return 1;
> +/*
> + * Check if a branch 'name' can be created as a new branch; die otherwise.
> + * 'force' can be used when it is OK for the named branch already exists.
> + * Return 1 if the named branch already exists; return 0 otherwise.
> + * Fill ref with the full refname for the branch.
> + */

I guess it's better to avoid repeating the comments in both the .h and
.c file as they might quite easily become stale. I would prefer keeping
it in the header file alone.

> +int validate_new_branchname(const char *name, struct strbuf *ref, int force)
> +{
> +	const char *head;
> +
> +	if (!validate_branchname(name, ref))
> +		return 0;
>  
>  	if (!force)
>  		die(_("A branch named '%s' already exists."),
> @@ -246,9 +258,9 @@ void create_branch(const char *name, const char *start_name,
>  	if (track == BRANCH_TRACK_EXPLICIT || track == BRANCH_TRACK_OVERRIDE)
>  		explicit_tracking = 1;
>  
> -	if (validate_new_branchname(name, &ref, force,
> -				    track == BRANCH_TRACK_OVERRIDE ||
> -				    clobber_head)) {
> +	if ((track == BRANCH_TRACK_OVERRIDE || clobber_head)
> +	    ? validate_branchname(name, &ref)
> +	    : validate_new_branchname(name, &ref, force)) {
>  		if (!force)
>  			dont_change_ref = 1;
> 

The change was simple by splitting the function into two and calling
the right function as required at every call site! As far as I could
see this doesn't seem to be reducing the confusion that the 'attr_only'
parameter caused. That's because the new validate_branchname function
seems to be called in some cases when the 'force' parameter is true and
in other cases the 'force' parameter is sent to the
'validate_new_branchname' function. So, I think consistency is lacking
in this change. That's just my opinion, of course.

-- 
Kaartic