[PATCHv2] branch.c: simplify chain of if statements

[PATCHv2] branch.c: simplify chain of if statements

[Dragos Foianu]

This patch uses a table to store the different messages that can
be emitted by the verbose install_branch_config function. It
computes an index based on the three flags and prints the message
located at the specific index in the table of messages. If the
index somehow is not within the table, we have a bug.

Signed-off-by: Dragos Foianu <dragos.foianu@gmail.com>
---
 branch.c | 44 +++++++++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 19 deletions(-)

diff --git a/branch.c b/branch.c
index 723a36b..95645d5 100644
--- a/branch.c
+++ b/branch.c
@@ -54,6 +54,18 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
 	struct strbuf key = STRBUF_INIT;
 	int rebasing = should_setup_rebase(origin);
 
+	const char *messages[] = {
+		N_("Branch %s set up to track local ref %s."),
+		N_("Branch %s set up to track remote ref %s."),
+		N_("Branch %s set up to track local branch %s."),
+		N_("Branch %s set up to track remote branch %s from %s."),
+		N_("Branch %s set up to track local ref %s by rebasing."),
+		N_("Branch %s set up to track remote ref %s by rebasing."),
+		N_("Branch %s set up to track local branch %s by rebasing."),
+		N_("Branch %s set up to track remote branch %s from %s by rebasing.")
+	};
+	int index = 0;
+
 	if (remote_is_branch
 	    && !strcmp(local, shortname)
 	    && !origin) {
@@ -76,28 +88,22 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
 	}
 	strbuf_release(&key);
 
+	if (origin)
+		index += 1;
+	if (remote_is_branch)
+		index += 2;
+	if (rebasing)
+		index += 4;
+
 	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);
+			printf_ln(_(messages[index]),
+				local, shortname, origin);
 		else
+			printf_ln(_(messages[index]),
+				local, (!remote_is_branch) ? remote : shortname);
+
+		if (index < 0 || index > sizeof(messages) / sizeof(*messages))
 			die("BUG: impossible combination of %d and %p",
 			    remote_is_branch, origin);
 	}
-- 
1.8.3.2

Re: [PATCHv2] branch.c: simplify chain of if statements

[Eric Sunshine]

Thanks for the resubmission. Comments below...

On Mon, Mar 17, 2014 at 11:51 AM, Dragos Foianu <dragos.foianu@gmail.com> wrote:
> This patch uses a table to store the different messages that can
> be emitted by the verbose install_branch_config function. It
> computes an index based on the three flags and prints the message
> located at the specific index in the table of messages. If the
> index somehow is not within the table, we have a bug.

Most of this text can be dropped due to redundancy.

Saying "This patch..." is unnecessary.

The remaining text primarily says in prose what the patch itself
conveys more concisely and precisely. It's easier to read and
understand the actual code than it is to wade through a lengthy
description of the code change.

Speak in imperative voice: "Use a table to store..."

You might, for instance, say instead something like this:

    install_branch_config() uses a long, somewhat complex if-chain to
    select a message to display in verbose mode.  Simplify the logic
    by moving the messages to a table from which they can be
    easily retrieved without complex logic.

> Signed-off-by: Dragos Foianu <dragos.foianu@gmail.com>
> ---
>  branch.c | 44 +++++++++++++++++++++++++-------------------
>  1 file changed, 25 insertions(+), 19 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index 723a36b..95645d5 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -54,6 +54,18 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
>         struct strbuf key = STRBUF_INIT;
>         int rebasing = should_setup_rebase(origin);
>
> +       const char *messages[] = {
> +               N_("Branch %s set up to track local ref %s."),
> +               N_("Branch %s set up to track remote ref %s."),
> +               N_("Branch %s set up to track local branch %s."),
> +               N_("Branch %s set up to track remote branch %s from %s."),
> +               N_("Branch %s set up to track local ref %s by rebasing."),
> +               N_("Branch %s set up to track remote ref %s by rebasing."),
> +               N_("Branch %s set up to track local branch %s by rebasing."),
> +               N_("Branch %s set up to track remote branch %s from %s by rebasing.")
> +       };
> +       int index = 0;
> +
>         if (remote_is_branch
>             && !strcmp(local, shortname)
>             && !origin) {
> @@ -76,28 +88,22 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
>         }
>         strbuf_release(&key);
>
> +       if (origin)
> +               index += 1;
> +       if (remote_is_branch)
> +               index += 2;
> +       if (rebasing)
> +               index += 4;

