Re: [PATCH/RFC v1 1/1] bug fix, diff whitespace ignore options

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jan 19, 2009, 03:53 UTC
Message-ID: <alpine.DEB.1.00.0901190446480.3586@pacific.mpi-cbg.de>
In-Reply-To: <alpine.GSO.2.00.0901181754190.9333@kiwi.cs.ucla.edu>

Hi,

On Sun, 18 Jan 2009, Keith Cascio wrote:

Show 25 quoted lines

>  Fixed bug in diff whitespace ignore options.
>  It is now OK to specify more than one whitespace ignore option
>  on the command line. In unit test 4015, expect success rather
>  than failure for 4 cases.
>  Note: I do not fully understand why this fix works, but it passes
>  all 68 t4???-* diff test scripts.
> 
> The semantics of the three whitespace ignore flags
> { -w, -b, --ignore-space-at-eol }
> obey a relation of transitive implication, i.e. the stronger
> options imply the weaker options:
> -w                    implies the other two
> -b                    implies --ignore-space-at-eol
> --ignore-space-at-eol implies only itself
> 
> Therefore it is never necessary to specify more than one of these
> on the command line.  Yet we imagine scenarios where software
> wrappers (e.g. GUIs, etc) generate command lines that switch on
> more than one of these flags simultaneously.  It is unreasonable
> to prohibit specifying more than one, since a new user might not
> immediately discern the implication relation.  Now we call such
> a command line valid and legal.
> 
> Signed-off-by: Keith Cascio <keith@cs.ucla.edu>
> ---

This does not really look all that similar to other commit messages.

For example, "Note: I do not fully understand why this fix works, but it passes all 68 t4???-* diff test scripts." is rather discouraging. If you are not convinced, how should we be?

However, I almost can excuse that, but...

Show 22 quoted lines

>  t/t4015-diff-whitespace.sh |    8 ++++----
>  xdiff/xutils.c             |   22 ++++++++++++----------
>  2 files changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/xdiff/xutils.c b/xdiff/xutils.c
> index d7974d1..b9bda86 100644
> --- a/xdiff/xutils.c
> +++ b/xdiff/xutils.c
> @@ -245,17 +245,19 @@ static unsigned long
> xdl_hash_record_with_whitespace(char const **data,
>  			while (ptr + 1 < top && isspace(ptr[1])
>  					&& ptr[1] != '\n')
>  				ptr++;
> -			if (flags & XDF_IGNORE_WHITESPACE_CHANGE
> -					&& ptr[1] != '\n') {
> -				ha += (ha << 5);
> -				ha ^= (unsigned long) ' ';
> -			}
> -			if (flags & XDF_IGNORE_WHITESPACE_AT_EOL
> -					&& ptr[1] != '\n') {
> -				while (ptr2 != ptr + 1) {
> +			if( ! (          flags & XDF_IGNORE_WHITESPACE

... this is just plain ugly, not to mention breaking the coding style of the surrounding code in a rather blatant way.

Show 15 quoted lines

> )){
> +				if(      flags & XDF_IGNORE_WHITESPACE_CHANGE
> +						&& ptr[1] != '\n') {
>  					ha += (ha << 5);
> -					ha ^= (unsigned long) *ptr2;
> -					ptr2++;
> +					ha ^= (unsigned long) ' ';
> +				}
> +				else if( flags & XDF_IGNORE_WHITESPACE_AT_EOL
> +						&& ptr[1] != '\n') {
> +					while (ptr2 != ptr + 1) {
> +						ha += (ha << 5);
> +						ha ^= (unsigned long) *ptr2;
> +						ptr2++;
> +					}

Besides, I think what you actually wanted is

		if (flags & XDF_IGNORE_WHITESPACE)
			; /* already handled */
		else if (flags & XDF_IGNORE_WHITESPACE_CHANGE)
			...
		else if (flags & XDF_IGNORE_WHITESPACE_AT_EOL)
			...

for improved readability both of the code and the patch.

Ciao, Dscho