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

[Eric Sunshine <sunshine@sunshineco.com>]

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