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

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

[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

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

[Stefan Beller]

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>

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

[Eric Sunshine]

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

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

[Tamer TAS]

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.

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

[Eric Sunshine]

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

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

[Tamer TAS]

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.

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

[Eric Sunshine]

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