threads / rfc / 58298

RFC patchsequencer: do not translate reflog messages

Subject: [RFC/PATCH] sequencer: do not translate reflog messages

## tl;dr

34 messages between Aug 12, 2022 and Aug 22, 2022. Diffs are folded; open one to read it.

replies: 33people: 6as markdown or json

Michael J Gruber· Aug 12, 2022, 15:38 UTC · lore

Traditionally, reflog messages were never translated, in particular not on storage.

Due to the switch of more parts of git to the sequencer, old changes in the sequencer code may lead to recent changes in git's behaviour. E.g.: c28cbc5ea6 ("sequencer: mark action_name() for translation", 2016-10-21) marked several uses of `action_name()` for translation. Recently, this lead to a partially translated reflog:

`rebase: fast-forward` is translated (e.g. in de to `Rebase: Vorspulen`) whereas other reflog entries such as `rebase (pick):` remain untranslated as they should be.

Change the relevant line in the sequencer so that this reflog entry remains untranslated, as well.

Signed-off-by: Michael J Gruber <git@grubix.eu>
---
The patch also changes `action_name()` not to translate the names. This
makes no difference for `rebase: fast-forward` (I don't quite grok why
so far) but in any case, the callers mark the result of `action_name()`
(or do not mark it) so that the result itself should not be translated.
The full test suite passes either way.
RFC for my lack of full grasp of the relevant code paths.
 sequencer.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Show changes to sequencer.c +4 −4
diff --git a/sequencer.c b/sequencer.c
index 5f22b7cd37..b456489590 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -395,11 +395,11 @@ static const char *action_name(const struct replay_opts *opts)
 {
 	switch (opts->action) {
 	case REPLAY_REVERT:
-		return N_("revert");
+		return "revert";
 	case REPLAY_PICK:
-		return N_("cherry-pick");
+		return "cherry-pick";
 	case REPLAY_INTERACTIVE_REBASE:
-		return N_("rebase");
+		return "rebase";
 	}
 	die(_("unknown action: %d"), opts->action);
 }
