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

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

[Michael J Gruber]

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";
 	}
 	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

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

[Junio C Hamano]

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.

> 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.

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

[Phillip Wood]

Hi Michael

On 12/08/2022 16:38, 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>
> ---
> 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

>   	}
>   	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 ||

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

[Junio C Hamano]

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.

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

[Johannes Schindelin]

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)));

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

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

[Phillip Wood]

Hi Dscho

On 15/08/2022 21:20, Johannes Schindelin wrote:
> 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

> 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

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

[Johannes Schindelin]

Hi Phillip,

On Tue, 16 Aug 2022, Phillip Wood wrote:

> 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

[PATCH 0/4] sequencer: clarify translations

[Michael J Gruber]

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

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

[Michael J Gruber]

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 ||
-- 
2.37.2.596.g72ccb331cf

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

[Michael J Gruber]

`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;
 	}
 
-- 
2.37.2.596.g72ccb331cf

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

[Michael J Gruber]

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(-)

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

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

[Michael J Gruber]

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
-- 
2.37.2.596.g72ccb331cf

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

[Ævar Arnfjörð Bjarmason]

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?".

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 :)

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

[Ævar Arnfjörð Bjarmason]

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()...

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

[Ævar Arnfjörð Bjarmason]

On Thu, Aug 18 2022, Michael J Gruber wrote:

> 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...

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

[Michael J Gruber]

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

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

Michael J Gruber <git@grubix.eu> writes:

> 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.

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

[Michael J Gruber]

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(-)

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

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1794 bytes --]

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.

Ciao,
Johannes

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1579 bytes --]

Hi Ævar,

On Thu, 18 Aug 2022, Ævar Arnfjörð Bjarmason wrote:

> 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

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

[Johannes Schindelin]

Hi Michael & Junio,

On Fri, 19 Aug 2022, Michael J Gruber wrote:

> 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

> 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
>
>

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

[Johannes Schindelin]

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.

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

Ciao,
Dscho

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

[Michael J Gruber]

Am Fr., 19. Aug. 2022 um 11:32 Uhr schrieb Johannes Schindelin
<Johannes.Schindelin@gmx.de>:
>
> 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

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

[Ævar Arnfjörð Bjarmason]

On Fri, Aug 19 2022, Johannes Schindelin wrote:

> 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?

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

[Junio C Hamano]

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.  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.

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

[Junio C Hamano]

Æ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.

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

[Ævar Arnfjörð Bjarmason]

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...

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 ||

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

[Junio C Hamano]

Æ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.

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

[Ævar Arnfjörð Bjarmason]

On Fri, Aug 19 2022, Junio C Hamano wrote:

> Æ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.

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

[Jeff King]

On Fri, Aug 19, 2022 at 11:13:21PM +0200, Ævar Arnfjörð Bjarmason wrote:

> 
> 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

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

[Johannes Schindelin]

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:

> 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

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

[Junio C Hamano]

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.