[PATCH] SoC 2014 MicroProject No.8:change multiple if-else statement to table-driven approach

[PATCH] SoC 2014 MicroProject No.8:change multiple if-else statement to table-driven approach

[Yao Zhao]

Signed-off-by: Yao Zhao <zhaox383@umn.edu>
---
 branch.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 53 insertions(+), 2 deletions(-)

diff --git a/branch.c b/branch.c
index 723a36b..6432e27 100644
--- a/branch.c
+++ b/branch.c
@@ -53,7 +53,20 @@ 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);
-
+	char** print_list = malloc(8 * sizeof(char*));
+	char* arg1=NULL;
+	char* arg2=NULL;
+	char* arg3=NULL;
+	int index=0;
+
+	print_list[7] = _("Branch %s set up to track remote branch %s from %s by rebasing.");
+	print_list[6] = _("Branch %s set up to track remote branch %s from %s.");
+	print_list[5] = _("Branch %s set up to track local branch %s by rebasing.");
+	print_list[4] = _("Branch %s set up to track local branch %s.");
+	print_list[3] = _("Branch %s set up to track remote ref %s by rebasing.");
+	print_list[2] = _("Branch %s set up to track remote ref %s.");
+	print_list[1] = _("Branch %s set up to track local ref %s by rebasing.");
+	print_list[0] = _("Branch %s set up to track local ref %s.");
 	if (remote_is_branch
 	    && !strcmp(local, shortname)
 	    && !origin) {
@@ -77,7 +90,44 @@ 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)
+		if(remote_is_branch)
+				index += 4;
+		if(origin)
+				index += 2;
+		if(rebasing)
+				index += 1;
+		
+		if(index < 0 || index > 7)
+		{
+			die("BUG: impossible combination of %d and %p",
+			    remote_is_branch, origin);
+		}
+
+		if(index <= 4) {
+			arg1 = local;
+			arg2 = remote;
+		}
+		else if(index > 6) {
+			arg1 = local;
+			arg2 = shortname;
+			arg3 = origin;
+		}
+		else {
+			arg1 = local;
+			arg2 = shortname;
+		}
+
+		if(!arg3) {
+			printf_ln(print_list[index],arg1,arg2);
+		}
+		else {
+			printf_ln(print_list[index],arg1,arg2,arg3);
+		}
+
+		free(print_list);
+
+
+/*		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."),
@@ -100,6 +150,7 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
 		else
 			die("BUG: impossible combination of %d and %p",
 			    remote_is_branch, origin);
+*/
 	}
 }
 
-- 
1.8.3.2

Re: [PATCH] SoC 2014 MicroProject No.8:change multiple if-else statement to table-driven approach

[Eric Sunshine]

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

On Tue, Mar 11, 2014 at 11:47 PM, Yao Zhao <zhaox383@umn.edu> wrote:
> Subject: SoC 2014 MicroProject No.8:change multiple if-else statement to table-driven approach

The email subject is extracted automatically by "git am" as the first
line of the patch's commit message so it should contain only text
which is relevant to the commit message. In this case, everything
before "changes" is merely commentary for readers of the email, and
not relevant to the commit message.

It is indeed a good idea to let reviewers know that this submission is
for GSoC, and you can indicate this as such:

    Subject: [PATCH GSoC] change multiple if-else statements to be table-driven

> Signed-off-by: Yao Zhao <zhaox383@umn.edu>
> ---

The additional information that this is GSoC microproject #8 would go
in the "commentary" area right here after the "---" following your
sign-off.

>  branch.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 53 insertions(+), 2 deletions(-)

The patch is rife with style violations. I'll point out the first
instance of each violation, but do be sure to fix all remaining ones
when you resubmit. See Documentation/CodingGuidelines for details.

> diff --git a/branch.c b/branch.c
> index 723a36b..6432e27 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -53,7 +53,20 @@ 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);
> -
> +       char** print_list = malloc(8 * sizeof(char*));

Style: char **print_list

Why allocate 'print_list' on the heap? An automatic variable 'char
const *print_list[]' would be more idiomatic and less likely to be
leaked.

In fact, your heap-allocated 'print_list' _is_ being leaked a few
lines down when the function returns early after warning that a branch
can not be its own upstream.

> +       char* arg1=NULL;
> +       char* arg2=NULL;
> +       char* arg3=NULL;

Style: char *var
Style: whitespace: var = NULL

> +       int index=0;
> +
> +       print_list[7] = _("Branch %s set up to track remote branch %s from %s by rebasing.");
> +       print_list[6] = _("Branch %s set up to track remote branch %s from %s.");
> +       print_list[5] = _("Branch %s set up to track local branch %s by rebasing.");
> +       print_list[4] = _("Branch %s set up to track local branch %s.");
> +       print_list[3] = _("Branch %s set up to track remote ref %s by rebasing.");
> +       print_list[2] = _("Branch %s set up to track remote ref %s.");
> +       print_list[1] = _("Branch %s set up to track local ref %s by rebasing.");
> +       print_list[0] = _("Branch %s set up to track local ref %s.");

