threads / patch / 36145

patch[GSOC2014] install_branch_config: change logical chain to lookup table

Subject: [PATCH][GSOC2014] install_branch_config: change logical chain to lookup table

## tl;dr

4 messages between Mar 12, 2014 and Mar 17, 2014. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

TamerTas· Mar 12, 2014, 21:24 UTC · lore
Signed-off-by: TamerTas <tamertas@outlook.com>
--
Thanks for the feedback. Comments below.

I've made the suggested changes [1] to patch [2] but, since there are different number of format specifiers, an if-else clause is necessary. Removing the if-else chain completely doesn't seem to be possible. So making the format table-driven seems to be like an optional change.

[1]: http://git.661346.n2.nabble.com/PATCH-GSOC2014-changed-logical-chain-in-branch-c-to-lookup-tables-tp7605343p7605444.html
[2]: http://git.661346.n2.nabble.com/PATCH-GSOC2014-changed-logical-chain-in-branch-c-to-lookup-tables-tp7605343p7605407.html
--
 branch.c |   44 +++++++++++++++++++++-----------------------
 1 file changed, 21 insertions(+), 23 deletions(-)
Show changes to branch.c +21 −23
diff --git a/branch.c b/branch.c
index 723a36b..1ccf30f 100644
--- a/branch.c
+++ b/branch.c
@@ -50,10 +50,25 @@ static int should_setup_rebase(const char *origin)
 void install_branch_config(int flag, const char *local, const char *origin, const char *remote)
 {
 	const char *shortname = remote + 11;
+	const char *setup_message[] = {
+		N_("Branch %s set up to track local ref %s."),
+		N_("Branch %s set up to track local branch %s."),
+		N_("Branch %s set up to track remote ref %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 local branch %s by rebasing."),
+		N_("Branch %s set up to track remote ref %s by rebasing."),
+		N_("Branch %s set up to track remote branch %s from %s by rebasing."),
+	}; 
+
 	int remote_is_branch = starts_with(remote, "refs/heads/");
 	struct strbuf key = STRBUF_INIT;
 	int rebasing = should_setup_rebase(origin);
 
+	int msg_index = (!!remote_is_branch << 0) +
+			(!!origin << 1) +
+			(!!rebasing << 2);
+
 	if (remote_is_branch
 	    && !strcmp(local, shortname)
 	    && !origin) {
@@ -77,29 +92,12 @@ 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);
+	    if(remote_is_branch && origin)
+		printf_ln(_(setup_message[msg_index]), local, shortname, origin);
+	    else if (remote_is_branch && !origin)
+		printf_ln(_(setup_message[msg_index]), local, shortname);
+	    else
+		printf_ln(_(setup_message[msg_index]), local, remote);
 	}
 }
 

-- 
1.7.9.5
Eric Sunshine· Mar 13, 2014, 22:21 UTC · re: TamerTas · lore

Re: [PATCH][GSOC2014] install_branch_config: change logical chain to lookup table

On Wed, Mar 12, 2014 at 5:24 PM, TamerTas <tamertas@outlook.com> wrote:
>
> Signed-off-by: TamerTas <tamertas@outlook.com>
> --

There should be three hyphens "---" here but somehow you have only two "--". Since "---" is detected automatically when the patch is applied, this deviation can be problematic.

> Thanks for the feedback. Comments below.
Thanks for the resubmission. Comments below. :-)
> I've made the suggested changes [1] to patch [2]
Better. This is a more well-crafted submission.
> but, since there are different number of format
> specifiers, an if-else clause is necessary.
> Removing the if-else chain completely doesn't seem to be possible.
> So making the format table-driven seems to be like an optional change.

Explaining why you chose one approach over another is indeed good etiquette, and can forestall questions which reviewers may otherwise ask.

