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

From: Lars Schneider <larsxschneider@gmail.com>
Date: Jan 30, 2018, 20:31 UTC
Message-ID: <396FBDFA-606F-41D9-988C-D6886089BC15@gmail.com>
In-Reply-To: <xmqqzi4vt8n1.fsf@gitster-ct.c.googlers.com>

Show 11 quoted lines

> On 30 Jan 2018, at 21:05, Junio C Hamano <gitster@pobox.com> wrote:
> 
> 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.

The idea is to perform the roundtrip check *only* if we actually write to Git. In all other cases we don't care if the encoding roundtrips.

"git checkout" is such a case where we don't care as noted by Peff here: https://public-inbox.org/git/20171215095838.GA3567@sigill.intra.peff.net/

Do you agree?

Show 7 quoted lines

> (2) hardcode SJIS and do this always and to SJIS alone.
> 
> ...
> 
> 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?

Well, in that case I would make it simpler and make core.checkRoundtripEncoding a boolean that applies to all encodings if enabled. We could make even simpler than that by removing the entire roundtrip check. The thing is, I was not able to come up with a sequence that would not generate a iconv error *and* not round trip. Would that be ok for you to remove all that roundtrip checking code?

Show 14 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.

Good catch. I think this code should go away if we keep the roundtrip code and you agree with my statement above.

Thanks a lot for the review, Lars