If you make print_list[] an automatic variable, then you can declare
and populate it via a simple initializer. No need for this manual
approach.

>         if (remote_is_branch
>             && !strcmp(local, shortname)
>             && !origin) {
> @@ -77,7 +90,44 @@ 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)
> +               if(remote_is_branch)

Style: whitespace: if (...)

> +                               index += 4;
> +               if(origin)
> +                               index += 2;
> +               if(rebasing)
> +                               index += 1;
> +
> +               if(index < 0 || index > 7)
> +               {
> +                       die("BUG: impossible combination of %d and %p",
> +                           remote_is_branch, origin);
> +               }
> +
> +               if(index <= 4) {
> +                       arg1 = local;
> +                       arg2 = remote;
> +               }
> +               else if(index > 6) {

Style: } else if (...) {

> +                       arg1 = local;
> +                       arg2 = shortname;
> +                       arg3 = origin;
> +               }
> +               else {
> +                       arg1 = local;
> +                       arg2 = shortname;
> +               }
> +
> +               if(!arg3) {
> +                       printf_ln(print_list[index],arg1,arg2);

Style: whitespace: printf_ln(x, y, z)

> +               }
> +               else {
> +                       printf_ln(print_list[index],arg1,arg2,arg3);
> +               }

Unfortunately, this is quite a bit more verbose and complex than the
original code, and all the magic numbers (4, 2, 1, 0, 7, 4, 6) place a
higher cognitive load on the reader, so this change probably is a net
loss as far as clarity is concerned.

Take a step back and consider again the GSoC miniproject: It talks
about making the code table-driven. Certainly, you have moved the
strings into a table, but all the complex logic is still in the code,
and not in a table, hence it's not table-driven. To make this
table-driven, you should try to figure out how most or all of this
logic can be moved into a table.

> +               free(print_list);
> +
> +
> +/*             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."),
> @@ -100,6 +150,7 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
>                 else
>                         die("BUG: impossible combination of %d and %p",
>                             remote_is_branch, origin);
> +*/

The code you wrote is meant to replace the old code, so your patch
should actually remove the old code, not just comment it out.

>         }
>  }
>
> --
> 1.8.3.2

[PATCH] GSoC Change multiple if-else statements to be table-driven

[Yao Zhao]

Signed-off-by: Yao Zhao <zhaox383@umn.edu>
---
GSoC_MicroProject_#8

Hello Eric,

Thanks for reviewing my code. I implemented table-driven method this time and correct the style
problems indicated in review.

Thank you,

Yao

 branch.c | 72 ++++++++++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 50 insertions(+), 22 deletions(-)

diff --git a/branch.c b/branch.c
index 723a36b..6451c99 100644
--- a/branch.c
+++ b/branch.c
@@ -49,10 +49,43 @@ 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;
 	int remote_is_branch = starts_with(remote, "refs/heads/");
 	struct strbuf key = STRBUF_INIT;
 	int rebasing = should_setup_rebase(origin);
+	int size=8, i;
+
+	typedef struct PRINT_LIST {
+		const char *print_str;
+		const char *arg2; 
+		//arg1 is always local, so I only add arg2 and arg3 in struct
+		const char *arg3;
+		int b_rebasing;
+	  int b_remote_is_branch;
+		int b_origin;
+	} PRINT_LIST;
+
+	PRINT_LIST print_list[] = {
+		{.print_str = _("Branch %s set up to track remote branch %s from %s by rebasing."), 
+				.arg2 = shortname, .arg3 = origin,
+					 .b_rebasing = 1, .b_remote_is_branch = 1, .b_origin = 1},
+	  {.print_str = _("Branch %s set up to track remote branch %s from %s."), 
+				.arg2 = shortname, .arg3 = origin,
+					 .b_rebasing = 0, .b_remote_is_branch = 1, .b_origin = 1},
+    {.print_str = _("Branch %s set up to track local branch %s by rebasing."), 
+				.arg2 = shortname, .b_rebasing = 1, .b_remote_is_branch = 1, .b_origin = 0},
+		{.print_str = _("Branch %s set up to track local branch %s."), 
+				.arg2 = shortname, .b_rebasing = 0, .b_remote_is_branch = 1, .b_origin = 0},
+		{.print_str = _("Branch %s set up to track remote ref %s by rebasing."), 
+				.arg2 = remote, .b_rebasing = 1, .b_remote_is_branch = 0, .b_origin = 1},
+		{.print_str = _("Branch %s set up to track remote ref %s."), 
+				.arg2 = remote, .b_rebasing = 0, .b_remote_is_branch = 0, .b_origin = 1},
+		{.print_str = _("Branch %s set up to track local ref %s by rebasing."), 
+				.arg2 = remote, .b_rebasing = 1, .b_remote_is_branch = 0, .b_origin = 0},
+		{.print_str = _("Branch %s set up to track local ref %s."), 
+				.arg2 = remote, .b_rebasing = 0, .b_remote_is_branch = 0, .b_origin = 0},
+};
I am confused here: I use struct initializer and I am not sure if it's ok
because it is only supported by ANSI 

 	if (remote_is_branch
 	    && !strcmp(local, shortname)
@@ -75,31 +108,26 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
 		git_config_set(key.buf, "true");
 	}
 	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
