From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 24 Jun 2008 23:35:33 GMT
Subject: Re: [PATCH 1/3] Allow git-apply to ignore the hunk headers (AKA recountdiff)
Message-ID: <7vfxr29zju.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <200806242108.16379.trast@student.ethz.ch>

Thomas Rast <trast@student.ethz.ch> writes:

> diff --git a/builtin-apply.c b/builtin-apply.c
> index c497889..34c220f 100644
> --- a/builtin-apply.c
> +++ b/builtin-apply.c
> @@ -153,6 +153,7 @@ struct patch {
>  	unsigned int is_binary:1;
>  	unsigned int is_copy:1;
>  	unsigned int is_rename:1;
> +	unsigned int recount:1;
>  	struct fragment *fragments;
>  	char *result;
>  	size_t resultsize;

Why doesn't anybody find this quite wrong?

What is a "struct patch"?  It describes a change to a single file
(i.e. information contained from one "diff --git" til next "diff --git"),
groups the hunks (called "fragments") together and holds the postimage
after applying these hunks.  Is this new "recount" field a per file
attribute?

> +	fragment->oldpos = 2;
> +	fragment->oldlines = fragment->newlines = 0;

Why is this discarding the position information?  

It is none of this function's business.  The ONLY THING this function
should do is to discard fragment->oldlines and fragment->newlines, count
the contents and update these two fields, and nothing else.  Touching
oldpos, leading and other things is an absolute no-no.

> @@ -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 recount should not cause parse_fragment() to fail out either.  If you
miscounted, the codepath that follows this part knows how to handle broken
patch correctly anyway.

I think I've already mentioned the above two points when this was
originally posted.

Somewhat disgusted...