threads / patch / 45828

patchi18n: remove i18n from tag reflog message

Subject: [PATCH] i18n: remove i18n from tag reflog message

## tl;dr

9 messages between Apr 29, 2017 and May 4, 2017. Diffs are folded; open one to read it.

replies: 8people: 3as markdown or json

Jean-Noel Avila· Apr 29, 2017, 10:02 UTC · lore

The building of the reflog message is using strbuf, which is not friendly with internationalization frameworks. No other reflog messages are translated right now and switching all the messages to i18n would require a major rework of the way the messages are built.

Signed-off-by: Jean-Noel Avila <jn.avila@free.fr>
---
 builtin/tag.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Show changes to builtin/tag.c +6 −6

diff --git a/builtin/tag.c b/builtin/tag.c
index 222404522..bdf1e88e9 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -309,7 +309,7 @@ static void create_reflog_msg(const unsigned char *sha1, struct strbuf *sb)
 	if (rla) {
 		strbuf_addstr(sb, rla);
 	} else {
-		strbuf_addstr(sb, _("tag: tagging "));
+		strbuf_addstr(sb, "tag: tagging ");
 		strbuf_add_unique_abbrev(sb, sha1, DEFAULT_ABBREV);
 	}
 
@@ -317,14 +317,14 @@ static void create_reflog_msg(const unsigned char *sha1, struct strbuf *sb)
 	type = sha1_object_info(sha1, NULL);
 	switch (type) {
 	default:
-		strbuf_addstr(sb, _("object of unknown type"));
+		strbuf_addstr(sb, "object of unknown type");
 		break;
 	case OBJ_COMMIT:
 		if ((buf = read_sha1_file(sha1, &type, &size)) != NULL) {
 			subject_len = find_commit_subject(buf, &subject_start);
 			strbuf_insert(sb, sb->len, subject_start, subject_len);
 		} else {
-			strbuf_addstr(sb, _("commit object"));
+			strbuf_addstr(sb, "commit object");
 		}
 		free(buf);
 
@@ -332,13 +332,13 @@ static void create_reflog_msg(const unsigned char *sha1, struct strbuf *sb)
 			strbuf_addf(sb, ", %s", show_date(c->date, 0, DATE_MODE(SHORT)));
 		break;
 	case OBJ_TREE:
-		strbuf_addstr(sb, _("tree object"));
+		strbuf_addstr(sb, "tree object");
 		break;
 	case OBJ_BLOB:
-		strbuf_addstr(sb, _("blob object"));
+		strbuf_addstr(sb, "blob object");
 		break;
 	case OBJ_TAG:
-		strbuf_addstr(sb, _("other tag object"));
+		strbuf_addstr(sb, "other tag object");
 		break;
 	}
 	strbuf_addch(sb, ')');

-- 
2.12.0

Jean-Noel Avila· Apr 30, 2017, 21:32 UTC · re: Jean-Noel Avila · lore

[PATCH v2] i18n patches to apply for rc2

Please apply the following patches for rc2 so that the localization can be applied on a cleaned up pot file.

Jean-Noel Avila· Apr 30, 2017, 21:32 UTC · re: Jean-Noel Avila · lore

[PATCH v2 1/2] i18n: remove i18n from tag reflog message

Signed-off-by: Jean-Noel Avila <jn.avila@free.fr>
---
 builtin/tag.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Show changes to builtin/tag.c +6 −6

diff --git a/builtin/tag.c b/builtin/tag.c
index 222404522..bdf1e88e9 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -309,7 +309,7 @@ static void create_reflog_msg(const unsigned char *sha1, struct strbuf *sb)
 	if (rla) {
 		strbuf_addstr(sb, rla);
 	} else {
-		strbuf_addstr(sb, _("tag: tagging "));
+		strbuf_addstr(sb, "tag: tagging ");
 		strbuf_add_unique_abbrev(sb, sha1, DEFAULT_ABBREV);
 	}
 
