Re: [PATCH v11 10/10] convert: add round trip check based on 'core.checkRoundtripEncoding'
On Fri, Mar 9, 2018 at 12:35 PM, wrote: > [...] > Add 'core.checkRoundtripEncoding', which contains a comma separated > list of encodings, to define for what encodings Git should check the > conversion round trip if they are used in the 'working-tree-encoding' > attribute. > [...] > Signed-off-by: Lars Schneider > --- > diff --git a/convert.c b/convert.c > @@ -1150,7 +1227,7 @@ static const char *git_path_check_encoding(struct > attr_check_item *check) > /* Don't encode to the default encoding */ > - if (!strcasecmp(value, default_encoding)) > + if (is_encoding_utf8(value) && is_encoding_utf8(default_encoding)) > return NULL; This change belongs in 6/10, not 10/10, methinks.
Re: [PATCH v11 10/10] convert: add round trip check based on 'core.checkRoundtripEncoding'
Eric Sunshine writes: > On Fri, Mar 9, 2018 at 12:35 PM, wrote: >> [...] >> Add 'core.checkRoundtripEncoding', which contains a comma separated >> list of encodings, to define for what encodings Git should check the >> conversion round trip if they are used in the 'working-tree-encoding' >> attribute. >> [...] >> Signed-off-by: Lars Schneider >> --- >> diff --git a/convert.c b/convert.c >> @@ -1150,7 +1227,7 @@ static const char *git_path_check_encoding(struct >> attr_check_item *check) >> /* Don't encode to the default encoding */ >> - if (!strcasecmp(value, default_encoding)) >> + if (is_encoding_utf8(value) && is_encoding_utf8(default_encoding)) >> return NULL; > > This change belongs in 6/10, not 10/10, methinks. It is actually worse than that, no? When default_encoding is (somehow) configured not to be UTF-8, e.g. "Shift_JIS", we used to avoid converting from Shift_JIS to Shift_JIS, but the optimization no longer happens with this code. In any case, I think same_encoding() is probably a good thing to use here at step 6/10, so the point is moot, I guess.
Re: [PATCH v11 10/10] convert: add round trip check based on 'core.checkRoundtripEncoding'
On Fri, Mar 9, 2018 at 3:22 PM, Junio C Hamano wrote: > Eric Sunshine writes: >> On Fri, Mar 9, 2018 at 12:35 PM, wrote: >>> /* Don't encode to the default encoding */ >>> - if (!strcasecmp(value, default_encoding)) >>> + if (is_encoding_utf8(value) && is_encoding_utf8(default_encoding)) >>> return NULL; >> >> This change belongs in 6/10, not 10/10, methinks. > > It is actually worse than that, no? When default_encoding is > (somehow) configured not to be UTF-8, e.g. "Shift_JIS", we used to > avoid converting from Shift_JIS to Shift_JIS, but the optimization > no longer happens with this code. > > In any case, I think same_encoding() is probably a good thing to use > here at step 6/10, so the point is moot, I guess. Agreed, same_encoding() would be superior.
