Re: [PATCH 1/2] xdiff-interface: export comparing and hashing strings

From: Stefan Beller <sbeller@google.com>
Date: Oct 27, 2017, 17:15 UTC
Message-ID: <CAGZ79kZgXZsBOf=9MZnoOCpJmARbaEaLPDBVAGN4GMvxhMotog@mail.gmail.com>
In-Reply-To: <xmqqefppcbec.fsf@gitster.mtv.corp.google.com>

On Fri, Oct 27, 2017 at 12:12 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 34 quoted lines

> René Scharfe <l.s.r@web.de> 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.

Show 14 quoted lines

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