@@ -317,14 +317,14 @@ static void create_reflog_msg(const unsigned char *sha1, struct strbuf *sb)
 	type = sha1_object_info(sha1, NULL);
 	switch (type) {
 	default:
-		strbuf_addstr(sb, _("object of unknown type"));
+		strbuf_addstr(sb, "object of unknown type");
 		break;
 	case OBJ_COMMIT:
 		if ((buf = read_sha1_file(sha1, &type, &size)) != NULL) {
 			subject_len = find_commit_subject(buf, &subject_start);
 			strbuf_insert(sb, sb->len, subject_start, subject_len);
 		} else {
-			strbuf_addstr(sb, _("commit object"));
+			strbuf_addstr(sb, "commit object");
 		}
 		free(buf);
 
@@ -332,13 +332,13 @@ static void create_reflog_msg(const unsigned char *sha1, struct strbuf *sb)
 			strbuf_addf(sb, ", %s", show_date(c->date, 0, DATE_MODE(SHORT)));
 		break;
 	case OBJ_TREE:
-		strbuf_addstr(sb, _("tree object"));
+		strbuf_addstr(sb, "tree object");
 		break;
 	case OBJ_BLOB:
-		strbuf_addstr(sb, _("blob object"));
+		strbuf_addstr(sb, "blob object");
 		break;
 	case OBJ_TAG:
-		strbuf_addstr(sb, _("other tag object"));
+		strbuf_addstr(sb, "other tag object");
 		break;
 	}
 	strbuf_addch(sb, ')');

-- 
2.12.0

Jean-Noel Avila· Apr 30, 2017, 21:32 UTC · re: Jean-Noel Avila · lore

[PATCH v2 2/2] i18n: read-cache: Fix typo

From: Peter Krefting <peter@softwolves.pp.se>

Signed-off-by: Peter Krefting <peter@softwolves.pp.se>
Signed-off-by: Jean-Noel Avila <jn.avila@free.fr>
---
 read-cache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Show changes to read-cache.c +1 −1

diff --git a/read-cache.c b/read-cache.c
index b3d0f3c30..0d0081a11 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2372,7 +2372,7 @@ static int should_delete_shared_index(const char *shared_index_path)
 	if (!expiration)
 		return 0;
 	if (stat(shared_index_path, &st))
-		return error_errno(_("could not stat '%s"), shared_index_path);
+		return error_errno(_("could not stat '%s'"), shared_index_path);
 	if (st.st_mtime > expiration)
 		return 0;

-- 
2.12.0

Junio C Hamano· May 1, 2017, 01:45 UTC · re: Jean-Noel Avila · lore

Re: [PATCH] i18n: remove i18n from tag reflog message

Jean-Noel Avila <jn.avila@free.fr> writes:

> The building of the reflog message is using strbuf, which is not
> friendly with internationalization frameworks. No other reflog
> messages are translated right now and switching all the messages to
> i18n would require a major rework of the way the messages are built.

Thanks for spotting. Before we declare that reflog messages are for i18n, we'd need to either drop (or redesign the implementation of) the "checkout -" feature, which relies on the exact phrasing of how reflog entries from "git checkout" looks like.

Will queue and merge down to 'master' quickly.

Show 53 quoted lines

>
> Signed-off-by: Jean-Noel Avila <jn.avila@free.fr>
> ---
>  builtin/tag.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 222404522..bdf1e88e9 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -309,7 +309,7 @@ static void create_reflog_msg(const unsigned char *sha1, struct strbuf *sb)
>  	if (rla) {
>  		strbuf_addstr(sb, rla);
>  	} else {
> -		strbuf_addstr(sb, _("tag: tagging "));
> +		strbuf_addstr(sb, "tag: tagging ");
>  		strbuf_add_unique_abbrev(sb, sha1, DEFAULT_ABBREV);
>  	}
>  
> @@ -317,14 +317,14 @@ static void create_reflog_msg(const unsigned char *sha1, struct strbuf *sb)
>  	type = sha1_object_info(sha1, NULL);
>  	switch (type) {
>  	default:
> -		strbuf_addstr(sb, _("object of unknown type"));
> +		strbuf_addstr(sb, "object of unknown type");
>  		break;
>  	case OBJ_COMMIT:
>  		if ((buf = read_sha1_file(sha1, &type, &size)) != NULL) {
>  			subject_len = find_commit_subject(buf, &subject_start);
>  			strbuf_insert(sb, sb->len, subject_start, subject_len);
>  		} else {
> -			strbuf_addstr(sb, _("commit object"));
> +			strbuf_addstr(sb, "commit object");
>  		}
>  		free(buf);
>  
> @@ -332,13 +332,13 @@ static void create_reflog_msg(const unsigned char *sha1, struct strbuf *sb)
>  			strbuf_addf(sb, ", %s", show_date(c->date, 0, DATE_MODE(SHORT)));
>  		break;
>  	case OBJ_TREE:
> -		strbuf_addstr(sb, _("tree object"));
> +		strbuf_addstr(sb, "tree object");
>  		break;
>  	case OBJ_BLOB:
> -		strbuf_addstr(sb, _("blob object"));
> +		strbuf_addstr(sb, "blob object");
>  		break;
>  	case OBJ_TAG:
> -		strbuf_addstr(sb, _("other tag object"));
> +		strbuf_addstr(sb, "other tag object");
>  		break;
>  	}
>  	strbuf_addch(sb, ')');