@@ -575,7 +575,7 @@ static int fast_forward_to(struct repository *r,
 	if (checkout_fast_forward(r, from, to, 1))
 		return -1; /* the callee should have complained already */
 
-	strbuf_addf(&sb, _("%s: fast-forward"), _(action_name(opts)));
+	strbuf_addf(&sb, "%s: fast-forward", action_name(opts));
 
 	transaction = ref_transaction_begin(&err);
 	if (!transaction ||

-- 
2.37.1.671.g27f8e4a42a
Junio C Hamano· Aug 12, 2022, 17:21 UTC · re: Michael J Gruber · lore

Re: [RFC/PATCH] sequencer: do not translate reflog messages

Michael J Gruber <git@grubix.eu> writes:
> Traditionally, reflog messages were never translated, in particular not
> on storage.

True, and it must (unfortunately) stay to be the way, because tools (like @{-<n>} syntax) expect to be able to parse out what we write.

Show 12 quoted lines
> Due to the switch of more parts of git to the sequencer, old changes in
> the sequencer code may lead to recent changes in git's behaviour. E.g.:
> c28cbc5ea6 ("sequencer: mark action_name() for translation", 2016-10-21)
> marked several uses of `action_name()` for translation. Recently, this
> lead to a partially translated reflog:
>
> `rebase: fast-forward` is translated (e.g. in de to `Rebase: Vorspulen`)
> whereas other reflog entries such as `rebase (pick):` remain
> untranslated as they should be.
>
> Change the relevant line in the sequencer so that this reflog entry
> remains untranslated, as well.
Good move, I would have to say X-<.

In the longer term, we need to transition to a new version of reflog message, where "git reflog" output can (meaning: with an option) or does (meaning: by default) show localized message, but the internal machinery as well as scripts can ask to see an untranslated message.

We would need to teach the reflog machinery to understand a reflog message specially formatted (e.g. with an unusual prefix like "::v2::"), from which both untranslated and translated messages can be parsed out or generated. Codepaths that write reflog messages may need to be adjusted to send both versions to the ref machinery.

Looking at recent reflog entries I happen to have in "git reflog --format="%gs" HEAD@{now}"

    checkout: moving from 219fe53025fdf5c3fb79d289a36eb2cad3f38a04 to master
    checkout: moving from master to next^0
    commit (amend): fsmonitor: option to allow fsmonitor to run against network-mounted repos
    checkout: moving from d5eaf969c17c196268d9db7af50f6767ec3a3d0a to ed/fsmonitor-on-network-disk
    am: fsmonitor: option to allow fsmonitor to run against network-mounted repos
    merge @{-1}: Merge made by the 'ort' strategy.
    checkout: moving from ll/disk-usage-humanise to seen
    am: rev-list: support human-readable output for `--disk-usage`
    checkout: moving from master to ll/disk-usage-humanise

one relatively easy way to do so may be to store the printf-like format string, possibly limiting to %s and nothing else, e.g.

    "checkout: moving from %s to %s"
    "am: %s"
    "merge %s: Merge made by the '%s' strategy"

together with the parameters to fill in these %s blanks, as a N-tuple of strings, i.e.

    ("checkout: moving from %s to %s",
     "219fe53025fdf5c3fb79d289a36eb2cad3f38a04", "master")

and then serialize them into a single long string (with that special prefix to allow us notice the format).

But I'll leave the details of how the new format can be made to allow storing raw and translated messages. The review thread of this patch is not a good place or time to discuss it.

Thanks.
Phillip Wood· Aug 12, 2022, 19:21 UTC · re: Michael J Gruber · lore

Re: [RFC/PATCH] sequencer: do not translate reflog messages

Hi Michael
On 12/08/2022 16:38, Michael J Gruber wrote:
Show 44 quoted lines
> Traditionally, reflog messages were never translated, in particular not
> on storage.
> 
> Due to the switch of more parts of git to the sequencer, old changes in
> the sequencer code may lead to recent changes in git's behaviour. E.g.:
> c28cbc5ea6 ("sequencer: mark action_name() for translation", 2016-10-21)
> marked several uses of `action_name()` for translation. Recently, this
> lead to a partially translated reflog:
> 
> `rebase: fast-forward` is translated (e.g. in de to `Rebase: Vorspulen`)
> whereas other reflog entries such as `rebase (pick):` remain
> untranslated as they should be.
> 
> Change the relevant line in the sequencer so that this reflog entry
> remains untranslated, as well.
> 
> Signed-off-by: Michael J Gruber <git@grubix.eu>
> ---
> The patch also changes `action_name()` not to translate the names This > makes no difference for `rebase: fast-forward` (I don't quite grok why
> so far) but in any case, the callers mark the result of `action_name()`
> (or do not mark it) so that the result itself should not be translated.
> The full test suite passes either way.
> 
> RFC for my lack of full grasp of the relevant code paths.
> 
>   sequencer.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 5f22b7cd37..b456489590 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -395,11 +395,11 @@ static const char *action_name(const struct replay_opts *opts)
>   {
>   	switch (opts->action) {
>   	case REPLAY_REVERT:
> -		return N_("revert");
> +		return "revert";
>   	case REPLAY_PICK:
> -		return N_("cherry-pick");
> +		return "cherry-pick";
>   	case REPLAY_INTERACTIVE_REBASE:
> -		return N_("rebase");
> +		return "rebase";

Removing the N_() stops these strings from being extracted for translation, but there are several callers left that are still using _() to get the (now non-existent) translated string. I only had a quick look but I think we should remove the _() from all the callers of action_name().

Best Wishes
Phillip
Show 12 quoted lines
>   	}
>   	die(_("unknown action: %d"), opts->action);
>   }
> @@ -575,7 +575,7 @@ static int fast_forward_to(struct repository *r,
>   	if (checkout_fast_forward(r, from, to, 1))
>   		return -1; /* the callee should have complained already */
>   
> -	strbuf_addf(&sb, _("%s: fast-forward"), _(action_name(opts)));
> +	strbuf_addf(&sb, "%s: fast-forward", action_name(opts));
>   
>   	transaction = ref_transaction_begin(&err);
>   	if (!transaction ||
Junio C Hamano· Aug 12, 2022, 20:43 UTC · re: Phillip Wood · lore

Re: [RFC/PATCH] sequencer: do not translate reflog messages

Phillip Wood <phillip.wood123@gmail.com> writes:
Show 5 quoted lines
> Removing the N_() stops these strings from being extracted for
> translation, but there are several callers left that are still using
> _() to get the (now non-existent) translated string. I only had a
> quick look but I think we should remove the _() from all the callers
> of action_name().
Thanks, that's all correct.
Johannes Schindelin· Aug 15, 2022, 20:20 UTC · re: Junio C Hamano · lore

Re: [RFC/PATCH] sequencer: do not translate reflog messages

Hi Junio,
On Fri, 12 Aug 2022, Junio C Hamano wrote:
Show 9 quoted lines
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
> > Removing the N_() stops these strings from being extracted for
> > translation, but there are several callers left that are still using
> > _() to get the (now non-existent) translated string. I only had a
> > quick look but I think we should remove the _() from all the callers
> > of action_name().
>
> Thanks, that's all correct.
I am afraid that it is not.

In https://github.com/git/git/blob/v2.37.2/sequencer.c#L502-L503, for example, we use the value returned by `action_name()` in a translated message:

	error(_("your local changes would be overwritten by %s."),
		_(action_name(opts)));
Michael, I am afraid that we need more nuance here.

I do see that https://github.com/git/git/blob/v2.37.2/sequencer.c#L4316 calls `action_name()` without wrapping it in `_(...)`:

	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);

This suggests to me that the proper solution will be to carefully vet which `_(action_name())` calls should drop the `_(...)` and which ones should not, and to leave the `N_(...)` parts alone.

The affected calls seem to fall into these categories:
- reflog (do _not_ translate the action name)
- parameter of `error_resolve_conflict()` (do _not_ translate the
  parameter)
- error messages talking about `git <command>` (do _not_ translate the
  action name)
- error messages talking about the operation (_do_ translate the action
  name)
My take on which lines need to be patched:
- https://github.com/git/git/blob/v2.37.2/sequencer.c#L500
- https://github.com/git/git/blob/v2.37.2/sequencer.c#L538
- https://github.com/git/git/blob/v2.37.2/sequencer.c#L2384
- https://github.com/git/git/blob/v2.37.2/sequencer.c#L2392
- https://github.com/git/git/blob/v2.37.2/sequencer.c#L3715
but not
- https://github.com/git/git/blob/v2.37.2/sequencer.c#L503
- https://github.com/git/git/blob/v2.37.2/sequencer.c#L689

Ciao, Dscho

Phillip Wood· Aug 16, 2022, 08:59 UTC · re: Johannes Schindelin · lore

Re: [RFC/PATCH] sequencer: do not translate reflog messages

Hi Dscho
On 15/08/2022 21:20, Johannes Schindelin wrote:
Show 22 quoted lines
> Hi Junio,
> 
> On Fri, 12 Aug 2022, Junio C Hamano wrote:
> 
>> Phillip Wood <phillip.wood123@gmail.com> writes:
>>
>>> Removing the N_() stops these strings from being extracted for
>>> translation, but there are several callers left that are still using
>>> _() to get the (now non-existent) translated string. I only had a
>>> quick look but I think we should remove the _() from all the callers
>>> of action_name().
>>
>> Thanks, that's all correct.
> 
> I am afraid that it is not.
> 
> In https://github.com/git/git/blob/v2.37.2/sequencer.c#L502-L503, for
> example, we use the value returned by `action_name()` in a translated
> message:
> 
> 	error(_("your local changes would be overwritten by %s."),
> 		_(action_name(opts)));

Isn't this message using action_name() to get the name of the command that the user ran? As that name is not localized when the user runs the command I don't see that we should be translating it (and playing sentence lego with the result) in this message. I think the same applies to the message at line 689 that you mention below.

Best Wishes
Phillip
Show 39 quoted lines
> Michael, I am afraid that we need more nuance here.
> 
> I do see that https://github.com/git/git/blob/v2.37.2/sequencer.c#L4316
> calls `action_name()` without wrapping it in `_(...)`:
> 
> 	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
> 
> This suggests to me that the proper solution will be to carefully vet
> which `_(action_name())` calls should drop the `_(...)` and which ones
> should not, and to leave the `N_(...)` parts alone.
> 
> The affected calls seem to fall into these categories:
> 
> - reflog (do _not_ translate the action name)
> 
> - parameter of `error_resolve_conflict()` (do _not_ translate the
>    parameter)
> 
> - error messages talking about `git <command>` (do _not_ translate the
>    action name)
> 
> - error messages talking about the operation (_do_ translate the action
>    name)
> 
> My take on which lines need to be patched:
> 
> - https://github.com/git/git/blob/v2.37.2/sequencer.c#L500
> - https://github.com/git/git/blob/v2.37.2/sequencer.c#L538
> - https://github.com/git/git/blob/v2.37.2/sequencer.c#L2384
> - https://github.com/git/git/blob/v2.37.2/sequencer.c#L2392
> - https://github.com/git/git/blob/v2.37.2/sequencer.c#L3715
> 
> but not
> 
> - https://github.com/git/git/blob/v2.37.2/sequencer.c#L503
> - https://github.com/git/git/blob/v2.37.2/sequencer.c#L689
> 
> Ciao,
> Dscho
Johannes Schindelin· Aug 16, 2022, 11:02 UTC · re: Phillip Wood · lore

Re: [RFC/PATCH] sequencer: do not translate reflog messages

Hi Phillip,
On Tue, 16 Aug 2022, Phillip Wood wrote:
Show 28 quoted lines
> On 15/08/2022 21:20, Johannes Schindelin wrote:
>
> > On Fri, 12 Aug 2022, Junio C Hamano wrote:
> >
> > > Phillip Wood <phillip.wood123@gmail.com> writes:
> > >
> > > > Removing the N_() stops these strings from being extracted for
> > > > translation, but there are several callers left that are still using
> > > > _() to get the (now non-existent) translated string. I only had a
> > > > quick look but I think we should remove the _() from all the callers
> > > > of action_name().
> > >
> > > Thanks, that's all correct.
> >
> > I am afraid that it is not.
> >
> > In https://github.com/git/git/blob/v2.37.2/sequencer.c#L502-L503, for
> > example, we use the value returned by `action_name()` in a translated
> > message:
> >
> >  error(_("your local changes would be overwritten by %s."),
> >   _(action_name(opts)));
>
> Isn't this message using action_name() to get the name of the command that the
> user ran? As that name is not localized when the user runs the command I don't
> see that we should be translating it (and playing sentence lego with the
> result) in this message. I think the same applies to the message at line 689
> that you mention below.

I do not believe that this error message talks about the command, otherwise it would use "`git %s`" instead of "%s" here. Imagine, for a second, that Git was written in French and you preferred to read your error messages in English, therefore set your locale, and you just issued a `git retour`, would this error message read well for you?

	error: your local changes would be overwritten by retour.
That looks wrong to me. I could see us changing this to:
	error: your local changes would be overwritten by `git retour`.
or to:
	error: your local changes would be overwritten by revert.

i.e. either use "`git %s`" without translating, or keeping "%s" with the translated `action_name()`. But it would probably read better to have the action name localized (which is what I suggested).

Ciao, Dscho

Michael J Gruber· Aug 18, 2022, 13:13 UTC · re: Johannes Schindelin · lore

[PATCH 0/4] sequencer: clarify translations

Hi there,

thanks for all your input to my RFC patch. I tried to summarize and pack everything up into this little series.

A follow-up could (but does not have to) turn translated action names into untranslated git command names in some places.

Cheers Michael

Michael J Gruber (4):
  sequencer: do not translate reflog messages
  sequencer: do not translate parameters to error_resolve_conflict()
  sequencer: do not translate command names
  po: adjust README to code
 po/README.md |  2 +-
 sequencer.c  | 10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)
-- 
2.37.2.596.g72ccb331cf
Michael J Gruber· Aug 18, 2022, 13:13 UTC · re: Michael J Gruber · lore

[PATCH 3/4] sequencer: do not translate command names

When action_name is used to denote a command `git %s` do not translate since command names are never translated.

Suggested-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Michael J Gruber <git@grubix.eu>
---
 sequencer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to sequencer.c +2 −2
diff --git a/sequencer.c b/sequencer.c
index 8b32b239b9..79dad522f5 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2422,7 +2422,7 @@ static int read_and_refresh_cache(struct repository *r,
 	if (repo_read_index(r) < 0) {
 		rollback_lock_file(&index_lock);
 		return error(_("git %s: failed to read the index"),
-			_(action_name(opts)));
+			action_name(opts));
 	}
 	refresh_index(r->index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
 
@@ -2430,7 +2430,7 @@ static int read_and_refresh_cache(struct repository *r,
 		if (write_locked_index(r->index, &index_lock,
 				       COMMIT_LOCK | SKIP_IF_UNCHANGED)) {
 			return error(_("git %s: failed to refresh the index"),
-				_(action_name(opts)));
+				action_name(opts));
 		}
 	}
 

-- 
2.37.2.596.g72ccb331cf
Michael J Gruber· Aug 18, 2022, 13:13 UTC · re: Michael J Gruber · lore

[PATCH 1/4] sequencer: do not translate reflog messages

Traditionally, reflog messages were never translated, in particular not on storage.

Due to the switch of more parts of git to the sequencer, old changes in the sequencer code may lead to recent changes in git's behaviour. E.g.: c28cbc5ea6 ("sequencer: mark action_name() for translation", 2016-10-21) marked several uses of `action_name()` for translation. Recently, this lead to a partially translated reflog:

`rebase: fast-forward` is translated (e.g. in de to `Rebase: Vorspulen`) whereas other reflog entries such as `rebase (pick):` remain untranslated as they should be.

Change the relevant line in the sequencer so that this reflog entry remains untranslated, as well.

Signed-off-by: Michael J Gruber <git@grubix.eu>
---
 sequencer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to sequencer.c +1 −1
diff --git a/sequencer.c b/sequencer.c
index 5f22b7cd37..51d75dfbe1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -575,7 +575,7 @@ static int fast_forward_to(struct repository *r,
 	if (checkout_fast_forward(r, from, to, 1))
 		return -1; /* the callee should have complained already */
 
-	strbuf_addf(&sb, _("%s: fast-forward"), _(action_name(opts)));
+	strbuf_addf(&sb, "%s: fast-forward", action_name(opts));
 
 	transaction = ref_transaction_begin(&err);
 	if (!transaction ||

-- 
2.37.2.596.g72ccb331cf
Ævar Arnfjörð Bjarmason· Aug 18, 2022, 14:55 UTC · re: Michael J Gruber · lore

Re: [PATCH 1/4] sequencer: do not translate reflog messages

On Thu, Aug 18 2022, Michael J Gruber wrote:
Show 34 quoted lines
> Traditionally, reflog messages were never translated, in particular not
> on storage.
>
> Due to the switch of more parts of git to the sequencer, old changes in
> the sequencer code may lead to recent changes in git's behaviour. E.g.:
> c28cbc5ea6 ("sequencer: mark action_name() for translation", 2016-10-21)
> marked several uses of `action_name()` for translation. Recently, this
> lead to a partially translated reflog:
>
> `rebase: fast-forward` is translated (e.g. in de to `Rebase: Vorspulen`)
> whereas other reflog entries such as `rebase (pick):` remain
> untranslated as they should be.
>
> Change the relevant line in the sequencer so that this reflog entry
> remains untranslated, as well.
>
> Signed-off-by: Michael J Gruber <git@grubix.eu>
> ---
>  sequencer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 5f22b7cd37..51d75dfbe1 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -575,7 +575,7 @@ static int fast_forward_to(struct repository *r,
>  	if (checkout_fast_forward(r, from, to, 1))
>  		return -1; /* the callee should have complained already */
>  
> -	strbuf_addf(&sb, _("%s: fast-forward"), _(action_name(opts)));
> +	strbuf_addf(&sb, "%s: fast-forward", action_name(opts));
>  
>  	transaction = ref_transaction_begin(&err);
>  	if (!transaction ||

I 95% agree with this direction, but the other 5% of me is thinking "isn't this fine then? Let's keep it?".

I.e. from the very beginning we've really tried not to translate file formats and plumbing, to the point of having the (now removed) "gettext poison" facility to try to smoke out any such cases (but it wouldn't have caught this one).

We've even done this to the point of not translating things like the "revert" template, even though that's an entirely "soft" file format as far as anyone being able to rely on it goes.

But reflogs are local-only, if you're using Git in German isn't it useful to you to have this messaging in German too? We don't "push" them around, and to the extent that there's shared environments they (should) ensure LC_ALL=C if they care.

Of course more useful would be if we wrote it in some language-agnostic format and changed it on the fly, but perhaps we've inadvertently run an experiment here that's shows us this is fine?

We do have some translated "file format" output already, notable whatever we write into the "gc.log". Perhaps we should treat this the same.

I'm *not* noting the other 95% argument(s) for accepting this change, just playing devil's advocate for the 5% one :)

Johannes Schindelin· Aug 19, 2022, 09:25 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 1/4] sequencer: do not translate reflog messages

Hi Ævar,
On Thu, 18 Aug 2022, Ævar Arnfjörð Bjarmason wrote:
Show 39 quoted lines
> On Thu, Aug 18 2022, Michael J Gruber wrote:
>
> > Traditionally, reflog messages were never translated, in particular not
> > on storage.
> >
> > Due to the switch of more parts of git to the sequencer, old changes in
> > the sequencer code may lead to recent changes in git's behaviour. E.g.:
> > c28cbc5ea6 ("sequencer: mark action_name() for translation", 2016-10-21)
> > marked several uses of `action_name()` for translation. Recently, this
> > lead to a partially translated reflog:
> >
> > `rebase: fast-forward` is translated (e.g. in de to `Rebase: Vorspulen`)
> > whereas other reflog entries such as `rebase (pick):` remain
> > untranslated as they should be.
> >
> > Change the relevant line in the sequencer so that this reflog entry
> > remains untranslated, as well.
> >
> > Signed-off-by: Michael J Gruber <git@grubix.eu>
> > ---
> >  sequencer.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/sequencer.c b/sequencer.c
> > index 5f22b7cd37..51d75dfbe1 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -575,7 +575,7 @@ static int fast_forward_to(struct repository *r,
> >  	if (checkout_fast_forward(r, from, to, 1))
> >  		return -1; /* the callee should have complained already */
> >
> > -	strbuf_addf(&sb, _("%s: fast-forward"), _(action_name(opts)));
> > +	strbuf_addf(&sb, "%s: fast-forward", action_name(opts));
> >
> >  	transaction = ref_transaction_begin(&err);
> >  	if (!transaction ||
>
> I 95% agree with this direction, but the other 5% of me is thinking
> "isn't this fine then? Let's keep it?".

No, it's not fine, we mustn't keep it, because we expect Git itself to parse the reflog.

Ciao, Johannes

Ævar Arnfjörð Bjarmason· Aug 19, 2022, 15:12 UTC · re: Johannes Schindelin · lore

Re: [PATCH 1/4] sequencer: do not translate reflog messages

On Fri, Aug 19 2022, Johannes Schindelin wrote:
Show 46 quoted lines
> Hi Ævar,
>
> On Thu, 18 Aug 2022, Ævar Arnfjörð Bjarmason wrote:
>
>> On Thu, Aug 18 2022, Michael J Gruber wrote:
>>
>> > Traditionally, reflog messages were never translated, in particular not
>> > on storage.
>> >
>> > Due to the switch of more parts of git to the sequencer, old changes in
>> > the sequencer code may lead to recent changes in git's behaviour. E.g.:
>> > c28cbc5ea6 ("sequencer: mark action_name() for translation", 2016-10-21)
>> > marked several uses of `action_name()` for translation. Recently, this
>> > lead to a partially translated reflog:
>> >
>> > `rebase: fast-forward` is translated (e.g. in de to `Rebase: Vorspulen`)
>> > whereas other reflog entries such as `rebase (pick):` remain
>> > untranslated as they should be.
>> >
>> > Change the relevant line in the sequencer so that this reflog entry
>> > remains untranslated, as well.
>> >
>> > Signed-off-by: Michael J Gruber <git@grubix.eu>
>> > ---
>> >  sequencer.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/sequencer.c b/sequencer.c
>> > index 5f22b7cd37..51d75dfbe1 100644
>> > --- a/sequencer.c
>> > +++ b/sequencer.c
>> > @@ -575,7 +575,7 @@ static int fast_forward_to(struct repository *r,
>> >  	if (checkout_fast_forward(r, from, to, 1))
>> >  		return -1; /* the callee should have complained already */
>> >
>> > -	strbuf_addf(&sb, _("%s: fast-forward"), _(action_name(opts)));
>> > +	strbuf_addf(&sb, "%s: fast-forward", action_name(opts));
>> >
>> >  	transaction = ref_transaction_begin(&err);
>> >  	if (!transaction ||
>>
>> I 95% agree with this direction, but the other 5% of me is thinking
>> "isn't this fine then? Let's keep it?".
>
> No, it's not fine, we mustn't keep it, because we expect Git itself to
> parse the reflog.

Doesn't that also mean that the relevant functionality is now also (and still?) broken on any repository where these translations ended up on-disk?

Junio C Hamano· Aug 19, 2022, 20:44 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 1/4] sequencer: do not translate reflog messages

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> Doesn't that also mean that the relevant functionality is now also (and
> still?) broken on any repository where these translations ended up
> on-disk?

It may, but the first response to that problem is not to make the breakage in repositires worse by keep adding unparseable data to them.

Ævar Arnfjörð Bjarmason· Aug 19, 2022, 21:13 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/4] sequencer: do not translate reflog messages

On Fri, Aug 19 2022, Junio C Hamano wrote:
Show 9 quoted lines
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> Doesn't that also mean that the relevant functionality is now also (and
>> still?) broken on any repository where these translations ended up
>> on-disk?
>
> It may, but the first response to that problem is not to make the
> breakage in repositires worse by keep adding unparseable data to
> them.

*nod*, but where is that breakage specifically? I don't see where we're parsing this message out again. I tried to test it out with the below (making the message as un-helpful as possible). All our tests pass, but of course our coverage may just be lacking...

Show changes to sequencer.c +10 −5
diff --git a/sequencer.c b/sequencer.c
index 5f22b7cd377..9e039e26b5a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -391,19 +391,24 @@ int sequencer_remove_state(struct replay_opts *opts)
 	return ret;
 }
 
-static const char *action_name(const struct replay_opts *opts)
+static const char *action_name_1(const struct replay_opts *opts, int revert)
 {
 	switch (opts->action) {
 	case REPLAY_REVERT:
-		return N_("revert");
+		return revert ? N_("trever") : N_("revert");
 	case REPLAY_PICK:
-		return N_("cherry-pick");
+		return revert ? N_("kcip-yrrehc") : N_("cherry-pick");
 	case REPLAY_INTERACTIVE_REBASE:
-		return N_("rebase");
+		return revert ? N_("esaber") : N_("rebase");
 	}
 	die(_("unknown action: %d"), opts->action);
 }
 
+static const char *action_name(const struct replay_opts *opts)
+{
+	return action_name_1(opts, 0);
+}
+
 struct commit_message {
 	char *parent_label;
 	char *label;
@@ -575,7 +580,7 @@ static int fast_forward_to(struct repository *r,
 	if (checkout_fast_forward(r, from, to, 1))
 		return -1; /* the callee should have complained already */
 
-	strbuf_addf(&sb, _("%s: fast-forward"), _(action_name(opts)));
+	strbuf_addf(&sb, _("drawrof-tsaf: %s"), _(action_name_1(opts, 1)));
 
 	transaction = ref_transaction_begin(&err);
 	if (!transaction ||
Junio C Hamano· Aug 19, 2022, 22:42 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 1/4] sequencer: do not translate reflog messages

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 13 quoted lines
> On Fri, Aug 19 2022, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>
>>> Doesn't that also mean that the relevant functionality is now also (and
>>> still?) broken on any repository where these translations ended up
>>> on-disk?
>>
>> It may, but the first response to that problem is not to make the
>> breakage in repositires worse by keep adding unparseable data to
>> them.
>
> *nod*, but where is that breakage specifically?

Set your LANG to something other than C and then run "git reflog" after running sequencer operations, and you'll see the same breakage that motivated Michael to send this patch set, I think.

Ævar Arnfjörð Bjarmason· Aug 19, 2022, 23:33 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/4] sequencer: do not translate reflog messages

On Fri, Aug 19 2022, Junio C Hamano wrote:
Show 19 quoted lines
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> On Fri, Aug 19 2022, Junio C Hamano wrote:
>>
>>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>>
>>>> Doesn't that also mean that the relevant functionality is now also (and
>>>> still?) broken on any repository where these translations ended up
>>>> on-disk?
>>>
>>> It may, but the first response to that problem is not to make the
>>> breakage in repositires worse by keep adding unparseable data to
>>> them.
>>
>> *nod*, but where is that breakage specifically?
>
> Set your LANG to something other than C and then run "git reflog"
> after running sequencer operations, and you'll see the same breakage
> that motivated Michael to send this patch set, I think.

Yes, I can see how and what we write to the reflog. But in order for this to cause anything other than cosmetic breakage we'd need more than that.

Or what do we mean by breakage here?

That it's broken because we intended for these to be LC_ALL=C, but they weren't? Fair enough, but that's got a smaller scope.

Or that it's broken because we expected to not only write "rebase: fast-forward" into the reflog, but to parse that out again, or to e.g. parse the "rebase" part of it out as a command-name. I haven't found *those* bits yet.

Of course we also have to worry about third-party software that expected LC_ALL=C breaking. I'm just wondering if we have some code in git.git that would also be similarly broken.

Because if we do it wouldn't be that hard to just hardcode all the translations we shipped at that time in some array in the C code, and not only parse out a "rebase: fast-forward", but also the German etc. equivalent.

Jeff King· Aug 20, 2022, 08:56 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 1/4] sequencer: do not translate reflog messages

