v4install_branch_config: simplify verbose messages logic

> 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.

>  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);

>         }
>  }
>
> --
> 1.8.3.2

>> +               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.

> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>>> +               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.
>
> Hmph.  Is it even necessary in the first place?  Does it hurt if you
> give more parameters than the number of placeholders in the format
> string?

> Shouldn't this logic [to decide what the printf arguments should
> be] also be encoded in the table?
> ...
> The same argument also applies to computation of the 'name' variable
> above. It too can be pushed into the the table.

> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> Shouldn't this logic [to decide what the printf arguments should
>> be] also be encoded in the table?
>> ...
>> The same argument also applies to computation of the 'name' variable
>> above. It too can be pushed into the the table.
>
> Because the "printf argument" logic does not have to be in the
> table, the same argument does not apply to the 'name' thing.
>
> After looking at the v5 patch, I do not think an extra two-element
> array to switch between remote vs shortname is making it any easier
> to read.  I would have to say that personally I find that
>
>         const char *name[] = {remote, shortname};
>         ... long swath of code ...
>         printf_ln(... name[!remote_is_branch] ...);
>
> is a lot harder to read than:
>
>         printf_ln(... remote_is_branch ? shortname : branch ...);

> A more table-driven approach might look something
> like this:
>
>     struct M { const char *s; const char **a1; const char **a2; }
>     message[][2][2] = {{{
>         { "Branch %s set ... %s ... %s", &shortname, &origin },
>         ...
>     }},{{
>         { "Branch %s set ... %s", &remote, NULL },
>         ...
>     }}};
>
>     const struct M *m = message[!remote_is_branch][!origin][!rebasing];
>     printf_ln(m->s, local, *m->a1, *m->a2);