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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 27, 2017, 07:12 UTC
Message-ID: <xmqqefppcbec.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <3f656948-273f-e01d-ad52-e460059571da@web.de>

René Scharfe <l.s.r@web.de> writes:

Show 16 quoted lines

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

   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.

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