On Fri, Aug 19, 2022 at 11:13:21PM +0200, Ævar Arnfjörð Bjarmason wrote:
Show 17 quoted lines
> 
> On Fri, Aug 19 2022, Junio C Hamano wrote:
> 
> > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> >
> >> Doesn't that also mean that the relevant functionality is now also (and
> >> still?) broken on any repository where these translations ended up
> >> on-disk?
> >
> > It may, but the first response to that problem is not to make the
> > breakage in repositires worse by keep adding unparseable data to
> > them.
> 
> *nod*, but where is that breakage specifically? I don't see where we're
> parsing this message out again. I tried to test it out with the below
> (making the message as un-helpful as possible). All our tests pass, but
> of course our coverage may just be lacking...

I'm not sure if you mean "where are we parsing this sequencer message specifically", or if you're just asking where we parse reflog messages at all. If the latter, try interpret_nth_prior_checkout() and its helper grab_nth_branch_switch().

As far as I know, that's the only one we parse, so the answer for _these_ messages is: nowhere.

I'm not sure if you're proposing to leave the "checkout" message untranslated, but translate everything else. If so, I'm not sure how I feel about that. On the one hand, it could help people who want the translation. On the other hand, it sounds like a maintainability nightmare. ;)

-Peff
Junio C Hamano· Aug 20, 2022, 21:20 UTC · re: Jeff King · lore

