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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Mar 11, 2014 at 8:33 PM, Paweł Wawruch <pawlo@xxxxxxx> wrote:
> Replace the chain of if statements with table of strings.
>
> Signed-off-by: Paweł Wawruch <pawlo@xxxxxxx>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]