threads / patch / 36114

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

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

## tl;dr

3 messages between Mar 10, 2014 and Mar 10, 2014. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Vincenzo di Cicco· Mar 10, 2014, 22:04 UTC · lore
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(-)
Show changes to branch.c +14 −24
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
Eric Sunshine· Mar 10, 2014, 23:17 UTC · re: Vincenzo di Cicco · lore

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

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).
Show 5 quoted lines
> 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.

Show 15 quoted lines
>  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_().

Show 30 quoted lines
>         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];
Show 5 quoted lines
> +               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.

Show 7 quoted lines
>         }
> +       strbuf_release(&msg);
>  }
>
>  /*
> --
> 1.9.0
Vincenzo di Cicco· Mar 10, 2014, 23:55 UTC · re: Eric Sunshine · lore

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

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

Show 8 quoted lines
> 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 :)

← back to recent threads