[PATCH][GSoC] branch.c:install_branch_config Simplified long chain of if statements

[PATCH][GSoC] branch.c:install_branch_config Simplified long chain of if statements

[Vincenzo di Cicco]

From: NaN <enzodicicco@gmail.com>

Hi there, I've made this patch in according to the rules to participate at GSoC.
I've seen other patches about this issue very well constructed, so this is only another way to solve this microproject and to test how I can send a patch and discuss about it.

Thanks,
NaN

Signed-off-by: Vincenzo di Cicco <enzodicicco@gmail.com>
---
 Table-driven approach to avoid the long chain of if statements.

 branch.c | 37 ++++++++++++++-----------------------
 1 file changed, 14 insertions(+), 23 deletions(-)

diff --git a/branch.c b/branch.c
index 723a36b..cb8a544 100644
--- a/branch.c
+++ b/branch.c
@@ -53,6 +53,10 @@ 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);
+	struct strbuf msg = STRBUF_INIT;
+	char *locations[2][2] = {{"locate ref \%s", "local branch \%s"},
+				 {"remote ref \%s", "remote branch \%s from \%s"}};
+	char *location;

 	if (remote_is_branch
 	    && !strcmp(local, shortname)
@@ -77,30 +81,17 @@ 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);
-		else
-			die("BUG: impossible combination of %d and %p",
-			    remote_is_branch, origin);
+		location = locations[origin != NULL][remote_is_branch];
+
+		strbuf_addstr(&msg, "Branch \%s set up to track ");
+		strbuf_addstr(&msg, location);
+		if(rebasing)
+			strbuf_addstr(&msg, " by rebasing");
+		strbuf_addch(&msg, '.');
+
+		printf_ln(_(msg.buf), local, remote_is_branch ? remote: shortname, origin);
 	}
+	strbuf_release(&msg);
 }

 /*
--
1.9.0

Re: [PATCH][GSoC] branch.c:install_branch_config Simplified long chain of if statements

[Eric Sunshine]

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

On Mon, Mar 10, 2014 at 6:04 PM, Vincenzo di Cicco
<enzodicicco@gmail.com> wrote:
> From: NaN <enzodicicco@gmail.com>

Drop this line unless it is intentionally different from your email
From: header, which "git am" will pick up automatically when applying
your patch. On this project, real names are preferred (as you
correctly used in your sign-off).

> Hi there, I've made this patch in according to the rules to participate at GSoC.
> I've seen other patches about this issue very well constructed, so this is only another way to solve this microproject and to test how I can send a patch and discuss about it.
>
> Thanks,
> NaN

These "commentary" lines, which are not intended as part of the
official commit message, belong below the "---" line following your
sign-off. Wrap them to 65-70 characters.

> Signed-off-by: Vincenzo di Cicco <enzodicicco@gmail.com>
> ---
>  Table-driven approach to avoid the long chain of if statements.

This non-commentary information is suitable for the commit message,
thus it belongs above your sign-off.

>  branch.c | 37 ++++++++++++++-----------------------
>  1 file changed, 14 insertions(+), 23 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index 723a36b..cb8a544 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -53,6 +53,10 @@ 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);
> +       struct strbuf msg = STRBUF_INIT;
> +       char *locations[2][2] = {{"locate ref \%s", "local branch \%s"},
> +                                {"remote ref \%s", "remote branch \%s from \%s"}};
> +       char *location;

Use 'const char *'. You can probably drop the hard-coded array dimensions.

These strings ought to be internationalized, as in the original code,
thus they should be wrapped with N_().

>         if (remote_is_branch
>             && !strcmp(local, shortname)
> @@ -77,30 +81,17 @@ 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);
> -               else
> -                       die("BUG: impossible combination of %d and %p",
> -                           remote_is_branch, origin);
> +               location = locations[origin != NULL][remote_is_branch];

On this project, it is more idiomatic to say !!origin than 'origin != NULL'.

If we take the time to trace through the code, we can see that
remote_is_branch is indeed either 0 or 1, thus this expression is safe
today, however, if the implementation of starts_with() ever changes so
that it returns a value other than 1 for true, then this code will
break. To avoid such breakage, and to avoid placing burden of tracing
code, you might instead write the expression as:

    location = locations[!!origin][!!remote_is_branch];

> +               strbuf_addstr(&msg, "Branch \%s set up to track ");
> +               strbuf_addstr(&msg, location);
> +               if(rebasing)
> +                       strbuf_addstr(&msg, " by rebasing");
> +               strbuf_addch(&msg, '.');

This approach of composing strings is problematic for translation,
which is why the GSoC microproject states:

    Don't forget that the strangely-named function _() is used for
    internationalization and limits the possibility of gluing
    strings together.

For further details about why this approach is undesirable, see this
other email thread [1].

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

> +               printf_ln(_(msg.buf), local, remote_is_branch ? remote: shortname, origin);

gettext function _() expects constant strings which it can present to
(human) translators, so passing it a buffer containing a string
composed at run-time will not work.

>         }
> +       strbuf_release(&msg);
>  }
>
>  /*
> --
> 1.9.0

Re: [PATCH][GSoC] branch.c:install_branch_config Simplified long chain of if statements

[Vincenzo di Cicco]

Thank you very much for the answer!
I've learned a lot in this few rows.

> If we take the time to trace through the code, we can see that
> remote_is_branch is indeed either 0 or 1, thus this expression is safe
> today, however, if the implementation of starts_with() ever changes so
> that it returns a value other than 1 for true, then this code will
> break. To avoid such breakage, and to avoid placing burden of tracing
> code, you might instead write the expression as:
>
>     location = locations[!!origin][!!remote_is_branch];

Thanks for the good tip. I've erroneously assumed that starts_with()
never will change.

> This approach of composing strings is problematic for translation,
> which is why the GSoC microproject states:

Thanks to show me this issue. I've read the other thread and
understood the problem.

I will do better in future :)