+		for (i=0;i<size;i++)
+		{
+			if (print_list[i].b_rebasing == (rebasing? 1 : 0) && 
+								print_list[i].b_remote_is_branch == (remote_is_branch? 1 : 0) &&
+								print_list[i].b_origin == (origin? 1 : 0))
+			{
+				if (print_list[i].arg3 == NULL)
+					printf_ln (print_list[i].print_str, local, print_list[i].arg2);
+				else
+					printf_ln (print_list[i].print_str, local, 
+							print_list[i].arg2, print_list[i].arg3);
+				
+				break;
+			}
+		}
+		if (i == size)
 			die("BUG: impossible combination of %d and %p",
 			    remote_is_branch, origin);
+
 	}
 }
 
-- 
1.8.3.2

Re: [PATCH] GSoC Change multiple if-else statements to be table-driven

[Eric Sunshine]

Thanks for the resubmission. Comments below.

On Thu, Mar 13, 2014 at 4:20 PM, Yao Zhao <zhaox383@umn.edu> wrote:
> Subject: [PATCH] GSoC Change multiple if-else statements to be table-driven

It's a good idea to let reviewers know that this is attempt 2. Do so
by saying [PATCH v2]. Your next one will be [PATCH v3]. The -v option
for "git format-email" can help.

When your patch is applied via "git am", text inside [...] gets
stripped automatically. The "GSoC" tells email readers what this
submission is about, but isn't relevant to the actual commit message.
It should be placed inside [...]. For instance: [PATCH/GSoC v2].

> Signed-off-by: Yao Zhao <zhaox383@umn.edu>
> ---
> GSoC_MicroProject_#8
>
> Hello Eric,
>
> Thanks for reviewing my code. I implemented table-driven method this time and correct the style
> problems indicated in review.

Explaining what you changed since the last version is indeed good
etiquette. Thanks. For bonus points, also provide a link to the
previous version, like this [1].

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