Other submissions have computed this value mathematically without need
for conditionals. For instance, we've seen:

    index = (!!origin << 0) + (!!remote_is_branch << 1) + (!!rebasing << 2)

as, well as the equivalent:

    index = !!origin + !!remote_is_branch * 2 + !!rebasing * 4

Although this works, it does place greater cognitive demands on the
reader by requiring more effort to figure out what is going on and how
it relates to table position. The original (ungainly) chain of 'if'
statements in the original code does not suffer this problem. It
likewise is harder to understand than merely indexing into a
multi-dimension table where each variable is a 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);
> +                       printf_ln(_(messages[index]),
> +                               local, shortname, origin);
>                 else
> +                       printf_ln(_(messages[index]),
> +                               local, (!remote_is_branch) ? remote : shortname);

It's possible to simplify this logic and have only a single
printf_ln() invocation. Hint: It's safe to pass in more arguments than
there are %s directives in the format string.

> +
> +               if (index < 0 || index > sizeof(messages) / sizeof(*messages))
>                         die("BUG: impossible combination of %d and %p",
>                             remote_is_branch, origin);

You can use ARRAY_SIZE() in place of sizeof(...)/sizeof(...).

Since an out-of-bound index would be a programmer bug, it would
probably be more appropriate to use an assert(), just after 'index' is
computed, rather than if+die(). The original code used die() because
it couldn't detect the error until the end of the if-chain.

>         }
> --
> 1.8.3.2

Re: [PATCHv2] branch.c: simplify chain of if statements

[Eric Sunshine]

On Tue, Mar 18, 2014 at 6:31 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Mon, Mar 17, 2014 at 11:51 AM, Dragos Foianu <dragos.foianu@gmail.com> wrote:
>> This patch uses a table to store the different messages that can
>> be emitted by the verbose install_branch_config function. It
>> computes an index based on the three flags and prints the message
>> located at the specific index in the table of messages. If the
>> index somehow is not within the table, we have a bug.
>>
>> Signed-off-by: Dragos Foianu <dragos.foianu@gmail.com>
>> ---
>>  branch.c | 44 +++++++++++++++++++++++++-------------------
>>  1 file changed, 25 insertions(+), 19 deletions(-)
>>
>> diff --git a/branch.c b/branch.c
>> index 723a36b..95645d5 100644
>> --- a/branch.c
>> +++ b/branch.c
>> @@ -54,6 +54,18 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
>> +       int index = 0;
>> +       if (origin)
>> +               index += 1;
>> +       if (remote_is_branch)
>> +               index += 2;
>> +       if (rebasing)
>> +               index += 4;
>> +
>> +               if (index < 0 || index > sizeof(messages) / sizeof(*messages))
>>                         die("BUG: impossible combination of %d and %p",
>>                             remote_is_branch, origin);

One other observation: You have a one-off error in your out-of-bounds
check. It should be 'index >= sizeof...'

> You can use ARRAY_SIZE() in place of sizeof(...)/sizeof(...).
>
> Since an out-of-bound index would be a programmer bug, it would
> probably be more appropriate to use an assert(), just after 'index' is
> computed, rather than if+die(). The original code used die() because
> it couldn't detect the error until the end of the if-chain.
>
>>         }
>> --
>> 1.8.3.2

Re: [PATCHv2] branch.c: simplify chain of if statements

[Dragos Foianu]

Eric Sunshine <sunshine <at> sunshineco.com> writes:

> 
> Other submissions have computed this value mathematically without need
> for conditionals. For instance, we've seen:
> 
>     index = (!!origin << 0) + (!!remote_is_branch << 1) + (!!rebasing << 2)
> 
> as, well as the equivalent:
> 
>     index = !!origin + !!remote_is_branch * 2 + !!rebasing * 4
> 
> Although this works, it does place greater cognitive demands on the
> reader by requiring more effort to figure out what is going on and how
> it relates to table position. The original (ungainly) chain of 'if'
> statements in the original code does not suffer this problem. It
> likewise is harder to understand than merely indexing into a
> multi-dimension table where each variable is a key.

I have seen other submissions using this logic, but I didn't think it
accomplished the goal of the patch - simplifying the code. Not that my
approach does either, but I found it a little easier to understand.

Indexing into a table like this is always going to have this problem so this
is probably not the right approach to accomplishing the microproject's goals.

