Re: [PATCH v2 2/2] diff: --ignore-cr-at-eol

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 15, 2017, 04:28 UTC
Message-ID: <xmqq60ac17yv.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <alpine.DEB.2.21.1.1711071345430.6482@virtualbox>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

I notice that I left a few things unanswered even after giving answers to the most important part (i.e. "what is this for was sold incorrectly"). Here are the leftover bits.

Show 16 quoted lines

>> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
>> index 89cc0f48de..aa2c0ff74d 100644
>> --- a/Documentation/diff-options.txt
>> +++ b/Documentation/diff-options.txt
>> @@ -519,6 +519,9 @@ endif::git-format-patch[]
>>  --text::
>>  	Treat all files as text.
>>  
>> +--ignore-cr-at-eol::
>> +	Ignore carrige-return at the end of line when doing a comparison.
>
> I am not a native speaker, either, yet I have the impression that "do a
> comparison" may be more colloquial than not. Also, it is a carriage-return
> (as in Sinatra's famous song about Love and Marriage) not a carrige-return.
>
> How about "Hide changed line endings"?

That felt like a good suggestion when I saw your reaction, especially with only the parts visible in the patch and its context, but after reviewing descriptions of other --ignore-* options, I no longer think so. The existing description of "--ignore-*" matches manpages of GNU diff and they do not say "hide", either.

Show 22 quoted lines

>> diff --git a/xdiff/xutils.c b/xdiff/xutils.c
>> index 04d7b32e4e..b2cbcc818f 100644
>> --- a/xdiff/xutils.c
>> +++ b/xdiff/xutils.c
>> @@ -156,6 +156,24 @@ int xdl_blankline(const char *line, long size, long flags)
>>  	return (i == size);
>>  }
>>  
>> +/*
>> + * Have we eaten everything on the line, except for an optional
>> + * CR at the very end?
>> + */
>> +static int ends_with_optional_cr(const char *l, long s, long i)
>> +{
>> +	int complete = s && l[s-1] == '\n';
>> +
>> +	if (complete)
>> +		s--;
>> +	if (s == i)
>> +		return 1;
>
> What is the role of `s`, what of `i`? Maybe `length` and `current_offset`?

I'd agree with that sentiment if this file were not borrowed code but our own, but after looking at xdiff/ code, I think the names of these variables follow the convention used there better, which consistently names the variable for lines they deal with l1, l2, etc., their sizes s1, s2, etc., and the indices into the line i1, i2, etc.

Show 6 quoted lines

>> +	/* do not ignore CR at the end of an incomplete line */
>> +	if (complete && s == i + 1 && l[i] == '\r')
>> +		return 1;
>
> This made me scratch my head: too many negations. The comment may better
> read "ignore CR only at the end of a complete line".

Perhaps. "incomplete line" is a term with a specific definition (and I think by "complete line" you mean a line that is not an incomplete line), so I do not see the above comment as having too many negations, though. If you feel strongly about it, you could "fix" it with a follow-up patch.

Show 14 quoted lines

>> @@ -204,6 +223,14 @@ int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags)
>>  			i1++;
>>  			i2++;
>>  		}
>> +	} else if (flags & XDF_IGNORE_CR_AT_EOL) {
>> +		/* Find the first difference and see how the line ends */
>> +		while (i1 < s1 && i2 < s2 && l1[i1] == l2[i2]) {
>> +			i1++;
>> +			i2++;
>> +		}
>> +		return (ends_with_optional_cr(l1, s1, i1) &&
>> +			ends_with_optional_cr(l2, s2, i2));
>
> There are extra parentheses around the `return` expression.

Yes, everybody knows that return is not a function that needs a parentheses around its parameter. I would drop them if this expression were not split into two lines, but because the expression is split at &&, I think it reads better with the extra parens. So I'll leave them as-is.

Thanks.