>  branch.c | 72 ++++++++++++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 50 insertions(+), 22 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index 723a36b..6451c99 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -49,10 +49,43 @@ static int should_setup_rebase(const char *origin)
>
>  void install_branch_config(int flag, const char *local, const char *origin, const char *remote)
>  {
> +

Unnecessary insertion of blank line.

>         const char *shortname = remote + 11;
>         int remote_is_branch = starts_with(remote, "refs/heads/");
>         struct strbuf key = STRBUF_INIT;
>         int rebasing = should_setup_rebase(origin);
> +       int size=8, i;

Style: whitespace: size = 8

You can use ARRAY_SIZE(print_list) to avoid hardcoding 8 (and then you
don't need the variable 'size').

> +       typedef struct PRINT_LIST {
> +               const char *print_str;
> +               const char *arg2;
> +               //arg1 is always local, so I only add arg2 and arg3 in struct

This commentary should be placed below the "---" under your sign-off
(or dropped altogether since it's pretty obvious).

Also, in this project avoid //-style comments.

> +               const char *arg3;
> +               int b_rebasing;
> +         int b_remote_is_branch;

Strange indentation. Use tabs for indentation, and set your editor so
tabs have width 8.

> +               int b_origin;
> +       } PRINT_LIST;

Read below for some commentary about b_rebasing, b_remote_is_branch, b_origin.

> +       PRINT_LIST print_list[] = {
> +               {.print_str = _("Branch %s set up to track remote branch %s from %s by rebasing."),
> +                               .arg2 = shortname, .arg3 = origin,
> +                                        .b_rebasing = 1, .b_remote_is_branch = 1, .b_origin = 1},
> +         {.print_str = _("Branch %s set up to track remote branch %s from %s."),
> +                               .arg2 = shortname, .arg3 = origin,
> +                                        .b_rebasing = 0, .b_remote_is_branch = 1, .b_origin = 1},
> +    {.print_str = _("Branch %s set up to track local branch %s by rebasing."),
> +                               .arg2 = shortname, .b_rebasing = 1, .b_remote_is_branch = 1, .b_origin = 0},
> +               {.print_str = _("Branch %s set up to track local branch %s."),
> +                               .arg2 = shortname, .b_rebasing = 0, .b_remote_is_branch = 1, .b_origin = 0},
> +               {.print_str = _("Branch %s set up to track remote ref %s by rebasing."),
> +                               .arg2 = remote, .b_rebasing = 1, .b_remote_is_branch = 0, .b_origin = 1},
> +               {.print_str = _("Branch %s set up to track remote ref %s."),
> +                               .arg2 = remote, .b_rebasing = 0, .b_remote_is_branch = 0, .b_origin = 1},
> +               {.print_str = _("Branch %s set up to track local ref %s by rebasing."),
> +                               .arg2 = remote, .b_rebasing = 1, .b_remote_is_branch = 0, .b_origin = 0},
> +               {.print_str = _("Branch %s set up to track local ref %s."),
> +                               .arg2 = remote, .b_rebasing = 0, .b_remote_is_branch = 0, .b_origin = 0},
> +};
> I am confused here: I use struct initializer and I am not sure if it's ok
> because it is only supported by ANSI

This commentary should be placed below "---" after your sign-off.

Indeed, you want to avoid named field initializers in this project and
instead use positional initializers.

Translatable strings in an initializer should be wrapped with N_()
instead of _(). You will still need to use _() later on when you
reference the string from the table. See section 4.7 [2] of the GNU
gettext manual for details.

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

>         if (remote_is_branch
>             && !strcmp(local, shortname)
> @@ -75,31 +108,26 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
>                 git_config_set(key.buf, "true");
>         }
>         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
> +               for (i=0;i<size;i++)

Style: whitespace: for (i = 0; i < size; i++)

> +               {
> +                       if (print_list[i].b_rebasing == (rebasing? 1 : 0) &&
> +                                                               print_list[i].b_remote_is_branch == (remote_is_branch? 1 : 0) &&
> +                                                               print_list[i].b_origin == (origin? 1 : 0))

Style: whitespace: (x ? 1 : 0)

Assigning &print_list[i] to a variable would allow you to reduce the
noise of this expression a bit.

On this project it is more idiomatic to say !!rebasing,
!!remote_is_branch, !!origin to constrain these values to 0 or 1.

An alternate approach might be to use a multi-dimensional array, where
the boolean values of rebasing, remote_is_branch, and origin are keys
into the array. This would allow you to pick out the correct
PRINT_LIST entry directly (no looping), thus eliminating the need for
those b_rebasing, b_remote_is_branch, and b_origin members.

> +                       {
> +                               if (print_list[i].arg3 == NULL)
> +                                       printf_ln (print_list[i].print_str, local, print_list[i].arg2);

Style: whitespace: printf_ln(...)

Reminder: wrap _() around print_list[i].print_str

> +                               else
> +                                       printf_ln (print_list[i].print_str, local,
> +                                                       print_list[i].arg2, print_list[i].arg3);

This logic can be simplified. Hint: It is not an error to pass more
arguments than there are %s's in the format string.

> +
> +                               break;
> +                       }
> +               }
> +               if (i == size)
>                         die("BUG: impossible combination of %d and %p",
>                             remote_is_branch, origin);
> +

Unnecessary insertion of blank line.

>         }
>  }
>
> --
> 1.8.3.2

Re: [PATCH] GSoC Change multiple if-else statements to be table-driven

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> Thanks for the resubmission. Comments below.

Thanks, Eric, for helping so many micro exercises.

> On Thu, Mar 13, 2014 at 4:20 PM, Yao Zhao <zhaox383@umn.edu> wrote:
>> Subject: [PATCH] GSoC Change multiple if-else statements to be table-driven
>
> It's a good idea to let reviewers know that this is attempt 2. Do so
> by saying [PATCH v2]. Your next one will be [PATCH v3]. The -v option
> for "git format-email" can help.

Yao, I think Eric meant "git format-patch".

> When your patch is applied via "git am", text inside [...] gets
> stripped automatically. The "GSoC" tells email readers what this
> submission is about, but isn't relevant to the actual commit message.
> It should be placed inside [...]. For instance: [PATCH/GSoC v2].

So in short,

	Subject: [PATCH/GSoC v2] branch.c: turn nested if-else logic to table-driven

or something.

>> +       typedef struct PRINT_LIST {
> ...
>> +               int b_origin;
>> +       } PRINT_LIST;

We do not do ALL_CAPS names and tend not to introduce one-off
typedefs for struct.  Instead we would just use "struct print_list"
throughout (if we were to indeed use such a new struct, that is).

>> +       PRINT_LIST print_list[] = {
>> +               {.print_str = _("Branch %s set up to track remote branch %s from %s by rebasing."),
>> +                               .arg2 = shortname, .arg3 = origin,
>> +                                        .b_rebasing = 1, .b_remote_is_branch = 1, .b_origin = 1},

>> I am confused here: I use struct initializer and I am not sure if it's ok
>> because it is only supported by ANSI
> ...
> Indeed, you want to avoid named field initializers in this project and
> instead use positional initializers.

Correct.

> Translatable strings in an initializer should be wrapped with N_()
> instead of _(). You will still need to use _() later on when you
> reference the string from the table. See section 4.7 [2] of the GNU
> gettext manual for details.

Correct.

> An alternate approach might be to use a multi-dimensional array,
> where the boolean values of rebasing, remote_is_branch, and origin
> are keys into the array. This would allow you to pick out the
> correct PRINT_LIST entry directly (no looping), thus eliminating
> the need for those b_rebasing, b_remote_is_branch, and b_origin
> members.

Correct.

After seeing so many "table driven" submissions, I however tend to
agree with your earlier comment on another thread on this same
micro, where you said an nested if-else cascade that was rewritten
in a clearer way (sorry, I do not remember whose submission it was
offhand) may be the best answer to the "Would it make sense to make
the code table-driven?" question, even though I tentatively queued
d7ea7894 (install_branch_config(): simplify verbose messages logic,
2014-03-13) from Paweł on 'pu'.

Thanks for a review.

[PATCH/GSoC_v3] branch.c: turn nested if-else logic to table-driven

[Yao Zhao]

Signed-off-by: Yao Zhao <zhaox383@umn.edu>
---
 branch.c | 53 +++++++++++++++++++++++++++++------------------------
 1 file changed, 29 insertions(+), 24 deletions(-)
Hello Eric,

Thank you and Junio for reviewing my code. It is really helpful to improve my code quality.

This is version 3 of patch. Previous address : http://thread.gmane.org/gmane.comp.version-control.git/243919. I do not use positional initializer because it is not allowed to use variable in it. I don't know if it's ok to use this redundant way to initialize "list".

I cannot find -v flag in documentation you indicated in last email so I use set-prefix to add it into prefix.
	
Now I am working on writing proposal for git project. I am really interested in last one, about improve git_config. I know it's important to get known about git_config first and have read documentation about it. But I am really confused about how to understand code of git_config. When user type in git config in terminal, what is the execute order of functions? How git config influence other git command? Does program read config file every time when they execuate config-related command?

Thank you,

Yao

diff --git a/branch.c b/branch.c
index 723a36b..1df30c7 100644
--- a/branch.c
+++ b/branch.c
@@ -53,7 +53,33 @@ 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 print_list {
+		const char *print_str;
+		const char *arg2; 
+		const char *arg3;
+	} ; 
+	struct print_list target;
+
+	struct print_list list[2][2][2];
+	list[0][0][0].print_str = N_("Branch %s set up to track local ref %s.");
+	list[0][0][0].arg2 = remote;
+	list[0][0][1].print_str = N_("Branch %s set up to track local ref %s by rebasing.");
+	list[0][0][1].arg2 = remote;
+	list[0][1][0].print_str = N_("Branch %s set up to track remote ref %s.");
+	list[0][1][0].arg2 = remote;
+	list[0][1][1].print_str = N_("Branch %s set up to track remote ref %s by rebasing.");
+	list[0][1][1].arg2 = remote;
+	list[1][0][0].print_str = N_("Branch %s set up to track local branch %s.");
+	list[1][0][0].arg2 =shortname;
+	list[1][0][1].print_str = N_("Branch %s set up to track local branch %s by rebasing.");
+	list[1][0][1].arg2 = shortname;
+	list[1][1][0].print_str = N_("Branch %s set up to track remote branch %s from %s.");
+	list[1][1][0].arg2 = shortname;
+	list[1][1][0].arg3 = origin;
+	list[1][1][1].print_str = N_("Branch %s set up to track remote branch %s from %s by rebasing.");
+	list[1][1][1].arg2 = shortname;
+	list[1][1][1].arg3 = origin;
+ 
 	if (remote_is_branch
 	    && !strcmp(local, shortname)
 	    && !origin) {
@@ -77,29 +103,8 @@ 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);
+			target = list[!!remote_is_branch][!!origin][!!rebasing];
+			printf_ln (_(target.print_str), local, target.arg2, target.arg3);
 	}
 }
 
