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

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

[Dragos Foianu]

This patch uses a table-driven approach in order to make the code
cleaner. Although not necessary, it helps code reability by not
forcing the user to read the print message when trying to
understand what the code does. The rebase check has been moved to
the verbose if statement to avoid making the same check in each of
the four if statements.

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

diff --git a/branch.c b/branch.c
index 723a36b..e2fe455 100644
--- a/branch.c
+++ b/branch.c
@@ -54,6 +54,14 @@ 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 *verbose_prints[4] = {
+		"Branch %s set up to track remote branch %s from %s%s",
+		"Branch %s set up to track local branch %s%s",
+		"Branch %s set up to track remote ref %s%s",
+		"Branch %s set up to track local ref %s%s"
+	};
+	char *verbose_rebasing = rebasing ? " by rebasing." : ".";
+
 	if (remote_is_branch
 	    && !strcmp(local, shortname)
 	    && !origin) {
@@ -78,25 +86,17 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
 
 	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);
+			printf_ln(_(verbose_prints[0]),
+				local, shortname, origin, verbose_rebasing);
 		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);
+			printf_ln(_(verbose_prints[1]),
+				local, shortname, verbose_rebasing);
 		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);
+			printf_ln(_(verbose_prints[2]),
+				local, remote, verbose_rebasing);
 		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(_(verbose_prints[3]),
+				local, remote, verbose_rebasing);
 		else
 			die("BUG: impossible combination of %d and %p",
 			    remote_is_branch, origin);
-- 
1.8.3.2

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

[Matthieu Moy]

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

> +	const char *verbose_prints[4] = {
> +		"Branch %s set up to track remote branch %s from %s%s",
> +		"Branch %s set up to track local branch %s%s",
> +		"Branch %s set up to track remote ref %s%s",
> +		"Branch %s set up to track local ref %s%s"
> +	};
> +	char *verbose_rebasing = rebasing ? " by rebasing." : ".";
> +

This seems to be a "lego construct" that makes translation harder: are
you sure that the "by rebasing" will be at the end of the sentence in
any languages?

Also, this lacks the _() on verbose_rebasing, which isn't translatable
anymore after your patch.

I personnally think that the table-driven approach is wrong here, it
makes the code shorter but much harder to read.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

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

[Eric Sunshine]

On Mon, Mar 17, 2014 at 3:23 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Dragos Foianu <dragos.foianu@gmail.com> writes:
>
>> +     const char *verbose_prints[4] = {
>> +             "Branch %s set up to track remote branch %s from %s%s",
>> +             "Branch %s set up to track local branch %s%s",
>> +             "Branch %s set up to track remote ref %s%s",
>> +             "Branch %s set up to track local ref %s%s"
>> +     };
>> +     char *verbose_rebasing = rebasing ? " by rebasing." : ".";
>> +
>
> This seems to be a "lego construct" that makes translation harder: are
> you sure that the "by rebasing" will be at the end of the sentence in
> any languages?

Read this thread [1] for more details about why this approach is problematic.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/243793/focus=243824

> Also, this lacks the _() on verbose_rebasing, which isn't translatable
> anymore after your patch.
>
> I personnally think that the table-driven approach is wrong here, it
> makes the code shorter but much harder to read.
>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/

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

[Eric Sunshine]

Thanks for the submission. Comments below to give you a taste of the
Git review process...

On Sun, Mar 16, 2014 at 5:22 PM, Dragos Foianu <dragos.foianu@gmail.com> wrote:
> This patch uses a table-driven approach in order to make the code
> cleaner.

In fact, this change is not table-driven (emphasis on *driven*). It
merely moves the strings into a table, but all the logic is still in
the code. To be table-driven, the logic would be encoded in the table
as well, and that logic would *drive* the code.

This is not to say that the code must be table-driven. The GSoC
microproject merely asks if doing so would make sense. Whether it
would is for you to decide, and then to explain to reviewers the
reason(s) why you did or did not make it so.

> Although not necessary, it helps code reability by not
> forcing the user to read the print message when trying to
> understand what the code does.

