Re: [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line

[Thell Fowler <git@tbfowler.name>]

Junio C Hamano (gitster@pobox.com) wrote on Aug 23, 2009:

> Thell Fowler <git@tbfowler.name> 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.
> 

That's good to hear!

> >> +		/*
> >> +		 * 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 */".
> 

In that case the only other outstanding issue to being able to use 
patch-id to validate a whitespace fixed patch is diff's -B option to catch 
the situations where the original has multiple blank newlines at the end 
of file.


> > 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.
> 

HA!  That's a nifty way to do that with the variables.

> > 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.
> 

Excellent.

> > Thank you again for taking the time to look at this change!
> 
> Thank _you_ for bringing this issue up in the first place.

My pleasure!  It has been quite the learning experience!

-- 
Thell