Note, however, that it is possible to move this logic into a table.
Clue: it is not an error to supply more arguments than there are %s's
in the format string. This is not saying that you must make it
table-driven, but perhaps it may alter the reasons you gave above for
rejecting it (assuming you still do).
Show 23 quoted lines
> [1]: http://git.661346.n2.nabble.com/PATCH-GSOC2014-changed-logical-chain-in-branch-c-to-lookup-tables-tp7605343p7605444.html
> [2]: http://git.661346.n2.nabble.com/PATCH-GSOC2014-changed-logical-chain-in-branch-c-to-lookup-tables-tp7605343p7605407.html
> --
>  branch.c |   44 +++++++++++++++++++++-----------------------
>  1 file changed, 21 insertions(+), 23 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index 723a36b..1ccf30f 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -50,10 +50,25 @@ static int should_setup_rebase(const char *origin)
>  void install_branch_config(int flag, const char *local, const char *origin, const char *remote)
>  {
>         const char *shortname = remote + 11;
> +       const char *setup_message[] = {
> +               N_("Branch %s set up to track local ref %s."),
> +               N_("Branch %s set up to track local branch %s."),
> +               N_("Branch %s set up to track remote ref %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 local branch %s by rebasing."),
> +               N_("Branch %s set up to track remote ref %s by rebasing."),
> +               N_("Branch %s set up to track remote branch %s from %s by rebasing."),
Some compilers will warn about the trailing comma, so you might want to drop it.
Show 9 quoted lines
> +       };
> +
>         int remote_is_branch = starts_with(remote, "refs/heads/");
>         struct strbuf key = STRBUF_INIT;
>         int rebasing = should_setup_rebase(origin);
>
> +       int msg_index = (!!remote_is_branch << 0) +
> +                       (!!origin << 1) +
> +                       (!!rebasing << 2);

Better than the last (buggy) attempt. Nevertheless, it's a fairly magical incantation requiring more thought than some other approaches. Have you considered instead using a multi-dimensional array for the messages and then indexing into it with these variables as direct keys (after using ! or !! to constrain them to 0 or 1)? Would that be better or worse?

Show 36 quoted lines
>         if (remote_is_branch
>             && !strcmp(local, shortname)
>             && !origin) {
> @@ -77,29 +92,12 @@ 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);
> +           if(remote_is_branch && origin)
> +               printf_ln(_(setup_message[msg_index]), local, shortname, origin);
> +           else if (remote_is_branch && !origin)
> +               printf_ln(_(setup_message[msg_index]), local, shortname);
> +           else
> +               printf_ln(_(setup_message[msg_index]), local, remote);

Assigning setup_message[msg_index] to a variable and referencing that variable in the printf_ln() invocations might make this less noisy.

Consider the clue given above about making the code table-driven. Even if you don't go the table-driven route, that clue can simplify this code considerably.

Show 5 quoted lines
>         }
>  }
>
> --
> 1.7.9.5
TamerTas· Mar 14, 2014, 21:30 UTC · re: Eric Sunshine · lore
Signed-off-by: TamerTas <tamertas@outlook.com>
---
Thanks again for the feedback it's been a great learning experience. Comments Below :)

I have refactored the commit [1] to * suggested changes [2]. format-patch was placing 2 hyphens instead of 3 but it's fixed now. I've turned the table into a multidimensional one and I didn't put the inner braces since this was used method in other multidimensional arrays throughout the project. I've also changed shortname to short_name since that seems to be how variables are named in this project. It appears that table-driven code might be more readable after all.

[1]http://git.661346.n2.nabble.com/PATCH-GSOC2014-install-branch-config-change-logical-chain-to-lookup-table-tp7605550.html
[2]http://git.661346.n2.nabble.com/PATCH-GSOC2014-install-branch-config-change-logical-chain-to-lookup-table-tp7605550p7605605.html
---
 branch.c |   42 +++++++++++++++++-------------------------
 1 file changed, 17 insertions(+), 25 deletions(-)
Show changes to branch.c +17 −25
diff --git a/branch.c b/branch.c
index 723a36b..eab6fa4 100644
--- a/branch.c
+++ b/branch.c
@@ -49,13 +49,27 @@ static int should_setup_rebase(const char *origin)
 
 void install_branch_config(int flag, const char *local, const char *origin, const char *remote)
 {
-	const char *shortname = remote + 11;
+	const char *short_name = remote + 11;
+	const char *setup_message[][2][2] = {
+		N_("Branch %s set up to track local ref %s."), 
+		N_("Branch %s set up to track local branch %s."),
+		N_("Branch %s set up to track remote ref %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 local branch %s by rebasing."),
+		N_("Branch %s set up to track remote ref %s by rebasing."), 
+		N_("Branch %s set up to track remote branch %s from %s by rebasing.")
+	}; 
+
 	int remote_is_branch = starts_with(remote, "refs/heads/");
 	struct strbuf key = STRBUF_INIT;
 	int rebasing = should_setup_rebase(origin);
 
+	const char *remote_name = remote_is_branch? short_name : remote;
+	const char *message = setup_message[!!rebasing][!!origin][!!remote_is_branch];
+
 	if (remote_is_branch
-	    && !strcmp(local, shortname)
+	    && !strcmp(local, short_name)
 	    && !origin) {
 		warning(_("Not setting branch %s as its own upstream."),
 			local);
@@ -77,29 +91,7 @@ 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);
+		printf_ln(_(message), local, remote_name, origin);
 	}
 }
 
