Re: *Really* noisy encoding warnings post-v2.33.0

[Jeff King <peff@peff.net>]

On Sat, Oct 09, 2021 at 03:47:16PM +0200, Ævar Arnfjörð Bjarmason wrote:

> But in this case this seems to have been because someone tried to feed
> "HTML" to it, which is not an encoding, and something iconv_open() has
> (I daresay) always and will always error on. It returns -1 and sets
> errno=EINVAL.
> 
> So having a warning or other detection in the revision loop seems
> backwards to me, surely we want something like the below instead?
> I.e. die as close to bad option parsing as possible?

Sorry for the slow response; this got thrown on my "to think about and
look at later" pile.

Yeah, I agree that if we sanity-checked the encoding up front, that
would cover the case we saw in practice, and goes a long way towards
catching any practical errors.

But I think this patch is tricky:

> diff --git a/environment.c b/environment.c
> index 43bb1b35ffe..c26b18f8e5c 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -357,8 +357,18 @@ void set_git_dir(const char *path, int make_realpath)
>  
>  const char *get_log_output_encoding(void)
>  {
> -	return git_log_output_encoding ? git_log_output_encoding
> +	const char *encoding = git_log_output_encoding ? git_log_output_encoding
>  		: get_commit_output_encoding();
> +#ifndef NO_ICONV
> +	iconv_t conv;
> +	conv = iconv_open(encoding, "UTF-8");
> +	if (conv == (iconv_t) -1 && errno == EINVAL)
> +		die_errno("the '%s' encoding is not known to iconv", encoding);
> +#else
> +	if (strcmp(encoding, "UTF-8"))
> +		die("compiled with NO_ICONV=Y, can't re-encode to '%s'", encoding);
> +#endif
> +	return encoding;
>  }

So one obvious problem here is that we call this function once per
commit, so it's a lot of extra iconv_open() calls. But obviously we
could use a static flag to do it once per process.

The other issue is that it is assuming UTF-8 on one end of the
conversion. But we aren't necessarily doing such a conversion; it
depends on the commit's on-disk encoding, and the requested output
encoding. In particular:

  - if both of those match, we do not need to call iconv at all (see the
    same_encoding() check in repo_logmsg_reencode()). With the patch
    above, the NO_ICONV case would start to die() when both are say
    iso8859-1, even though it currently works.

  - likewise, even if you have iconv support, it's possible that your
    preferred encoding is not compatible with utf8. In which case
    iconv_open() may complain, even though the actual conversion we'd
    ask it to do would succeed.

I.e., I don't think there's a way to just ask iconv "does this encoding
name by itself make any sense". You can only ask it about to/from
combos.

So I think a much better version of this is to catch the _actual_
iconv_open() call we make. And if it fails, say "woah, this combo of
encodings isn't supported". The reason I didn't do that in the earlier
patch is that all of this is obscured inside reencode_string_len(),
which does both the iconv_open() and the iconv() call. We could surface
that error information.

But I'm not sure it would make sense to die() in that case. While for
something like "git log --encoding=nonsense" every commit is going to
fail to re-encode, it's still possible that iconv_open() failures are
commit-specific. I.e., you could have some garbage commit in your
history with an unsupported encoding, and you wouldn't want to die() for
it (it's the same case you are complaining about having a warning for,
but much worse).

I suspect the best we could do along these lines is to wait until a real
iconv_open(to, from) fails, and then as a fallback try:

  iconv_open("UTF-8", from);
  iconv_open(to, "UTF-8");

to sanity-check them individually, and guess that one of them is broken
if it can't go to/from UTF-8. But even that feels like it's making
assumptions about both the system iconv, and the charsets people use.

To be clear, I'd expect that most people just use utf-8 in the first
place, and even if they don't that their system has some basic utf-8
support. But we are deep into the realm of weird corner cases here, and
the utility of this warning / error-checking doesn't seem high enough to
merit the possible regressions we'd get by trying to make too many
assumptions.

-Peff