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;
>
> ---
>
> 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.