-- 
1.8.3.2

Re: [PATCH] GSoC Change multiple if-else statements to be table-driven

[Eric Sunshine]

On Fri, Mar 14, 2014 at 12:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> On Thu, Mar 13, 2014 at 4:20 PM, Yao Zhao <zhaox383@umn.edu> wrote:
>>> Subject: [PATCH] GSoC Change multiple if-else statements to be table-driven
>>
>> It's a good idea to let reviewers know that this is attempt 2. Do so
>> by saying [PATCH v2]. Your next one will be [PATCH v3]. The -v option
>> for "git format-email" can help.
>
> Yao, I think Eric meant "git format-patch".

Correct. Sorry for the confusion.

>> An alternate approach might be to use a multi-dimensional array,
>> where the boolean values of rebasing, remote_is_branch, and origin
>> are keys into the array. This would allow you to pick out the
>> correct PRINT_LIST entry directly (no looping), thus eliminating
>> the need for those b_rebasing, b_remote_is_branch, and b_origin
>> members.
>
> Correct.
>
> After seeing so many "table driven" submissions, I however tend to
> agree with your earlier comment on another thread on this same
> micro, where you said an nested if-else cascade that was rewritten
> in a clearer way (sorry, I do not remember whose submission it was

It was Adam NoLastName [1].

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

> offhand) may be the best answer to the "Would it make sense to make
> the code table-driven?" question, even though I tentatively queued
> d7ea7894 (install_branch_config(): simplify verbose messages logic,
> 2014-03-13) from Paweł on 'pu'.

Perhaps it is time to mark this microproject as "taken" on the GSoC
page [2], along a fews others for which we have received multiple
submissions.

[2]: https://github.com/git/git.github.io/blob/master/SoC-2014-Microprojects.md

