Thomas Rast <trast@xxxxxxxxxxxxxxx> 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...
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
