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

From: Lars Schneider <larsxschneider@gmail.com>
Date: Jan 3, 2018, 09:37 UTC
Message-ID: <8C03FD92-FB9A-4065-9D88-0FBD268F8737@gmail.com>
In-Reply-To: <20180103053654.GA428@tor.lan>

Show 41 quoted lines

> On 03 Jan 2018, at 06:36, Torsten Bögershausen <tboegi@web.de> 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;
  }

Show 13 quoted lines

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