* [PATCH][GSOC2014] changed logical chain in branch.c to lookup tables [not found] <1394478262-17911-1-git-send-email-tamertas@outlook.com> @ 2014-03-10 19:04 ` TamerTas 2014-03-10 21:05 ` Stefan Beller 2014-03-10 21:25 ` Eric Sunshine 0 siblings, 2 replies; 7+ messages in thread From: TamerTas @ 2014-03-10 19:04 UTC (permalink / raw) To: git; +Cc: TamerTas Signed-off-by: TamerTas <tamertas@outlook.com> --- branch.c | 31 ++++++++----------------------- 1 file changed, 8 insertions(+), 23 deletions(-) diff --git a/branch.c b/branch.c index 723a36b..397edd3 100644 --- a/branch.c +++ b/branch.c @@ -50,6 +50,9 @@ static int should_setup_rebase(const char *origin) void install_branch_config(int flag, const char *local, const char *origin, const char *remote) { const char *shortname = remote + 11; + const char *location[] = {"local", "remote"}; + const char *type[] = {"branch", "ref"}; + int remote_is_branch = starts_with(remote, "refs/heads/"); struct strbuf key = STRBUF_INIT; int rebasing = should_setup_rebase(origin); @@ -77,29 +80,11 @@ 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(rebasing ? + _("Branch %s set up to track %s %s %s by rebasing.") : + _("Branch %s set up to track %s %s %s."), + local, location[!origin], type[remote_is_branch], remote); } } -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH][GSOC2014] changed logical chain in branch.c to lookup tables 2014-03-10 19:04 ` [PATCH][GSOC2014] changed logical chain in branch.c to lookup tables TamerTas @ 2014-03-10 21:05 ` Stefan Beller 2014-03-10 21:25 ` Eric Sunshine 1 sibling, 0 replies; 7+ messages in thread From: Stefan Beller @ 2014-03-10 21:05 UTC (permalink / raw) To: TamerTas, git On 10.03.2014 20:04, TamerTas wrote: > > Signed-off-by: TamerTas <tamertas@outlook.com> > --- > branch.c | 31 ++++++++----------------------- > 1 file changed, 8 insertions(+), 23 deletions(-) > > diff --git a/branch.c b/branch.c > index 723a36b..397edd3 100644 > --- a/branch.c > +++ b/branch.c > @@ -50,6 +50,9 @@ static int should_setup_rebase(const char *origin) > void install_branch_config(int flag, const char *local, const char *origin, const char *remote) > { > const char *shortname = remote + 11; > + const char *location[] = {"local", "remote"}; > + const char *type[] = {"branch", "ref"}; > + > int remote_is_branch = starts_with(remote, "refs/heads/"); > struct strbuf key = STRBUF_INIT; > int rebasing = should_setup_rebase(origin); > @@ -77,29 +80,11 @@ 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(rebasing ? > + _("Branch %s set up to track %s %s %s by rebasing.") : > + _("Branch %s set up to track %s %s %s."), > + local, location[!origin], type[remote_is_branch], remote); > } > } > > This one is neat, I'd favor this one as opposed to the one posted earlier today. Acked-by: Stefan Beller <stefanbeller@googlemail.com> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][GSOC2014] changed logical chain in branch.c to lookup tables 2014-03-10 19:04 ` [PATCH][GSOC2014] changed logical chain in branch.c to lookup tables TamerTas 2014-03-10 21:05 ` Stefan Beller @ 2014-03-10 21:25 ` Eric Sunshine 2014-03-10 21:47 ` Tamer TAS 1 sibling, 1 reply; 7+ messages in thread From: Eric Sunshine @ 2014-03-10 21:25 UTC (permalink / raw) To: TamerTas; +Cc: Git List On Mon, Mar 10, 2014 at 3:04 PM, TamerTas <tamertas@outlook.com> wrote: > Signed-off-by: TamerTas <tamertas@outlook.com> Thanks for the submission. It appears to be well executed. Read below for a concern about the approach taken. > --- > branch.c | 31 ++++++++----------------------- > 1 file changed, 8 insertions(+), 23 deletions(-) > > diff --git a/branch.c b/branch.c > index 723a36b..397edd3 100644 > --- a/branch.c > +++ b/branch.c > @@ -50,6 +50,9 @@ static int should_setup_rebase(const char *origin) > void install_branch_config(int flag, const char *local, const char *origin, const char *remote) > { > const char *shortname = remote + 11; > + const char *location[] = {"local", "remote"}; > + const char *type[] = {"branch", "ref"}; > + > int remote_is_branch = starts_with(remote, "refs/heads/"); > struct strbuf key = STRBUF_INIT; > int rebasing = should_setup_rebase(origin); > @@ -77,29 +80,11 @@ 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(rebasing ? > + _("Branch %s set up to track %s %s %s by rebasing.") : > + _("Branch %s set up to track %s %s %s."), > + local, location[!origin], type[remote_is_branch], remote); The GSoC microproject talks about the _() function used for internationalization and the limitations it places on string composition. This change assumes, probably incorrectly, that strings "local", "remote", "branch" and "ref" do not need to be localized. Even allowing internationalization of them (via N_() in the location[] and type[] tables) might not be sufficient since grammatical rules differ from language to language. > } > } > > -- > 1.7.9.5 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][GSOC2014] changed logical chain in branch.c to lookup tables 2014-03-10 21:25 ` Eric Sunshine @ 2014-03-10 21:47 ` Tamer TAS 2014-03-10 22:39 ` Eric Sunshine 0 siblings, 1 reply; 7+ messages in thread From: Tamer TAS @ 2014-03-10 21:47 UTC (permalink / raw) To: git Eric Sunshine wrote > Even allowing internationalization of them (via N_() in the location[] > and type[] tables) might not be sufficient since grammatical rules > differ from language to language. I didn't fully understand what you meant by that. Since they were being internationalized before using _() in the if-else bodies, wouldn't it produce the same output if I were to use the same method for location[] and type[] tables? -- View this message in context: http://git.661346.n2.nabble.com/PATCH-GSOC2014-changed-logical-chain-in-branch-c-to-lookup-tables-tp7605343p7605372.html Sent from the git mailing list archive at Nabble.com. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][GSOC2014] changed logical chain in branch.c to lookup tables 2014-03-10 21:47 ` Tamer TAS @ 2014-03-10 22:39 ` Eric Sunshine 2014-03-11 11:33 ` Tamer TAS 0 siblings, 1 reply; 7+ messages in thread From: Eric Sunshine @ 2014-03-10 22:39 UTC (permalink / raw) To: Tamer TAS; +Cc: Git List On Mon, Mar 10, 2014 at 5:47 PM, Tamer TAS <tamertas@outlook.com> wrote: > Eric Sunshine wrote >> Even allowing internationalization of them (via N_() in the location[] >> and type[] tables) might not be sufficient since grammatical rules >> differ from language to language. > > I didn't fully understand what you meant by that. Since they were being > internationalized before using _() in the if-else bodies, wouldn't it > produce > the same output if I were to use the same method for location[] and type[] > tables? In the original code, the person translating the text has the full context: "Branch %s set up to track remote ref %s." With your revision, the translator has to translate standalone words "local", "remote", "branch", "ref" without context about how they are used. He then has to translate: "Branch %s set up to track %s %s %s." without context of the words being inserted, which, depending upon grammatical rules of the language can result in a different (and perhaps worse) translation than the original full-context translation. 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. [1]: http://www.gnu.org/software/gettext/manual/gettext.html#Preparing-Strings [2]: https://github.com/git/git/commit/d53a35032a67fde5b59c8a6a66e0466837cbaf1e ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][GSOC2014] changed logical chain in branch.c to lookup tables 2014-03-10 22:39 ` Eric Sunshine @ 2014-03-11 11:33 ` Tamer TAS 2014-03-11 20:32 ` Eric Sunshine 0 siblings, 1 reply; 7+ messages in thread From: Tamer TAS @ 2014-03-11 11:33 UTC (permalink / raw) To: git 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. Signed-off-by: TamerTas <tamertas@outlook.com> --- 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) { 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.") + }; + 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); + 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); } } -- 1.7.9.5 -- View this message in context: http://git.661346.n2.nabble.com/PATCH-GSOC2014-changed-logical-chain-in-branch-c-to-lookup-tables-tp7605343p7605407.html Sent from the git mailing list archive at Nabble.com. ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH][GSOC2014] changed logical chain in branch.c to lookup tables 2014-03-11 11:33 ` Tamer TAS @ 2014-03-11 20:32 ` Eric Sunshine 0 siblings, 0 replies; 7+ messages in thread From: Eric Sunshine @ 2014-03-11 20:32 UTC (permalink / raw) To: Tamer TAS; +Cc: Git List 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 < > >> 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 > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-03-11 20:32 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <1394478262-17911-1-git-send-email-tamertas@outlook.com> 2014-03-10 19:04 ` [PATCH][GSOC2014] changed logical chain in branch.c to lookup tables TamerTas 2014-03-10 21:05 ` Stefan Beller 2014-03-10 21:25 ` Eric Sunshine 2014-03-10 21:47 ` Tamer TAS 2014-03-10 22:39 ` Eric Sunshine 2014-03-11 11:33 ` Tamer TAS 2014-03-11 20:32 ` Eric Sunshine
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).