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
