Re: [PATCH v2] utf8.c: print warning about iconv errors

Jeff King [off-list ref] writes:

On Fri, Aug 14, 2015 at 03:35:58PM -0700, Junio C Hamano wrote:

quoted

Max Kirillov [off-list ref] writes:

quoted

* do not limit number of warnings - does not worth complicating the code

Unless the warning leads to a quick "die()", wouldn't this make Git
unusable by spewing a "falling back to verbatim copy" for each and
every line of the message of a commit that has 'encoding' element in
its header in the "git log" output, no?

We only do the reencode once per commit. So it would be once per commit
rather than once per line. Which still sounds kind of annoying, if you
are using "git log --oneline" or similar.

I think I'd favor a single warning in general, along the lines of
"some encodings could not be converted". But of course if you are trying
to figure out _which_ encodings your system doesn't have, that's not
very helpful. Maybe we could have an advice.encodingFailure config flag
with a tristate:

  - false: don't spew any warnings

  - true: give a generic warning once per program

  - all: give a specific warning for each case, like "unable to convert
    EUC-JP to UTF-8: iconv_open: Invalid argument". (Sadly EINVAL is
    what iconv_open seems to return when you it doesn't know about a
    particular encoding; it may be nicer to translate to something more
    reasonable than what strerror() provides).

Sounds sensible.

quoted

quoted

+char *reencode_string_len(const char *in, int insz,
+			  const char *out_encoding, const char *in_encoding,
+			  int *outsz)
+{
+	if (!same_encoding(in_encoding, out_encoding))
+ warning("Iconv support is disabled at compile time. It is likely
that\nincorrect data will be printed or stored in
repository.\nConsider using other build for this task.");
+	return NULL;
+}

Hmmm, I suspect this may be seen as regression by those who build
Git without ICONV for performance, knowing that there is nothing in
their data that requires character set conversion.

I don't think it matters that much.

Yeah, I think I agree.  Thanks.