* [PATCH 01/11] tests: remove unnecessary GIT_TEST_GETTEXT_POISON=false constructs
2021-01-12 8:47 [PATCH 00/11] [RFH] Introduce support for GETTEXT_POISON=rot13 Johannes Schindelin via GitGitGadget
@ 2021-01-12 8:47 ` Johannes Schindelin via GitGitGadget
2021-01-12 9:07 ` SZEDER Gábor
2021-01-12 8:47 ` [PATCH 02/11] Support GIT_TEST_GETTEXT_POISON=rot13 Johannes Schindelin via GitGitGadget
` (11 subsequent siblings)
12 siblings, 1 reply; 31+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2021-01-12 8:47 UTC (permalink / raw)
To: git
Cc: Nguyễn Thái Ngọc Duy,
Ævar Arnfjörð, SZEDER Gábor,
Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The idea of the `GETTEXT_POISON` mode is to test translated messages, at
least _somewhat_.
There is not really any point in turning off that mode by force, except
_maybe_ to test the mode itself.
So let's avoid overriding `GIT_TEST_GETTEXT_POISON` in the test suite
unless testing the `GETTEXT_POISON` functionality itself.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/t0017-env-helper.sh | 6 ++----
t/t1305-config-include.sh | 6 ++----
t/t7201-co.sh | 4 ++--
t/t9902-completion.sh | 1 -
4 files changed, 6 insertions(+), 11 deletions(-)
diff --git a/t/t0017-env-helper.sh b/t/t0017-env-helper.sh
index c1ecf6aeac6..e0931310306 100755
--- a/t/t0017-env-helper.sh
+++ b/t/t0017-env-helper.sh
@@ -85,10 +85,8 @@ test_expect_success 'env--helper reads config thanks to trace2' '
git config -f home/.gitconfig include.path cycle &&
git config -f home/cycle include.path .gitconfig &&
- test_must_fail \
- env HOME="$(pwd)/home" GIT_TEST_GETTEXT_POISON=false \
- git config -l 2>err &&
- grep "exceeded maximum include depth" err &&
+ test_must_fail env HOME="$(pwd)/home" git config -l 2>err &&
+ test_i18ngrep "exceeded maximum include depth" err &&
test_must_fail \
env HOME="$(pwd)/home" GIT_TEST_GETTEXT_POISON=true \
diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
index f1e1b289f98..308c5d530b3 100755
--- a/t/t1305-config-include.sh
+++ b/t/t1305-config-include.sh
@@ -352,10 +352,8 @@ test_expect_success 'include cycles are detected' '
git init --bare cycle &&
git -C cycle config include.path cycle &&
git config -f cycle/cycle include.path config &&
- test_must_fail \
- env GIT_TEST_GETTEXT_POISON=false \
- git -C cycle config --get-all test.value 2>stderr &&
- grep "exceeded maximum include depth" stderr
+ test_must_fail git -C cycle config --get-all test.value 2>stderr &&
+ test_i18ngrep "exceeded maximum include depth" stderr
'
test_done
diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index b36a93056fd..630406a73c5 100755
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh
@@ -245,8 +245,8 @@ test_expect_success 'checkout to detach HEAD' '
rev=$(git rev-parse --short renamer^) &&
git checkout -f renamer &&
git clean -f &&
- GIT_TEST_GETTEXT_POISON=false git checkout renamer^ 2>messages &&
- grep "HEAD is now at $rev" messages &&
+ git checkout renamer^ 2>messages &&
+ test_i18ngrep "HEAD is now at $rev" messages &&
test_line_count -gt 1 messages &&
H=$(git rev-parse --verify HEAD) &&
M=$(git show-ref -s --verify refs/heads/master) &&
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index a1c4f1f6d40..e5adee27d41 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -2363,7 +2363,6 @@ test_expect_success 'sourcing the completion script clears cached commands' '
'
test_expect_success 'sourcing the completion script clears cached merge strategies' '
- GIT_TEST_GETTEXT_POISON=false &&
__git_compute_merge_strategies &&
verbose test -n "$__git_merge_strategies" &&
. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash" &&
--
gitgitgadget
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 01/11] tests: remove unnecessary GIT_TEST_GETTEXT_POISON=false constructs
2021-01-12 8:47 ` [PATCH 01/11] tests: remove unnecessary GIT_TEST_GETTEXT_POISON=false constructs Johannes Schindelin via GitGitGadget
@ 2021-01-12 9:07 ` SZEDER Gábor
0 siblings, 0 replies; 31+ messages in thread
From: SZEDER Gábor @ 2021-01-12 9:07 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Nguyễn Thái Ngọc Duy,
Ævar Arnfjörð, Johannes Schindelin
On Tue, Jan 12, 2021 at 08:47:32AM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> The idea of the `GETTEXT_POISON` mode is to test translated messages, at
> least _somewhat_.
>
> There is not really any point in turning off that mode by force, except
> _maybe_ to test the mode itself.
>
> So let's avoid overriding `GIT_TEST_GETTEXT_POISON` in the test suite
> unless testing the `GETTEXT_POISON` functionality itself.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index a1c4f1f6d40..e5adee27d41 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -2363,7 +2363,6 @@ test_expect_success 'sourcing the completion script clears cached commands' '
> '
>
> test_expect_success 'sourcing the completion script clears cached merge strategies' '
> - GIT_TEST_GETTEXT_POISON=false &&
I think this change caused the failure in t9902 that you mentioned in
the cover letter.
If 'git merge' is invoked with a nonexisting merge strategy, then it
errors out with an error message that contains a list of available
merge strategies. The completion script relies on this behavior and
"parses" this error message to get the available merge strategies, but
it breaks when it can't find the expected text because it was
poisoned.
> __git_compute_merge_strategies &&
> verbose test -n "$__git_merge_strategies" &&
> . "$GIT_BUILD_DIR/contrib/completion/git-completion.bash" &&
> --
> gitgitgadget
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 02/11] Support GIT_TEST_GETTEXT_POISON=rot13
2021-01-12 8:47 [PATCH 00/11] [RFH] Introduce support for GETTEXT_POISON=rot13 Johannes Schindelin via GitGitGadget
2021-01-12 8:47 ` [PATCH 01/11] tests: remove unnecessary GIT_TEST_GETTEXT_POISON=false constructs Johannes Schindelin via GitGitGadget
@ 2021-01-12 8:47 ` Johannes Schindelin via GitGitGadget
2021-01-12 19:37 ` Junio C Hamano
2021-01-12 8:47 ` [PATCH 03/11] parse-options: add forgotten translations Johannes Schindelin via GitGitGadget
` (10 subsequent siblings)
12 siblings, 1 reply; 31+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2021-01-12 8:47 UTC (permalink / raw)
To: git
Cc: Nguyễn Thái Ngọc Duy,
Ævar Arnfjörð, SZEDER Gábor,
Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
gettext.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
gettext.h | 5 +++--
2 files changed, 66 insertions(+), 4 deletions(-)
diff --git a/gettext.c b/gettext.c
index 35d2c1218db..7e8966d8326 100644
--- a/gettext.c
+++ b/gettext.c
@@ -68,11 +68,72 @@ const char *get_preferred_languages(void)
int use_gettext_poison(void)
{
static int poison_requested = -1;
- if (poison_requested == -1)
- poison_requested = git_env_bool("GIT_TEST_GETTEXT_POISON", 0);
+ if (poison_requested == -1) {
+ const char *v = getenv("GIT_TEST_GETTEXT_POISON");
+ if (v && !strcmp(v, "rot13"))
+ poison_requested = 2;
+ else
+ poison_requested =
+ git_env_bool("GIT_TEST_GETTEXT_POISON", 0);
+ }
return poison_requested;
}
+static inline char do_rot13(char c)
+{
+ if (c >= 'a' && c <= 'm')
+ return c + 'n' - 'a';
+ if (c >= 'n' && c <= 'z')
+ return c + 'a' - 'n';
+ if (c >= 'A' && c <= 'M')
+ return c + 'N' - 'A';
+ if (c >= 'N' && c <= 'Z')
+ return c + 'A' - 'N';
+ return c;
+}
+
+const char *gettext_maybe_rot13(const char *msgid)
+{
+ static struct strbuf round_robin[4] = {
+ STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT
+ };
+ static int current;
+ struct strbuf *buf;
+
+ if (use_gettext_poison() != 2)
+ return "# GETTEXT POISON #";
+
+ buf = &round_robin[current++];
+ if (current >= ARRAY_SIZE(round_robin))
+ current = 0;
+
+ strbuf_reset(buf);
+ while (*msgid) {
+ const char *p = strchrnul(msgid, '%'), *spec;
+
+ while (*p && p[1] == '%')
+ p = strchrnul(p + 2, '%');
+
+ if (p != msgid) {
+ strbuf_addstr(buf, "<rot13>");
+ while (p != msgid)
+ strbuf_addch(buf, do_rot13(*(msgid++)));
+ strbuf_addstr(buf, "</rot13>");
+ }
+
+ if (!*p)
+ break;
+
+ spec = strpbrk(p + 1, "diouxXeEfFgGaAcsCSpnm%");
+ if (!spec)
+ BUG("Unrecognized format string: %s", p);
+ strbuf_add(buf, p, spec + 1 - p);
+ msgid = spec + 1;
+ }
+
+ return buf->buf;
+}
+
#ifndef NO_GETTEXT
static int test_vsnprintf(const char *fmt, ...)
{
diff --git a/gettext.h b/gettext.h
index bee52eb1134..298adda7e15 100644
--- a/gettext.h
+++ b/gettext.h
@@ -29,6 +29,7 @@
#define FORMAT_PRESERVING(n) __attribute__((format_arg(n)))
int use_gettext_poison(void);
+const char *gettext_maybe_rot13(const char *msgid);
#ifndef NO_GETTEXT
void git_setup_gettext(void);
@@ -48,14 +49,14 @@ static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)
{
if (!*msgid)
return "";
- return use_gettext_poison() ? "# GETTEXT POISON #" : gettext(msgid);
+ return use_gettext_poison() ? gettext_maybe_rot13(msgid) : gettext(msgid);
}
static inline FORMAT_PRESERVING(1) FORMAT_PRESERVING(2)
const char *Q_(const char *msgid, const char *plu, unsigned long n)
{
if (use_gettext_poison())
- return "# GETTEXT POISON #";
+ return gettext_maybe_rot13(n < 2 ? msgid : plu);
return ngettext(msgid, plu, n);
}
--
gitgitgadget
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 02/11] Support GIT_TEST_GETTEXT_POISON=rot13
2021-01-12 8:47 ` [PATCH 02/11] Support GIT_TEST_GETTEXT_POISON=rot13 Johannes Schindelin via GitGitGadget
@ 2021-01-12 19:37 ` Junio C Hamano
0 siblings, 0 replies; 31+ messages in thread
From: Junio C Hamano @ 2021-01-12 19:37 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Nguyễn Thái Ngọc Duy,
Ævar Arnfjörð, SZEDER Gábor,
Johannes Schindelin
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> +const char *gettext_maybe_rot13(const char *msgid)
> +{
> +...
> + while (*msgid) {
> + const char *p = strchrnul(msgid, '%'), *spec;
> +
> + while (*p && p[1] == '%')
> + p = strchrnul(p + 2, '%');
We are at '%', and while the next is '%' (i.e. a literal '%' is
asked), we skip these two bytes and then we go back to the
equivalent of the initialization of 'p'. Effects? We leave the
loop when we are at the end of the string, or at '%' that is not
followed by a '%'.
It would have been easier to understand what is going on in the loop
if there weren't duplicated strchrnul() call, perhaps
const char *p = msgid;
while (*(p = strchrnul(p, '%')) == '%' && p[1] == '%')
p += 2;
That reads "find '%' and if the next char is also '%', skip these
two and redo the loop", which would be equivalent. And it would be
much easier to follow the logic.
> + if (p != msgid) {
And if 'p' is different (actually we know p is the same as, or
always ahead of, msgid, so "if (msgid < p)" would be easier to
follow for readers) ...
> + strbuf_addstr(buf, "<rot13>");
> + while (p != msgid)
Ditto.
> + strbuf_addch(buf, do_rot13(*(msgid++)));
> + strbuf_addstr(buf, "</rot13>");
... we add a section of obfuscated output enclosed with an xml
looking marker.
> + }
> +
> + if (!*p)
> + break;
And if we are at the end, we are done.
> + spec = strpbrk(p + 1, "diouxXeEfFgGaAcsCSpnm%");
> + if (!spec)
> + BUG("Unrecognized format string: %s", p);
It is a bit surprising that things like "%.*d" and "%.7f" do not
appear in our translatable strings (or perhaps this is one of the
places why the series is marked RFH and is not complete?).
> + strbuf_add(buf, p, spec + 1 - p);
> + msgid = spec + 1;
> + }
> +
> + return buf->buf;
> +}
Anyway, thanks for a fun reading.
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 03/11] parse-options: add forgotten translations
2021-01-12 8:47 [PATCH 00/11] [RFH] Introduce support for GETTEXT_POISON=rot13 Johannes Schindelin via GitGitGadget
2021-01-12 8:47 ` [PATCH 01/11] tests: remove unnecessary GIT_TEST_GETTEXT_POISON=false constructs Johannes Schindelin via GitGitGadget
2021-01-12 8:47 ` [PATCH 02/11] Support GIT_TEST_GETTEXT_POISON=rot13 Johannes Schindelin via GitGitGadget
@ 2021-01-12 8:47 ` Johannes Schindelin via GitGitGadget
2021-01-12 8:47 ` [PATCH 04/11] sha1dc: mark forgotten message for translation Johannes Schindelin via GitGitGadget
` (9 subsequent siblings)
12 siblings, 0 replies; 31+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2021-01-12 8:47 UTC (permalink / raw)
To: git
Cc: Nguyễn Thái Ngọc Duy,
Ævar Arnfjörð, SZEDER Gábor,
Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
parse-options-cb.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/parse-options-cb.c b/parse-options-cb.c
index 4542d4d3f92..00862af486e 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -91,7 +91,7 @@ int parse_opt_commits(const struct option *opt, const char *arg, int unset)
if (!arg)
return -1;
if (get_oid(arg, &oid))
- return error("malformed object name %s", arg);
+ return error(_("malformed object name %s"), arg);
commit = lookup_commit_reference(the_repository, &oid);
if (!commit)
return error("no such commit %s", arg);
@@ -110,7 +110,7 @@ int parse_opt_commit(const struct option *opt, const char *arg, int unset)
if (!arg)
return -1;
if (get_oid(arg, &oid))
- return error("malformed object name %s", arg);
+ return error(_("malformed object name %s"), arg);
commit = lookup_commit_reference(the_repository, &oid);
if (!commit)
return error("no such commit %s", arg);
--
gitgitgadget
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH 04/11] sha1dc: mark forgotten message for translation
2021-01-12 8:47 [PATCH 00/11] [RFH] Introduce support for GETTEXT_POISON=rot13 Johannes Schindelin via GitGitGadget
` (2 preceding siblings ...)
2021-01-12 8:47 ` [PATCH 03/11] parse-options: add forgotten translations Johannes Schindelin via GitGitGadget
@ 2021-01-12 8:47 ` Johannes Schindelin via GitGitGadget
2021-01-12 11:37 ` Jeff King
2021-01-12 8:47 ` [PATCH 05/11] t0006: use `test_i18ncmp` only for C locales Johannes Schindelin via GitGitGadget
` (8 subsequent siblings)
12 siblings, 1 reply; 31+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2021-01-12 8:47 UTC (permalink / raw)
To: git
Cc: Nguyễn Thái Ngọc Duy,
Ævar Arnfjörð, SZEDER Gábor,
Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sha1dc_git.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sha1dc_git.c b/sha1dc_git.c
index 5c300e812e0..fecf5da1483 100644
--- a/sha1dc_git.c
+++ b/sha1dc_git.c
@@ -18,7 +18,7 @@ void git_SHA1DCFinal(unsigned char hash[20], SHA1_CTX *ctx)
{
if (!SHA1DCFinal(hash, ctx))
return;
- die("SHA-1 appears to be part of a collision attack: %s",
+ die(_("SHA-1 appears to be part of a collision attack: %s"),
hash_to_hex_algop(hash, &hash_algos[GIT_HASH_SHA1]));
}
--
gitgitgadget
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 04/11] sha1dc: mark forgotten message for translation
2021-01-12 8:47 ` [PATCH 04/11] sha1dc: mark forgotten message for translation Johannes Schindelin via GitGitGadget
@ 2021-01-12 11:37 ` Jeff King
2021-01-12 19:40 ` Junio C Hamano
2021-01-15 15:43 ` Johannes Schindelin
0 siblings, 2 replies; 31+ messages in thread
From: Jeff King @ 2021-01-12 11:37 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Nguyễn Thái Ngọc Duy,
Ævar Arnfjörð, SZEDER Gábor,
Johannes Schindelin
On Tue, Jan 12, 2021 at 08:47:35AM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/sha1dc_git.c b/sha1dc_git.c
> index 5c300e812e0..fecf5da1483 100644
> --- a/sha1dc_git.c
> +++ b/sha1dc_git.c
> @@ -18,7 +18,7 @@ void git_SHA1DCFinal(unsigned char hash[20], SHA1_CTX *ctx)
> {
> if (!SHA1DCFinal(hash, ctx))
> return;
> - die("SHA-1 appears to be part of a collision attack: %s",
> + die(_("SHA-1 appears to be part of a collision attack: %s"),
> hash_to_hex_algop(hash, &hash_algos[GIT_HASH_SHA1]));
I didn't find any list discussion, but I think I may have actually left
this untranslated intentionally. Like a BUG(), we'd expect it to come up
basically never. And when it does, being able to search for the exact
wording online may be more important than providing a translated
version.
It probably doesn't matter that much either way, though.
-Peff
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 04/11] sha1dc: mark forgotten message for translation
2021-01-12 11:37 ` Jeff King
@ 2021-01-12 19:40 ` Junio C Hamano
2021-01-15 15:43 ` Johannes Schindelin
1 sibling, 0 replies; 31+ messages in thread
From: Junio C Hamano @ 2021-01-12 19:40 UTC (permalink / raw)
To: Jeff King
Cc: Johannes Schindelin via GitGitGadget, git,
Nguyễn Thái Ngọc Duy,
Ævar Arnfjörð, SZEDER Gábor,
Johannes Schindelin
Jeff King <peff@peff.net> writes:
> On Tue, Jan 12, 2021 at 08:47:35AM +0000, Johannes Schindelin via GitGitGadget wrote:
>
>> diff --git a/sha1dc_git.c b/sha1dc_git.c
>> index 5c300e812e0..fecf5da1483 100644
>> --- a/sha1dc_git.c
>> +++ b/sha1dc_git.c
>> @@ -18,7 +18,7 @@ void git_SHA1DCFinal(unsigned char hash[20], SHA1_CTX *ctx)
>> {
>> if (!SHA1DCFinal(hash, ctx))
>> return;
>> - die("SHA-1 appears to be part of a collision attack: %s",
>> + die(_("SHA-1 appears to be part of a collision attack: %s"),
>> hash_to_hex_algop(hash, &hash_algos[GIT_HASH_SHA1]));
>
> I didn't find any list discussion, but I think I may have actually left
> this untranslated intentionally. Like a BUG(), we'd expect it to come up
> basically never. And when it does, being able to search for the exact
> wording online may be more important than providing a translated
> version.
>
> It probably doesn't matter that much either way, though.
I can believe if this was intentionally left untranslated for the
reasons you stated. Without introducing "message ID" that can be
universally readable, so that customers can report "We got E42234
when running Git version 1.2.3.4", that is the best we could do.
But I am OK to see this translated, too.
Thanks.
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 04/11] sha1dc: mark forgotten message for translation
2021-01-12 11:37 ` Jeff King
2021-01-12 19:40 ` Junio C Hamano
@ 2021-01-15 15:43 ` Johannes Schindelin
2021-01-15 16:29 ` Jeff King
1 sibling, 1 reply; 31+ messages in thread
From: Johannes Schindelin @ 2021-01-15 15:43 UTC (permalink / raw)
To: Jeff King
Cc: Johannes Schindelin via GitGitGadget, git,
Nguyễn Thái Ngọc Duy,
Ævar Arnfjörð, SZEDER Gábor
Hi Peff,
On Tue, 12 Jan 2021, Jeff King wrote:
> On Tue, Jan 12, 2021 at 08:47:35AM +0000, Johannes Schindelin via GitGitGadget wrote:
>
> > diff --git a/sha1dc_git.c b/sha1dc_git.c
> > index 5c300e812e0..fecf5da1483 100644
> > --- a/sha1dc_git.c
> > +++ b/sha1dc_git.c
> > @@ -18,7 +18,7 @@ void git_SHA1DCFinal(unsigned char hash[20], SHA1_CTX *ctx)
> > {
> > if (!SHA1DCFinal(hash, ctx))
> > return;
> > - die("SHA-1 appears to be part of a collision attack: %s",
> > + die(_("SHA-1 appears to be part of a collision attack: %s"),
> > hash_to_hex_algop(hash, &hash_algos[GIT_HASH_SHA1]));
>
> I didn't find any list discussion, but I think I may have actually left
> this untranslated intentionally. Like a BUG(), we'd expect it to come up
> basically never. And when it does, being able to search for the exact
> wording online may be more important than providing a translated
> version.
I disagree with that reasoning. By that rationale, any message we deem to
be somewhat rare should be _untranslated_.
A much better rule, at least from my perspective is: is the target
audience the Git users? If so, the message is to be translated. If not,
then not.
In this instance, it is quite obviously targeting the Git users who need
to understand why the command they tried to run was failing. The test in
t0013 is totally agreeing with this:
test_expect_success 'test-sha1 detects shattered pdf' '
test_must_fail test-tool sha1 <"$TEST_DATA/shattered-1.pdf" 2>err &&
test_i18ngrep collision err &&
grep 38762cf7f55934b34d179ae6a4c80cadccbb7f0a err
'
Notice that `test_i18ngrep`? It tells me that we expect this message to be
translated.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 04/11] sha1dc: mark forgotten message for translation
2021-01-15 15:43 ` Johannes Schindelin
@ 2021-01-15 16:29 ` Jeff King
2021-01-18 14:26 ` Johannes Schindelin
0 siblings, 1 reply; 31+ messages in thread
From: Jeff King @ 2021-01-15 16:29 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Johannes Schindelin via GitGitGadget, git,
Nguyễn Thái Ngọc Duy,
Ævar Arnfjörð, SZEDER Gábor
On Fri, Jan 15, 2021 at 04:43:05PM +0100, Johannes Schindelin wrote:
> > > - die("SHA-1 appears to be part of a collision attack: %s",
> > > + die(_("SHA-1 appears to be part of a collision attack: %s"),
> > > hash_to_hex_algop(hash, &hash_algos[GIT_HASH_SHA1]));
> >
> > I didn't find any list discussion, but I think I may have actually left
> > this untranslated intentionally. Like a BUG(), we'd expect it to come up
> > basically never. And when it does, being able to search for the exact
> > wording online may be more important than providing a translated
> > version.
>
> I disagree with that reasoning. By that rationale, any message we deem to
> be somewhat rare should be _untranslated_.
>
> A much better rule, at least from my perspective is: is the target
> audience the Git users? If so, the message is to be translated. If not,
> then not.
That's what I was getting at. The audience is really Git developers,
just like it would be for a BUG(). We don't expect either of those
things to happen.
> In this instance, it is quite obviously targeting the Git users who need
> to understand why the command they tried to run was failing. The test in
> t0013 is totally agreeing with this:
>
> test_expect_success 'test-sha1 detects shattered pdf' '
> test_must_fail test-tool sha1 <"$TEST_DATA/shattered-1.pdf" 2>err &&
> test_i18ngrep collision err &&
> grep 38762cf7f55934b34d179ae6a4c80cadccbb7f0a err
> '
>
> Notice that `test_i18ngrep`? It tells me that we expect this message to be
> translated.
Well, I wrote both that line and the untranslated original code, so I'm
not sure what we can deduce from that. ;)
But yeah, I am not strongly opposed to translating this. I brought it up
more in the line of "I don't think it's that big a deal that it was not
translated".
-Peff
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 04/11] sha1dc: mark forgotten message for translation
2021-01-15 16:29 ` Jeff King
@ 2021-01-18 14:26 ` Johannes Schindelin
2021-01-18 21:06 ` Junio C Hamano
0 siblings, 1 reply; 31+ messages in thread
From: Johannes Schindelin @ 2021-01-18 14:26 UTC (permalink / raw)
To: Jeff King
Cc: Johannes Schindelin via GitGitGadget, git,
Nguyễn Thái Ngọc Duy,
Ævar Arnfjörð, SZEDER Gábor
Hi Peff,
On Fri, 15 Jan 2021, Jeff King wrote:
> On Fri, Jan 15, 2021 at 04:43:05PM +0100, Johannes Schindelin wrote:
>
> > > > - die("SHA-1 appears to be part of a collision attack: %s",
> > > > + die(_("SHA-1 appears to be part of a collision attack: %s"),
> > > > hash_to_hex_algop(hash, &hash_algos[GIT_HASH_SHA1]));
> > >
> > > I didn't find any list discussion, but I think I may have actually left
> > > this untranslated intentionally. Like a BUG(), we'd expect it to come up
> > > basically never. And when it does, being able to search for the exact
> > > wording online may be more important than providing a translated
> > > version.
> >
> > I disagree with that reasoning. By that rationale, any message we deem to
> > be somewhat rare should be _untranslated_.
> >
> > A much better rule, at least from my perspective is: is the target
> > audience the Git users? If so, the message is to be translated. If not,
> > then not.
>
> That's what I was getting at. The audience is really Git developers,
> just like it would be for a BUG(). We don't expect either of those
> things to happen.
While a SHA-1 collision might not be anything we expect to happen, I am
fairly certain it won't be a bug in Git causing it. Nor will it be
anything that core Git developers have to react on. For those reasons, I
disagree that core Git developers are the target audience of this message.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 04/11] sha1dc: mark forgotten message for translation
2021-01-18 14:26 ` Johannes Schindelin
@ 2021-01-18 21:06 ` Junio C Hamano
2021-01-19 15:52 ` Johannes Schindelin
0 siblings, 1 reply; 31+ messages in thread
From: Junio C Hamano @ 2021-01-18 21:06 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Jeff King, Johannes Schindelin via GitGitGadget, git,
Nguyễn Thái Ngọc Duy,
Ævar Arnfjörð, SZEDER Gábor
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> That's what I was getting at. The audience is really Git developers,
>> just like it would be for a BUG(). We don't expect either of those
>> things to happen.
>
> While a SHA-1 collision might not be anything we expect to happen, I am
> fairly certain it won't be a bug in Git causing it. Nor will it be
> anything that core Git developers have to react on. For those reasons, I
> disagree that core Git developers are the target audience of this message.
I do not know if this is what Peff meant by "the audience is really
Git developers", but when any end-user encounters this message, we
want to learn about it a lot more urgently than all the ordinary
"there is no such command line option", so in that sense, even it
is not "a bug in git", it is more special than ordinary errors.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 04/11] sha1dc: mark forgotten message for translation
2021-01-18 21:06 ` Junio C Hamano
@ 2021-01-19 15:52 ` Johannes Schindelin
0 siblings, 0 replies; 31+ messages in thread
From: Johannes Schindelin @ 2021-01-19 15:52 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Johannes Schindelin via GitGitGadget, git,
Nguyễn Thái Ngọc Duy,
Ævar Arnfjörð, SZEDER Gábor
Hi Junio,
On Mon, 18 Jan 2021, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> That's what I was getting at. The audience is really Git developers,
> >> just like it would be for a BUG(). We don't expect either of those
> >> things to happen.
> >
> > While a SHA-1 collision might not be anything we expect to happen, I am
> > fairly certain it won't be a bug in Git causing it. Nor will it be
> > anything that core Git developers have to react on. For those reasons, I
> > disagree that core Git developers are the target audience of this message.
>
> I do not know if this is what Peff meant by "the audience is really
> Git developers", but when any end-user encounters this message, we
> want to learn about it a lot more urgently than all the ordinary
> "there is no such command line option", so in that sense, even it
> is not "a bug in git", it is more special than ordinary errors.
I suggest that we modify the message to state exactly that: "Please
contact the Git mailing list at git@vger.kernel.org about this". And then
mark the message for translation, so that even Git users with low/no
knowledge of the English language are in a position to help us.
Ciao,
Dscho
P.S.: Yes, I realize that this means we could receive a message reporting
a SHA-1 collision written in, say, Chinese. Is this a problem? I don't
think so. In any case, this would still be much better than keeping the
message untranslated and _not_ receiving a mail about it.
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 05/11] t0006: use `test_i18ncmp` only for C locales
2021-01-12 8:47 [PATCH 00/11] [RFH] Introduce support for GETTEXT_POISON=rot13 Johannes Schindelin via GitGitGadget
` (3 preceding siblings ...)
2021-01-12 8:47 ` [PATCH 04/11] sha1dc: mark forgotten message for translation Johannes Schindelin via GitGitGadget
@ 2021-01-12 8:47 ` Johannes Schindelin via GitGitGadget
2021-01-12 8:47 ` [PATCH 06/11] t0041: stop using `test_i18ngrep` on untranslated output Johannes Schindelin via GitGitGadget
` (7 subsequent siblings)
12 siblings, 0 replies; 31+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2021-01-12 8:47 UTC (permalink / raw)
To: git
Cc: Nguyễn Thái Ngọc Duy,
Ævar Arnfjörð, SZEDER Gábor,
Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
We cannot really test anything else, as we do not control the output of
`strftime()`.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/t0006-date.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t0006-date.sh b/t/t0006-date.sh
index 75ee9a96b80..9f27e202d36 100755
--- a/t/t0006-date.sh
+++ b/t/t0006-date.sh
@@ -9,7 +9,7 @@ GIT_TEST_DATE_NOW=1251660000; export GIT_TEST_DATE_NOW
check_relative() {
t=$(($GIT_TEST_DATE_NOW - $1))
echo "$t -> $2" >expect
- test_expect_${3:-success} "relative date ($2)" "
+ test_expect_${3:-success} C_LOCALE_OUTPUT "relative date ($2)" "
test-tool date relative $t >actual &&
test_i18ncmp expect actual
"
@@ -137,7 +137,7 @@ check_approxidate '2009-12-01' '2009-12-01 19:20:00'
check_date_format_human() {
t=$(($GIT_TEST_DATE_NOW - $1))
echo "$t -> $2" >expect
- test_expect_success "human date $t" '
+ test_expect_success C_LOCALE_OUTPUT "human date $t" '
test-tool date human $t >actual &&
test_i18ncmp expect actual
'
--
gitgitgadget
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH 06/11] t0041: stop using `test_i18ngrep` on untranslated output
2021-01-12 8:47 [PATCH 00/11] [RFH] Introduce support for GETTEXT_POISON=rot13 Johannes Schindelin via GitGitGadget
` (4 preceding siblings ...)
2021-01-12 8:47 ` [PATCH 05/11] t0006: use `test_i18ncmp` only for C locales Johannes Schindelin via GitGitGadget
@ 2021-01-12 8:47 ` Johannes Schindelin via GitGitGadget
2021-01-12 8:47 ` [PATCH 07/11] t0027: mark a function as requiring the C locale Johannes Schindelin via GitGitGadget
` (6 subsequent siblings)
12 siblings, 0 replies; 31+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2021-01-12 8:47 UTC (permalink / raw)
To: git
Cc: Nguyễn Thái Ngọc Duy,
Ævar Arnfjörð, SZEDER Gábor,
Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/t0041-usage.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t0041-usage.sh b/t/t0041-usage.sh
index 5b927b76fe5..27309b1a8f8 100755
--- a/t/t0041-usage.sh
+++ b/t/t0041-usage.sh
@@ -42,7 +42,7 @@ test_expect_success 'tag usage error' '
test_expect_success 'branch --contains <existent_commit>' '
git branch --contains "master" >actual 2>actual.err &&
- test_i18ngrep "master" actual &&
+ grep "master" actual &&
test_line_count = 0 actual.err
'
--
gitgitgadget
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH 07/11] t0027: mark a function as requiring the C locale
2021-01-12 8:47 [PATCH 00/11] [RFH] Introduce support for GETTEXT_POISON=rot13 Johannes Schindelin via GitGitGadget
` (5 preceding siblings ...)
2021-01-12 8:47 ` [PATCH 06/11] t0041: stop using `test_i18ngrep` on untranslated output Johannes Schindelin via GitGitGadget
@ 2021-01-12 8:47 ` Johannes Schindelin via GitGitGadget
2021-01-12 8:47 ` [PATCH 08/11] t6301: do not expect the output of `for-each-ref` to be translated Johannes Schindelin via GitGitGadget
` (5 subsequent siblings)
12 siblings, 0 replies; 31+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2021-01-12 8:47 UTC (permalink / raw)
To: git
Cc: Nguyễn Thái Ngọc Duy,
Ævar Arnfjörð, SZEDER Gábor,
Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The `check_warning` function uses regular expressions matching English
text, it therefore won't work reliably with anything but the C locale.
In particular, we won't be able to match the needle 'in' under
GETTEXT_POISON=rot13.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/t0027-auto-crlf.sh | 3 +++
1 file changed, 3 insertions(+)
diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
index 9fcd56fab37..baa5c6f435b 100755
--- a/t/t0027-auto-crlf.sh
+++ b/t/t0027-auto-crlf.sh
@@ -76,6 +76,9 @@ create_NNO_MIX_files () {
}
check_warning () {
+ # The `grep` and `sed` below expect the locale to be "C"
+ test_have_prereq C_LOCALE_OUTPUT || return 0
+
case "$1" in
LF_CRLF) echo "warning: LF will be replaced by CRLF" >"$2".expect ;;
CRLF_LF) echo "warning: CRLF will be replaced by LF" >"$2".expect ;;
--
gitgitgadget
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH 08/11] t6301: do not expect the output of `for-each-ref` to be translated
2021-01-12 8:47 [PATCH 00/11] [RFH] Introduce support for GETTEXT_POISON=rot13 Johannes Schindelin via GitGitGadget
` (6 preceding siblings ...)
2021-01-12 8:47 ` [PATCH 07/11] t0027: mark a function as requiring the C locale Johannes Schindelin via GitGitGadget
@ 2021-01-12 8:47 ` Johannes Schindelin via GitGitGadget
2021-01-12 8:47 ` [PATCH 09/11] GETTEXT_POISON=rot13: do compare the output in `test_i18ncmp` Johannes Schindelin via GitGitGadget
` (4 subsequent siblings)
12 siblings, 0 replies; 31+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2021-01-12 8:47 UTC (permalink / raw)
To: git
Cc: Nguyễn Thái Ngọc Duy,
Ævar Arnfjörð, SZEDER Gábor,
Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The `test_i18ncmp` function is reserved for output that we expect to be
translated.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/t6301-for-each-ref-errors.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t6301-for-each-ref-errors.sh b/t/t6301-for-each-ref-errors.sh
index 809854fc0ce..0cf2a41c26c 100755
--- a/t/t6301-for-each-ref-errors.sh
+++ b/t/t6301-for-each-ref-errors.sh
@@ -20,7 +20,7 @@ test_expect_success 'Broken refs are reported correctly' '
test_when_finished "rm -f .git/$r" &&
echo "warning: ignoring broken ref $r" >broken-err &&
git for-each-ref >out 2>err &&
- test_i18ncmp full-list out &&
+ test_cmp full-list out &&
test_i18ncmp broken-err err
'
--
gitgitgadget
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH 09/11] GETTEXT_POISON=rot13: do compare the output in `test_i18ncmp`
2021-01-12 8:47 [PATCH 00/11] [RFH] Introduce support for GETTEXT_POISON=rot13 Johannes Schindelin via GitGitGadget
` (7 preceding siblings ...)
2021-01-12 8:47 ` [PATCH 08/11] t6301: do not expect the output of `for-each-ref` to be translated Johannes Schindelin via GitGitGadget
@ 2021-01-12 8:47 ` Johannes Schindelin via GitGitGadget
2021-01-12 19:47 ` Junio C Hamano
2021-01-12 8:47 ` [PATCH 10/11] GETTEXT_POISON=rot13: perform actual checks in `test_i18ngrep` Johannes Schindelin via GitGitGadget
` (3 subsequent siblings)
12 siblings, 1 reply; 31+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2021-01-12 8:47 UTC (permalink / raw)
To: git
Cc: Nguyễn Thái Ngọc Duy,
Ævar Arnfjörð, SZEDER Gábor,
Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Makefile | 1 +
t/helper/test-i18n.c | 89 +++++++++++++++++++++++++++++++++++++++++
t/helper/test-tool.c | 1 +
t/helper/test-tool.h | 1 +
t/test-lib-functions.sh | 8 +++-
t/test-lib.sh | 1 +
6 files changed, 100 insertions(+), 1 deletion(-)
create mode 100644 t/helper/test-i18n.c
diff --git a/Makefile b/Makefile
index 7b64106930a..d49f80cb524 100644
--- a/Makefile
+++ b/Makefile
@@ -710,6 +710,7 @@ TEST_BUILTINS_OBJS += test-genzeros.o
TEST_BUILTINS_OBJS += test-hash-speed.o
TEST_BUILTINS_OBJS += test-hash.o
TEST_BUILTINS_OBJS += test-hashmap.o
+TEST_BUILTINS_OBJS += test-i18n.o
TEST_BUILTINS_OBJS += test-index-version.o
TEST_BUILTINS_OBJS += test-json-writer.o
TEST_BUILTINS_OBJS += test-lazy-init-name-hash.o
diff --git a/t/helper/test-i18n.c b/t/helper/test-i18n.c
new file mode 100644
index 00000000000..4b572e6efad
--- /dev/null
+++ b/t/helper/test-i18n.c
@@ -0,0 +1,89 @@
+#include "test-tool.h"
+#include "cache.h"
+
+static const char *usage_msg = "\n"
+" test-tool i18n cmp <file1> <file2>\n";
+
+static inline char do_rot13(char c)
+{
+ if (c >= 'a' && c <= 'm')
+ return c + 'n' - 'a';
+ if (c >= 'n' && c <= 'z')
+ return c + 'a' - 'n';
+ if (c >= 'A' && c <= 'M')
+ return c + 'N' - 'A';
+ if (c >= 'N' && c <= 'Z')
+ return c + 'A' - 'N';
+ return c;
+}
+
+static size_t unrot13(char *buf)
+{
+ char *p = buf, *q = buf;
+
+ while (*p) {
+ const char *begin = strstr(p, "<rot13>"), *end;
+
+ if (!begin)
+ break;
+
+ while (p != begin)
+ *(q++) = *(p++);
+
+ p += strlen("<rot13>");
+ end = strstr(p, "</rot13>");
+ if (!end)
+ BUG("could not find </rot13> in\n%s", buf);
+
+ while (p != end)
+ *(q++) = do_rot13(*(p++));
+ p += strlen("</rot13>");
+ }
+
+ while (*p)
+ *(q++) = *(p++);
+
+ return q - buf;
+}
+
+static void unrot13_strbuf(struct strbuf *buf)
+{
+ size_t len = unrot13(buf->buf);
+
+ if (len == buf->len)
+ die("not ROT13'ed:\n%s", buf->buf);
+ buf->len = len;
+}
+
+static int i18n_cmp(const char **argv)
+{
+ const char *path1 = *(argv++), *path2 = *(argv++);
+ struct strbuf a = STRBUF_INIT, b = STRBUF_INIT;
+
+ if (!path1 || !path2 || *argv)
+ usage(usage_msg);
+
+ if (strbuf_read_file(&a, path1, 0) < 0)
+ die_errno("could not read %s", path1);
+ if (strbuf_read_file(&b, path2, 0) < 0)
+ die_errno("could not read %s", path2);
+ unrot13_strbuf(&b);
+
+ if (a.len != b.len || memcmp(a.buf, b.buf, a.len))
+ return 1;
+
+ return 0;
+}
+
+int cmd__i18n(int argc, const char **argv)
+{
+ argv++;
+ if (!*argv)
+ usage(usage_msg);
+ if (!strcmp(*argv, "cmp"))
+ return i18n_cmp(argv+1);
+ else
+ usage(usage_msg);
+
+ return 0;
+}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 9d6d14d9293..7e1680ac108 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -34,6 +34,7 @@ static struct test_cmd cmds[] = {
{ "genzeros", cmd__genzeros },
{ "hashmap", cmd__hashmap },
{ "hash-speed", cmd__hash_speed },
+ { "i18n", cmd__i18n },
{ "index-version", cmd__index_version },
{ "json-writer", cmd__json_writer },
{ "lazy-init-name-hash", cmd__lazy_init_name_hash },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index a6470ff62c4..43282a520ea 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -24,6 +24,7 @@ int cmd__genrandom(int argc, const char **argv);
int cmd__genzeros(int argc, const char **argv);
int cmd__hashmap(int argc, const char **argv);
int cmd__hash_speed(int argc, const char **argv);
+int cmd__i18n(int argc, const char **argv);
int cmd__index_version(int argc, const char **argv);
int cmd__json_writer(int argc, const char **argv);
int cmd__lazy_init_name_hash(int argc, const char **argv);
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 999982fe4a9..08731bae854 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -993,7 +993,13 @@ test_cmp_bin () {
# under GIT_TEST_GETTEXT_POISON this pretends that the command produced expected
# results.
test_i18ncmp () {
- ! test_have_prereq C_LOCALE_OUTPUT || test_cmp "$@"
+ if test rot13 = "$GIT_TEST_GETTEXT_POISON"
+ then
+ test-tool i18n cmp "$@"
+ elif test_have_prereq C_LOCALE_OUTPUT
+ then
+ test_cmp "$@"
+ fi
}
# Use this instead of "grep expected-string actual" to see if the
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 9fa7c1d0f6d..c9f9e2804fd 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1537,6 +1537,7 @@ then
fi
test_lazy_prereq C_LOCALE_OUTPUT '
+ test rot13 != "$GIT_TEST_GETTEXT_POISON" &&
! test_bool_env GIT_TEST_GETTEXT_POISON false
'
--
gitgitgadget
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 09/11] GETTEXT_POISON=rot13: do compare the output in `test_i18ncmp`
2021-01-12 8:47 ` [PATCH 09/11] GETTEXT_POISON=rot13: do compare the output in `test_i18ncmp` Johannes Schindelin via GitGitGadget
@ 2021-01-12 19:47 ` Junio C Hamano
2021-01-15 15:44 ` Johannes Schindelin
0 siblings, 1 reply; 31+ messages in thread
From: Junio C Hamano @ 2021-01-12 19:47 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Nguyễn Thái Ngọc Duy,
Ævar Arnfjörð, SZEDER Gábor,
Johannes Schindelin
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> +static size_t unrot13(char *buf)
> +{
> + char *p = buf, *q = buf;
> +
> + while (*p) {
> + const char *begin = strstr(p, "<rot13>"), *end;
AFAIR from my reading of [02/11], the encoding side did not special
case the payload that has <ebg13> or </ebg13>; if we want to make it
reversible conversion (which is excellent improvement over the
current "# GETTEXT_POISON #" obfuscation), we'd need to do something
about it, I think.
But on second thought, nobody can prevent a caller to die(_("%s", msg));
to have "<rot13>" in the msg part, so perhaps punting like this
series does is sufficient. I dunno.
> + if (!begin)
> + break;
> +
> + while (p != begin)
> + *(q++) = *(p++);
> +
> + p += strlen("<rot13>");
> + end = strstr(p, "</rot13>");
> + if (!end)
> + BUG("could not find </rot13> in\n%s", buf);
And the user of this looks quite straightforward and nice.
> test_i18ncmp () {
> - ! test_have_prereq C_LOCALE_OUTPUT || test_cmp "$@"
> + if test rot13 = "$GIT_TEST_GETTEXT_POISON"
> + then
> + test-tool i18n cmp "$@"
> + elif test_have_prereq C_LOCALE_OUTPUT
> + then
> + test_cmp "$@"
> + fi
> }
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 09/11] GETTEXT_POISON=rot13: do compare the output in `test_i18ncmp`
2021-01-12 19:47 ` Junio C Hamano
@ 2021-01-15 15:44 ` Johannes Schindelin
2021-01-15 19:58 ` Junio C Hamano
0 siblings, 1 reply; 31+ messages in thread
From: Johannes Schindelin @ 2021-01-15 15:44 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin via GitGitGadget, git,
Nguyễn Thái Ngọc Duy,
Ævar Arnfjörð, SZEDER Gábor
Hi Junio,
On Tue, 12 Jan 2021, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > +static size_t unrot13(char *buf)
> > +{
> > + char *p = buf, *q = buf;
> > +
> > + while (*p) {
> > + const char *begin = strstr(p, "<rot13>"), *end;
>
> AFAIR from my reading of [02/11], the encoding side did not special
> case the payload that has <ebg13> or </ebg13>; if we want to make it
> reversible conversion (which is excellent improvement over the
> current "# GETTEXT_POISON #" obfuscation), we'd need to do something
> about it, I think.
Do you expect any message to be translated _twice_?
Ciao,
Dscho
> But on second thought, nobody can prevent a caller to die(_("%s", msg));
> to have "<rot13>" in the msg part, so perhaps punting like this
> series does is sufficient. I dunno.
>
> > + if (!begin)
> > + break;
> > +
> > + while (p != begin)
> > + *(q++) = *(p++);
> > +
> > + p += strlen("<rot13>");
> > + end = strstr(p, "</rot13>");
> > + if (!end)
> > + BUG("could not find </rot13> in\n%s", buf);
>
> And the user of this looks quite straightforward and nice.
>
> > test_i18ncmp () {
> > - ! test_have_prereq C_LOCALE_OUTPUT || test_cmp "$@"
> > + if test rot13 = "$GIT_TEST_GETTEXT_POISON"
> > + then
> > + test-tool i18n cmp "$@"
> > + elif test_have_prereq C_LOCALE_OUTPUT
> > + then
> > + test_cmp "$@"
> > + fi
> > }
>
>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 09/11] GETTEXT_POISON=rot13: do compare the output in `test_i18ncmp`
2021-01-15 15:44 ` Johannes Schindelin
@ 2021-01-15 19:58 ` Junio C Hamano
2021-01-18 14:36 ` Johannes Schindelin
0 siblings, 1 reply; 31+ messages in thread
From: Junio C Hamano @ 2021-01-15 19:58 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Johannes Schindelin via GitGitGadget, git,
Nguyễn Thái Ngọc Duy,
Ævar Arnfjörð, SZEDER Gábor
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> On Tue, 12 Jan 2021, Junio C Hamano wrote:
>
>> > +static size_t unrot13(char *buf)
>> > +{
>> > + char *p = buf, *q = buf;
>> > +
>> > + while (*p) {
>> > + const char *begin = strstr(p, "<rot13>"), *end;
>>
>> AFAIR from my reading of [02/11], the encoding side did not special
>> case the payload that has <ebg13> or </ebg13>; if we want to make it
>> reversible conversion (which is excellent improvement over the
>> current "# GETTEXT_POISON #" obfuscation), we'd need to do something
>> about it, I think.
>
> Do you expect any message to be translated _twice_?
Not at all.
But what I had in mind, when I wrote the above, was that the
programmers are entitled to expect that they are allowed to say:
die(_("message with <ebg13/>, <ebg13> and <rot13> in it"));
This will be rot13'd, and the entire thing will be enclosed inside
"<rot13>...</rot13>"; I would expect that somewhere inside "..." the
receiving end that attempts to parse it by relying on these markers
will be confused.
>> But on second thought, nobody can prevent a caller to die(_("%s", msg));
>> to have "<rot13>" in the msg part, so perhaps punting like this
>> series does is sufficient. I dunno.
And this comment still stands.
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 09/11] GETTEXT_POISON=rot13: do compare the output in `test_i18ncmp`
2021-01-15 19:58 ` Junio C Hamano
@ 2021-01-18 14:36 ` Johannes Schindelin
0 siblings, 0 replies; 31+ messages in thread
From: Johannes Schindelin @ 2021-01-18 14:36 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin via GitGitGadget, git,
Nguyễn Thái Ngọc Duy,
Ævar Arnfjörð, SZEDER Gábor
Hi Junio,
On Fri, 15 Jan 2021, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > On Tue, 12 Jan 2021, Junio C Hamano wrote:
> >
> >> > +static size_t unrot13(char *buf)
> >> > +{
> >> > + char *p = buf, *q = buf;
> >> > +
> >> > + while (*p) {
> >> > + const char *begin = strstr(p, "<rot13>"), *end;
> >>
> >> AFAIR from my reading of [02/11], the encoding side did not special
> >> case the payload that has <ebg13> or </ebg13>; if we want to make it
> >> reversible conversion (which is excellent improvement over the
> >> current "# GETTEXT_POISON #" obfuscation), we'd need to do something
> >> about it, I think.
> >
> > Do you expect any message to be translated _twice_?
>
> Not at all.
>
> But what I had in mind, when I wrote the above, was that the
> programmers are entitled to expect that they are allowed to say:
>
> die(_("message with <ebg13/>, <ebg13> and <rot13> in it"));
>
> This will be rot13'd, and the entire thing will be enclosed inside
> "<rot13>...</rot13>"; I would expect that somewhere inside "..." the
> receiving end that attempts to parse it by relying on these markers
> will be confused.
Oh, _that_ is what you meant.
Yep, I don't expect any of Git's messages to contain "<ebg13>", ever.
> >> But on second thought, nobody can prevent a caller to die(_("%s", msg));
> >> to have "<rot13>" in the msg part, so perhaps punting like this
> >> series does is sufficient. I dunno.
>
> And this comment still stands.
Slightly pedantic: the call would be `die(_("%s"), msg)` (note that the
`msg` is not inside the `_(...)`).
Similar to my point above, I do not expect any of Git's tests to emit a
message like that, and certainly not once/if we turn on rot13 mode in the
GETTEXT_POISON job of the CI build.
A blocker would have been if there might be the possibility that we _must_
add a test in the future that wants to emit a `msg` containing `<rot13>`.
But that seems utterly unlikely. And even if: then we'd just change the
rot13 mode to use a different needle.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 10/11] GETTEXT_POISON=rot13: perform actual checks in `test_i18ngrep`
2021-01-12 8:47 [PATCH 00/11] [RFH] Introduce support for GETTEXT_POISON=rot13 Johannes Schindelin via GitGitGadget
` (8 preceding siblings ...)
2021-01-12 8:47 ` [PATCH 09/11] GETTEXT_POISON=rot13: do compare the output in `test_i18ncmp` Johannes Schindelin via GitGitGadget
@ 2021-01-12 8:47 ` Johannes Schindelin via GitGitGadget
2021-01-12 8:47 ` [PATCH 11/11] test-tool i18n: do not complain about empty messages Johannes Schindelin via GitGitGadget
` (2 subsequent siblings)
12 siblings, 0 replies; 31+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2021-01-12 8:47 UTC (permalink / raw)
To: git
Cc: Nguyễn Thái Ngọc Duy,
Ævar Arnfjörð, SZEDER Gábor,
Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/helper/test-i18n.c | 42 ++++++++++++++++++++++++++++++++++++++++-
t/test-lib-functions.sh | 6 ++++++
2 files changed, 47 insertions(+), 1 deletion(-)
diff --git a/t/helper/test-i18n.c b/t/helper/test-i18n.c
index 4b572e6efad..8a16df6b40f 100644
--- a/t/helper/test-i18n.c
+++ b/t/helper/test-i18n.c
@@ -1,8 +1,10 @@
#include "test-tool.h"
#include "cache.h"
+#include "grep.h"
static const char *usage_msg = "\n"
-" test-tool i18n cmp <file1> <file2>\n";
+" test-tool i18n cmp <file1> <file2>\n"
+" test-tool i18n grep <regex> <file>\n";
static inline char do_rot13(char c)
{
@@ -75,6 +77,42 @@ static int i18n_cmp(const char **argv)
return 0;
}
+static int i18n_grep(const char **argv)
+{
+ int dont_match = 0;
+ const char *pattern, *path;
+
+ struct grep_opt opt;
+ struct grep_source source;
+ struct strbuf buf = STRBUF_INIT;
+ int hit;
+
+ if (*argv && !strcmp("!", *argv)) {
+ dont_match = 1;
+ argv++;
+ }
+
+ pattern = *(argv++);
+ path = *(argv++);
+
+ if (!pattern || !path || *argv)
+ usage(usage_msg);
+
+ grep_init(&opt, the_repository, NULL);
+ append_grep_pattern(&opt, pattern, "command line", 0, GREP_PATTERN);
+ compile_grep_patterns(&opt);
+
+ if (strbuf_read_file(&buf, path, 0) < 0)
+ die_errno("could not read %s", path);
+ unrot13_strbuf(&buf);
+ grep_source_init(&source, GREP_SOURCE_BUF, path, path, path);
+ source.buf = buf.buf;
+ source.size = buf.len;
+ hit = grep_source(&opt, &source);
+ strbuf_release(&buf);
+ return dont_match ^ !hit;
+}
+
int cmd__i18n(int argc, const char **argv)
{
argv++;
@@ -82,6 +120,8 @@ int cmd__i18n(int argc, const char **argv)
usage(usage_msg);
if (!strcmp(*argv, "cmp"))
return i18n_cmp(argv+1);
+ else if (!strcmp(*argv, "grep"))
+ return i18n_grep(argv+1);
else
usage(usage_msg);
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 08731bae854..394fd2ef5be 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1021,6 +1021,12 @@ test_i18ngrep () {
if test_have_prereq !C_LOCALE_OUTPUT
then
+ if test rot13 = "$GIT_TEST_GETTEXT_POISON"
+ then
+ test-tool i18n grep "$@"
+ return $?
+ fi
+
# pretend success
return 0
fi
--
gitgitgadget
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH 11/11] test-tool i18n: do not complain about empty messages
2021-01-12 8:47 [PATCH 00/11] [RFH] Introduce support for GETTEXT_POISON=rot13 Johannes Schindelin via GitGitGadget
` (9 preceding siblings ...)
2021-01-12 8:47 ` [PATCH 10/11] GETTEXT_POISON=rot13: perform actual checks in `test_i18ngrep` Johannes Schindelin via GitGitGadget
@ 2021-01-12 8:47 ` Johannes Schindelin via GitGitGadget
2021-01-12 11:34 ` [PATCH 00/11] [RFH] Introduce support for GETTEXT_POISON=rot13 Jeff King
2021-01-12 13:32 ` Ævar Arnfjörð Bjarmason
12 siblings, 0 replies; 31+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2021-01-12 8:47 UTC (permalink / raw)
To: git
Cc: Nguyễn Thái Ngọc Duy,
Ævar Arnfjörð, SZEDER Gábor,
Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
They might, or might not be 'translated'. Let's not assume that they
aren't marked with `_()` for translation.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/helper/test-i18n.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/helper/test-i18n.c b/t/helper/test-i18n.c
index 8a16df6b40f..82efc66f1f5 100644
--- a/t/helper/test-i18n.c
+++ b/t/helper/test-i18n.c
@@ -52,7 +52,7 @@ static void unrot13_strbuf(struct strbuf *buf)
{
size_t len = unrot13(buf->buf);
- if (len == buf->len)
+ if (len && len == buf->len)
die("not ROT13'ed:\n%s", buf->buf);
buf->len = len;
}
--
gitgitgadget
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 00/11] [RFH] Introduce support for GETTEXT_POISON=rot13
2021-01-12 8:47 [PATCH 00/11] [RFH] Introduce support for GETTEXT_POISON=rot13 Johannes Schindelin via GitGitGadget
` (10 preceding siblings ...)
2021-01-12 8:47 ` [PATCH 11/11] test-tool i18n: do not complain about empty messages Johannes Schindelin via GitGitGadget
@ 2021-01-12 11:34 ` Jeff King
2021-01-12 19:50 ` Junio C Hamano
2021-01-12 13:32 ` Ævar Arnfjörð Bjarmason
12 siblings, 1 reply; 31+ messages in thread
From: Jeff King @ 2021-01-12 11:34 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Nguyễn Thái Ngọc Duy,
Ævar Arnfjörð, SZEDER Gábor,
Johannes Schindelin
On Tue, Jan 12, 2021 at 08:47:31AM +0000, Johannes Schindelin via GitGitGadget wrote:
> Ævar suggested recently
> [https://lore.kernel.org/git/xmqqim836v6m.fsf@gitster.c.googlers.com/T/#m6fdc43d4f1eb3f20f841096c59e985b69c84875e]
> that this whole GETTEXT_POISON business is totally useless.
>
> I do not believe that it is useless. To back up my belief with something
> tangible, I implemented a GETTEXT_POISON=rot13 mode and ran the test suite
> to see whether we erroneously expect translated messages where they aren't,
> and related problems.
>
> And the experiment delivered. It is just a demonstration (I only addressed a
> handful of the test suite failures, 124 test scripts still need to be
> inspected to find out why they fail), of course. Yet I think that finding
> e.g. the missing translations of sha1dc-cb and parse-options show that it
> would be very useful to complete this patch series and then use the rot13
> mode in our CI jobs (instead of the current GETTEXT_POISON=true mode, which
> really is less useful).
I'm not entirely convinced by this. The original point of the poison
code was not to find opportunities to translate strings, but to make
sure we did not accidentally translate a string that some script was
relying on. And I don't see any fixes for the latter here (and as Ævar
suggested in the linked thread, the fact that we're not combing through
existing code looking for translations makes such an error a lot less
likely).
Which isn't to say repurposing it in the other direction might not be
worthwhile. But I suspect a lot of the test failures are just false
positives. Until now it was always reasonable to conservatively use
test_i18ngrep for cases which could reasonably translated, even if they
were not yet.
Likewise, I'm not sure that one can reliably rot13 an output for
test_i18ncmp. It could contain mixed translated and untranslated bits
from different messages.
So I dunno. You did find two spots where translations could be used.
But if nobody actually saw them to care that they got translated, were
they important? I may be a bit biased as somebody who would not use the
translations in the first place, of course.
-Peff
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 00/11] [RFH] Introduce support for GETTEXT_POISON=rot13
2021-01-12 11:34 ` [PATCH 00/11] [RFH] Introduce support for GETTEXT_POISON=rot13 Jeff King
@ 2021-01-12 19:50 ` Junio C Hamano
2021-01-13 7:32 ` Jeff King
2021-01-16 6:38 ` Junio C Hamano
0 siblings, 2 replies; 31+ messages in thread
From: Junio C Hamano @ 2021-01-12 19:50 UTC (permalink / raw)
To: Jeff King
Cc: Johannes Schindelin via GitGitGadget, git,
Nguyễn Thái Ngọc Duy,
Ævar Arnfjörð, SZEDER Gábor,
Johannes Schindelin
Jeff King <peff@peff.net> writes:
> Likewise, I'm not sure that one can reliably rot13 an output for
> test_i18ncmp. It could contain mixed translated and untranslated bits
> from different messages.
As long as <rot13>...</rot13> marking can be trusted (and it cannot
be, but if we declare it is OK to punt, then by definition we can),
I think the messages can be compared.
> So I dunno. You did find two spots where translations could be used.
> But if nobody actually saw them to care that they got translated, were
> they important? I may be a bit biased as somebody who would not use the
> translations in the first place, of course.
I viewed the series with interest, mostly (i.e. 80%+) for its "fun
value"; I tend to agree with you that I doubt its usefulness.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 00/11] [RFH] Introduce support for GETTEXT_POISON=rot13
2021-01-12 19:50 ` Junio C Hamano
@ 2021-01-13 7:32 ` Jeff King
2021-01-16 6:38 ` Junio C Hamano
1 sibling, 0 replies; 31+ messages in thread
From: Jeff King @ 2021-01-13 7:32 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin via GitGitGadget, git,
Nguyễn Thái Ngọc Duy,
Ævar Arnfjörð, SZEDER Gábor,
Johannes Schindelin
On Tue, Jan 12, 2021 at 11:50:52AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > Likewise, I'm not sure that one can reliably rot13 an output for
> > test_i18ncmp. It could contain mixed translated and untranslated bits
> > from different messages.
>
> As long as <rot13>...</rot13> marking can be trusted (and it cannot
> be, but if we declare it is OK to punt, then by definition we can),
> I think the messages can be compared.
Good point. I should have looked at the implementation more carefully
before responding (and I agree that while not foolproof, tagging like
that would work OK in practice).
-Peff
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 00/11] [RFH] Introduce support for GETTEXT_POISON=rot13
2021-01-12 19:50 ` Junio C Hamano
2021-01-13 7:32 ` Jeff King
@ 2021-01-16 6:38 ` Junio C Hamano
1 sibling, 0 replies; 31+ messages in thread
From: Junio C Hamano @ 2021-01-16 6:38 UTC (permalink / raw)
To: Jeff King
Cc: Johannes Schindelin via GitGitGadget, git,
Nguyễn Thái Ngọc Duy,
Ævar Arnfjörð, SZEDER Gábor,
Johannes Schindelin
Junio C Hamano <gitster@pobox.com> writes:
> Jeff King <peff@peff.net> writes:
> ...
>> So I dunno. You did find two spots where translations could be used.
>> But if nobody actually saw them to care that they got translated, were
>> they important? I may be a bit biased as somebody who would not use the
>> translations in the first place, of course.
>
> I viewed the series with interest, mostly (i.e. 80%+) for its "fun
> value"; I tend to agree with you that I doubt its usefulness.
Actually, "I doubt its usefulness" is way too strong than what I
really wanted to say. I was just skeptical, and I still somewhat
am. I'd just need a bit more convincing ;-)
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 00/11] [RFH] Introduce support for GETTEXT_POISON=rot13
2021-01-12 8:47 [PATCH 00/11] [RFH] Introduce support for GETTEXT_POISON=rot13 Johannes Schindelin via GitGitGadget
` (11 preceding siblings ...)
2021-01-12 11:34 ` [PATCH 00/11] [RFH] Introduce support for GETTEXT_POISON=rot13 Jeff King
@ 2021-01-12 13:32 ` Ævar Arnfjörð Bjarmason
2021-01-15 16:13 ` Johannes Schindelin
12 siblings, 1 reply; 31+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-01-12 13:32 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Nguyễn Thái Ngọc Duy, SZEDER Gábor,
Johannes Schindelin, Jeff King
On Tue, Jan 12 2021, Johannes Schindelin via GitGitGadget wrote:
> This patch series is incomplete because I lack the time to finish it, and I
> hope that there are interested developers who can help with it.
>
> In https://lore.kernel.org/git/20181022153633.31757-1-pclouds@gmail.com/,
> Duy proposed introducing a fake language they called "Ook" to be able to
> test translations better. "Ook" would "translate" messages without any
> printf format specifiers to "Eek", otherwise convert all upper-case letters
> to "Ook" and lower-case letters to "ook".
>
> Gábor replied with a different proposal that has the advantage of being
> bijective, i.e. it is possible to reconstruct the untranslated message from
> the translated one.
>
> Ævar suggested recently
> [https://lore.kernel.org/git/xmqqim836v6m.fsf@gitster.c.googlers.com/T/#m6fdc43d4f1eb3f20f841096c59e985b69c84875e]
> that this whole GETTEXT_POISON business is totally useless.
To clarify what I really mean, but admittedly am not always the best at
articulating: I do not think GETTEXT_POISON is useless, it's useful.
The interesting question is "useful enough to bother?", or in economics
terms "is this investment in time/money worth the opportunity cost?".
Any hoops we make people jump through when writing tests takes away time
and attention from other things.
My motivation here is that I feel this whole poison business is all my
fault. Every time some newbie submits a patch and needs to re-roll a v2
because they used "grep" instead of "test_i18ngrep" in a case that
clearly didn't involve a plumbing string I cringe a little about the
technical debt.
So while I still think my series (just nuke it) would be better overall,
I'm secretly rooting for yours. If the consensus is that this is worth
keeping and improving perhaps we haven't been mostly wasting our time :)
On to discussing this series:
> I do not believe that it is useless. To back up my belief with something
> tangible, I implemented a GETTEXT_POISON=rot13 mode and ran the test suite
> to see whether we erroneously expect translated messages where they aren't,
> and related problems.
I agree that rot13ing it makes it much more useful in the sense that
before we could over-tag things with test_i18n* and we'd just "return 0"
there in the poison mode, now we actually check if the string is
poisoned.
We could get most of the way there by checking e.g. if "GETTEXT POISON"
is in the output, but it wouldn't assert that the message is 100%
translated as this WIP series does.
In our off-list discussion you maintained that
"GIT_TEST_GETTEXT_POISON=false <cmd>" was an anti-pattern. Was it
because you thought "test_i18n*" et al was actually doing some "is
poison?" comparison as we now do, or just because you didn't
conceptually like a pattern like:
GIT_TEST_GETTEXT_POISON=false git cmd >out &&
grep string out
Being changed later to:
GIT_TEST_GETTEXT_POISON=false git cmd >out &&
grep string out &&
grep plumbing-string out
Or whatever, as opposed to:
git cmd >out &&
test_i18ngrep string out &&
test_i18ngrep plumbing-string out
I get the aesthetic preference, I'm just wondering if I'm missing
something else about it...
> And the experiment delivered. It is just a demonstration (I only addressed a
> handful of the test suite failures, 124 test scripts still need to be
> inspected to find out why they fail), of course. Yet I think that finding
> e.g. the missing translations of sha1dc-cb and parse-options show that it
> would be very useful to complete this patch series and then use the rot13
> mode in our CI jobs (instead of the current GETTEXT_POISON=true mode, which
> really is less useful).
The following patch on top cuts down on the failures by more than half:
-----------------
diff --git a/git-sh-i18n.sh b/git-sh-i18n.sh
index 8eef60b43f..f9239d2917 100644
--- a/git-sh-i18n.sh
+++ b/git-sh-i18n.sh
@@ -17,7 +17,10 @@ export TEXTDOMAINDIR
# First decide what scheme to use...
GIT_INTERNAL_GETTEXT_SH_SCHEME=fallthrough
-if test -n "$GIT_TEST_GETTEXT_POISON" &&
+if test -n "$GIT_TEST_GETTEXT_POISON" -a "$GIT_TEST_GETTEXT_POISON" = "rot13"
+then
+ GIT_INTERNAL_GETTEXT_SH_SCHEME=rot13poison
+elif test -n "$GIT_TEST_GETTEXT_POISON" &&
git env--helper --type=bool --default=0 --exit-code \
GIT_TEST_GETTEXT_POISON
then
@@ -63,6 +66,21 @@ gettext_without_eval_gettext)
)
}
;;
+rot13poison)
+ # Emit garbage so that tests that incorrectly rely on translatable
+ # strings will fail.
+ gettext () {
+ printf "%s" "# SHELL GETTEXT POISON #"
+ }
+
+ eval_gettext () {
+ printf "%s" "# SHELL GETTEXT POISON #"
+ }
+
+ eval_ngettext () {
+ printf "%s" "# SHELL GETTEXT POISON #"
+ }
+ ;;
poison)
# Emit garbage so that tests that incorrectly rely on translatable
# strings will fail.
diff --git a/t/helper/test-i18n.c b/t/helper/test-i18n.c
index 82efc66f1f..1f0fa3d041 100644
--- a/t/helper/test-i18n.c
+++ b/t/helper/test-i18n.c
@@ -1,6 +1,8 @@
#include "test-tool.h"
#include "cache.h"
#include "grep.h"
+#include "config.h"
static const char *usage_msg = "\n"
" test-tool i18n cmp <file1> <file2>\n"
@@ -34,8 +36,12 @@ static size_t unrot13(char *buf)
p += strlen("<rot13>");
end = strstr(p, "</rot13>");
- if (!end)
- BUG("could not find </rot13> in\n%s", buf);
+ if (!end) {
+ if (git_env_bool("GIT_TEST_GETTEXT_POISON_PEDANTIC", 0))
+ BUG("could not find </rot13> in\n%s", buf);
+ else
+ break;
+ }
while (p != end)
*(q++) = do_rot13(*(p++));
@@ -67,8 +73,12 @@ static int i18n_cmp(const char **argv)
if (strbuf_read_file(&a, path1, 0) < 0)
die_errno("could not read %s", path1);
+ if (strstr(a.buf, "# SHELL GETTEXT POISON #"))
+ return 0;
if (strbuf_read_file(&b, path2, 0) < 0)
die_errno("could not read %s", path2);
+ if (strstr(b.buf, "# SHELL GETTEXT POISON #"))
+ return 0;
unrot13_strbuf(&b);
if (a.len != b.len || memcmp(a.buf, b.buf, a.len))
@@ -79,7 +89,6 @@ static int i18n_cmp(const char **argv)
static int i18n_grep(const char **argv)
{
- int dont_match = 0;
const char *pattern, *path;
struct grep_opt opt;
@@ -87,11 +96,6 @@ static int i18n_grep(const char **argv)
struct strbuf buf = STRBUF_INIT;
int hit;
- if (*argv && !strcmp("!", *argv)) {
- dont_match = 1;
- argv++;
- }
-
pattern = *(argv++);
path = *(argv++);
@@ -104,13 +108,15 @@ static int i18n_grep(const char **argv)
if (strbuf_read_file(&buf, path, 0) < 0)
die_errno("could not read %s", path);
+ if (strstr(buf.buf, "# SHELL GETTEXT POISON #"))
+ return 0;
unrot13_strbuf(&buf);
grep_source_init(&source, GREP_SOURCE_BUF, path, path, path);
source.buf = buf.buf;
source.size = buf.len;
hit = grep_source(&opt, &source);
strbuf_release(&buf);
- return dont_match ^ !hit;
+ return !hit;
}
int cmd__i18n(int argc, const char **argv)
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 394fd2ef5b..6c580a3000 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1019,14 +1019,12 @@ test_i18ngrep () {
BUG "too few parameters to test_i18ngrep"
fi
- if test_have_prereq !C_LOCALE_OUTPUT
+ grep_cmd=grep
+ if test "$GIT_TEST_GETTEXT_POISON" == "rot13"
+ then
+ grep_cmd="test-tool i18n grep"
+ elif test_have_prereq !C_LOCALE_OUTPUT
then
- if test rot13 = "$GIT_TEST_GETTEXT_POISON"
- then
- test-tool i18n grep "$@"
- return $?
- fi
-
# pretend success
return 0
fi
@@ -1034,13 +1032,13 @@ test_i18ngrep () {
if test "x!" = "x$1"
then
shift
- ! grep "$@" && return 0
+ ! $grep_cmd "$@" && return 0
- echo >&4 "error: '! grep $@' did find a match in:"
+ echo >&4 "error: '! $grep_cmd $@' did find a match in:"
else
- grep "$@" && return 0
+ $grep_cmd "$@" && return 0
- echo >&4 "error: 'grep $@' didn't find a match in:"
+ echo >&4 "error: '$grep_cmd $@' didn't find a match in:"
fi
if test -s "$last_arg"
-----------------
I.e. most of this was because you didn't add any support for this to the
shellscript translations.
We still punt on that here, it should ideally preserve the shell
variables like the format specifiers, but I think that's not worth the
effort with us actively cutting down our shell code to nothing.
We still have failures e.g. due to "test_i18ngrep -F fixed file" not
being supported. Wouldn't a better/simpler approach be to just have a
light rot13 helper, and then call the actual "cmp"/"grep" with the
munged input/output?
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 00/11] [RFH] Introduce support for GETTEXT_POISON=rot13
2021-01-12 13:32 ` Ævar Arnfjörð Bjarmason
@ 2021-01-15 16:13 ` Johannes Schindelin
0 siblings, 0 replies; 31+ messages in thread
From: Johannes Schindelin @ 2021-01-15 16:13 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Johannes Schindelin via GitGitGadget, git,
Nguyễn Thái Ngọc Duy, SZEDER Gábor,
Jeff King
[-- Attachment #1: Type: text/plain, Size: 8076 bytes --]
Hi Ævar,
On Tue, 12 Jan 2021, Ævar Arnfjörð Bjarmason wrote:
> On Tue, Jan 12 2021, Johannes Schindelin via GitGitGadget wrote:
>
> > I implemented a GETTEXT_POISON=rot13 mode and ran the test suite to
> > see whether we erroneously expect translated messages where they
> > aren't, and related problems.
>
> I agree that rot13ing it makes it much more useful in the sense that
> before we could over-tag things with test_i18n* and we'd just "return 0"
> there in the poison mode, now we actually check if the string is
> poisoned.
Precisely.
> In our off-list discussion you maintained that
> "GIT_TEST_GETTEXT_POISON=false <cmd>" was an anti-pattern. Was it
> because you thought "test_i18n*" et al was actually doing some "is
> poison?" [...]
I maintain this view because the whole point of having a GETTEXT_POISON
job is to have the entire test suite run in that mode. Using this
anti-pattern says to me "the author could not make this work correctly in
GETTEXT_POISON mode".
Even more, _iff_ we expect certain validations to be skipped in the
GETTEXT_POISON job, using `GIT_TEST_GETTEXT_POISON=false` opens the
possibility that this job fails when it never should have failed.
> > And the experiment delivered. It is just a demonstration (I only addressed a
> > handful of the test suite failures, 124 test scripts still need to be
> > inspected to find out why they fail), of course. Yet I think that finding
> > e.g. the missing translations of sha1dc-cb and parse-options show that it
> > would be very useful to complete this patch series and then use the rot13
> > mode in our CI jobs (instead of the current GETTEXT_POISON=true mode, which
> > really is less useful).
>
> The following patch on top cuts down on the failures by more than half:
Interesting! I forgot the shell half... Curious that the scripted parts
_still_ affect so much of our test suite :-(
> -----------------
> diff --git a/git-sh-i18n.sh b/git-sh-i18n.sh
> index 8eef60b43f..f9239d2917 100644
> --- a/git-sh-i18n.sh
> +++ b/git-sh-i18n.sh
> @@ -17,7 +17,10 @@ export TEXTDOMAINDIR
>
> # First decide what scheme to use...
> GIT_INTERNAL_GETTEXT_SH_SCHEME=fallthrough
> -if test -n "$GIT_TEST_GETTEXT_POISON" &&
> +if test -n "$GIT_TEST_GETTEXT_POISON" -a "$GIT_TEST_GETTEXT_POISON" = "rot13"
> +then
> + GIT_INTERNAL_GETTEXT_SH_SCHEME=rot13poison
> +elif test -n "$GIT_TEST_GETTEXT_POISON" &&
> git env--helper --type=bool --default=0 --exit-code \
> GIT_TEST_GETTEXT_POISON
> then
> @@ -63,6 +66,21 @@ gettext_without_eval_gettext)
> )
> }
> ;;
> +rot13poison)
> + # Emit garbage so that tests that incorrectly rely on translatable
> + # strings will fail.
> + gettext () {
> + printf "%s" "# SHELL GETTEXT POISON #"
> + }
> +
> + eval_gettext () {
> + printf "%s" "# SHELL GETTEXT POISON #"
> + }
> +
> + eval_ngettext () {
> + printf "%s" "# SHELL GETTEXT POISON #"
> + }
Right, and for that, I'd rather use the `test-tool i18n` helper (it is
appropriate to expect `test-tool` to be in the `PATH` in GETTEXT_POISON
mode, right?)
> + ;;
> poison)
> # Emit garbage so that tests that incorrectly rely on translatable
> # strings will fail.
> diff --git a/t/helper/test-i18n.c b/t/helper/test-i18n.c
> index 82efc66f1f..1f0fa3d041 100644
> --- a/t/helper/test-i18n.c
> +++ b/t/helper/test-i18n.c
> @@ -1,6 +1,8 @@
> #include "test-tool.h"
> #include "cache.h"
> #include "grep.h"
> +#include "config.h"
>
> static const char *usage_msg = "\n"
> " test-tool i18n cmp <file1> <file2>\n"
> @@ -34,8 +36,12 @@ static size_t unrot13(char *buf)
>
> p += strlen("<rot13>");
> end = strstr(p, "</rot13>");
> - if (!end)
> - BUG("could not find </rot13> in\n%s", buf);
> + if (!end) {
> + if (git_env_bool("GIT_TEST_GETTEXT_POISON_PEDANTIC", 0))
> + BUG("could not find </rot13> in\n%s", buf);
> + else
> + break;
> + }
>
> while (p != end)
> *(q++) = do_rot13(*(p++));
> @@ -67,8 +73,12 @@ static int i18n_cmp(const char **argv)
>
> if (strbuf_read_file(&a, path1, 0) < 0)
> die_errno("could not read %s", path1);
> + if (strstr(a.buf, "# SHELL GETTEXT POISON #"))
> + return 0;
> if (strbuf_read_file(&b, path2, 0) < 0)
> die_errno("could not read %s", path2);
> + if (strstr(b.buf, "# SHELL GETTEXT POISON #"))
> + return 0;
> unrot13_strbuf(&b);
>
> if (a.len != b.len || memcmp(a.buf, b.buf, a.len))
> @@ -79,7 +89,6 @@ static int i18n_cmp(const char **argv)
>
> static int i18n_grep(const char **argv)
> {
> - int dont_match = 0;
> const char *pattern, *path;
>
> struct grep_opt opt;
> @@ -87,11 +96,6 @@ static int i18n_grep(const char **argv)
> struct strbuf buf = STRBUF_INIT;
> int hit;
>
> - if (*argv && !strcmp("!", *argv)) {
> - dont_match = 1;
> - argv++;
> - }
> -
> pattern = *(argv++);
> path = *(argv++);
>
> @@ -104,13 +108,15 @@ static int i18n_grep(const char **argv)
>
> if (strbuf_read_file(&buf, path, 0) < 0)
> die_errno("could not read %s", path);
> + if (strstr(buf.buf, "# SHELL GETTEXT POISON #"))
> + return 0;
> unrot13_strbuf(&buf);
> grep_source_init(&source, GREP_SOURCE_BUF, path, path, path);
> source.buf = buf.buf;
> source.size = buf.len;
> hit = grep_source(&opt, &source);
> strbuf_release(&buf);
> - return dont_match ^ !hit;
> + return !hit;
> }
>
> int cmd__i18n(int argc, const char **argv)
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 394fd2ef5b..6c580a3000 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -1019,14 +1019,12 @@ test_i18ngrep () {
> BUG "too few parameters to test_i18ngrep"
> fi
>
> - if test_have_prereq !C_LOCALE_OUTPUT
> + grep_cmd=grep
> + if test "$GIT_TEST_GETTEXT_POISON" == "rot13"
> + then
> + grep_cmd="test-tool i18n grep"
> + elif test_have_prereq !C_LOCALE_OUTPUT
> then
> - if test rot13 = "$GIT_TEST_GETTEXT_POISON"
> - then
> - test-tool i18n grep "$@"
> - return $?
> - fi
> -
> # pretend success
> return 0
> fi
> @@ -1034,13 +1032,13 @@ test_i18ngrep () {
> if test "x!" = "x$1"
> then
> shift
> - ! grep "$@" && return 0
> + ! $grep_cmd "$@" && return 0
>
> - echo >&4 "error: '! grep $@' did find a match in:"
> + echo >&4 "error: '! $grep_cmd $@' did find a match in:"
> else
> - grep "$@" && return 0
> + $grep_cmd "$@" && return 0
>
> - echo >&4 "error: 'grep $@' didn't find a match in:"
> + echo >&4 "error: '$grep_cmd $@' didn't find a match in:"
> fi
>
> if test -s "$last_arg"
> -----------------
>
> I.e. most of this was because you didn't add any support for this to the
> shellscript translations.
>
> We still punt on that here, it should ideally preserve the shell
> variables like the format specifiers, but I think that's not worth the
> effort with us actively cutting down our shell code to nothing.
I'm not sure how fast we'll get there, what with `git submodule` being
slow to get over the finish line, and with `mergetool`/`difftool` still
being scripted, and probably for a long time, too.
> We still have failures e.g. due to "test_i18ngrep -F fixed file" not
> being supported. Wouldn't a better/simpler approach be to just have a
> light rot13 helper, and then call the actual "cmp"/"grep" with the
> munged input/output?
Hmm. I hoped that this work would open the door to introducing more C
helpers in the test suite, thereby accelerating it on Windows.
We could also potentially move the `cmp`/`grep` parts into separate
helpers that give more useful output in case of a failure. (Right now, if
anything fails in CI involving `grep` or `cmp`, the user investigating
this pretty much _has_ to try to reproduce the issue locally because the
output is so not helpful, and reproducing it might be a challenge e.g.
when the failure is macOS-specific and the developer does not have access
to a macOS setup).
Ciao,
Dscho
^ permalink raw reply [flat|nested] 31+ messages in thread