git-am applies half of a patch

git-am applies half of a patch

[Jeff Garzik]

[-- Attachment #1: Type: text/plain, Size: 617 bytes --]

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:

> [jgarzik@pretzel libata-dev]$ git-am --signoff --utf8 /g/tmp/mbox 
> 
> Applying 'Add pci class code for SATA AHCI'
> 
> error: patch fragment without header at line 35: @@ -862,7 +862,7 @@
> error: patch fragment without header at line 50: @@ -15,6 +15,8 @@
> Wrote tree 5d6f3a93bea932c950ac880deca173dd3e84dfcc
> Committed: 317b180bad43133027dc07455f1600f4e8a47d76


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

	Jeff



[-- Attachment #2: mbox.bz2 --]
[-- Type: application/x-bzip, Size: 2426 bytes --]

Re: git-am applies half of a patch

[Junio C Hamano]

Jeff Garzik <jeff@garzik.org> writes:

> 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:
>
>> [jgarzik@pretzel libata-dev]$ git-am --signoff --utf8 /g/tmp/mbox
>>
>> Applying 'Add pci class code for SATA AHCI'
>>
>> error: patch fragment without header at line 35: @@ -862,7 +862,7 @@
>> error: patch fragment without header at line 50: @@ -15,6 +15,8 @@
>> Wrote tree 5d6f3a93bea932c950ac880deca173dd3e84dfcc
>> Committed: 317b180bad43133027dc07455f1600f4e8a47d76
>
> It seems quite bad to apply a patch that git-am KNOWS is incomplete.

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>
---
diff --git a/builtin-apply.c b/builtin-apply.c
index 1c35837..918822b 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -811,7 +811,7 @@ static int find_header(char *line, unsigned long size, int *hdrsize, struct patc
 			struct fragment dummy;
 			if (parse_fragment_header(line, len, &dummy) < 0)
 				continue;
-			error("patch fragment without header at line %d: %.*s", linenr, (int)len-1, line);
+			return error("patch fragment without header at line %d: %.*s", linenr, (int)len-1, line);
 		}
 
 		if (size < len + 6)

Re: git-am applies half of a patch

[Jeff Garzik]

Junio C Hamano wrote:
> 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>


Thanks for the ultra super duper lightning fast fix :)

	Jeff

Re: git-am applies half of a patch

[Linus Torvalds]

Oh well, I should read my whole mbox ..

On Tue, 9 Jan 2007, Junio C Hamano wrote:
>
> ---
> diff --git a/builtin-apply.c b/builtin-apply.c
> index 1c35837..918822b 100644
> --- a/builtin-apply.c
> +++ b/builtin-apply.c
> @@ -811,7 +811,7 @@ static int find_header(char *line, unsigned long size, int *hdrsize, struct patc
>  			struct fragment dummy;
>  			if (parse_fragment_header(line, len, &dummy) < 0)
>  				continue;
> -			error("patch fragment without header at line %d: %.*s", linenr, (int)len-1, line);
> +			return error("patch fragment without header at line %d: %.*s", linenr, (int)len-1, line);

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

Re: git-am applies half of a patch

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

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

Ooooooops.

Re: git-am applies half of a patch

[Linus Torvalds]

On Tue, 9 Jan 2007, Jeff Garzik wrote:
>
> 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:
> 
> > [jgarzik@pretzel libata-dev]$ git-am --signoff --utf8 /g/tmp/mbox 
> > Applying 'Add pci class code for SATA AHCI'
> > 
> > error: patch fragment without header at line 35: @@ -862,7 +862,7 @@
> > error: patch fragment without header at line 50: @@ -15,6 +15,8 @@
> > Wrote tree 5d6f3a93bea932c950ac880deca173dd3e84dfcc
> > Committed: 317b180bad43133027dc07455f1600f4e8a47d76
> 
> It seems quite bad to apply a patch that git-am KNOWS is incomplete.

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

---
diff --git a/builtin-apply.c b/builtin-apply.c
index 1c35837..d3fdf7e 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -811,7 +811,7 @@ static int find_header(char *line, unsigned long size, int *hdrsize, struct patc
 			struct fragment dummy;
 			if (parse_fragment_header(line, len, &dummy) < 0)
 				continue;
-			error("patch fragment without header at line %d: %.*s", linenr, (int)len-1, line);
+			die("patch fragment without header at line %d: %.*s", linenr, (int)len-1, line);
 		}
 
 		if (size < len + 6)