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

On 03 Jan 2018, at 06:36, Torsten Bögershausen [off-list ref] wrote:

On Tue, Jan 02, 2018 at 08:11:51PM +0100, Lars Schneider wrote:

[snip]

quoted

quoted

/*****************************************************************

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

quoted

---

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