Re: [PATCHv1bis 1/2] git apply: option to ignore whitespace differences
- From
Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
- Date
- Jul 2, 2009, 19:02 UTC
- Message-ID
- <cb7bb73a0907021202ra322425pc64b54953f4f544d@mail.gmail.com>
- In-Reply-To
- <7vvdmb6ium.fsf@alter.siamese.dyndns.org>
On Thu, Jul 2, 2009 at 8:27 PM, Junio C Hamano<gitster@pobox.com> wrote:
Show 28 quoted lines
> 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.
Show 17 quoted lines
>> +/*
>> + * 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