Re: [RFC PATCH v3 4/4] builtin/branch: give more useful error messages when renaming

From: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Date: Nov 12, 2017, 14:05 UTC
Message-ID: <1510495525.2683.12.camel@gmail.com>
In-Reply-To: <xmqq375sjfzk.fsf@gitster.mtv.corp.google.com>

On Mon, 2017-11-06 at 11:30 +0900, Junio C Hamano wrote:

Show 5 quoted lines

> Kaartic Sivaraam <kaartic.sivaraam@gmail.com> 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.

Show 14 quoted lines

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

Show 24 quoted lines

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