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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 8, 2015, 16:16 UTC
Message-ID: <xmqqfv62ch0v.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1433624551-20730-1-git-send-email-max@max630.net>

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.