# git-am applies half of a patch 6 messages from 2007-01-09 to 2007-01-09. Participants: Jeff Garzik, Junio C Hamano, Linus Torvalds. Thread: https://gitlist.dev/t/6299 ## Jeff Garzik, 2007-01-09 10:24 Subject: git-am applies half of a patch Message-ID: <45A36D6B.4010303@garzik.org> URL: https://gitlist.dev/e/45A36D6B.4010303%40garzik.org ``` 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 ``` ## Junio C Hamano, 2007-01-09 10:52 Subject: Re: git-am applies half of a patch Message-ID: <7vtzz0mpzk.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vtzz0mpzk.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <45A36D6B.4010303@garzik.org> ``` Jeff Garzik 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 --- 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) ``` ## Jeff Garzik, 2007-01-09 11:10 Subject: Re: git-am applies half of a patch Message-ID: <45A37812.1030408@garzik.org> URL: https://gitlist.dev/e/45A37812.1030408%40garzik.org In-Reply-To: <7vtzz0mpzk.fsf@assigned-by-dhcp.cox.net> ``` 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 Thanks for the ultra super duper lightning fast fix :) Jeff ``` ## Linus Torvalds, 2007-01-09 16:02 Subject: Re: git-am applies half of a patch Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0701090748020.3661%40woody.osdl.org In-Reply-To: <45A36D6B.4010303@garzik.org> ``` 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) ``` ## Linus Torvalds, 2007-01-09 16:08 Subject: Re: git-am applies half of a patch Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0701090803070.3661%40woody.osdl.org In-Reply-To: <7vtzz0mpzk.fsf@assigned-by-dhcp.cox.net> ``` 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 ``` ## Junio C Hamano, 2007-01-09 18:12 Subject: Re: git-am applies half of a patch Message-ID: <7vac0sm5m0.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vac0sm5m0.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Linus Torvalds 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. ```