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