Re: [PATCH] GSoC Change multiple if-else statements to be table-driven

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> Perhaps it is time to mark this microproject as "taken" on the GSoC
> page [2], along a fews others for which we have received multiple
> submissions.
>
> [2]: https://github.com/git/git.github.io/blob/master/SoC-2014-Microprojects.md

I actually have been of multiple minds on this.

 * After seeing that many candidates tried to solve the same micro,
   apparently without checking answers by other people, and seeing
   how they responded to the reviews given to them, I found that we
   had as equally good interactions with them to judge their skills,
   both techincal and social, as we would have had if each of them
   solved different micros.

 * Many reviewers may have gotten tired of seeing many novice
   attempts on the same micro over and over again, giving gentle
   suggestions for improvements. Because the _sole_ purpose of these
   micros were to help candidates get their toes wet in the process,
   duplicated efforts on the candidates' side are not wasted---they
   each hopefully learned how to interact with this community.

   But it is true that, if we were wishing to also get some trivial
   clean-ups in the codebase as a side effect of these micros, we
   have wasted reviewer bandwidth by not having enough micros, and
   reviewers may have had some feeling that their efforts did not
   fully contribute to improving our codebase, which may have been
   discouraging.

   Big thanks go to all reviewers who participated despite this.  If
   we had more micros, the apparent wastage of the reviewer efforts
   might have been avoided.

 * Many candidates did not even bother to check if others are
   working on the same micro, however, which would be a bad sign by
   itself. Concentrating on one's own topic, without paying any
   attention to what others are working on the same software, is
   never a good discipline.

   Some may argue that it would be unfair to blame the candidates on
   this---they may have picked up micros that haven't been solved if
   we had more---but after seeing that many candidates' submissions
   apparently did not take into account the reviews given to others'
   submissions on the same micro and/or had many exactly the same
   issues like log message styles as submissions on other micros
   that have already been reviewed, I personally do not think they
   are blameless.

So in short, yes it would have been nicer if we had more micros than
candidates, but I do not think it was detrimental for the purpose of
these micro exercises that multiple candidates ended up attempting
the same micro.

Again, Big Thanks to Michael for the excellent "micro" idea, and all
reviewers who participated.

Re: [PATCH/GSoC_v3] branch.c: turn nested if-else logic to table-driven

[Eric Sunshine]

On Sun, Mar 16, 2014 at 2:08 AM, Yao Zhao <zhaox383@umn.edu> wrote:
> Signed-off-by: Yao Zhao <zhaox383@umn.edu>
> ---
>  branch.c | 53 +++++++++++++++++++++++++++++------------------------
>  1 file changed, 29 insertions(+), 24 deletions(-)
> Hello Eric,
>
> Thank you and Junio for reviewing my code. It is really helpful to improve my code quality.

Wrap commit message (and preferably commentary) to 65-70 characters.

> This is version 3 of patch. Previous address : http://thread.gmane.org/gmane.comp.version-control.git/243919. I do not use positional initializer because it is not allowed to use variable in it. I don't know if it's ok to use this redundant way to initialize "list".

Thanks for the resubmission. A few comments below, but I don't think
you need to resubmit. 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. A better
use of your time now would be to make your GSoC proposal and converse
with the mentors.

> I cannot find -v flag in documentation you indicated in last email so I use set-prefix to add it into prefix.

Sorry for the confusion. As Junio pointed out [1], I meant the -v
option of "git format-patch", not "git format-email" (which doesn't
exist).

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

> Now I am working on writing proposal for git project. I am really interested in last one, about improve git_config. I know it's important to get known about git_config first and have read documentation about it. But I am really confused about how to understand code of git_config. When user type in git config in terminal, what is the execute order of functions? How git config influence other git command? Does program read config file every time when they execuate config-related command?

You will get a better response if you ask these questions in a new
email thread rather than re-using this one. Choose a subject for your
email which indicates clearly that you have questions about that
particular GSoC project, address it to the git mailing list, and cc:
the mentors of that project.

> Thank you,
>
> Yao
>
> diff --git a/branch.c b/branch.c
> index 723a36b..1df30c7 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -53,7 +53,33 @@ 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 print_list {
> +               const char *print_str;
> +               const char *arg2;
> +               const char *arg3;
> +       } ;
> +       struct print_list target;

This should be 'const struct print_list *target'. There is no need to
copy the entire structure (below) just to access its members.

