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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 21, 2017, 09:01 UTC
Message-ID: <xmqqlgk4g9h7.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1508561910.2516.22.camel@gmail.com>

Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:

Show 10 quoted lines

>> +/*
>> + * 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.

True. I wrote this while designing the code, so the copy in .c file came first, and then that was copied to .h file; the one in .c file can go.

> 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.

The confusion primarily was the way the parameter was named. "forcing" does not have strong connection to marking "this is only asking about attributes". And removing that confusion, by separating the caller and making it explicit that the caller needs two separate behaviours depending on the condition, and naming the functions more appropriately (i.e. is this about creating a new one that must not exist already?), is the focus of this step.