---
1.7.9.5
Eric Sunshine· Mar 17, 2014, 06:52 UTC · re: TamerTas · lore

Re: [PATCH][GSOC2014] install_branch_config: change logical chain to lookup table

On Fri, Mar 14, 2014 at 5:30 PM, TamerTas <tamertas@outlook.com> wrote:
Show 9 quoted lines
> Signed-off-by: TamerTas <tamertas@outlook.com>
> ---
> Thanks again for the feedback it's been a great learning experience. Comments Below :)
>
> I have refactored the commit [1] to * suggested changes [2].
> format-patch was placing 2 hyphens instead of 3 but it's fixed now.
> I've turned the table into a multidimensional one and I didn't put
> the inner braces since this was used method in other multidimensional arrays
> throughout the project.

Thanks, this is looking better. A few minor comments below, but it's probably not worth a re-roll. What is important is that you have had a taste of the review process on this project, and the GSoC mentors have had a chance to observe your abilities and reviewer interaction.

> I've also changed shortname to short_name since that seems to be how variables are named
> in this project.

This is a conceptually distinct change which should be done as a separate "cleanup" patch since it otherwise adds noise which obscures the "real" change (rewriting the if-chain). One of the goals of a patch submitter is to make the review process as streamlined as possible, so noise should be avoided.

Apart from that, unless the variable is really improperly named and misleading, this sort of change (renaming "shortname" to "short_name") is likely to be considered unnecessary code churn which will probably be rejected. At any given time, there are many patch series in-flight which Junio has to juggle, and code churn increases possibility of conflict between them, which makes his job more difficult.

Show 35 quoted lines
> It appears that table-driven code might be more readable after all.
>
> [1]http://git.661346.n2.nabble.com/PATCH-GSOC2014-install-branch-config-change-logical-chain-to-lookup-table-tp7605550.html
> [2]http://git.661346.n2.nabble.com/PATCH-GSOC2014-install-branch-config-change-logical-chain-to-lookup-table-tp7605550p7605605.html
> ---
>  branch.c |   42 +++++++++++++++++-------------------------
>  1 file changed, 17 insertions(+), 25 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index 723a36b..eab6fa4 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -49,13 +49,27 @@ static int should_setup_rebase(const char *origin)
>
>  void install_branch_config(int flag, const char *local, const char *origin, const char *remote)
>  {
> -       const char *shortname = remote + 11;
> +       const char *short_name = remote + 11;
> +       const char *setup_message[][2][2] = {
> +               N_("Branch %s set up to track local ref %s."),
> +               N_("Branch %s set up to track local branch %s."),
> +               N_("Branch %s set up to track remote ref %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 local branch %s by rebasing."),
> +               N_("Branch %s set up to track remote ref %s by rebasing."),
> +               N_("Branch %s set up to track remote branch %s from %s by rebasing.")
> +       };
> +
>         int remote_is_branch = starts_with(remote, "refs/heads/");
>         struct strbuf key = STRBUF_INIT;
>         int rebasing = should_setup_rebase(origin);
>
> +       const char *remote_name = remote_is_branch? short_name : remote;
> +       const char *message = setup_message[!!rebasing][!!origin][!!remote_is_branch];

In the previous review, it was suggested to pull this value out into its own variable to make the code less noisy since it was being accessed frequently in just a few lines of code. However, since you've collapsed the code to a single printf_ln() invocation in this patch, the separate variable may be not helping clarity (especially as the assignment is divorced by some distance from the code which references it). The same is probably true of 'remote_name' which is used in just the one printf_ln() call.

Show 39 quoted lines
>         if (remote_is_branch
> -           && !strcmp(local, shortname)
> +           && !strcmp(local, short_name)
>             && !origin) {
>                 warning(_("Not setting branch %s as its own upstream."),
>                         local);
> @@ -77,29 +91,7 @@ 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);
> +               printf_ln(_(message), local, remote_name, origin);
>         }
>  }
>
> ---
> 1.7.9.5

← back to recent threads