| Message ID | ea6c65c254bb08b20ea6c4d81200b847755b555c.1660828108.git.git@grubix.eu (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 5670e0ec1576fd0cd68f7cb610c878c645786972 |
| Headers | show |
| Series | sequencer: clarify translations | expand |
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 :)
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
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?
Æ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.
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 ||
Æ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.
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.
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
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.
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 ||
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(-)