> +       struct print_list list[2][2][2];
> +       list[0][0][0].print_str = N_("Branch %s set up to track local ref %s.");
> +       list[0][0][0].arg2 = remote;
> +       list[0][0][1].print_str = N_("Branch %s set up to track local ref %s by rebasing.");
> +       list[0][0][1].arg2 = remote;
> +       list[0][1][0].print_str = N_("Branch %s set up to track remote ref %s.");
> +       list[0][1][0].arg2 = remote;
> +       list[0][1][1].print_str = N_("Branch %s set up to track remote ref %s by rebasing.");
> +       list[0][1][1].arg2 = remote;
> +       list[1][0][0].print_str = N_("Branch %s set up to track local branch %s.");
> +       list[1][0][0].arg2 =shortname;
> +       list[1][0][1].print_str = N_("Branch %s set up to track local branch %s by rebasing.");
> +       list[1][0][1].arg2 = shortname;
> +       list[1][1][0].print_str = N_("Branch %s set up to track remote branch %s from %s.");
> +       list[1][1][0].arg2 = shortname;
> +       list[1][1][0].arg3 = origin;
> +       list[1][1][1].print_str = N_("Branch %s set up to track remote branch %s from %s by rebasing.");
> +       list[1][1][1].arg2 = shortname;
> +       list[1][1][1].arg3 = origin;

Since this is not in an initializer table, you would normally use _()
rather than N_() (and you don't have to use _() in the printf_ln()
invocation).

It disturbs me to see that .arg3 does not get initialized in most of
these cases, yet it is accessed below by printf_ln(). Code analysis
tools are likely to complain about accessing an uninitialized value.

One way you could put this into the initializer would be to use a
constant expression to indicate which variable to pass to printf_ln().
Something like this:

    struct print_list {
        const char *print_str;
        int use_shortname;
        int use_origin;
    } print_list[][2][2] = {
        N_("Branch %s ..."), 0, 0,
        ...
        N_("Branch %s ..."), 1, 0,
        ...
    }

    printf_ln(_(target->print_str), local,
        target->use_shortname ? shortname : remote,
        target->use_origin ? origin : NULL);

To make it clearer, use named constants rather than 0 and 1.

NOTE: I am *not* suggesting that you resubmit with this change. It's
getting ugly and bordering on being too complex and difficult to read.
As noted above, your time is probably better spent working on your
GSoC proposal.

>         if (remote_is_branch
>             && !strcmp(local, shortname)
>             && !origin) {
> @@ -77,29 +103,8 @@ 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);
> +                       target = list[!!remote_is_branch][!!origin][!!rebasing];

With the suggestion above of making 'target' a pointer to the struct,
this would become:

    target = &list[...][...][...];

> +                       printf_ln (_(target.print_str), local, target.arg2, target.arg3);

Style: whitespace: printf_ln(...)

And this would become:

    printf_ln(_(target->print_str), local, target->arg2, target->arg3);

>         }
>  }
>
> --
> 1.8.3.2

Re: [PATCH] GSoC Change multiple if-else statements to be table-driven

[Michael Haggerty]

On 03/17/2014 08:31 AM, Junio C Hamano wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
>> Perhaps it is time to mark this microproject as "taken" on the GSoC
>> page [2], along a fews others for which we have received multiple
>> submissions.

I just marked #8 as taken, as it's been beaten to death.

I haven't been keeping careful track of which other microprojects have
been overused.  If you have suggestions for the chopping block, let me know.

>> [2]: https://github.com/git/git.github.io/blob/master/SoC-2014-Microprojects.md
> 
> I actually have been of multiple minds on this.
> 
>  * After seeing that many candidates tried to solve the same micro,
>    apparently without checking answers by other people, and seeing
>    how they responded to the reviews given to them, I found that we
>    had as equally good interactions with them to judge their skills,
>    both techincal and social, as we would have had if each of them
>    solved different micros.
> 
>  * Many reviewers may have gotten tired of seeing many novice
>    attempts on the same micro over and over again, giving gentle
>    suggestions for improvements. Because the _sole_ purpose of these
>    micros were to help candidates get their toes wet in the process,
>    duplicated efforts on the candidates' side are not wasted---they
>    each hopefully learned how to interact with this community.
> 
>    But it is true that, if we were wishing to also get some trivial
>    clean-ups in the codebase as a side effect of these micros, we
>    have wasted reviewer bandwidth by not having enough micros, and
>    reviewers may have had some feeling that their efforts did not
>    fully contribute to improving our codebase, which may have been
>    discouraging.
> 
>    Big thanks go to all reviewers who participated despite this.  If
>    we had more micros, the apparent wastage of the reviewer efforts
>    might have been avoided.
> 
>  * Many candidates did not even bother to check if others are
>    working on the same micro, however, which would be a bad sign by
>    itself. Concentrating on one's own topic, without paying any
>    attention to what others are working on the same software, is
>    never a good discipline.
> 
>    Some may argue that it would be unfair to blame the candidates on
>    this---they may have picked up micros that haven't been solved if
>    we had more---but after seeing that many candidates' submissions
>    apparently did not take into account the reviews given to others'
>    submissions on the same micro and/or had many exactly the same
>    issues like log message styles as submissions on other micros
>    that have already been reviewed, I personally do not think they
>    are blameless.
> 
> So in short, yes it would have been nicer if we had more micros than
> candidates, but I do not think it was detrimental for the purpose of
> these micro exercises that multiple candidates ended up attempting
> the same micro.

