[PATCH] utf8.c: print warning about disabled iconv

[PATCH] utf8.c: print warning about disabled iconv

[Max Kirillov]

It is an allowed compile-time option to build git without iconv
support. Resulting build almost always functions correctly, and
never displays that it is missing anything, but reencode_string_len()
just never modifies its input. This gives undesirable result that
returned data or even data written into repository is incorrect
and user is not aware about it.

Show warning there is non-trivial reencoding requested. Show it only once
during program run.

Signed-off-by: Max Kirillov <max@max630.net>
---
Hi.

I have noticed there is some activity around the option, so it's not completely
abandoned. Maybe then this patch could be useful?
 utf8.c | 13 +++++++++++++
 utf8.h |  5 ++---
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/utf8.c b/utf8.c
index 520fbb4..b1b836e 100644
--- a/utf8.c
+++ b/utf8.c
@@ -521,6 +521,19 @@ char *reencode_string_len(const char *in, int insz,
 	iconv_close(conv);
 	return out;
 }
+#else
+static int noiconv_warning_shown = 0;
+
+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) && !noiconv_warning_shown) {
+		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.");
+		noiconv_warning_shown = 1;
+	}
+	return NULL;
+}
 #endif
 
 /*
diff --git a/utf8.h b/utf8.h
index e4d9183..3d900f1 100644
--- a/utf8.h
+++ b/utf8.h
@@ -23,13 +23,12 @@ void strbuf_utf8_replace(struct strbuf *sb, int pos, int width,
 #ifndef NO_ICONV
 char *reencode_string_iconv(const char *in, size_t insz,
 			    iconv_t conv, int *outsz);
+#endif
+
 char *reencode_string_len(const char *in, int insz,
 			  const char *out_encoding,
 			  const char *in_encoding,
 			  int *outsz);
-#else
-#define reencode_string_len(a,b,c,d,e) NULL
-#endif
 
 static inline char *reencode_string(const char *in,
 				    const char *out_encoding,
-- 
2.3.4.2801.g3d0809b

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

[Junio C Hamano]

Max Kirillov <max@max630.net> writes:

> It is an allowed compile-time option to build git without iconv
> support. Resulting build almost always functions correctly, and
> never displays that it is missing anything, but reencode_string_len()
> just never modifies its input.

Correct.

> This gives undesirable result that
> returned data or even data written into repository is incorrect
> and user is not aware about it.

I do not necessarily agree with that.  The user knows what s/he is
doing, data written to or shown from the repository is correct as
far as the user is concerned, and the user takes the full
respoinsibility when compiling out certain features.

> +	if (!same_encoding(in_encoding, out_encoding) && !noiconv_warning_shown) {
> +		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.");
> +		noiconv_warning_shown = 1;
> +	}

I actually am OK if the user gets exactly the same warning between
the two cases:

 - iconv failed to convert in the real reencode_string_len()

 - we compiled out iconv() and real conversion was asked.

and this patch is about the latter; I do not think it is reasonable
to give noise only for the latter but not for the former.  The
latter is expected by users who compile out the feature, but the
former is not and deserves the wranings more, so the patch is
backwards in that sense.

Thanks.

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

[Max Kirillov]

Hi.

On Mon, Jun 08, 2015 at 09:16:16AM -0700, Junio C Hamano wrote:
> Max Kirillov <max@max630.net> writes:

>> This gives undesirable result that returned data or even
>> data written into repository is incorrect and user is not
>> aware about it.
> 
> I do not necessarily agree with that.  The user knows what
> s/he is doing, data written to or shown from the
> repository is correct as far as the user is concerned, and
> the user takes the full respoinsibility when compiling out
> certain features.

User, in theory, can be not the same person who builds, or
can be not aware that the case needs recoding. It actually
started when I compiled git without iconv support and got
about 10 failed tests, and only 2 of them mentioned i18n in
their name.

Compiling out other features is not exactly the same. If
user compiles out curl, for example, git will not be able to
push or fetch through http, but it is not going to pretend
to be working, it will fail visibly.

> I actually am OK if the user gets exactly the same warning between
> the two cases:
> 
>  - iconv failed to convert in the real reencode_string_len()
> 
>  - we compiled out iconv() and real conversion was asked.

Does 'exactly the same' mean the same text? Shouldn't it
describe the reason? I can see 2 possible failures in case
of real iconv: unknown or unsupported encoding and invalid
input. Wouldn't them better to be detailed in warning?

-- 
Max

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

[Junio C Hamano]

Max Kirillov <max@max630.net> writes:

> User, in theory, can be not the same person who builds, or
> can be not aware that the case needs recoding.

Because you can pretty much say the same for build with iconv
enabled, I think that line of argument is futile.  The users do not
have control over platform's iconv (and sysadmin's choice of locale
packages) what charset/encoding can be converted to what other ones.

>> I actually am OK if the user gets exactly the same warning between
>> the two cases:
>> 
>>  - iconv failed to convert in the real reencode_string_len()
>> 
>>  - we compiled out iconv() and real conversion was asked.
>
> Does 'exactly the same' mean the same text?

No, I was trying to point out the total lack of corresponding
warnings in the iconv-enabled build.

After all, if you had to convert between UTF-8 and ISO-2022-JP, the
latter of which your system does not support, whether you use
iconv-disabled build of Git or iconv-enabled build of Git, we pass
the bytestream through, right?  Your patch gives warning for the
former (which is a good starting point if we want to warn "user
expected them to be converted, we didn't" case) but does not do
anything to the latter, even though users of the iconv-disabled
build is more likely to be aware of the potential issue (and are
likely to be willing to accept that) than the ones with
iconv-enabled build that runs on a sysmet that cannot convert the
specific encoding.

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

[Max Kirillov]

If reencoding a text data from one encoding to another fails, the original
version is used insted. Currently there is no warning about failed reencoding,
which can have an undesired outcome that returned data is incorrect but user
is not aware about it.

Add printing warning when conversion fails.

Also add test script to assert that warning is actually printed and output is
not changed, as expected.

Signed-off-by: Max Kirillov <max@max630.net>
---
Changes since v1:
* rebase to recent changes
* add handling runtime errors
* add test
* do not limit number of warnings - does not worth complicating the code
* noticed that incomplete utf8 sequence in input silently treated as latin1.
  so mark the testcase as expect_failure. Actually, it's quite surprising,
  would be nice if somebody tries it in various environments
Actually, as far as I could grep, all uses of the resoding happen
only for printing, so probably it is not that important.
 t/t3911-show-reencode.sh | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 utf8.c                   | 24 +++++++++++++++++++++++-
 utf8.h                   |  7 ++-----
 3 files changed, 71 insertions(+), 6 deletions(-)
 create mode 100755 t/t3911-show-reencode.sh

diff --git a/t/t3911-show-reencode.sh b/t/t3911-show-reencode.sh
new file mode 100755
index 0000000..061d820
--- /dev/null
+++ b/t/t3911-show-reencode.sh
@@ -0,0 +1,46 @@
+#!/bin/sh
+
+test_description='reencoding'
+
+. ./test-lib.sh
+
+printf '\304\201\n' >a_macron_utf8
+printf '\303\244\n' >a_diaeresis_utf8
+printf '\303\244\304\n' >incomplete_utf8
+printf '\344\n' >a_diaeresis_latin1
+
+test_expect_success 'setup' '
+	git commit --allow-empty -F a_diaeresis_utf8 &&
+	git tag latin1_utf8 &&
+	git commit --allow-empty -F a_macron_utf8 &&
+	git tag extended_utf8 &&
+	git commit --allow-empty -F incomplete_utf8 &&
+	git tag invalid_utf8
+'
+
+test_expect_success 'encoding to latin1' '
+	git log --encoding=latin1 --pretty=format:%B -1 latin1_utf8 >out 2>err &&
+	test_must_be_empty err &&
+	test_cmp out a_diaeresis_latin1
+'
+
+test_expect_success 'unknown encoding' '
+	git log --encoding=no-encoding --pretty=format:%B -1 latin1_utf8 >out 2>err &&
+	grep -q "not supported" err &&
+	test_cmp out a_diaeresis_utf8
+'
+
+# apparently incomplete UTF8 byte sequences silently treated as latin1
+test_expect_failure 'incomplete utf8' '
+	git log --encoding=latin1 --pretty=format:%B -1 invalid_utf8 >out 2>err &&
+	grep -q "Invalid input" err &&
+	test_cmp out incomplete_utf8
+'
+
+test_expect_success 'does not fit into latin1' '
+	git log --encoding=latin1 --pretty=format:%B -1 extended_utf8 >out 2>err &&
+	grep -q "Invalid input" err &&
+	test_cmp out a_macron_utf8
+'
+
+test_done
diff --git a/utf8.c b/utf8.c
index 28e6d76..d284bb0 100644
--- a/utf8.c
+++ b/utf8.c
@@ -465,7 +465,9 @@ char *reencode_string_iconv(const char *in, size_t insz, iconv_t conv, int *outs
 		if (cnt == (size_t) -1) {
 			size_t sofar;
 			if (errno != E2BIG) {
+				int failure_errno = errno;
 				free(out);
+				errno = failure_errno;
 				return NULL;
 			}
 			/* insz has remaining number of bytes.
@@ -513,14 +515,34 @@ char *reencode_string_len(const char *in, int insz,
 		if (is_encoding_utf8(out_encoding))
 			out_encoding = "UTF-8";
 		conv = iconv_open(out_encoding, in_encoding);
-		if (conv == (iconv_t) -1)
+		if (conv == (iconv_t) -1) {
+			if (errno == EINVAL)
+				warning("Conversion from %s to %s not supported, falling back to verbatim copy", in_encoding, out_encoding);
+			else
+				warning("Conversion from %s to %s failed: %s, falling back to verbatim copy", in_encoding, out_encoding, strerror(errno));
 			return NULL;
+		}
 	}
 
 	out = reencode_string_iconv(in, insz, conv, outsz);
+	if (out == NULL) {
+		if (errno == EILSEQ || errno == EINVAL)
+			warning("Invalid input for conversion from %s to %s, falling back to verbatim copy", in_encoding, out_encoding);
+		else
+			warning("Conversion from %s to %s failed: %s, falling back to verbatim copy", in_encoding, out_encoding, strerror(errno));
+	}
 	iconv_close(conv);
 	return out;
 }
+#else
+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;
+}
 #endif
 
 /*
diff --git a/utf8.h b/utf8.h
index 5a9e94b..c72998b 100644
--- a/utf8.h
+++ b/utf8.h
@@ -26,15 +26,12 @@ void strbuf_utf8_replace(struct strbuf *sb, int pos, int width,
 #ifndef NO_ICONV
 char *reencode_string_iconv(const char *in, size_t insz,
 			    iconv_t conv, int *outsz);
+#endif
+
 char *reencode_string_len(const char *in, int insz,
 			  const char *out_encoding,
 			  const char *in_encoding,
 			  int *outsz);
-#else
-static inline char *reencode_string_len(const char *a, int b,
-					const char *c, const char *d, int *e)
-{ if (e) *e = 0; return NULL; }
-#endif
 
 static inline char *reencode_string(const char *in,
 				    const char *out_encoding,
-- 
2.3.4.2801.g3d0809b

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

[Junio C Hamano]

Max Kirillov <max@max630.net> writes:

> * 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?

I suspect that this may be a huge mistake.

> +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.

We'd call same_encoding() every time, which would involve a few
strcasecmp() calls.  Originally, we didn't even have a function call
overhead.

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

[Jeff King]

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

> Max Kirillov <max@max630.net> writes:
> 
> > * 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).

> > +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. The obvious tight loop is
logmsg_reencode, and it already checks same_encoding (because it really
wants to avoid reallocation in the first place if it can). So anybody
who cares about the performance of reencode_string_len would do better
to optimize out any calls to it. :)

If anything, we could make same_encoding faster by memo-izing its ptrs,
like:

diff --git a/utf8.c b/utf8.c
index 28e6d76..50a8ac0 100644
--- a/utf8.c
+++ b/utf8.c
@@ -409,13 +409,26 @@ int is_encoding_utf8(const char *name)
 	return 0;
 }
 
-int same_encoding(const char *src, const char *dst)
+static int same_encoding_1(const char *src, const char *dst)
 {
+	warning("actually checking same_encoding(%s, %s)", src, dst);
 	if (is_encoding_utf8(src) && is_encoding_utf8(dst))
 		return 1;
 	return !strcasecmp(src, dst);
 }
 
+int same_encoding(const char *src, const char *dst)
+{
+	static const char *cached_src, *cached_dst;
+	static int cached_ret = -1;
+
+	if (src == cached_src && dst == cached_dst && cached_ret >= 0)
+		return cached_ret;
+	cached_src = src;
+	cached_dst = dst;
+	return cached_ret = same_encoding_1(src, dst);
+}
+
 /*
  * Wrapper for fprintf and returns the total number of columns required
  * for the printed string, assuming that the string is utf8.


But I couldn't measure any real speedup on "git log --oneline" from
doing so. It's also kind of gross (it will yield the wrong answer if you
write a different encoding to the same buffer; I don't think we do that,
but it's quite a gotcha).

Another approach would be to preserve NULL encodings (which we treat as
utf8) through the code base more. The common case of utf8 should be a
quick check for two NULLs, then. Unfortunately just teaching
get_commit_encoding and get_log_output_encoding to return NULL isn't
enough. Some parts of the code want to output the actual value (e.g.,
format-patch for a charset header), and would need to be adjusted. Given
that I couldn't measure any speedup, I don't think it's worth pursuing,
though.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Aug 14, 2015 at 03:35:58PM -0700, Junio C Hamano wrote:
>
>> Max Kirillov <max@max630.net> writes:
>> 
>> > * 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.

>> > +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.