[PATHv2] branch.c:install_branch_config():Simplify code generating verbose message.

Simplify the long if chain in install_branch_config().

There is a long chain of if statements. The code can be more clear.
Replace the chain with table of strings. New approach is more
compact.

Signed-off-by: Paweł Wawruch <redacted>
---
 branch.c | 40 ++++++++++++++++++----------------------
 1 file changed, 18 insertions(+), 22 deletions(-)

diff --git a/branch.c b/branch.c
index 723a36b..8d3b219 100644
--- a/branch.c
+++ b/branch.c

@@ -53,6 +53,18 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
 	int remote_is_branch = starts_with(remote, "refs/heads/");
 	struct strbuf key = STRBUF_INIT;
 	int rebasing = should_setup_rebase(origin);
+	const char *messages[] = {
+		N_("Branch %s set up to track remote branch %s from %s by rebasing."),
+		N_("Branch %s set up to track remote branch %s from %s."),
+		N_("Branch %s set up to track local branch %s by rebasing."),
+		N_("Branch %s set up to track local branch %s."),
+		N_("Branch %s set up to track remote ref %s by rebasing."),
+		N_("Branch %s set up to track remote ref %s."),
+		N_("Branch %s set up to track local ref %s by rebasing."),
+		N_("Branch %s set up to track local ref %s.")
+	};
+	const char *name = remote_is_branch ? remote : shortname;
+	int message_number;
 
 	if (remote_is_branch
 	    && !strcmp(local, shortname)

@@ -77,29 +89,13 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
 	strbuf_release(&key);
 
 	if (flag & BRANCH_CONFIG_VERBOSE) {
-		if (remote_is_branch && origin)
-			printf_ln(rebasing ?
-				  _("Branch %s set up to track remote branch %s from %s by rebasing.") :
-				  _("Branch %s set up to track remote branch %s from %s."),
-				  local, shortname, origin);
-		else if (remote_is_branch && !origin)
-			printf_ln(rebasing ?
-				  _("Branch %s set up to track local branch %s by rebasing.") :
-				  _("Branch %s set up to track local branch %s."),
-				  local, shortname);
-		else if (!remote_is_branch && origin)
-			printf_ln(rebasing ?
-				  _("Branch %s set up to track remote ref %s by rebasing.") :
-				  _("Branch %s set up to track remote ref %s."),
-				  local, remote);
-		else if (!remote_is_branch && !origin)
-			printf_ln(rebasing ?
-				  _("Branch %s set up to track local ref %s by rebasing.") :
-				  _("Branch %s set up to track local ref %s."),
-				  local, remote);
+		message_number = (!!rebasing) + 2 * (!!origin) + 4 * (!!remote_is_branch);
+		assert(message_number < ARRAY_SIZE(messages));
+
+		if (message_number < 2)
+			printf_ln(messages[message_number], local, name, origin);
 		else
-			die("BUG: impossible combination of %d and %p",
-			    remote_is_branch, origin);
+			printf_ln(messages[message_number], local, name);
 	}
 }
 

-- 
1.8.3.2

Thanks for taking review comments from your previous attempt into
account. This is a more well-crafted submission. Additional comments
below.

On Tue, Mar 11, 2014 at 4:40 AM, Paweł Wawruch [off-list ref] wrote:

Subject: [PATHv2] branch.c:install_branch_config():Simplify code generating verbose message.

PATCH is misspelled as PATH. Adding the v2 is indeed good etiquette.
It's typical to have a space between PATCH and vN.

Insert a space before "Simplify".

SubmittingPatches suggests omitting the final period in the subject.
Also downcase "simplify".

Simplify the long if chain in install_branch_config().

A bit misleading. You're not actually simplifying the 'if' chain, but
rather replacing it.

There is a long chain of if statements. The code can be more clear.
Replace the chain with table of strings. New approach is more
compact.

The first sentence merely repeats what was said earlier. It can be
dropped. The second sentence is self-evident since you already talk
about "simplification" and also can be dropped, as can the fourth
sentence. The third sentence is a good explanation of how the code is
being simplified and should be kept. You could, therefore, collapse
the commit message to something along these lines:

    Subject: install_branch_config: simplify verbose diagnostic logic

    Replace the 'if' chain with a table of strings.

Signed-off-by: Paweł Wawruch <redacted>
---

It is considerate to reviewers to provide a link to the previous
version of the patch, like this [1], and to explain what changed in
this version relative to the last. This area just below the "---" line
after your sign off is where you would write such commentary.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/243502

On this project, arrays are usually (though not consistently) named in
singular form (for instance message[]) so that a reference to a single
item, such as message[42], reads more grammatically correct.

Unnecessary parentheses make the code more cumbersome to read and
should be dropped.

+               assert(message_number < ARRAY_SIZE(messages));
+
+               if (message_number < 2)
+                       printf_ln(messages[message_number], local, name, origin);

Even though the strings are marked for translation via N_() in the
initializer, they still need to be passed through _() when they are
actually used, so the invocation should be:

    printf_ln(_(messages[message_number]), local, name, origin);

Ditto for the printf_ln() below.

See section 4.7 of the GNU gettext manual [2] for an explanation (and
note that N_() is shorthand for gettext_noop() mentioned in the
manual).

[2]: http://www.gnu.org/software/gettext/manual/gettext.html

                else
-                       die("BUG: impossible combination of %d and %p",
-                           remote_is_branch, origin);
+                       printf_ln(messages[message_number], local, name);
        }

Although this approach of making message retrieval table-driven does
collapse the code a bit, it's not obvious that it is indeed more
clear. Magical incantations, such as '!!rebasing + 2 * !!origin + 4 *
!!remote_is_branch' and 'if (message_number < 2)', convey little
immediate meaning, demand a greater cognitive load, and may make the
code less straightforward to modify in the future. Have you considered
other ways of making the code table-driven which exhibit fewer
shortcomings?

 }

--
1.8.3.2