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

[Eric Sunshine <sunshine@sunshineco.com>]

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