Jean-Noël AVILA· May 3, 2017, 16:38 UTC · re: Junio C Hamano · lore

Re: [PATCH] i18n: remove i18n from tag reflog message

Le dimanche 30 avril 2017, 18:45:05 CEST Junio C Hamano a écrit :

Show 13 quoted lines

> Jean-Noel Avila <jn.avila@free.fr> writes:
> > The building of the reflog message is using strbuf, which is not
> > friendly with internationalization frameworks. No other reflog
> > messages are translated right now and switching all the messages to
> > i18n would require a major rework of the way the messages are built.
> 
> Thanks for spotting.  Before we declare that reflog messages are for
> i18n, we'd need to either drop (or redesign the implementation of)
> the "checkout -" feature, which relies on the exact phrasing of how
> reflog entries from "git checkout" looks like.
> 
> Will queue and merge down to 'master' quickly.
>

I didn't know this "side effect". Maybe adding a test against it would be requiered. Unfortunately, I don't know enough of it.

Junio C Hamano· May 4, 2017, 01:35 UTC · re: Jean-Noël AVILA · lore

Re: [PATCH] i18n: remove i18n from tag reflog message

Jean-Noël AVILA <jn.avila@free.fr> writes:

Show 17 quoted lines

> Le dimanche 30 avril 2017, 18:45:05 CEST Junio C Hamano a écrit :
>> Jean-Noel Avila <jn.avila@free.fr> writes:
>> > The building of the reflog message is using strbuf, which is not
>> > friendly with internationalization frameworks. No other reflog
>> > messages are translated right now and switching all the messages to
>> > i18n would require a major rework of the way the messages are built.
>> 
>> Thanks for spotting.  Before we declare that reflog messages are for
>> i18n, we'd need to either drop (or redesign the implementation of)
>> the "checkout -" feature, which relies on the exact phrasing of how
>> reflog entries from "git checkout" looks like.
>> 
>> Will queue and merge down to 'master' quickly.
>> 
>
> I didn't know this "side effect". Maybe adding a test against it would be 
> requiered. Unfortunately, I don't know enough of it.

Sorry, but I am not sure what "side effect" you are referring to. I am saying that in the current codebase not translating tag reflog message (i.e. your fix) is the right thing to do.

builtin/checkout.c does

		strbuf_addf(&msg, "checkout: moving from %s to %s",
			old_desc ? old_desc : "(invalid)", new->name);

when "git checkout" is used to switch from old to new branch. The above phrasing is used by sha1_name.c::grab_nth_branch_switch() to know what branch you were on, and not having _() around the message is the right thing to do, at least in the current codebase.

If somebody wants to i18n the reflog message, that needs to be done at the reading/dispaying end, I would think. Instead of being purely textual records, a new reflog format would have to be a machine parsable message-id with parameters, and the code that wants to understand the reflog record like grab_nth_branch_switch(), as opposed to show the record to the humans, would just use the machine parsable raw data, while "git reflog" and friends that format the reflog record would give the message-id to gettext and feed the parameters recorded using the result of gettext() as the format string to fprintf(), or something like that.

Junio C Hamano· May 4, 2017, 01:38 UTC · re: Jean-Noël AVILA · lore

Re: [PATCH] i18n: remove i18n from tag reflog message

Jean-Noël AVILA <jn.avila@free.fr> writes:

Show 17 quoted lines

> Le dimanche 30 avril 2017, 18:45:05 CEST Junio C Hamano a écrit :
>> Jean-Noel Avila <jn.avila@free.fr> writes:
>> > The building of the reflog message is using strbuf, which is not
>> > friendly with internationalization frameworks. No other reflog
>> > messages are translated right now and switching all the messages to
>> > i18n would require a major rework of the way the messages are built.
>> 
>> Thanks for spotting.  Before we declare that reflog messages are for
>> i18n, we'd need to either drop (or redesign the implementation of)
>> the "checkout -" feature, which relies on the exact phrasing of how
>> reflog entries from "git checkout" looks like.
>> 
>> Will queue and merge down to 'master' quickly.
>> 
>
> I didn't know this "side effect". Maybe adding a test against it would be 
> requiered. Unfortunately, I don't know enough of it.

