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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 2, 2009, 18:27 UTC
Message-ID: <7vvdmb6ium.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1246556921-5819-2-git-send-email-giuseppe.bilotta@gmail.com>

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

Show 22 quoted lines

> +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?

Show 12 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 think the handling of correct_ws_error in the caller should also be moved here. IOW, shouldn't this function look like this?

	lines_match()
        {
        	/* if the user wants fuzz, so be it */
        	if (ignore_whitespace)
                	return compare_lines_wo_ws();
		/* most common case: matches without fuzzing */
		if (length matches && !memcmp())
                	return 1;
		/* we are done, if we are not correcting */
		if (ws_error_action != correct_ws_error)
                	return 0;
		... apply configured ws fix to preimage ...;
                /* does it apply with ws breakage in its context fixed? */
                if (length now matches && !memcmp(fixed preimage, postimage)
			return 1;
		/* noop, this won't apply */
		return 0;
	}

which would make the large-ish loop near the end of match_fragment() to something like:

	for (i = 0; i < preimage->nr; i++) {
        	... set up arguments to lines_match ...;
		match = lines_match();
		if (!match)
                	goto unmatch_exit;
		... adjust for next iteration ...;
        }