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

>> On 25 Feb 2018, at 08:15, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Sat, Feb 24, 2018 at 11:27 AM,  <lars.schneider@autodesk.com> wrote:
>> The above paragraph is giving an example of the scenario described in
>> the paragraph above it. It might, therefore, be clearer to start this
>> paragraph with "For example,...". Also, as an aid to non-Windows
>> developers, it might be helpful to introduce ".proj" files as
>> "Microsoft Visual Studio `*.proj` files...".
>
> Agreed. How about this?
>
>   For example, Microsoft Visual Studio resources files (`*.rc`) or
>   PowerShell script files (`*.ps1`) are sometimes encoded in UTF-16.
>   If you declare `*.ps1` as files as UTF-16 and you add `foo.ps1` with
>   a `working-tree-encoding` enabled Git client, then `foo.ps1` will be
>   stored as UTF-8 internally. A client without `working-tree-encoding`
>   support will checkout `foo.ps1` as UTF-8 encoded file. This will
>   typically cause trouble for the users of this file.

>>> diff --git a/convert.c b/convert.c
>>> +       } else if (is_missing_required_utf_bom(enc->name, src, src_len)) {
>>> +               const char *error_msg = _(
>>> +                       "BOM is required in '%s' if encoded as %s");
>>> +               const char *advise_msg = _(
>>> +                       "The file '%s' is missing a byte order mark (BOM). "
>>> +                       "Please use %sBE or %sLE (depending on the byte order) "
>>> +                       "as working-tree-encoding.");
>>> +               advise(advise_msg, path, enc->name, enc->name);
>>> +               if (conv_flags & CONV_WRITE_OBJECT)
>>> +                       die(error_msg, path, enc->name);
>>> +               else
>>> +                       error(error_msg, path, enc->name);
>>> +       }
>>
>> For a function which presumably is agnostic about the working-tree
>> encoding (supporting whatever iconv does), it nevertheless has
>> unusually intimate knowledge about UTF BOMs, in general, and the
>> internals of has_prohibited_utf_bom() and
>> is_missing_required_utf_bom(), in particular. It might be cleaner, and
>> would simplify this function, if all this UTF BOM-specific gunk was
>> moved elsewhere and generalized into a "validate conversion" function.
>
> Agreed! How about this?
>
>         /*
>          * If we convert to an UTF encoding, then check for common BOM errors.
>          */
>         if (!memcmp("UTF-", enc, 4))
>                 if (has_utf_bom_error(path, enc, src, src_len, die_on_error))
>                         return 0;