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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 2, 2009, 16:46 UTC
Message-ID: <7vws6r822y.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1246539137-24754-2-git-send-email-giuseppe.bilotta@gmail.com>

Show 9 quoted lines

> 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//;

Show 5 quoted lines

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

You are going to use this function to compare a line that is in the target buffer (i.e. the contents of the current blob) with a line read from the patch context when the user says they might match if you ignore whitespace differences.

How on earth can you do that with only a single length parameter "n"? Length of which side are you talking about?

Remember that git-apply is designed to be able to handle a patch that has NUL in it (generated with "diff --text"), so strlen() is not an acceptable answer.

Show 8 quoted lines

> +static int memcmp_switch(const char *s1, const char *s2, size_t n)
> +{
> +	if (ignore_whitespace)
> +		return memcmp_ignore_whitespace(s1, s2, n);
> +	else
> +		return memcmp(s1, s2, n);
> +}
> +

This function signature shares the same bogosity with the previous one.

In addition, it's name and semantics is bogus.

The original implementation had encapsulated the notion that it is comparing two lines entirely in match_fragment(). Its use of memcmp() was an implementation detail of a half of the open-coded logic to compare two lines (the other half being the comparison of lengths), and direct use of memcmp() made perfect sense. It checked the length matched. Now it wanted to make sure they matched byte-for-byte.

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

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