All of lore.kernel.org
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Max Kirillov <max@max630.net>
Cc: Eric Sunshine <sunshine@sunshineco.com>,
	Jeff King <peff@peff.net>,
	git@vger.kernel.org
Subject: Re: [PATCH] utf8.c: print warning about disabled iconv
Date: Mon, 08 Jun 2015 09:16:16 -0700	[thread overview]
Message-ID: <xmqqfv62ch0v.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <1433624551-20730-1-git-send-email-max@max630.net> (Max Kirillov's message of "Sun, 7 Jun 2015 00:02:31 +0300")

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.

  reply	other threads:[~2015-06-08 16:16 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-06-06 21:02 [PATCH] utf8.c: print warning about disabled iconv Max Kirillov
2015-06-08 16:16 ` Junio C Hamano [this message]
2015-06-08 21:07   ` Max Kirillov
2015-06-08 21:14     ` Junio C Hamano
2015-08-14 21:55 ` [PATCH v2] utf8.c: print warning about iconv errors Max Kirillov
2015-08-14 22:35   ` Junio C Hamano
2015-08-17 19:02     ` Jeff King
2015-08-17 19:49       ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqfv62ch0v.fsf@gitster.dls.corp.google.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=max@max630.net \
    --cc=peff@peff.net \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.