git-am applies half of a patch

I ran

	git-am --signoff --utf8 /g/tmp/mbox

on the attached file, to apply a patch to libata-dev.git#upstream, and it wound up only applying a portion of the patch:

It seems quite bad to apply a patch that git-am KNOWS is incomplete.

	Jeff

Jeff Garzik <jeff@garzik.org> writes:

Quite true.

This turns out to be quite an old bug (dates back to May 2005). I've scanned builtin-apply.c while coming up with this fix to see if there other "detected but ignored" errors, but it appears this is the only one.

Sorry for the trouble and thanks for the report.

-- >8 -- [PATCH] Do not ignore a detected patchfile brokenness.

find_header() function is used to read and parse the patchfile and it detects errors in the patch, but one place ignored the error and went ahead, which was quite bad.

Noticed by Jeff Garzik.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

Junio C Hamano wrote:

Thanks for the ultra super duper lightning fast fix :)

	Jeff

Oh well, I should read my whole mbox ..

On Tue, 9 Jan 2007, Junio C Hamano wrote:

I don't think this really does anything, Junio.

Yes, it will now return an error value from "find_header()", but that's strictly normal: we _always_ get an error value from find_header() at the very end of a patch ("there's no more header").

And the caller (parse_chunk()) just ends up returning the error value, and the caller of THAT just ends up breaking out of the loop that does the patching, ie it does

	..
	while (size > 0) {
		..
		nr = parse_chunk(buffer + offset, size, patch);
		if (nr < 0)
			break;
		..

so you really need to make the "return error(..)" thing be a "die()" instead.

Alternatively, you could just say that _any_ garbage at the end of a patch is fatal, and make apply_patch() just die on a negative error from parse_chunk(), but garbage at the end of a patch is strictly normal (things like signatures or just even empty lines), so that wouldn't really work either.

In other words, I really don't think your patch does anything (except if the garbage is at the BEGINNING of a patch), and that you should take mine instead.

But test it.

		Linus

Linus Torvalds <torvalds@osdl.org> writes:

Ooooooops.

On Tue, 9 Jan 2007, Jeff Garzik wrote:

It doesn't actually "know" it. What is going on is that the "patch" program generally allows random crap in between multiple patches, and while git-apply is normally stricter than "patch" ever is, git-apply does allow this "garbage between patches" thing too.

Sometimes the garbage is just commentary (some people say things in between patches), but quite often it's also stuff like "Index: " (and in fact, git considers the "diff " line at the head to be "garbage" too, unless it's a native git diff that has the "diff --git" signature).

So what git is actually doing is seeing that you have _garbage_ after the real patch, but then it _warns_ about the garbage containing what looks like a patch fragment. And the reason it's garbage (rather than a real patch) is obviously because your patch was so heavily whitespace-damaged that the header that was supposed to start it wasn't a valid header at all..

But if you'd like to make it fatal, the patch is certainly simple..

Junio - I certainly wouldn't mind making this a fatal error, but it's ultimately your decision.

		Linus

---