Re: [PATCH 1/5] convert_to_git(): checksafe becomes an integer

From: Lars Schneider <larsxschneider@gmail.com>
Date: Dec 31, 2017, 12:23 UTC
Message-ID: <96B6CD4C-0A0C-47F5-922D-B8BAFB832FD1@gmail.com>
In-Reply-To: <20171231080552.22258-1-tboegi@web.de>

Show 11 quoted lines

> On 31 Dec 2017, at 09:05, tboegi@web.de wrote:
> 
> From: Torsten Bögershausen <tboegi@web.de>
> 
> When calling convert_to_git(), the checksafe parameter has been used to
> check if commit would give a non-roundtrip conversion of EOL.
> 
> When checksafe was introduced, 3 values had been in use:
> SAFE_CRLF_FALSE: no warning
> SAFE_CRLF_FAIL:  reject the commit if EOL do not roundtrip
> SAFE_CRLF_WARN:  warn the user if EOL do not roundtrip

In general, I really like the direction as this simplifies my patch later on in 5/5. However, I see a few problems:

(1) The prefix "SAFE_CRLF" confuses me because the main theme
    is EOL and CRLF just happens to be a EOL type.

    What do you think about something like this:
    CONVERT_ERROR_IGNORE     0
    CONVERT_EOL_ERROR_DIE    (1<<0)
    CONVERT_EOL_ERROR_WARN   (1<<1)
    CONVERT_EOL_TO_LF        (1<<2)
    CONVERT_EOL_KEEP_CRLF    (1<<3)
    CONVERT_ENCODE_ERROR_DIE (1<<4)

(2) We mix error reporting switches (FALSE/FAIL/WARN) with
    switches that change the content (RENORMALIZE/KEEP_CRLF).
    Plus, these the switches should be mutually exclusive
    (e.g. we don't want to enable the FAIL and WARN bit at
    the same time).

(3) We kind of replicate some flags defined in cache.h:
    #define HASH_WRITE_OBJECT 1
    #define HASH_RENORMALIZE  4

- Lars