Re: [PATCH v3 1/1] convert_to_git(): checksafe becomes int conv_flags

["Torsten Bögershausen" <tboegi@web.de>]

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.