Re: [PATCHv2 22/25] diff.c: color moved lines differently

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> writes:

> On Fri, Jun 30, 2017 at 2:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> +             return (int)' ';
>>
>> Do we need a cast here?
>
> No, I figured it is good to have it here explicitly, though.
> We can drop that if you have strong preferences one way or another.

I was merely wondering if you were protecting against a funny
platform where ' ' is a negative number ;-)

>>> +static unsigned get_string_hash(struct emitted_diff_symbol *es, struct diff_options *o)
>>> +{
>>> +     if (o->xdl_opts & XDF_WHITESPACE_FLAGS) {
>>> +             static struct strbuf sb = STRBUF_INIT;
>>> +             const char *ap = es->line, *ae = es->line + es->len;
>>> +             int c;
>>> +
>>> +             strbuf_reset(&sb);
>>> +             while (ae > ap && isspace(*ae))
>>> +                     ae--;
>>
>> Not testing for the AT_EOL option here?
>
> No, because the other options are stronger than the AT_EOL,
> such that as you note it is still correct.
>
> If in the future, we'd have another new option e.g.
> IGNORE_TAB_BLANK_CONVERSION_BUT_WARN_ON_LENGTH_DIFF
> (useful for python programmers ;)
> this would break.

That remark makes it sound as if this is a timebomb ticking, but I
do not think that is the case.  This is merely stripping whitespaces
at the end; mixing tabs and spaces without changing the indentation
level matters only when you have something that is !isspace() to
indent to begin with ;-)

So, the effect of not checking is only a hashmap that is bit less
efficient than necessary, but is there an undue cost of actually
checking and doing this skipping only when we are ignoring the
whitespaces at the end of lines?

>> By the way, this is an unrelated tangent because I think you
> ...
> I agree.  I can make a cleanup throughout the whole code base,
> but I would prefer if that is done in a separate series, as this
> is already slightly lengthy.

Oh, I 100% agree that it is an unrelated tangent.