| Message ID | b8ab40b2b0e3e5d762b414329ad2f4552f935d28.1660318162.git.git@grubix.eu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC/PATCH] sequencer: do not translate reflog messages | expand |
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.
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 ||
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.
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
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
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
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 ||
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(-)