Re: [PATCH 1/4] sequencer: do not translate reflog messages

Jeff King <peff@peff.net> writes:
Show 7 quoted lines
> I'm not sure if you mean "where are we parsing this sequencer message
> specifically", or if you're just asking where we parse reflog messages
> at all. If the latter, try interpret_nth_prior_checkout() and its helper
> grab_nth_branch_switch().
>
> As far as I know, that's the only one we parse, so the answer for
> _these_ messages is: nowhere.

Unless translation in some language of these messages looks similar to what 'nth-prior' wants to find. So the answer really is "asking if somebody parses _these_ messages is pointless" ;-)

I outlined one possible approach to allow translat{able,ed} reflog messages without breaking 'nth-prior' and would allow us add more code to mechanically parse them if we wanted to elsewhere in the thread, by the way. I do not plan to work on it soon, but without doing something like that first, letting translated messages randomly into reflog is asking for trouble, I am afraid.

Michael J Gruber· Aug 18, 2022, 13:13 UTC · re: Michael J Gruber · lore

[PATCH 2/4] sequencer: do not translate parameters to error_resolve_conflict()

`error_resolve_conflict()` checks the untranslated action_name parameter, so pass it as is.

Suggested-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Michael J Gruber <git@grubix.eu>
---
 sequencer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to sequencer.c +2 −2
