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

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

On Thu, Jul 2, 2009 at 6:46 PM, Junio C Hamano<gitster@pobox.com> wrote:
>> diff --git a/builtin-apply.c b/builtin-apply.c
>> index dc0ff5e..86860d6 100644
>> --- a/builtin-apply.c
>> +++ b/builtin-apply.c
>> @@ -39,6 +39,7 @@ static int diffstat;
>>  static int numstat;
>>  static int summary;
>>  static int check;
>> +static int ignore_whitespace = 0;
>
> s/ = 0//;

Ah, I wondered about that. I assume this leaves no possibility for
uninitialized values because of the way option parsing is done?

>> +/*
>> + * Compare two memory areas ignoring whitespace differences
>> + */
>> +static int memcmp_ignore_whitespace(const char *s1, const char *s2, size_t n)
>> +{
>
> Don't you think this function signature is bogus?

I did have a few doubts on it. I think I worked out a proper solution,
per your suggestion.

>> +static int memcmp_switch(const char *s1, const char *s2, size_t n)
>
> This function signature shares the same bogosity with the previous one.
>
> In addition, it's name and semantics is bogus.

> If you are separating that "compare two lines" logic into a helper
> function, I would expect its name actually reflect its purpose, whose
> behaviour may change depending on --ignore-whitespace.  The traditional
> codepath would say "do they have the same length and match byte-for-byte?"
> while the new loosened codepath would say "we do not care about the
> whitespaces; do they match if we disregard ws differences?"

Indeed. See upcoming patch for a better name 8-)

> I also suspect that you might be able to optimize the existing "allow
> whitespace-fixed match" a bit by "fix"ing the target buffer only once,
> instead of doing so line-by-line for every offset that find_pos() checks
> by calling match_fragment().  It is an independent issue, but it appears
> to me that the change this patch wants to do to match_fragment() might
> become cleaner if we did that conversion first, as match_fragment() itself
> won't have to have two cases (early return for exact match, and match with
> whitespace-fixed target).

But OTOH that would 'waste time' fixing whitespace when it might not
be needed ...

-- 
Giuseppe "Oblomov" Bilotta