Sorry, but I am not sure what "side effect" you are referring to. I am saying that in the current codebase not translating tag reflog message (i.e. your fix) is the right thing to do.

builtin/checkout.c does

		strbuf_addf(&msg, "checkout: moving from %s to %s",
			old_desc ? old_desc : "(invalid)", new->name);

That would solve the issue in multi-user repository Ævar raised, too. You'd see Chinese reflog entry not because the person who did the operation that created the reflog entry were writing Chinese, but because your locale is set to show Chinese when you tried to read the reflog. German users would see the reflog entry that records what the Chinese developer did in German if we follow that route.

Ævar Arnfjörð Bjarmason· May 1, 2017, 09:15 UTC · re: Jean-Noel Avila · lore

Re: [PATCH] i18n: remove i18n from tag reflog message

On Sat, Apr 29, 2017 at 12:02 PM, Jean-Noel Avila <jn.avila@free.fr> wrote:

> The building of the reflog message is using strbuf, which is not
> friendly with internationalization frameworks. No other reflog
> messages are translated right now and switching all the messages to
> i18n would require a major rework of the way the messages are built.

[CC'd Cornelius Weig who added the code you're modifying]

If I'm reading this correctly this is a good patch but quite a bad explanation of what we're changing.

The problem here is not that it's cumbersome to translate this using strbuf, but that we're injecting i18n'd messages into the reflog, so e.g. someone with a Chinese locale creating a tag in a shared repo will cause the Chinese i18n message to be inserted into the reflog, and then someone with e.g. a German or English locale will see the Chinese messages when they run "git reflog".

That's a very bad thing, and in some ways worse than translating plumbing messages, because here we're injecting i18n'd messages into the on-disk format.

Luckily this hasn't made it into 2.13 yet. Junio, if the above description is accurate then this is something we definitely want in the final 2.13.

Show 55 quoted lines

> Signed-off-by: Jean-Noel Avila <jn.avila@free.fr>
> ---
>  builtin/tag.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 222404522..bdf1e88e9 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -309,7 +309,7 @@ static void create_reflog_msg(const unsigned char *sha1, struct strbuf *sb)
>         if (rla) {
>                 strbuf_addstr(sb, rla);
>         } else {
> -               strbuf_addstr(sb, _("tag: tagging "));
> +               strbuf_addstr(sb, "tag: tagging ");
>                 strbuf_add_unique_abbrev(sb, sha1, DEFAULT_ABBREV);
>         }
>
> @@ -317,14 +317,14 @@ static void create_reflog_msg(const unsigned char *sha1, struct strbuf *sb)
>         type = sha1_object_info(sha1, NULL);
>         switch (type) {
>         default:
> -               strbuf_addstr(sb, _("object of unknown type"));
> +               strbuf_addstr(sb, "object of unknown type");
>                 break;
>         case OBJ_COMMIT:
>                 if ((buf = read_sha1_file(sha1, &type, &size)) != NULL) {
>                         subject_len = find_commit_subject(buf, &subject_start);
>                         strbuf_insert(sb, sb->len, subject_start, subject_len);
>                 } else {
> -                       strbuf_addstr(sb, _("commit object"));
> +                       strbuf_addstr(sb, "commit object");
>                 }
>                 free(buf);
>
> @@ -332,13 +332,13 @@ static void create_reflog_msg(const unsigned char *sha1, struct strbuf *sb)
>                         strbuf_addf(sb, ", %s", show_date(c->date, 0, DATE_MODE(SHORT)));
>                 break;
>         case OBJ_TREE:
> -               strbuf_addstr(sb, _("tree object"));
> +               strbuf_addstr(sb, "tree object");
>                 break;
>         case OBJ_BLOB:
> -               strbuf_addstr(sb, _("blob object"));
> +               strbuf_addstr(sb, "blob object");
>                 break;
>         case OBJ_TAG:
> -               strbuf_addstr(sb, _("other tag object"));
> +               strbuf_addstr(sb, "other tag object");
>                 break;
>         }
>         strbuf_addch(sb, ')');
> --
> 2.12.0
>