Re: [PATCH v5 5/7] convert: add 'working-tree-encoding' attribute

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 30, 2018, 20:05 UTC
Message-ID: <xmqqzi4vt8n1.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180129201908.9398-1-tboegi@web.de>

tboegi@web.de writes:

> +	if ((conv_flags & CONV_WRITE_OBJECT) && !strcmp(enc->name, "SHIFT-JIS")) {
> +		char *re_src;
> +		int re_src_len;

I think it is a bad idea to

 (1) not check without CONV_WRITE_OBJECT here.
 (2) hardcode SJIS and do this always and to SJIS alone.

For (1), a fix would be obvious (and that will resurrect the dead code below).

For (2), perhaps introduce a multi-value configuration variable core.checkRoundtripEncoding, whose default value consists of just SJIS, but allow users to add or clear it?

Show 12 quoted lines

> +		re_src = reencode_string_len(dst, dst_len,
> +					     enc->name, default_encoding,
> +					     &re_src_len);
> +
> +		if (!re_src || src_len != re_src_len ||
> +		    memcmp(src, re_src, src_len)) {
> +			const char* msg = _("encoding '%s' from %s to %s and "
> +					    "back is not the same");
> +			if (conv_flags & CONV_WRITE_OBJECT)
> +				die(msg, path, enc->name, default_encoding);
> +			else
> +				error(msg, path, enc->name, default_encoding);

The "error" side of this inner if() is dead code, I think.