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

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

On Mon, 2017-11-06 at 11:30 +0900, Junio C Hamano wrote:
> 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.


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