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

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

Kaartic Sivaraam [off-list ref] 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.

quoted

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

quoted

 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