> It's possible to simplify this logic and have only a single
> printf_ln() invocation. Hint: It's safe to pass in more arguments than
> there are %s directives in the format string.

Indeed. It's a habit of mine to pass the exact number of arguments to printf
functions and I can't seem to get away from it.

> You can use ARRAY_SIZE() in place of sizeof(...)/sizeof(...).
> 
> Since an out-of-bound index would be a programmer bug, it would
> probably be more appropriate to use an assert(), just after 'index' is
> computed, rather than if+die(). The original code used die() because
> it couldn't detect the error until the end of the if-chain.

Thank you for this hint. Using already defined helpers in the project is
better and will prevent the need to patch the constructs later on.

> On Tue, Mar 18, 2014 at 6:31 PM, Eric Sunshine <sunshine <at>
sunshineco.com> wrote:
> 
> One other observation: You have a one-off error in your out-of-bounds
> check. It should be 'index >= sizeof...'

Well this is embarrasing.

Thank you again for the feedback. It's incredibily helpful and I learned a
lot from submitting these patches. Making the code simple is harder than it
appears at first sight.

I'm not sure it's worth pursuing the table approach further, especially
since a solution has already been accepted and merged into the codebase.

In this case, is it okay to try another microproject? I was thinking about
trying #17 (finding bugs/inefficiencies in builtin/apply.c), but I've
already had my one microproject.

All the best,
Dragos

Re: [PATCHv2] branch.c: simplify chain of if statements

[Eric Sunshine]

On Wed, Mar 19, 2014 at 7:12 PM, Dragos Foianu <dragos.foianu@gmail.com> wrote:
> Eric Sunshine <sunshine <at> sunshineco.com> writes:
>> On Tue, Mar 18, 2014 at 6:31 PM, Eric Sunshine <sunshine <at>
> sunshineco.com> wrote:
>>
>> One other observation: You have a one-off error in your out-of-bounds
>> check. It should be 'index >= sizeof...'
>
> Well this is embarrassing.

It's a good illustration of the value of the review process. It's easy
to overlook omissions and problems in our one's work because one reads
it with the bias of knowing what it's _supposed_ to say. Reviewers
(hopefully) don't have such bias: they read the code afresh.

> Thank you again for the feedback. It's incredibily helpful and I learned a
> lot from submitting these patches. Making the code simple is harder than it
> appears at first sight.
>
> I'm not sure it's worth pursuing the table approach further, especially
> since a solution has already been accepted and merged into the codebase.

Agreed.

> In this case, is it okay to try another microproject? I was thinking about
> trying #17 (finding bugs/inefficiencies in builtin/apply.c), but I've
> already had my one micro project.

According to the description for #17, there are plenty of opportunities, so...

> All the best,
> Dragos

Re: [PATCHv2] branch.c: simplify chain of if statements

[Eric Sunshine]

On Thu, Mar 20, 2014 at 8:40 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Mar 19, 2014 at 7:12 PM, Dragos Foianu <dragos.foianu@gmail.com> wrote:
>> Eric Sunshine <sunshine <at> sunshineco.com> writes:
>>> On Tue, Mar 18, 2014 at 6:31 PM, Eric Sunshine <sunshine <at>
>> sunshineco.com> wrote:
>>>
>>> One other observation: You have a one-off error in your out-of-bounds
>>> check. It should be 'index >= sizeof...'
>>
>> Well this is embarrassing.
>
> It's a good illustration of the value of the review process. It's easy
> to overlook omissions and problems in our one's work because one reads
> it with the bias of knowing what it's _supposed_ to say. Reviewers
> (hopefully) don't have such bias: they read the code afresh.

And, this is a perfect example. I knew that I wanted to say "problems
in one's own work", and even though I proof-read, I still missed that
I wrote "problems in our one's work".

Re: [PATCHv2] branch.c: simplify chain of if statements

[Junio C Hamano]

Dragos Foianu <dragos.foianu@gmail.com> writes:

> I'm not sure it's worth pursuing the table approach further, especially
> since a solution has already been accepted and merged into the codebase.

Yes.

I would further say that you already qualify as having finished a
microproject, if I were a part of the candidate selection panel.

The important thing is for potential candidates to learn the
process, not to have their change merged somewhere my tree, and you
and many others who did a microproject and tasted the process of
proposing a change, getting reviewed and learning what are expected
of their patch submissions have finished that part already.