On Mon, 2017-11-06 at 11:30 +0900, Junio C Hamano wrote:
> Kaartic Sivaraam <kaartic.sivaraam@xxxxxxxxx> writes:
>
> No {} around a single statement block of "if", especially when there
> is no "else" that has multi-statement block that needs {}.
>
The code has changed a little since v3 so this if has been replaced
with a switch case.
> > + switch (res) {
> > + case BRANCH_EXISTS_NO_FORCE:
> > + strbuf_addf(error_msg, "%s", (!old_branch_exists) ? connector_string : "");
> > + strbuf_addf(error_msg,_("branch '%s' already exists"), newname);
> > + break;
>
> The case arms and their statements are indented by one level too much.
> The lines are getting overlong. Find a good place to split, e.g.
>
> strbuf_addf(error_msg, "%s",
> !old_branch_exists ? connector_string : "");
>
> Leave a single SP after each "," in an arguments list.
>
Fixed these.
> As Eric pointed out, this certainly smells like a sentence lego that
> we would be better off without.
>
As stated in that mail, I've replaced the connector " and " with "; "
as it seemed to be the most simple way to overcome the issue, at least
in my opinion. In case there's any better way to fix this let me know.
> > static void copy_or_rename_branch(const char *oldname, const char *newname, int copy, int force)
> > {
> > struct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = STRBUF_INIT;
> > struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;
> > int recovery = 0;
> > + struct strbuf error_msg = STRBUF_INIT, empty = STRBUF_INIT;
> > + enum branch_validation_result res;
> >
> > if (!oldname) {
> > if (copy)
> > @@ -471,15 +502,13 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
> > die(_("cannot rename the current branch while not on any."));
> > }
> >
> > - if (strbuf_check_branch_ref(&oldref, oldname)) {
> > + if (strbuf_check_branch_ref(&oldref, oldname) && ref_exists(oldref.buf))
> > + {
>
> Opening brace { that begins a block comes at the end of the line
> that closes the condition of "if"; if you found that your line is
> overlong, perhaps do it like so instead:
>
> if (strbuf_check_branch_ref(&oldref, oldname) &&
> ref_exists(oldref.buf)) {
This part changed too. Anyways thanks for the detailed review :-)
--
Kaartic
