Hi Ævar
On 28/03/2022 13:16, Ævar Arnfjörð Bjarmason wrote:
On Mon, Mar 28 2022, Johannes Sixt wrote:
Am 27.03.22 um 13:15 schrieb Ævar Arnfjörð Bjarmason:
On Sun, Mar 27 2022, Johannes Sixt wrote:
Am 26.03.22 um 18:14 schrieb Ævar Arnfjörð Bjarmason:
Partially revert d323c6b6410 (i18n: git-sh-setup.sh: mark strings for
translation, 2016-06-17).
These strings are no longer used in-tree, and we shouldn't be wasting
translator time on them for the benefit of a hypothetical out-of-tree
user of git-sh-setup.sh.
The out of tree users of git-sh-setup.sh are not hypothetical, they
exist and objected when you recently tried to remove these functions
entirely[1].
There is public documentation for these functions. Hence, you must
assume that they are used in scripts outside of Git. Castrating their
functionality like this patch does is unacceptable.
For require_clean_work_tree() the public documentation for this function
states that it will emit a specific error message in English, and you're
expected to Lego-interpolate a string that we'll concatenate with it:
The documentation does not say whether the message is translated or not,
probably because it was not updated when the translations were added six
years ago.
[...]It emits an error message of the form `Cannot
<action>: <reason>. <hint>`, and dies. Example:
This is not a promising a "specific error message in English"
+
----------------
require_clean_work_tree rebase "Please commit or stash them."
This is an example message you cannot use that to argue that we will
always show a message in English
So I think that marking it for translation like this as d323c6b6410 was
always broken in that it broke that documented promise.
I can buy this argument. But then this must be a separate commit with
this justification.
Sure, I can elaborate on that point & split it up.
But that's just an argument for keeping the require_clean_work_tree()
part of this patch, not require_work_tree_exists().
For that one and others in git-sh-setup we've never said that we'd
provide these translated (and to the extent we've implied anything about
the rest, have strongly implied the opposite with
require_clean_work_tree()'s docs).
Nothing will break for out-of-tree users as a result of this
change.
The strings the user sees will change
When we added these functions and their documentation their
output wouldn't be translated,
Where does the documentation say "the output will not be translated"?
then sometimes it was, now it's not
again.
This does not sound convincing at all, but rather like "I want the code
to be so, and here is some handwaving to justify it". What is wrong with
the status quo?
The larger context for why I was looking at this again is that I'm
trying to slowly get us to the point where we can remove the
i18n-in-shellscript entirtely.
But I thought that was a rather large digression for the commit message,
and that these being both unused, and not something the "public" API
affected ever promised it would do was sufficient.
I think if that is what you want to do then you should propose a series
that does just that and explains why it is desirable, rather than coming
up with other reasons to justify the changes you want.
We need also need to be mindful of translator time, it's a *lot* of
strings to go through, and e.g. I've commented in the past on patches
that marked stuff in t/helper/ for translation.
Translator's time is your concern? No translator sifts through 5000
strings on every release. There are tools that show only new strings to
translate.
Yes, I'm the person who added this entire i18n infrastructure to git, I
know how it works :)
A text is translated once and then it lies under the radar
until someone changes it. Don't tell me that is time-consuming.
Yes, individual orphaned strings aren't, but they add up.
Just like having that "USE_PIC" comment in configure.ac isn't much of a
big deal, but it makes sense to clean up unused code, just as we're
adding new code.
I will say that your implicit proposal of keeping this forever instead
is assuming that we won't have more translations for git, and every new
translator will look at this.
Context is critical for translators, so even if it's one string it's a
string you'll quickly grep for and find ... no uses for, and then likely
go hunting around for where it's used only to (hopefully, in that case)
find this thread. Better not to have it.
On the other hand, there is a lot of *reviewer* time that you are
spending with changes like this. *That* should be your concern.
I'd think most of the that time, if any, will be spent on this
sub-thread you started, so ... :)
This sub-tread exists because you posted this patch to the mailing list.
Blaming reviewers for asking perfectly reasonable questions is neither
fair nor helpful.
This patch does not remove dead code as the rest of the series does but
instead changes user facing messages in code that we recently
established is part of the public api[2]. Nothing has changed since that
recent discussion so I'm confused as to why you are proposing to modify
the api again so soon.
Best Wishes
Phillip
[1]
https://lore.kernel.org/git/CAJm9OHfN9iXDt-rzu-wb=67y4PPpmCUgMfmZPy1JMBJkHG256g@xxxxxxxxxxxxxx/
[2] https://lore.kernel.org/git/xmqq5yvik8bc.fsf@gitster.g/
Which isn't to say it shouldn't have been brought up, but from my
perspective I was (and still am) making a rather small change that I
think won't harm anyone in practice, and gives us some incremental
tidyness & contributes to an eventual large "git rm git-sh-i18n.sh" et
al.
But on reflection I don't think it's worth worrying about, and we can
just do this change.
