Re: [PATCHv1bis 1/2] git apply: option to ignore whitespace differences

[Giuseppe Bilotta <giuseppe.bilotta@gmail.com>]

On Thu, Jul 2, 2009 at 8:27 PM, Junio C Hamano<gitster@pobox.com> wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>
>> +static int memcmp_ignore_whitespace(const char *s1, size_t n1, const char *s2, size_t n2)
>> +{
>> +     const char *stop1 = s1 + n1;
>> +     const char *stop2 = s2 + n2;
>> +     int result;
>> +
>> +     if (!(n1 | n2))
>> +             return 0;
>> +
>> +     do {
>> +             if (isspace(*s1) && isspace(*s2)) {
>> +                     while (isspace(*s1)) {
>> +                             s1++;
>> +                     }
>> +                     while (isspace(*s2))
>> +                             s2++;
>> +             }
>> +             /* Check here instead of in the while because
>> +                the whitespace discarding might have moved us
>> +                past the end */
>> +             if ((s1 >= stop1) || (s2 >= stop2))
>> +                     break;
>
> If s1 is longer than s2 (or vice versa) but one is a prefix of the other,
> you will return "they match", because previous round compared *s1 and *s2
> and found them the same?

Yes. Actually, more specifically, now that I think more about this,
that's the reason why my previous code was fine: when we compare the
lines in the preimage against the image, we only look for a match
which is as long as the preimage, we don't care what ELSE is there in
the image. So it makes sense to pass a single length parameter, the
preimage length.

However, my first version should be fixed so that the order of the
parameter becomes significant, and the early bailout is only done if
the preimage length has been totally consumed.

>> +/*
>> + * Returns true if the given lines (buffer + len) match
>> + * according to the ignore_whitespace setting
>> + */
>> +static int lines_match(const char *s1, size_t n1, const char *s2, size_t n2)
>> +{
>> +     if (ignore_whitespace)
>> +             return !memcmp_ignore_whitespace(s1, n1, s2, n2);
>> +     else
>> +             return (n1 == n2) && !memcmp(s1, s2, n1);
>> +}
>> +
>
> I think this still is an abstraction at the wrong level.  For one thing,
> if ignore-whitespace is set, you do not even need nor want to do the "fix
> only the ws breakages we are going to fix anyway according to the ws_rule"
> transformation applied to the preimage.

I've thought some more about this, and you are right. We still want to
ws fix the postimage, but that's done elsewhere.

-- 
Giuseppe "Oblomov" Bilotta