> On 30 Jan 2018, at 21:05, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> tboegi@xxxxxx 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@xxxxxxxxxxxxxxxxxxxxx/
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