I wish I had had time to invent more microprojects.  I think we were
lucky: since students didn't seem to learn from each other's attempts,
the fact that many people tried the same microprojects wasn't much of a
problem.  But if a student had arrived with a "perfect" solution to a
well-trodden microproject on his/her first attempt, I would hate to have
to be suspicious that the student plagiarized from somebody else's
answer plus all of the review comments about the earlier answer,
especially since a perfect solution would itself reduce the amount of
interaction between the cheating student and the mailing list compared
to a student who required several iterations.  I also hope that students
didn't feel unwelcomed during the times when the list of untaken
microprojects was nearly empty.

If we really wanted to make this system cheat-proof, there would have to
be not only enough microprojects to go around, but also some mechanism
to ensure that student B didn't start work on a microproject that
student A was just about to submit a solution to.  Maybe students would
have to claim a microproject when they started work on them, or request
a microproject to work on as opposed to picking one from a list on a web
page.  But realistically, I think that this hypothetical improved system
would be more work than we would have been able to invest.

Maybe in the future our microprojects should have two parts:

a. Fix ...
b. Invent a microproject for the next student.

(Just kidding.)

> Again, Big Thanks to Michael for the excellent "micro" idea, and all
> reviewers who participated.

I'd really like to thank the reviewers, who put in enormous effort
reviewing submissions promptly, and showed superhuman patience when
dealing with the same issues over an over again.  Without such great
reviewers the whole idea would have just resulted in frustration for the
students; with them, I think that even the students who don't get a GSoC
spot will have learned something valuable from their participation.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH] GSoC Change multiple if-else statements to be table-driven

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> On 03/17/2014 08:31 AM, Junio C Hamano wrote:
>
>> So in short, yes it would have been nicer if we had more micros than
>> candidates, but I do not think it was detrimental for the purpose of
>> these micro exercises that multiple candidates ended up attempting
>> the same micro.
>
> I wish I had had time to invent more microprojects.  I think we were
> lucky: since students didn't seem to learn from each other's attempts,
> the fact that many people tried the same microprojects wasn't much of a
> problem.  But if a student had arrived with a "perfect" solution to a
> well-trodden microproject on his/her first attempt, I would hate to have
> to be suspicious that the student plagiarized from somebody else's
> answer plus all of the review comments about the earlier answer,
> especially since a perfect solution would itself reduce the amount of
> interaction between the cheating student and the mailing list compared
> to a student who required several iterations.

It may probably be not a huge problem.  If anything, a cheater would
have also learned how the mailing interaction goes when trying to
plagiarise.  And not really having interacted us, a cheater would
have left us no impression on his or her communication skills, so it
would probably have been detrimental for his or her own chance to be
chosen as a student, compared to the ones who started from their own
solution and polished it by responding to reviews.

> I also hope that students
> didn't feel unwelcomed during the times when the list of untaken
> microprojects was nearly empty.

Yeah, that is why I said I was of multiple minds.  I wasn't enthused
by seeing the ones that somebody is attempting marked as "taken" in
the first place.  Solving one that others attempted in his or her
own way and interacting with reviewers seemed to have just as much
information on the candidate, at least to me.

> If we really wanted to make this system cheat-proof, there would have to
> be not only enough microprojects to go around, but also some mechanism
> to ensure that student B didn't start work on a microproject that
> student A was just about to submit a solution to.

Nah, I do not think there is any need to go there.  It is over for
this year anyway, but I do not think such a provision is necessary
for future years, if Git project will participate in future GSoCs;
see above.

> Maybe in the future our microprojects should have two parts:
>
> a. Fix ...
> b. Invent a microproject for the next student.
>
> (Just kidding.)

;-)

Re: [PATCH] GSoC Change multiple if-else statements to be table-driven

[Eric Sunshine]

On Mon, Mar 17, 2014 at 10:11 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 03/17/2014 08:31 AM, Junio C Hamano wrote:
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>
>>> Perhaps it is time to mark this microproject as "taken" on the GSoC
>>> page [2], along a fews others for which we have received multiple
>>> submissions.
>
> I just marked #8 as taken, as it's been beaten to death.
>
> I haven't been keeping careful track of which other microprojects have
> been overused.  If you have suggestions for the chopping block, let me know.

A quick (perhaps inaccurate) search of the mailing list shows that, of
the remaining "untaken" items, #10, 11, 12, 15, 16, and 18 have had
just one submission, and #13 had two, so we're okay. (When I wrote the
above, I was probably thinking of some of the earlier items on the
list which we saw submitted repeatedly.)

Re: [PATCH] GSoC Change multiple if-else statements to be table-driven

[Quint Guvernator]

2014-03-17 15:27 GMT-04:00 Eric Sunshine <sunshine@sunshineco.com>:
> A quick (perhaps inaccurate) search of the mailing list shows that, of
> the remaining "untaken" items, #10, 11, 12, 15, 16, and 18 have had
> just one submission, and #13 had two, so we're okay.

I am still working on #14: "Change fetch-pack.c:filter_refs() to use
starts_with() instead of memcmp(). Try to find other sites that could
be rewritten similarly."
Another version of the patch should be on this list within the hour.

Quint