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

From: Lars Schneider <larsxschneider@gmail.com>
Date: Jan 31, 2018, 19:12 UTC
Message-ID: <57086A32-6A2A-4B66-A355-10408C9FE0B0@gmail.com>
In-Reply-To: <xmqqfu6nrowm.fsf@gitster-ct.c.googlers.com>

Show 29 quoted lines

> On 30 Jan 2018, at 22:56, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Lars Schneider <larsxschneider@gmail.com> writes:
> 
>>> 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?
> 
> I am not sure why this is special cased and other codepaths have "if
> WRITE_OBJECT then die, otherwise error" checks, so no, I do not
> agree with your reasoning, at least not yet.

The convert_to_git()/encode_to_git() machinery is used in two different kinds of code paths:

Some code paths actually write to the Git database (indicated by the CONV_WRITE_OBJECT flag). I consider these the "critical/important" code paths and I don't want to tolerate any encoding errors in these cases as the errors would be "forever" in the Git database. That's why I call die() on errors for these cases to abort whatever we are doing.

Other code paths do not write to the Git database (e.g. during "git checkout" we use the code to ensure that we are moving away from the exact state that we think we are moving away). In these code paths I am less concerned about encoding errors. I also don't want to abort the operation (e.g. "git checkout") in these cases. That's why I only inform the user about the problem with an error message.

The encoding round-trip check can be expensive. That's why I decided initially to only execute the check in the "critical/important" write-to-Git-database situations (CONV_WRITE_OBJECT flag!). I also decided to run it only if the "SHIFT-JIS" encoding is used as this was the only encoding that I could find which reportedly does not round-trip with UTF-8 (although I was not able to replicate the round-trip problems).

I want to change the current implementation as follows:

I want to check the round-trip encoding only if the the environment variable "GIT_WORKING_TREE_ENCODING_ROUNDTRIP_CHECK" is set. This way a user can check the round-trip if necessary for *any* encoding. I don't want to make it a git config because that setting should only rarely be used for debugging purposes.

Performing the round-trip check every time is not necessary from my point of view because it can be expensive and I was not able to generate a test case which *does not* round-trip without triggering any other iconv error.

- Lars