The if-chain is just sufficiently complex that the print messages may
actually help the reader understand the logic of the code, so this
argument seems specious.

> The rebase check has been moved to
> the verbose if statement to avoid making the same check in each of
> the four if statements.
>
> Signed-off-by: Dragos Foianu <dragos.foianu@gmail.com>
> ---

Overall, the patch appears to be properly constructed and you seem to
have digested Documentation/SubmittingPatches. Good.

>  branch.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index 723a36b..e2fe455 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -54,6 +54,14 @@ 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 *verbose_prints[4] = {

No need to hard-code 4. 'const char *verbose_prints[]' is sufficient.

On this project, it is preferred (though not consistent) to name
arrays using singular form, such as verbose_print[], so that accessing
a single element, such as verbose_print[42], reads more grammatically
correct.

verbose_prints[] is not as descriptive as it could be. Perhaps
something like verbose_messages[] would be more informative (though
awfully long; maybe just messages[]).

> +               "Branch %s set up to track remote branch %s from %s%s",

Even though you are correctly accessing these strings via _() in the
printf_ln() invocation, you still need to mark them translatable here
with N_(). See section 4.7 [1] of the gettext manual.

[1]: http://www.gnu.org/software/gettext/manual/gettext.html#Special-cases

> +               "Branch %s set up to track local branch %s%s",
> +               "Branch %s set up to track remote ref %s%s",
> +               "Branch %s set up to track local ref %s%s"
> +       };
> +       char *verbose_rebasing = rebasing ? " by rebasing." : ".";

This should be 'const char *'.

Matthieu already mentioned [2] that this sort of "lego" string
construction is not internationalization-friendly. See section 4.3 [3]
of the gettext manual for details.

[2]: http://thread.gmane.org/gmane.comp.version-control.git/244210/focus=244226
[3]: http://www.gnu.org/software/gettext/manual/gettext.html#Preparing-Strings

>         if (remote_is_branch
>             && !strcmp(local, shortname)
>             && !origin) {
> @@ -78,25 +86,17 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
>
>         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);
> +                       printf_ln(_(verbose_prints[0]),
> +                               local, shortname, origin, verbose_rebasing);
>                 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);
> +                       printf_ln(_(verbose_prints[1]),
> +                               local, shortname, verbose_rebasing);
>                 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);
> +                       printf_ln(_(verbose_prints[2]),
> +                               local, remote, verbose_rebasing);
>                 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(_(verbose_prints[3]),
> +                               local, remote, verbose_rebasing);

These hard-coded indexing constants (0, 1, 2, 3) are fragile and
convey little meaning to the reader. Try to consider how to compute
the index into verbose_prints[] based upon the values of
'remote_is_branch' and 'origin'. What are the different ways you could
do so?

>                 else
>                         die("BUG: impossible combination of %d and %p",
>                             remote_is_branch, origin);
> --
> 1.8.3.2

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

[Dragos Foianu]

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

> In fact, this change is not table-driven (emphasis on *driven*). It
> merely moves the strings into a table, but all the logic is still in
> the code. To be table-driven, the logic would be encoded in the table
> as well, and that logic would *drive* the code.
> 
> This is not to say that the code must be table-driven. The GSoC
> microproject merely asks if doing so would make sense. Whether it
> would is for you to decide, and then to explain to reviewers the
> reason(s) why you did or did not make it so.
>
> The if-chain is just sufficiently complex that the print messages may
> actually help the reader understand the logic of the code, so this
> argument seems specious.

I understand now after reading your review. I will try to fix this in a
future attempt.
 
> Matthieu already mentioned [2] that this sort of "lego" string
> construction is not internationalization-friendly. See section 4.3 [3]
> of the gettext manual for details.

I was hoping to get away with using less memory by having only four entries
in the table. I suppose that is not possible. The rebasing check can still
be moved outside of the four if statements and calculate the index
correctly. The strings would then have to be arranged in such a way to make
this work.

Using a multiple-dimension array as suggested in other submissions for this
particular microproject would probably be better, but it has already been done.

