Re: [PATCH] branch.c: simplify chain of if statements

[Eric Sunshine <sunshine@sunshineco.com>]

Thanks for the submission. Comments below to give you a taste of the
Git review process...

On Sun, Mar 16, 2014 at 5:22 PM, Dragos Foianu <dragos.foianu@gmail.com> wrote:
> This patch uses a table-driven approach in order to make the code
> cleaner.

In fact, this change is not table-driven (emphasis on *driven*). It
merely moves the strings into a table, but all the logic is still in
the code. To be table-driven, the logic would be encoded in the table
as well, and that logic would *drive* the code.

This is not to say that the code must be table-driven. The GSoC
microproject merely asks if doing so would make sense. Whether it
would is for you to decide, and then to explain to reviewers the
reason(s) why you did or did not make it so.

> Although not necessary, it helps code reability by not
> forcing the user to read the print message when trying to
> understand what the code does.

The if-chain is just sufficiently complex that the print messages may
actually help the reader understand the logic of the code, so this
argument seems specious.

> The rebase check has been moved to
> the verbose if statement to avoid making the same check in each of
> the four if statements.
>
> Signed-off-by: Dragos Foianu <dragos.foianu@gmail.com>
> ---

Overall, the patch appears to be properly constructed and you seem to
have digested Documentation/SubmittingPatches. Good.

>  branch.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index 723a36b..e2fe455 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -54,6 +54,14 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
>         struct strbuf key = STRBUF_INIT;
>         int rebasing = should_setup_rebase(origin);
>
> +       const char *verbose_prints[4] = {

No need to hard-code 4. 'const char *verbose_prints[]' is sufficient.

On this project, it is preferred (though not consistent) to name
arrays using singular form, such as verbose_print[], so that accessing
a single element, such as verbose_print[42], reads more grammatically
correct.

verbose_prints[] is not as descriptive as it could be. Perhaps
something like verbose_messages[] would be more informative (though
awfully long; maybe just messages[]).

> +               "Branch %s set up to track remote branch %s from %s%s",

Even though you are correctly accessing these strings via _() in the
printf_ln() invocation, you still need to mark them translatable here
with N_(). See section 4.7 [1] of the gettext manual.

[1]: http://www.gnu.org/software/gettext/manual/gettext.html#Special-cases

> +               "Branch %s set up to track local branch %s%s",
> +               "Branch %s set up to track remote ref %s%s",
> +               "Branch %s set up to track local ref %s%s"
> +       };
> +       char *verbose_rebasing = rebasing ? " by rebasing." : ".";

This should be 'const char *'.

Matthieu already mentioned [2] that this sort of "lego" string
construction is not internationalization-friendly. See section 4.3 [3]
of the gettext manual for details.

[2]: http://thread.gmane.org/gmane.comp.version-control.git/244210/focus=244226
[3]: http://www.gnu.org/software/gettext/manual/gettext.html#Preparing-Strings

>         if (remote_is_branch
>             && !strcmp(local, shortname)
>             && !origin) {
> @@ -78,25 +86,17 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
>
>         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);
> +                       printf_ln(_(verbose_prints[0]),
> +                               local, shortname, origin, verbose_rebasing);
>                 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);
> +                       printf_ln(_(verbose_prints[1]),
> +                               local, shortname, verbose_rebasing);
>                 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);
> +                       printf_ln(_(verbose_prints[2]),
> +                               local, remote, verbose_rebasing);
>                 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);
> +                       printf_ln(_(verbose_prints[3]),
> +                               local, remote, verbose_rebasing);

These hard-coded indexing constants (0, 1, 2, 3) are fragile and
convey little meaning to the reader. Try to consider how to compute
the index into verbose_prints[] based upon the values of
'remote_is_branch' and 'origin'. What are the different ways you could
do so?

>                 else
>                         die("BUG: impossible combination of %d and %p",
>                             remote_is_branch, origin);
> --
> 1.8.3.2