Re: [PATCH v4] install_branch_config: simplify verbose messages logic

[Eric Sunshine <sunshine@sunshineco.com>]

On Tue, Mar 11, 2014 at 8:33 PM, Paweł Wawruch <pawlo@aleg.pl> wrote:
> Replace the chain of if statements with table of strings.
>
> Signed-off-by: Paweł Wawruch <pawlo@aleg.pl>
> ---
> The changes proposed by Junio C Hamano:
> Improvement of indentations. Removed an unused variable.

Better, thanks. More below.

> [1]: http://thread.gmane.org/gmane.comp.version-control.git/243502
> [2]: http://thread.gmane.org/gmane.comp.version-control.git/243849
> [3]: http://thread.gmane.org/gmane.comp.version-control.git/243865

The [n]: notation typically is used for footnotes which you reference
in the running text as [n] (or sometimes [*n*], depending upon
preference). If you're simply listing links to previous attempts, it
would be less confusing to say:

v3: http://...
v2: http://...
v1: http://...

>  branch.c | 41 +++++++++++++++++++----------------------
>  1 file changed, 19 insertions(+), 22 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index 723a36b..2a4b911 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -53,6 +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);
> +       const char *message[][2][2] = {{{
> +               N_("Branch %s set up to track remote branch %s from %s by rebasing."),
> +               N_("Branch %s set up to track remote branch %s from %s."),
> +               },{
> +               N_("Branch %s set up to track local branch %s by rebasing."),
> +               N_("Branch %s set up to track local branch %s."),
> +               }},{{
> +               N_("Branch %s set up to track remote ref %s by rebasing."),
> +               N_("Branch %s set up to track remote ref %s."),
> +               },{
> +               N_("Branch %s set up to track local ref %s by rebasing."),
> +               N_("Branch %s set up to track local ref %s.")
> +       }}};
> +       const char *name = remote_is_branch ? remote : shortname;
>
>         if (remote_is_branch
>             && !strcmp(local, shortname)
> @@ -77,29 +91,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);
> +               if (origin && remote_is_branch)
> +                       printf_ln(_(message[!remote_is_branch][!origin][!rebasing]),
> +                               local, name, origin);
>                 else
> -                       die("BUG: impossible combination of %d and %p",
> -                           remote_is_branch, origin);
> +                       printf_ln(_(message[!remote_is_branch][!origin][!rebasing]),
> +                               local, name);

Shouldn't this logic also be encoded in the table? After all, the
point of making the code table-driven is so that such hard-coded logic
can be avoided. It shouldn't be difficult to do.

The same argument also applies to computation of the 'name' variable
above. It too can be pushed into the the table.

>         }
>  }
>
> --
> 1.8.3.2