Re: [PATCH][GSOC2014] changed logical chain in branch.c to lookup tables

[Eric Sunshine <sunshine@sunshineco.com>]

Thanks for the resubmission. Comments below.

On Tue, Mar 11, 2014 at 7:33 AM, Tamer TAS <tamertas@outlook.com> wrote:
> Subject: changed logical chain in branch.c to lookup tables

Use imperative tone: "change" rather than "changed"

Prefix the message with the part of the project you are touching. So,
for instance, you might write:

    Subject: install_branch_config: change logical chain to lookup table

> Eric Sunshine wrote
>> On Mon, Mar 10, 2014 at 5:47 PM, Tamer TAS &lt;
>
>> tamertas@
>
>> &gt; wrote:
>>
>> Section 4.3 of the GNU gettext manual [1] explains the issues in more
>> detail. I urge you to read it. The upshot is that translators fare
>> best when handed full sentences.
>>
>> Note also that your change effectively reverts d53a35032a67 [2], which
>> did away with the sort of string composition used in your patch.
>
> Eric thank you for your constructive feedbacks.
> I read the section 4.3 of GNU gettext manual and also checked the commit you
> mentioned.
> It seems like that my previous changes were not internationalization
> compatible.
> In order for a table-driven change to be compatible, the sentences has to be
> meaningful and not tokenized.
> I made the following change to the branch.c in order for the function to be
> both table-driven and
> internationalization compatible. Let me know if there are any oversights on
> my part.

This commentary is relevant to the ongoing email thread but not
suitable for the permanent commit message. Place it below the "---"
line just under your sign-off.

> Signed-off-by: TamerTas <tamertas@outlook.com>
> ---

It's considerate to reviewers to provide a link to the previous
attempt, like this [1]. This area below the "---" line is the
appropriate place to do so. Likewise, explain how this version differs
from the previous one.

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

>  branch.c |   39 ++++++++++++++++-----------------------
>  1 file changed, 16 insertions(+), 23 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index 723a36b..4c04638 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -50,10 +50,25 @@ static int should_setup_rebase(const char *origin)
>  void install_branch_config(int flag, const char *local, const char *origin,
> const char *remote)

Your patch is whitespace damaged, probably due to being pasted into
your email. "git send-email" avoids such problems.

Indentation is also broken. Use tabs for indentation, and set tab
width to 8 in your editor, as explained in
Documentation/CodingGuidelines.

>  {
>         const char *shortname = remote + 11;
> +    const char *setup_messages[] = {
> +               _("Branch %s set up to track remote branch %s from %s."),
> +               _("Branch %s set up to track local branch %s."),
> +               _("Branch %s set up to track remote ref %s."),
> +               _("Branch %s set up to track local ref %s."),
> +               _("Branch %s set up to track remote branch %s from %s by rebasing."),
> +               _("Branch %s set up to track local branch %s by rebasing."),
> +               _("Branch %s set up to track remote ref %s by rebasing."),
> +               _("Branch %s set up to track local ref %s by rebasing.")
> +       };

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

It's not correct to use _() inside the initializer list. Instead use
N_(). Read section 4.7 of the GNU gettext manual [2] for an
explanation. You will still need to use _() at the point where you
extract the message from the array.

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

>         int remote_is_branch = starts_with(remote, "refs/heads/");
>         struct strbuf key = STRBUF_INIT;
>         int rebasing = should_setup_rebase(origin);
>
> +    int msg_index = (!!origin           >> 0) +
> +                                       (!!remote_is_branch >> 1) +
> +                                       (!!rebasing         >> 2);

Are you sure this is correct? As I read it, msg_index will only ever
have a value of 0 or 1, which is unlikely what you intended.

>         if (remote_is_branch
>             && !strcmp(local, shortname)
>             && !origin) {
> @@ -77,29 +92,7 @@ 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);
> -               else
> -                       die("BUG: impossible combination of %d and %p",
> -                           remote_is_branch, origin);
> +               printf_ln(setup_messages[msg_index], local, remote);

This can't be correct.

In the original code, all of the printf_ln() invocations received
'local' as the first argument, but only two of them received 'remote',
yet you are passing 'remote' on every invocation.

Worse, the first and fifths items in your setup_messages[] table
expect three arguments (they have three %s's), but you're passing only
two, which will surely lead to a crash.

Also, wrap _() around setup_messages[msg_index], as noted above.

>         }
>  }
>
> --
> 1.7.9.5