On Fri, Oct 27, 2017 at 12:12 AM, Junio C Hamano <[email protected]> wrote:
> René Scharfe <[email protected]> writes:
>
>> Am 25.10.2017 um 20:49 schrieb Stefan Beller:
>>> +/*
>>> + * Compare the strings l1 with l2 which are of size s1 and s2 respectively.
>>> + * Returns 1 if the strings are deemed equal, 0 otherwise.
>>> + * The `flags` given as XDF_WHITESPACE_FLAGS determine how white spaces
>>> + * are treated for the comparision.
>>> + */
>>> +extern int xdiff_compare_lines(const char *l1, long s1,
>>> + const char *l2, long s2, long flags);
>>
>> With the added comment it's OK here.
>>
>> Still, I find the tendency in libxdiff to use the shortest possible
>> variable names to be hard on the eyes. That math-like notation may have
>> its place in that external library, but I think we should be careful
>> lest it spreads.
>
> Yeah, I tend to agree. The xdiff-interface is at the (surprise!)
> interface layer, so we could make it follow the style of either one,
> but we seem to have picked up the convention of the lower layer,
> so...
>
> By the way, I was looking at the code around this area while
> reviewing the cr-at-eol thing, and noticed a couple of things:
>
>
> * combine-diff.c special cases only IGNORE_WHITESPACE and
> IGNORE_WHITESPACE_CHANGE but no other IGNORE_WHITESPACE things; I
> have a suspicion that this is not because IGNORE_WHITESPACE_AT_EOL
> does not have to special cased by the codepath, but only because
> the combine-diff.c refactoring predates the introduction of
> ws-at-eol thing?
I wonder how much overlap between the IGNORE_WHITESPACE_AT_EOL
and CRLF-AT-EOL is (maybe these can be combined, as per the root of
this discussion)
> The machinery uses its own match_string_spaces() helper; it may
> make sense to use the same xdiff_compare_lines() in its callers
> and get rid of this helper function.
Speaking of xdiff_compare_lines, it serves the special purpose of the
move detection as well as its internal use cases, but we may need to
change its interface/implementation in the future, to align it with strcmp,
currently the compare function only returns equality, not an order.
> * diff_setup_done() sets DIFF_FROM_CONTENTS when any of the
> IGNORE_WHITESPACE bits is true, to allow "git diff -q
> --ignore-something" would do the right thing. We do not however
> give a similar special case to XDF_IGNORE_BLANK_LINES.
>
> $ echo >>Makefile && git diff $option --ignore-blank-lines Makefile
>
> exits with 1 when option=--exit-code and it exits with 0 when
> option=-q
>
>
> For now I'll leve these as #leftoverbits, but I may come back to the
> latter soonish. I won't come back to the former until Stefan's
> refactor hits 'master', though.
which is presumably after the 2.15 release.
To tack on the #leftoverbits: The --color-moved detection doesn't
pay attention to XDF_IGNORE_BLANK_LINES (which is tricky as
it is on the per-line layer. If we want to ignore spurious blank lines,
we'd have to check for this flag in diff.c in mark_color_as_moved(..)
in the block
/* Check any potential block runs, advance each or nullify */
Thanks,
Stefan
