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

[Lars Schneider <larsxschneider@gmail.com>]

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


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


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