Re: [PATCH v3 1/2] Allow git-apply to ignore the hunk headers (AKA recountdiff)

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 6, 2008, 04:55 UTC
Message-ID: <7vve0nw4b7.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <alpine.DEB.1.00.0806060006370.21190@racer>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> +static int recount_diff(char *line, int size, struct fragment *fragment)
> +{
> +	int line_nr = 0;

At this point, line points at the beginning of the line that immediately follows "@@ -oldpos,oldlines +newpos,newlines @@ ...\n", right?

> +	if (size < 1)
> +		return -1;
> +
> +	fragment->oldpos = 2;

Why do you discard oldpos information, and use magic number "2"?

Show 6 quoted lines

> +	fragment->oldlines = fragment->newlines = 0;
> +
> +	for (;;) {
> +		int len = linelen(line, size);
> +		size -= len;
> +		line += len;

And you look at the line of the patch, measure how long it is, and you already advance line to point at the next line without ever looking at the contents of the line (you could look at line[-len], but that is crazy).

> +		if (size < 1)
> +			return 0;

Why? It may be the last line in the hunk but you haven't done anything to the current line yet.

Show 14 quoted lines

> +		switch (*line) {
> +		case ' ': case '\n':
> +			fragment->newlines++;
> +			/* fall through */
> +		case '-':
> +			fragment->oldlines++;
> +			break;
> +		case '+':
> +			fragment->newlines++;
> +			if (line_nr == 0) {
> +				fragment->leading = 1;
> +				fragment->oldpos = 1;
> +			}
> +			fragment->trailing = 1;

Again, why muck with oldpos? Also leading and trailing? After you recount the newlines and oldlines you ignored from the hunk header, the caller will behave as if the original patch had the right numbers on the hunk header to compute leading and trailing, doesn't it?

Show 7 quoted lines

> +			break;
> +		case '@':
> +			return size < 3 || prefixcmp(line, "@@ ");
> +		case 'd':
> +			return size < 5 || prefixcmp(line, "diff ");
> +		default:
> +			return -1;

I do not understand these return values. Your caller, parse_fragment() with your patch, gives you chance to recount the old and new line count, and you are responsible for only recounting them. The change you made to parse_fragment() returns error, aborting the whole git-apply, when recount_diff() returns non-zero, but having extra lines after the patch text is perfectly fine and there is no reason to force aborting from here. IOW, this function should not even have power to do so. The one and only thing this function should do well is to reliably count the number of patch lines.

Show 7 quoted lines

> @@ -1013,6 +1058,9 @@ static int parse_fragment(char *line, unsigned long size,
>  	offset = parse_fragment_header(line, len, fragment);
>  	if (offset < 0)
>  		return -1;
> +	if (offset > 0 && patch->recount &&
> +			recount_diff(line + offset, size - offset, fragment))
> +		return -1;

... and this "return -1" is uncalled for.