Thell Fowler <git@xxxxxxxxxxxxx> writes:
> Because the flow is much more direct it also makes the test additions to
> t4015 obsolete as they essentially tested for line end conditions instead
> of whitespace (like they should have).
Your patch 6/6 that added the tests were useful to find a bug I originally
had, which is the one below that is commented out.
>> + /*
>> + * If we do not want -b to imply --ignore-space-at-eol
>> + * then you would need to add this:
>> + *
>> + * if (!(flags & XDF_IGNORE_WHITESPACE_AT_EOL))
>> + * return (s1 <= i1 && s2 <= i2);
>> + *
>> + */
>> +
>
> While it would be nice to have -b and --ignore-space-at-eol be two
> different options that could be merged together the documentation states
> that -b ignores spaces at eol, and there are scripts that depend on this
> behavior.
Also that is how "diff -b" behaves, and that is why I said your tests
found a _bug_ in my original. I'll drop the above large comment and
replace it with just a "/* -b implies --ignore-space-at-eol */".
> Right now the xdl_recmatch() checks three distinct flags before having the
> opportunity to do the default behavior of a straight diff. In
> xdl_hash_record there is an initial check for whitespace flags.
>
> ...
> if (flags & XDF_WHITESPACE_FLAGS)
> return xdl_hash_record_with_whitespace(data, top, flags);
> ...
>
> Perhaps a similar setup for xdl_rematch() and a
> xdl_recmatch_with_whitespace() ?
Or we can just move the final else clause up and start the function like
this:
int i1, i2;
if (!(flags & XDF_WHITESPACE_FLAGS))
return s1 == s2 && !memcmp(l1, l2, s1);
i1 = i2 = 0;
if (flags & XDF_IGNORE_WHITESPACE) {
...
that would get rid of two unnecessary clearing of variables (i1 and i2,
even though I suspect that the compiler _could_ optimize them out without
such an change), and three flags-bit check in the most common case of not
ignoring any whitespaces.
> Since your to counter-proposals give the same results, provide safer and
> faster processing, eliminate the additional test, as well as being easier
> to read and comprehend I propose a v3 with just those two patches. I'll
> be glad to post it, with or without a xdl_recmatch_with_whitespace, if
> need be. And should I, or do I need to, add something to the commit (ie:
> ack, tested, ...) ?
I can amend the counterproposal patches with tests from your 6/6 and add
your "Tested-by:" and commit them myself.
> Thank you again for taking the time to look at this change!
Thank _you_ for bringing this issue up in the first place.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html