> These hard-coded indexing constants (0, 1, 2, 3) are fragile and
> convey little meaning to the reader. Try to consider how to compute
> the index into verbose_prints[] based upon the values of
> 'remote_is_branch' and 'origin'. What are the different ways you could
> do so?

I was going to do something like this: if !remote_is_branch the index goes
incremented by 2, because the first two entries are of no interest and if
!origin, the index is incremented by 1. This would correctly compute the
index. It should also work with the rebasing check if the four
rebasing-specific messages are at the end of the table and when rebasing the
index is set to start at those messages.

The reason I did not go with this is because I would still need the four ifs
in order to keep the bug check part of the code. I might be able to find a
work-around for it on the second attempt.

I have seen N_() used in other code but I wasn't sure what its purpose was.

Thank you very much for the review.

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

[Ævar Arnfjörð Bjarmason]

On Mon, Mar 17, 2014 at 12:46 PM, Dragos Foianu <dragos.foianu@gmail.com> wrote:
> The reason I did not go with this is because I would still need the four ifs
> in order to keep the bug check part of the code. I might be able to find a
> work-around for it on the second attempt.
>
> I have seen N_() used in other code but I wasn't sure what its purpose was.

Aside from other comments here, more generally if you see code that
looks odd it helps to see why it was introduced initially.

In this case if you'd ran e.g.:

    git log --reverse -p -G'Branch %s set up to track remote branch %s
from %s by rebasing' -- branch.c

or otherwise searched for the first occurrence of that odd-looking
code you'd have gotten:

    commit d53a3503
    Author: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
    Date:   Thu Jun 7 19:05:10 2012 +0700

    Remove i18n legos in notifying new branch tracking setup

    Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

And searching for that commit has plenty of context for why that was
done: https://www.google.com/search?q=%22Remove%20i18n%20legos%20in%20notifying%20new%20branch%20tracking%20setup%22

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

[Eric Sunshine]

On Mon, Mar 17, 2014 at 7:46 AM, Dragos Foianu <dragos.foianu@gmail.com> wrote:
> Eric Sunshine <sunshine <at> sunshineco.com> writes:
>> Matthieu already mentioned [2] that this sort of "lego" string
>> construction is not internationalization-friendly. See section 4.3 [3]
>> of the gettext manual for details.
>
> I was hoping to get away with using less memory by having only four entries
> in the table. I suppose that is not possible. The rebasing check can still
> be moved outside of the four if statements and calculate the index
> correctly. The strings would then have to be arranged in such a way to make
> this work.
>
> Using a multiple-dimension array as suggested in other submissions for this
> particular microproject would probably be better, but it has already been done.

If a multi-dimension table is indeed better than other alternatives,
then that's a good reason to choose it, even if others have already
used that approach in their submissions. It's more important that the
code is clean and easy to understand and maintain than to be clever.

If you're really interested in trying an approach not already
submitted by others, take a look at Jonathan's idea [1]. If you play
around with it and find that it actually does make the code clearer
and simpler, then perhaps it's worth submitting. If not, then not.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/198882/focus=198902

>> These hard-coded indexing constants (0, 1, 2, 3) are fragile and
>> convey little meaning to the reader. Try to consider how to compute
>> the index into verbose_prints[] based upon the values of
>> 'remote_is_branch' and 'origin'. What are the different ways you could
>> do so?
>
> I was going to do something like this: if !remote_is_branch the index goes
> incremented by 2, because the first two entries are of no interest and if
> !origin, the index is incremented by 1. This would correctly compute the
> index. It should also work with the rebasing check if the four
> rebasing-specific messages are at the end of the table and when rebasing the
> index is set to start at those messages.
>
> The reason I did not go with this is because I would still need the four ifs
> in order to keep the bug check part of the code. I might be able to find a
> work-around for it on the second attempt.

Since the result is just a number, its possible to compute it directly
without conditionals, however, it does start resembling a magical
incantation. (I'll comment further in your v2 submission.)

> I have seen N_() used in other code but I wasn't sure what its purpose was.
>
> Thank you very much for the review.