Re: [PATCH][GSOC2014] changed logical chain in branch.c to lookup tables
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Mar 11, 2014, 20:32 UTC
- Message-ID
- <CAPig+cSWw+3v8oCV2=kG_ASLPvg9J4S_FNg5UUFKXrFf2Lcb8w@mail.gmail.com>
- In-Reply-To
- <1394537604079-7605407.post@n2.nabble.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
Show 25 quoted lines
> Eric Sunshine wrote >> On Mon, Mar 10, 2014 at 5:47 PM, Tamer TAS < > >> tamertas@ > >> > 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
Show 10 quoted lines
> 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.
Show 12 quoted lines
> {
> 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
Show 7 quoted lines
> 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.
Show 33 quoted lines
> 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.
Show 5 quoted lines
> } > } > > -- > 1.7.9.5