{"thread":{"id":"6299","subject":"git-am applies half of a patch","startedAt":"2007-01-09T10:24:43Z","lastAt":"2007-01-09T18:12:39Z","messageCount":6,"participants":["Jeff Garzik","Junio C Hamano","Linus Torvalds"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"31221","messageId":"45A36D6B.4010303@garzik.org","threadId":"6299","inReplyTo":null,"subject":"git-am applies half of a patch","fromName":"Jeff Garzik","fromEmail":"jeff@garzik.org","sentAt":"2007-01-09T10:24:43Z","receivedAt":"2007-01-09T10:24:43Z","isPatch":false,"sender":{"key":"jeff@garzik.org","avatar":null},"body":"I ran\n\n\tgit-am --signoff --utf8 /g/tmp/mbox\n\non the attached file, to apply a patch to libata-dev.git#upstream, and \nit wound up only applying a portion of the patch:\n\n> [jgarzik@pretzel libata-dev]$ git-am --signoff --utf8 /g/tmp/mbox \n> \n> Applying 'Add pci class code for SATA AHCI'\n> \n> error: patch fragment without header at line 35: @@ -862,7 +862,7 @@\n> error: patch fragment without header at line 50: @@ -15,6 +15,8 @@\n> Wrote tree 5d6f3a93bea932c950ac880deca173dd3e84dfcc\n> Committed: 317b180bad43133027dc07455f1600f4e8a47d76\n\n\nIt seems quite bad to apply a patch that git-am KNOWS is incomplete.\n\n\tJeff\n\n\n"},{"id":"31232","messageId":"7vtzz0mpzk.fsf@assigned-by-dhcp.cox.net","threadId":"6299","inReplyTo":"45A36D6B.4010303@garzik.org","subject":"Re: git-am applies half of a patch","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-01-09T10:52:31Z","receivedAt":"2007-01-09T10:52:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff Garzik <jeff@garzik.org> writes:\n\n> I ran\n>\n> \tgit-am --signoff --utf8 /g/tmp/mbox\n>\n> on the attached file, to apply a patch to libata-dev.git#upstream, and\n> it wound up only applying a portion of the patch:\n>\n>> [jgarzik@pretzel libata-dev]$ git-am --signoff --utf8 /g/tmp/mbox\n>>\n>> Applying 'Add pci class code for SATA AHCI'\n>>\n>> error: patch fragment without header at line 35: @@ -862,7 +862,7 @@\n>> error: patch fragment without header at line 50: @@ -15,6 +15,8 @@\n>> Wrote tree 5d6f3a93bea932c950ac880deca173dd3e84dfcc\n>> Committed: 317b180bad43133027dc07455f1600f4e8a47d76\n>\n> It seems quite bad to apply a patch that git-am KNOWS is incomplete.\n\nQuite true.\n\nThis turns out to be quite an old bug (dates back to May 2005).\nI've scanned builtin-apply.c while coming up with this fix to\nsee if there other \"detected but ignored\" errors, but it appears\nthis is the only one.\n\nSorry for the trouble and thanks for the report.\n\n-- >8 --\n[PATCH] Do not ignore a detected patchfile brokenness.\n\nfind_header() function is used to read and parse the patchfile\nand it detects errors in the patch, but one place ignored the\nerror and went ahead, which was quite bad.\n\nNoticed by Jeff Garzik.\n\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n---\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex 1c35837..918822b 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -811,7 +811,7 @@ static int find_header(char *line, unsigned long size, int *hdrsize, struct patc\n \t\t\tstruct fragment dummy;\n \t\t\tif (parse_fragment_header(line, len, &dummy) < 0)\n \t\t\t\tcontinue;\n-\t\t\terror(\"patch fragment without header at line %d: %.*s\", linenr, (int)len-1, line);\n+\t\t\treturn error(\"patch fragment without header at line %d: %.*s\", linenr, (int)len-1, line);\n \t\t}\n \n \t\tif (size < len + 6)\n"},{"id":"31234","messageId":"45A37812.1030408@garzik.org","threadId":"6299","inReplyTo":"7vtzz0mpzk.fsf@assigned-by-dhcp.cox.net","subject":"Re: git-am applies half of a patch","fromName":"Jeff Garzik","fromEmail":"jeff@garzik.org","sentAt":"2007-01-09T11:10:10Z","receivedAt":"2007-01-09T11:10:10Z","isPatch":false,"sender":{"key":"jeff@garzik.org","avatar":null},"body":"Junio C Hamano wrote:\n> Sorry for the trouble and thanks for the report.\n> \n> -- >8 --\n> [PATCH] Do not ignore a detected patchfile brokenness.\n> \n> find_header() function is used to read and parse the patchfile\n> and it detects errors in the patch, but one place ignored the\n> error and went ahead, which was quite bad.\n> \n> Noticed by Jeff Garzik.\n> \n> Signed-off-by: Junio C Hamano <junkio@cox.net>\n\n\nThanks for the ultra super duper lightning fast fix :)\n\n\tJeff\n"},{"id":"31252","messageId":"Pine.LNX.4.64.0701090748020.3661@woody.osdl.org","threadId":"6299","inReplyTo":"45A36D6B.4010303@garzik.org","subject":"Re: git-am applies half of a patch","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2007-01-09T16:02:50Z","receivedAt":"2007-01-09T16:02:50Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 9 Jan 2007, Jeff Garzik wrote:\n>\n> I ran\n> \n> \tgit-am --signoff --utf8 /g/tmp/mbox\n> \n> on the attached file, to apply a patch to libata-dev.git#upstream, and it\n> wound up only applying a portion of the patch:\n> \n> > [jgarzik@pretzel libata-dev]$ git-am --signoff --utf8 /g/tmp/mbox \n> > Applying 'Add pci class code for SATA AHCI'\n> > \n> > error: patch fragment without header at line 35: @@ -862,7 +862,7 @@\n> > error: patch fragment without header at line 50: @@ -15,6 +15,8 @@\n> > Wrote tree 5d6f3a93bea932c950ac880deca173dd3e84dfcc\n> > Committed: 317b180bad43133027dc07455f1600f4e8a47d76\n> \n> It seems quite bad to apply a patch that git-am KNOWS is incomplete.\n\nIt doesn't actually \"know\" it. What is going on is that the \"patch\" \nprogram generally allows random crap in between multiple patches, and \nwhile git-apply is normally stricter than \"patch\" ever is, git-apply does \nallow this \"garbage between patches\" thing too.\n\nSometimes the garbage is just commentary (some people say things in \nbetween patches), but quite often it's also stuff like \"Index: \" (and in \nfact, git considers the \"diff \" line at the head to be \"garbage\" too, \nunless it's a native git diff that has the \"diff --git\" signature).\n\nSo what git is actually doing is seeing that you have _garbage_ after the \nreal patch, but then it _warns_ about the garbage containing what looks \nlike a patch fragment. And the reason it's garbage (rather than a real \npatch) is obviously because your patch was so heavily whitespace-damaged \nthat the header that was supposed to start it wasn't a valid header at \nall..\n\nBut if you'd like to make it fatal, the patch is certainly simple..\n\nJunio - I certainly wouldn't mind making this a fatal error, but it's \nultimately your decision.\n\n\t\tLinus\n\n---\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex 1c35837..d3fdf7e 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -811,7 +811,7 @@ static int find_header(char *line, unsigned long size, int *hdrsize, struct patc\n \t\t\tstruct fragment dummy;\n \t\t\tif (parse_fragment_header(line, len, &dummy) < 0)\n \t\t\t\tcontinue;\n-\t\t\terror(\"patch fragment without header at line %d: %.*s\", linenr, (int)len-1, line);\n+\t\t\tdie(\"patch fragment without header at line %d: %.*s\", linenr, (int)len-1, line);\n \t\t}\n \n \t\tif (size < len + 6)\n"},{"id":"31253","messageId":"Pine.LNX.4.64.0701090803070.3661@woody.osdl.org","threadId":"6299","inReplyTo":"7vtzz0mpzk.fsf@assigned-by-dhcp.cox.net","subject":"Re: git-am applies half of a patch","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2007-01-09T16:08:53Z","receivedAt":"2007-01-09T16:08:53Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nOh well, I should read my whole mbox ..\n\nOn Tue, 9 Jan 2007, Junio C Hamano wrote:\n>\n> ---\n> diff --git a/builtin-apply.c b/builtin-apply.c\n> index 1c35837..918822b 100644\n> --- a/builtin-apply.c\n> +++ b/builtin-apply.c\n> @@ -811,7 +811,7 @@ static int find_header(char *line, unsigned long size, int *hdrsize, struct patc\n>  \t\t\tstruct fragment dummy;\n>  \t\t\tif (parse_fragment_header(line, len, &dummy) < 0)\n>  \t\t\t\tcontinue;\n> -\t\t\terror(\"patch fragment without header at line %d: %.*s\", linenr, (int)len-1, line);\n> +\t\t\treturn error(\"patch fragment without header at line %d: %.*s\", linenr, (int)len-1, line);\n\nI don't think this really does anything, Junio.\n\nYes, it will now return an error value from \"find_header()\", but that's \nstrictly normal: we _always_ get an error value from find_header() at the \nvery end of a patch (\"there's no more header\").\n\nAnd the caller (parse_chunk()) just ends up returning the error value, and \nthe caller of THAT just ends up breaking out of the loop that does the \npatching, ie it does\n\n\t..\n\twhile (size > 0) {\n\t\t..\n\t\tnr = parse_chunk(buffer + offset, size, patch);\n\t\tif (nr < 0)\n\t\t\tbreak;\n\t\t..\n\nso you really need to make the \"return error(..)\" thing be a \"die()\" \ninstead.\n\nAlternatively, you could just say that _any_ garbage at the end of a patch \nis fatal, and make apply_patch() just die on a negative error from \nparse_chunk(), but garbage at the end of a patch is strictly normal \n(things like signatures or just even empty lines), so that wouldn't really \nwork either.\n\nIn other words, I really don't think your patch does anything (except if \nthe garbage is at the BEGINNING of a patch), and that you should take mine \ninstead.\n\nBut test it.\n\n\t\tLinus\n"},{"id":"31264","messageId":"7vac0sm5m0.fsf@assigned-by-dhcp.cox.net","threadId":"6299","inReplyTo":"Pine.LNX.4.64.0701090803070.3661@woody.osdl.org","subject":"Re: git-am applies half of a patch","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-01-09T18:12:39Z","receivedAt":"2007-01-09T18:12:39Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@osdl.org> writes:\n\n> Oh well, I should read my whole mbox ..\n>\n> On Tue, 9 Jan 2007, Junio C Hamano wrote:\n>> ...\n> I don't think this really does anything, Junio.\n\nOoooooops.\n"}]}