diff --git a/sequencer.c b/sequencer.c
index 51d75dfbe1..8b32b239b9 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -537,7 +537,7 @@ static struct tree *empty_tree(struct repository *r)
 static int error_dirty_index(struct repository *repo, struct replay_opts *opts)
 {
 	if (repo_read_index_unmerged(repo))
-		return error_resolve_conflict(_(action_name(opts)));
+		return error_resolve_conflict(action_name(opts));
 
 	error(_("your local changes would be overwritten by %s."),
 		_(action_name(opts)));
@@ -3753,7 +3753,7 @@ static int do_reset(struct repository *r,
 	init_checkout_metadata(&unpack_tree_opts.meta, name, &oid, NULL);
 
 	if (repo_read_index_unmerged(r)) {
-		ret = error_resolve_conflict(_(action_name(opts)));
+		ret = error_resolve_conflict(action_name(opts));
 		goto cleanup;
 	}
 

-- 
2.37.2.596.g72ccb331cf
Ævar Arnfjörð Bjarmason· Aug 18, 2022, 15:01 UTC · re: Michael J Gruber · lore

Re: [PATCH 2/4] sequencer: do not translate parameters to error_resolve_conflict()

On Thu, Aug 18 2022, Michael J Gruber wrote:
Show 30 quoted lines
> `error_resolve_conflict()` checks the untranslated action_name
> parameter, so pass it as is.
>
> Suggested-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Signed-off-by: Michael J Gruber <git@grubix.eu>
> ---
>  sequencer.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 51d75dfbe1..8b32b239b9 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -537,7 +537,7 @@ static struct tree *empty_tree(struct repository *r)
>  static int error_dirty_index(struct repository *repo, struct replay_opts *opts)
>  {
>  	if (repo_read_index_unmerged(repo))
> -		return error_resolve_conflict(_(action_name(opts)));
> +		return error_resolve_conflict(action_name(opts));
>  
>  	error(_("your local changes would be overwritten by %s."),
>  		_(action_name(opts)));
> @@ -3753,7 +3753,7 @@ static int do_reset(struct repository *r,
>  	init_checkout_metadata(&unpack_tree_opts.meta, name, &oid, NULL);
>  
>  	if (repo_read_index_unmerged(r)) {
> -		ret = error_resolve_conflict(_(action_name(opts)));
> +		ret = error_resolve_conflict(action_name(opts));
>  		goto cleanup;
>  	}

Perhaps we should have the error_resolve_conflict() function take a "enum replay_action" instead? We could just do this more isolated change, but perhaps that "while-we're-at-it" would be acceptable to reduce the risk of running with this particular set of scissors.

Then we could note in a comment in that function that we do not want to translate the string we'd get from action_name()...

Michael J Gruber· Aug 18, 2022, 15:23 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 2/4] sequencer: do not translate parameters to error_resolve_conflict()

Am Do., 18. Aug. 2022 um 17:02 Uhr schrieb Ævar Arnfjörð Bjarmason <avarab@gmail.com>:

Show 42 quoted lines
>
>
> On Thu, Aug 18 2022, Michael J Gruber wrote:
>
> > `error_resolve_conflict()` checks the untranslated action_name
> > parameter, so pass it as is.
> >
> > Suggested-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> > Signed-off-by: Michael J Gruber <git@grubix.eu>
> > ---
> >  sequencer.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/sequencer.c b/sequencer.c
> > index 51d75dfbe1..8b32b239b9 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -537,7 +537,7 @@ static struct tree *empty_tree(struct repository *r)
> >  static int error_dirty_index(struct repository *repo, struct replay_opts *opts)
> >  {
> >       if (repo_read_index_unmerged(repo))
> > -             return error_resolve_conflict(_(action_name(opts)));
> > +             return error_resolve_conflict(action_name(opts));
> >
> >       error(_("your local changes would be overwritten by %s."),
> >               _(action_name(opts)));
> > @@ -3753,7 +3753,7 @@ static int do_reset(struct repository *r,
> >       init_checkout_metadata(&unpack_tree_opts.meta, name, &oid, NULL);
> >
> >       if (repo_read_index_unmerged(r)) {
> > -             ret = error_resolve_conflict(_(action_name(opts)));
> > +             ret = error_resolve_conflict(action_name(opts));
> >               goto cleanup;
> >       }
>
> Perhaps we should have the error_resolve_conflict() function take a
> "enum replay_action" instead? We could just do this more isolated
> change, but perhaps that "while-we're-at-it" would be acceptable to
> reduce the risk of running with this particular set of scissors.
>
> Then we could note in a comment in that function that we do not want to
> translate the string we'd get from action_name()...

Rather than setting out to do that, I'd retract 2/3/4 just to get 1 done, which was my original motivation ... or switch git to C again as I did for a while in the past ...

Junio C Hamano· Aug 18, 2022, 20:30 UTC · re: Michael J Gruber · lore

Re: [PATCH 2/4] sequencer: do not translate parameters to error_resolve_conflict()

Michael J Gruber <git@grubix.eu> writes:
> Rather than setting out to do that, I'd retract 2/3/4 just to get 1
> done, which was my original motivation ... or switch git to C again as
> I did for a while in the past ...

While I found 4/4 a bit questionable, these early three patches looked eminently sensible to me.

Johannes Schindelin· Aug 19, 2022, 09:26 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 2/4] sequencer: do not translate parameters to error_resolve_conflict()

Hi Ævar,
On Thu, 18 Aug 2022, Ævar Arnfjörð Bjarmason wrote:
Show 35 quoted lines
> On Thu, Aug 18 2022, Michael J Gruber wrote:
>
> > `error_resolve_conflict()` checks the untranslated action_name
> > parameter, so pass it as is.
> >
> > Suggested-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> > Signed-off-by: Michael J Gruber <git@grubix.eu>
> > ---
> >  sequencer.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/sequencer.c b/sequencer.c
> > index 51d75dfbe1..8b32b239b9 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -537,7 +537,7 @@ static struct tree *empty_tree(struct repository *r)
> >  static int error_dirty_index(struct repository *repo, struct replay_opts *opts)
> >  {
> >  	if (repo_read_index_unmerged(repo))
> > -		return error_resolve_conflict(_(action_name(opts)));
> > +		return error_resolve_conflict(action_name(opts));
> >
> >  	error(_("your local changes would be overwritten by %s."),
> >  		_(action_name(opts)));
> > @@ -3753,7 +3753,7 @@ static int do_reset(struct repository *r,
> >  	init_checkout_metadata(&unpack_tree_opts.meta, name, &oid, NULL);
> >
> >  	if (repo_read_index_unmerged(r)) {
> > -		ret = error_resolve_conflict(_(action_name(opts)));
> > +		ret = error_resolve_conflict(action_name(opts));
> >  		goto cleanup;
> >  	}
>
> Perhaps we should have the error_resolve_conflict() function take a
> "enum replay_action" instead?

We could do that. We could also just delete the sequencer code. It's just that both are a bad idea.

Ciao, Johannes

Junio C Hamano· Aug 19, 2022, 17:36 UTC · re: Johannes Schindelin · lore

Re: [PATCH 2/4] sequencer: do not translate parameters to error_resolve_conflict()

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 5 quoted lines
>> Perhaps we should have the error_resolve_conflict() function take a
>> "enum replay_action" instead?
>
> We could do that. We could also just delete the sequencer code. It's just
> that both are a bad idea.

Sorry, but I do not quite understand this comment. You may think some parts of the sequencer code are a bad idea but I think overall it is eminently useful and usable enough that it does not make sense to "just delete the sequencer code"---if there are things we find bad ideas in there, we should fix them instead, no?

In any case, can you keep the conversation more civil? I have to say that between you two, you may by no means be the only one who is unnecessarily abrasive, but if you do not understand why the other side suggests a solution you feel you do not like, you can ask more constructively why they think it is a good idea, without assuming that they are doing so only to block you. Or explain why you think it is a bad idea by showing the consequences of their solution, e.g. "there are 20 callsites, among which only 1 has the enum readily available so it would be a lot of churn to give the other 19 the enum, even though the error helper may become simpler with a single switch() statement if we allow it to take an enum." or something (I know this function is called only from very few places, so 1 out of 20 is a totally made-up reasoning that would not apply in this case, but you get the idea).

Thanks.
Johannes Schindelin· Aug 22, 2022, 13:53 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/4] sequencer: do not translate parameters to error_resolve_conflict()

Hi Junio,

[Michael, I do not consider what I wrote below relevant for your patch series, you may ignore it if you want]

On Fri, 19 Aug 2022, Junio C Hamano wrote:
Show 9 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> Perhaps we should have the error_resolve_conflict() function take a
> >> "enum replay_action" instead?
> >
> > We could do that. We could also just delete the sequencer code. It's just
> > that both are a bad idea.
>
> Sorry, but I do not quite understand this comment.

I expected a seasoned reviewer to offer such a suggestion only after looking up (or remembering) how `error_resolve_conflict()` is defined, and where, and where its callers are.

After all, many suggestions that come to mind during a review turn out to be a bad idea when considering them carefully, and if that can be determined before the mail is sent, everybody wins back some time.

In this instance, `error_resolve_conflict()` is declared in `advice.h`. The suggestion to use a sequencer-specific data type there sounds... controversial. But okay, maybe there are good reasons to suggest that.

Let's look at the callers. Two callers in `sequencer.c`. Okay, maybe it makes a bit more sense. But one caller in `advice.c`? Let's dig deeper.

That caller in `advice.c` is `die_resolve_conflict()`, which is called in the built-ins `commit`, `merge-recursive`, `merge` and `pull`.

Those callers have nothing to do with the sequencer, therefore it is a bad idea to suggest using a sequencer-specific data type in that call chain.

From my perspective, that is enough to retire the suggestion.

When I wrote what I wrote, I thought that it was a pretty quick thing to determine, so quick that I really expected to not see such a suggestion on the mailing list in the first place.

In hindsight, I understand that you would have had to look at the code, and not just at the patch, to see this. And therefore it is probably not quite as obvious as I thought. I did not expect new contributors to be able to analyze this quickly, but a Git mailing list regular, yes.

For my flippant response, I apologize.

As for the suggestion I criticized: I stand by my assessment. It is not a good idea, and it was not necessary to send it out before doing a cursory sanity check. We want code contribution to have a high quality, and the code reviews should meet at least the same bar.

Ciao, Dscho

Junio C Hamano· Aug 22, 2022, 16:12 UTC · re: Johannes Schindelin · lore

Re: [PATCH 2/4] sequencer: do not translate parameters to error_resolve_conflict()

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> ... We want code contribution to have a high quality, and the
> code reviews should meet at least the same bar.

I like that one. Ævar is not alone, but many of us often throw an unrelated "observation" into a review thread that is a total tangent. While I do not think it is necessarily a bad thing, because these tangential discussions often turn into separate idea that lead to improvements, we should learn to (1) mark a tangent clearly as such and (2) keep the quality of the tangent reasonably high.

Thanks.
Michael J Gruber· Aug 18, 2022, 13:13 UTC · re: Michael J Gruber · lore

[PATCH 4/4] po: adjust README to code

When we talk about sequencer action names as such (as opposed to command names) we do translate the action name. Adjust the po README to reflect this and to match the code base.

Signed-off-by: Michael J Gruber <git@grubix.eu>
---
 po/README.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to po/README.md +1 −1
diff --git a/po/README.md b/po/README.md
index 3e4f897d93..90b8455401 100644
--- a/po/README.md
+++ b/po/README.md
@@ -273,7 +273,7 @@ General advice:
 
   ```c
   /* TRANSLATORS: %s will be "revert" or "cherry-pick" */
-  die(_("%s: Unable to write new index file"), action_name(opts));
+  die(_("%s: Unable to write new index file"), _(action_name(opts)));
   ```
 
 We provide wrappers for C, Shell and Perl programs. Here's how they're

-- 
2.37.2.596.g72ccb331cf
Ævar Arnfjörð Bjarmason· Aug 18, 2022, 15:03 UTC · re: Michael J Gruber · lore

Re: [PATCH 4/4] po: adjust README to code

On Thu, Aug 18 2022, Michael J Gruber wrote:
Show 22 quoted lines
> When we talk about sequencer action names as such (as opposed to command
> names) we do translate the action name. Adjust the po README to reflect
> this and to match the code base.
>
> Signed-off-by: Michael J Gruber <git@grubix.eu>
> ---
>  po/README.md | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/po/README.md b/po/README.md
> index 3e4f897d93..90b8455401 100644
> --- a/po/README.md
> +++ b/po/README.md
> @@ -273,7 +273,7 @@ General advice:
>  
>    ```c
>    /* TRANSLATORS: %s will be "revert" or "cherry-pick" */
> -  die(_("%s: Unable to write new index file"), action_name(opts));
> +  die(_("%s: Unable to write new index file"), _(action_name(opts)));
>    ```
>  
>  We provide wrappers for C, Shell and Perl programs. Here's how they're

Is the end-state of this series such that we do that anywhere? Perhaps that's OK for an isolated fix, but it would really be preferred to avoid the "lego" with:

	die(action == REVERT ? _("revert: Unable to write new index file") : ...);
Or whatever.

The "TRANSLATORS" comment above the example you're modifying is now inaccurate, the "%s" will *not* be "revert" or "cherry-pick" in the post-image.

I think the right thing here would be to grep our source for TRANSLATORS comments that mention %s and replace this existing example with an entirely different one...

Junio C Hamano· Aug 18, 2022, 20:36 UTC · re: Michael J Gruber · lore

Re: [PATCH 4/4] po: adjust README to code

Michael J Gruber <git@grubix.eu> writes:
Show 20 quoted lines
> When we talk about sequencer action names as such (as opposed to command
> names) we do translate the action name. Adjust the po README to reflect
> this and to match the code base.
>
> Signed-off-by: Michael J Gruber <git@grubix.eu>
> ---
>  po/README.md | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/po/README.md b/po/README.md
> index 3e4f897d93..90b8455401 100644
> --- a/po/README.md
> +++ b/po/README.md
> @@ -273,7 +273,7 @@ General advice:
>  
>    ```c
>    /* TRANSLATORS: %s will be "revert" or "cherry-pick" */
> -  die(_("%s: Unable to write new index file"), action_name(opts));
> +  die(_("%s: Unable to write new index file"), _(action_name(opts)));
>    ```

While "revert" and "cherry-pick" may have localized words in our po/ dictionary, the message uses "%s:" placeholder to identify the Git operation that is reporting the problem, and the way the end-user who is getting the message triggered the Git operation was by running a subcommand of "git", isn't it?

Isn't it confusing for a user who typed "git revert" to see an error from _("revert")? _("Unable to write new index file") is perfectly fine, though.

Michael J Gruber· Aug 19, 2022, 07:50 UTC · re: Junio C Hamano · lore

[PATCH 4/4 v2] sequencer: spell out command names and do not translate them

When we talk about sequencer action names as such we do translate the action name. In all cases, we talk about the like-named git command name, though, which is not translated.

In order to make the correspondence clearer, reword those error messages to use the (untranslated) git command name, and adjust the po README to match the code base.

Signed-off-by: Michael J Gruber <git@grubix.eu>
---
I guess there are two extreme views regarding these cases (in terms of
how much to translate) and a few in between. v2 here implements the
one of these. As a result, we don't need to N_()-mark the action names
any more unless I'm overlooking something. I'm holding this back until
the consensus is clear.

Overall, we are not consistent with the prefixes in our error messages (command or not) nor the capitalisation. One could say that at the point of an error/die worse has gone wrong than the wording, of course ;)

 po/README.md | 2 +-
 sequencer.c  | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)
Show changes to 2 files +5 −5

po/README.md, sequencer.c

diff --git a/po/README.md b/po/README.md
index 3e4f897d93..7b7ad24412 100644
--- a/po/README.md
+++ b/po/README.md
@@ -273,7 +273,7 @@ General advice:
 
   ```c
   /* TRANSLATORS: %s will be "revert" or "cherry-pick" */
-  die(_("%s: Unable to write new index file"), action_name(opts));
+  die(_("git %s: unable to write new index file"), action_name(opts));
   ```
 
 We provide wrappers for C, Shell and Perl programs. Here's how they're
diff --git a/sequencer.c b/sequencer.c
index 79dad522f5..c26dc46268 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -539,8 +539,8 @@ static int error_dirty_index(struct repository *repo, struct replay_opts *opts)
 	if (repo_read_index_unmerged(repo))
 		return error_resolve_conflict(action_name(opts));
 
-	error(_("your local changes would be overwritten by %s."),
-		_(action_name(opts)));
+	error(_("git %s: your local changes would be overwritten"),
+		action_name(opts)));
 
 	if (advice_enabled(ADVICE_COMMIT_BEFORE_MERGE))
 		advise(_("commit your changes or stash them to proceed."));
@@ -725,8 +725,8 @@ static int do_recursive_merge(struct repository *r,
 		 * TRANSLATORS: %s will be "revert", "cherry-pick" or
 		 * "rebase".
 		 */
-		return error(_("%s: Unable to write new index file"),
-			_(action_name(opts)));
+		return error(_("git %s: unable to write new index file"),
+			action_name(opts));
 
 	if (!clean)
 		append_conflicts_hint(r->index, msgbuf,

-- 
2.37.2.653.g5b2587383a
Johannes Schindelin· Aug 19, 2022, 09:30 UTC · re: Michael J Gruber · lore

Re: [PATCH 4/4 v2] sequencer: spell out command names and do not translate them

Hi Michael & Junio,
On Fri, 19 Aug 2022, Michael J Gruber wrote:
Show 15 quoted lines
> When we talk about sequencer action names as such we do translate the
> action name. In all cases, we talk about the like-named git command
> name, though, which is not translated.
>
> In order to make the correspondence clearer, reword those error messages
> to use the (untranslated) git command name, and adjust the po README to
> match the code base.
>
> Signed-off-by: Michael J Gruber <git@grubix.eu>
> ---
> I guess there are two extreme views regarding these cases (in terms of
> how much to translate) and a few in between. v2 here implements the
> one of these. As a result, we don't need to N_()-mark the action names
> any more unless I'm overlooking something. I'm holding this back until
> the consensus is clear.
Thank you for being careful.

In general, I would like to leave the decision whether or not to mention the _English_ word for an operation (or whether to treat the error message's prefix as a short-hand for the Git command) to the l10n maintainer, so that things can be consistent between translations.

Ciao, Dscho

Show 51 quoted lines
> Overall, we are not consistent with the prefixes in our error messages
> (command or not) nor the capitalisation. One could say that at the point
> of an error/die worse has gone wrong than the wording, of course ;)
>
>  po/README.md | 2 +-
>  sequencer.c  | 8 ++++----
>  2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/po/README.md b/po/README.md
> index 3e4f897d93..7b7ad24412 100644
> --- a/po/README.md
> +++ b/po/README.md
> @@ -273,7 +273,7 @@ General advice:
>
>    ```c
>    /* TRANSLATORS: %s will be "revert" or "cherry-pick" */
> -  die(_("%s: Unable to write new index file"), action_name(opts));
> +  die(_("git %s: unable to write new index file"), action_name(opts));
>    ```
>
>  We provide wrappers for C, Shell and Perl programs. Here's how they're
> diff --git a/sequencer.c b/sequencer.c
> index 79dad522f5..c26dc46268 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -539,8 +539,8 @@ static int error_dirty_index(struct repository *repo, struct replay_opts *opts)
>  	if (repo_read_index_unmerged(repo))
>  		return error_resolve_conflict(action_name(opts));
>
> -	error(_("your local changes would be overwritten by %s."),
> -		_(action_name(opts)));
> +	error(_("git %s: your local changes would be overwritten"),
> +		action_name(opts)));
>
>  	if (advice_enabled(ADVICE_COMMIT_BEFORE_MERGE))
>  		advise(_("commit your changes or stash them to proceed."));
> @@ -725,8 +725,8 @@ static int do_recursive_merge(struct repository *r,
>  		 * TRANSLATORS: %s will be "revert", "cherry-pick" or
>  		 * "rebase".
>  		 */
> -		return error(_("%s: Unable to write new index file"),
> -			_(action_name(opts)));
> +		return error(_("git %s: unable to write new index file"),
> +			action_name(opts));
>
>  	if (!clean)
>  		append_conflicts_hint(r->index, msgbuf,
> --
> 2.37.2.653.g5b2587383a
>
>
Johannes Schindelin· Aug 19, 2022, 09:32 UTC · re: Michael J Gruber · lore

Re: [PATCH 0/4] sequencer: clarify translations

Hi Michael,
On Thu, 18 Aug 2022, Michael J Gruber wrote:
Show 5 quoted lines
> thanks for all your input to my RFC patch. I tried to summarize and pack
> everything up into this little series.
>
> A follow-up could (but does not have to) turn translated action names
> into untranslated git command names in some places.

Thank you for persisting on this, and on behalf of the core Git reviewers I would like to apologize for the hornets' nest.

I offer my ACK to this iteration of the patch series, with or without the v2 of patch 4/4.

Ciao, Dscho

Michael J Gruber· Aug 19, 2022, 10:19 UTC · re: Johannes Schindelin · lore

Re: [PATCH 0/4] sequencer: clarify translations

Am Fr., 19. Aug. 2022 um 11:32 Uhr schrieb Johannes Schindelin <Johannes.Schindelin@gmx.de>:

Show 13 quoted lines
>
> Hi Michael,
>
> On Thu, 18 Aug 2022, Michael J Gruber wrote:
>
> > thanks for all your input to my RFC patch. I tried to summarize and pack
> > everything up into this little series.
> >
> > A follow-up could (but does not have to) turn translated action names
> > into untranslated git command names in some places.
>
> Thank you for persisting on this, and on behalf of the core Git reviewers
> I would like to apologize for the hornets' nest.

No need to. After all, this thoroughness is a huge part of what makes git into what it is.

It's also why I'ven been participating much less: simply for lack of time. That thoroughness takes time, sometimes much more than expected (and can be frustrating, even in the very technical-physical meaning). But discussions here are always about the best solution (not "whose" solution "wins"), and that is why I'm always happy to be back for a bit.

> I offer my ACK to this iteration of the patch series, with or without the
> v2 of patch 4/4.
Thanks!

Personally, I care only about the consistent reflog, which currently means untranslated. Not that I look at the reflog all the time, but ...

Michael

← back to recent threads