Re: [PATHv2] branch.c:install_branch_config():Simplify code generating verbose message.

[Eric Sunshine <sunshine@sunshineco.com>]

Thanks for taking review comments from your previous attempt into
account. This is a more well-crafted submission. Additional comments
below.

On Tue, Mar 11, 2014 at 4:40 AM, Paweł Wawruch <pawlo@aleg.pl> wrote:
> Subject: [PATHv2] branch.c:install_branch_config():Simplify code generating verbose message.

PATCH is misspelled as PATH. Adding the v2 is indeed good etiquette.
It's typical to have a space between PATCH and vN.

Insert a space before "Simplify".

SubmittingPatches suggests omitting the final period in the subject.
Also downcase "simplify".

> Simplify the long if chain in install_branch_config().

A bit misleading. You're not actually simplifying the 'if' chain, but
rather replacing it.

> There is a long chain of if statements. The code can be more clear.
> Replace the chain with table of strings. New approach is more
> compact.

The first sentence merely repeats what was said earlier. It can be
dropped. The second sentence is self-evident since you already talk
about "simplification" and also can be dropped, as can the fourth
sentence. The third sentence is a good explanation of how the code is
being simplified and should be kept. You could, therefore, collapse
the commit message to something along these lines:

    Subject: install_branch_config: simplify verbose diagnostic logic

    Replace the 'if' chain with a table of strings.

> Signed-off-by: Paweł Wawruch <pawlo@aleg.pl>
> ---

It is considerate to reviewers to provide a link to the previous
version of the patch, like this [1], and to explain what changed in
this version relative to the last. This area just below the "---" line
after your sign off is where you would write such commentary.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/243502

>  branch.c | 40 ++++++++++++++++++----------------------
>  1 file changed, 18 insertions(+), 22 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index 723a36b..8d3b219 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -53,6 +53,18 @@ 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 *messages[] = {
> +               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.")
> +       };

On this project, arrays are usually (though not consistently) named in
singular form (for instance message[]) so that a reference to a single
item, such as message[42], reads more grammatically correct.

> +       const char *name = remote_is_branch ? remote : shortname;
> +       int message_number;
>
>         if (remote_is_branch
>             && !strcmp(local, shortname)
> @@ -77,29 +89,13 @@ 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);
> +               message_number = (!!rebasing) + 2 * (!!origin) + 4 * (!!remote_is_branch);

Unnecessary parentheses make the code more cumbersome to read and
should be dropped.

> +               assert(message_number < ARRAY_SIZE(messages));
> +
> +               if (message_number < 2)
> +                       printf_ln(messages[message_number], local, name, origin);

Even though the strings are marked for translation via N_() in the
initializer, they still need to be passed through _() when they are
actually used, so the invocation should be:

    printf_ln(_(messages[message_number]), local, name, origin);

Ditto for the printf_ln() below.

See section 4.7 of the GNU gettext manual [2] for an explanation (and
note that N_() is shorthand for gettext_noop() mentioned in the
manual).

[2]: http://www.gnu.org/software/gettext/manual/gettext.html

>                 else
> -                       die("BUG: impossible combination of %d and %p",
> -                           remote_is_branch, origin);
> +                       printf_ln(messages[message_number], local, name);
>         }

Although this approach of making message retrieval table-driven does
collapse the code a bit, it's not obvious that it is indeed more
clear. Magical incantations, such as '!!rebasing + 2 * !!origin + 4 *
!!remote_is_branch' and 'if (message_number < 2)', convey little
immediate meaning, demand a greater cognitive load, and may make the
code less straightforward to modify in the future. Have you considered
other ways of making the code table-driven which exhibit fewer
shortcomings?

>  }
>
> --
> 1.8.3.2