> On 03 Jan 2018, at 06:36, Torsten Bögershausen <tboegi@xxxxxx> wrote:
>
> On Tue, Jan 02, 2018 at 08:11:51PM +0100, Lars Schneider wrote:
>
> [snip]
>
>>> /*****************************************************************
>>> diff --git a/diff.c b/diff.c
>>> index fb22b19f09..2470af52b2 100644
>>> --- a/diff.c
>>> +++ b/diff.c
>>> @@ -3524,9 +3524,9 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)
>>> * demote FAIL to WARN to allow inspecting the situation
>>> * instead of refusing.
>>> */
>>> - enum safe_crlf crlf_warn = (safe_crlf == SAFE_CRLF_FAIL
>>> - ? SAFE_CRLF_WARN
>>> - : safe_crlf);
>>> + int conv_flags = (conv_flags_eol == CONV_EOL_RNDTRP_DIE
>>> + ? CONV_EOL_RNDTRP_WARN
>>> + : conv_flags_eol);
>>
>> If there is garbage in conv_flags_eol then we would not demote the DIE
>> flag here.
>>
>> How about something like that:
>> int conv_flags = conv_flags_eol & ~CONV_EOL_RNDTRP_DIE;
>
> The next version will probably look like this:
>
> int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)
> {
> int size_only = flags & CHECK_SIZE_ONLY;
> int err = 0;
> int conv_flags = conv_flags_eol;
> /*
> * demote FAIL to WARN to allow inspecting the situation
> * instead of refusing.
> */
> if (conv_flags & CONV_EOL_RNDTRP_DIE)
> conv_flags =CONV_EOL_RNDTRP_WARN;
This looks good. Sorry, I just realized that my suggestion
was garbage as it only disabled the bit. It did not demote
DIE to WARN.
One final nit:
Can we be sure that 'conv_flags_eol' is only ever used for CONV_EOL_RNDTRP_DIE
and CONV_EOL_RNDTRP_WARN? Maybe a solution like that would be more future
proof?
if (conv_flags & CONV_EOL_RNDTRP_DIE) {
conv_flags &= ~CONV_EOL_RNDTRP_DIE;
conv_flags |= CONV_EOL_RNDTRP_WARN;
}
>>
>> ---
>>
>> In general I like the patch as I think the variables are a bit easier to understand.
>> One thing I stumbled over while reading the patch:
>>
>> The global variable "conv_flags_eol". I think the Git coding guidelines
>> have no recommendation for naming global variables. I think a "global_conv_flags_eol"
>> or something would have helped me. I can also see how others might frown upon such
>> a naming scheme.
>
> I don't have a problem with "global_conv_flags_eol".
> Thank for the comments, let's wait for more comments before I send out V4.
Sounds good. A "global_" like prefix is not used anywhere else in Git...
- Lars
