Re: racy test failure in tb/upload-pack-filters

On Wed, Aug 05, 2020 at 05:26:58AM -0400, Jeff King wrote:

On Wed, Aug 05, 2020 at 11:06:41AM +0200, SZEDER Gábor wrote:

quoted

quoted

We can fix it by using test_i18ngrep, which just makes this grep a noop
in the poison mode.

I wonder whether changing that die to

  die("%s: %s", _("remote error"), buffer + 4)

would be better.

That would definitely work, but it seems sad to have to make our code
uglier. Plus I think it would hurt translations that want to format
differently (e.g., would an RTL language want to swap the order?).

This is reason enough to not do this; I know that we are pretty averse
to translation lego, and even though this one seems innocuous, I think
it may be a deal breaker for such languages.

It also wouldn't help other poison uses that could be expecting a "%s"
to be filled.  Another option would be to make our poison code more
realistic by copying placeholders like "%s" into the poison string. That
would fix this problem, and allow some tests to relax a bit (e.g., if
I'm looking for an error message that contains a filename, I _could_
grep for just that filename, which would never actually be translated).

But I think that gets pretty tricky, as we'd have to understand the
whole set of placeholders (e.g., that "%s" is complete after two bytes,
but "%lu" needs three bytes).

Might be a good direction to go in, but I agree it's outside of the
scope of this patch.

Anyway, it seemed like limiting the damage to the tests themselves was
the least bad thing.

By the way, grepping for "remote error:" shows that when we get an error
over sideband 3 we produce the same message but _don't_ translate it.
That seems inconsistent.

-Peff

Thanks,
Taylor