{"thread":{"id":"4270","subject":"[PATCH] Add a test-case for git-apply trying to add an ending line","startedAt":"2006-05-23T21:48:36Z","lastAt":"2006-05-24T14:49:46Z","messageCount":9,"participants":["Catalin Marinas","Junio C Hamano","Linus Torvalds"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"20586","messageId":"20060523214836.22628.2179.stgit@localhost.localdomain","threadId":"4270","inReplyTo":null,"subject":"[PATCH] Add a test-case for git-apply trying to add an ending line","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2006-05-23T21:48:36Z","receivedAt":"2006-05-23T21:48:36Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"From: Catalin Marinas <catalin.marinas@gmail.com>\n\ngit-apply adding an ending line doesn't seem to fail if the ending line is\nalready present in the patched file.\n\nSigned-off-by: Catalin Marinas <catalin.marinas@gmail.com>\n---\n\n t/t4113-apply-ending.sh |   35 +++++++++++++++++++++++++++++++++++\n 1 files changed, 35 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh\nnew file mode 100755\nindex 0000000..d021ae8\n--- /dev/null\n+++ b/t/t4113-apply-ending.sh\n@@ -0,0 +1,35 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2006 Catalin Marinas\n+#\n+\n+test_description='git-apply trying to add an ending line.\n+\n+'\n+. ./test-lib.sh\n+\n+# setup\n+\n+cat >test-patch <<\\EOF\n+diff --git a/file b/file\n+--- a/file\n++++ b/file\n+@@ -1,2 +1,3 @@\n+ a\n+ b\n++c\n+EOF\n+\n+echo 'a' >file\n+echo 'b' >>file\n+echo 'c' >>file\n+\n+test_expect_success setup \\\n+    'git-update-index --add file'\n+\n+# test\n+\n+test_expect_failure apply \\\n+    'git-apply --index test-patch'\n+\n+test_done\n"},{"id":"20595","messageId":"7vd5e4z2je.fsf@assigned-by-dhcp.cox.net","threadId":"4270","inReplyTo":"20060523214836.22628.2179.stgit@localhost.localdomain","subject":"Re: [PATCH] Add a test-case for git-apply trying to add an ending line","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-05-24T00:31:33Z","receivedAt":"2006-05-24T00:31:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Hmmmmm.\n\nTo git-apply, this patch:\n\n        diff --git a/file b/file\n        --- a/file\n        +++ b/file\n        @@ -1,2 +1,3 @@\n         a\n         b\n        +c\n\ncurrently means \"I want to append a line c immediately after the\nlines that have a and then b\".  Nothing else.  And applying it\nto\n\n\ta\n        b\n        c\n\nproduces\n\n\ta\n        b\n        c\n        c\n\nThe first c is what your patch added, and the second c is what\nwas originally there.\n\nI do not think this is necessarily a bug.  You _could_ make the\nEOF a special case (i.e. you _could_ interpret the patch that it\nalso says, with \"@@ -1,2\", that \"the result of this patch _must_\nend with this line I just added\"), and if you are going to do\nthat, you would also need a symmetric special case for the\nbeginning of the file, but I do not think it is the right thing\nto do in general.\n\nRealistically, you would have something like this:\n\n        diff --git a/apply.c b/apply.c\n        index 0ed9d13..f99c6fe 100644\n        --- a/apply.c\n        +++ b/apply.c\n        @@ -2297,3 +2297,8 @@ int main(int argc, char **argv)\n\n                return 0; /* end of main */\n         }\n        +\n        +static void this_function_is_unused(void)\n        +{\n        +\tprintf(\"hello, world\\n\");\n        +}\n\nYou added a useless function at the end of the file.\n\nWhile you prepared the above patch, the upstream updated the\nsame file to end like this:\n\n                return 0; /* end of main */\n        }\n\n        static int some_new_function(void)\n        {\n                return 314;\n        }\n\nNow, git-apply would produce this file if you apply the above\npatch:\n\n                return 0; /* end of main */\n        }\n\n        static void this_function_is_unused(void)\n        {\n                printf(\"hello, world\\n\");\n        }\n\n        static int some_new_function(void)\n        {\n                return 314;\n        }\n\nI think this current behaviour is more desirable than special\ncasing the end of file and refusing to apply.\n\nIn this particular case, expecting failure like your new test\ndoes is somewhat understandable, but if you change the test case\nto start with this file, you would realize that your expectation\nis not the only valid understanding of what is really happening:\n\n\techo 'a' >file\n\techo 'b' >>file\n\techo 'd' >>file\n\nApplying test-patch to this would result in\n\n\ta\n        b\n        c\n        d\n\nwhich I think is more useful behaviour.\n"},{"id":"20596","messageId":"7vpsi4xm7q.fsf@assigned-by-dhcp.cox.net","threadId":"4270","inReplyTo":"7vd5e4z2je.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Add a test-case for git-apply trying to add an ending line","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-05-24T01:09:29Z","receivedAt":"2006-05-24T01:09:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <junkio@cox.net> writes:\n\n> To git-apply, this patch:\n>\n>         diff --git a/file b/file\n>         --- a/file\n>         +++ b/file\n>         @@ -1,2 +1,3 @@\n>          a\n>          b\n>         +c\n>\n> currently means \"I want to append a line c immediately after the\n> lines that have a and then b\".\n>...\n> I do not think this is necessarily a bug.  You _could_ make the\n> EOF a special case (i.e. you _could_ interpret the patch that it\n> also says, with \"@@ -1,2\", that \"the result of this patch _must_\n> end with this line I just added\"), and if you are going to do\n> that, you would also need a symmetric special case for the\n> beginning of the file, but I do not think it is the right thing\n> to do in general.\n\nCome to think of it, the above argument is bogus.  We _would_\nwant to make EOF just like any other context lines.\n\nThe issue is if we can reliably tell if there is such an EOF\ncontext by looking at the diff.  Not having the same number of\nlines that starts with ' ' in the hunk is not really a nice way\nof doing so (you could make a unified diff that does not have\ntrailing context at all), and I do not offhand think of a good\nway to do so.\n"},{"id":"20597","messageId":"7vhd3gxm73.fsf@assigned-by-dhcp.cox.net","threadId":"4270","inReplyTo":"7vd5e4z2je.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Add a test-case for git-apply trying to add an ending line","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-05-24T01:09:52Z","receivedAt":"2006-05-24T01:09:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <junkio@cox.net> writes:\n\n> To git-apply, this patch:\n>\n>         diff --git a/file b/file\n>         --- a/file\n>         +++ b/file\n>         @@ -1,2 +1,3 @@\n>          a\n>          b\n>         +c\n>\n> currently means \"I want to append a line c immediately after the\n> lines that have a and then b\".\n>...\n> I do not think this is necessarily a bug.  You _could_ make the\n> EOF a special case (i.e. you _could_ interpret the patch that it\n> also says, with \"@@ -1,2\", that \"the result of this patch _must_\n> end with this line I just added\"), and if you are going to do\n> that, you would also need a symmetric special case for the\n> beginning of the file, but I do not think it is the right thing\n> to do in general.\n\nCome to think of it, the above argument is bogus.  We _would_\nwant to make EOF just like any other context lines.\n\nThe issue is if we can reliably tell if there is such an EOF\ncontext by looking at the diff.  Not having the same number of\nlines that starts with ' ' in the hunk is not really a nice way\nof doing so (you could make a unified diff that does not have\ntrailing context at all), and I do not offhand think of a good\nway to do so.\n"},{"id":"20599","messageId":"Pine.LNX.4.64.0605231905470.5623@g5.osdl.org","threadId":"4270","inReplyTo":"7vhd3gxm73.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Add a test-case for git-apply trying to add an ending line","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-05-24T02:08:01Z","receivedAt":"2006-05-24T02:08:01Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 23 May 2006, Junio C Hamano wrote:\n> \n> Come to think of it, the above argument is bogus.  We _would_\n> want to make EOF just like any other context lines.\n> \n> The issue is if we can reliably tell if there is such an EOF\n> context by looking at the diff.  Not having the same number of\n> lines that starts with ' ' in the hunk is not really a nice way\n> of doing so (you could make a unified diff that does not have\n> trailing context at all), and I do not offhand think of a good\n> way to do so.\n\nWe can. Something like this should do it.\n\n(The same thing could be done for \"match_beginning\", perhaps).\n\nTotally untested, of course.\n\n(It might be better to pass in \"match_end\" to find_offset(), so that it \ncould do the \"look forwards\" pass to see if it finds a better line offset \nthat is at the end - as it is, this will _fail_ the patch if it could \napply better at a non-end thing, even if it would _also_ have applied at \nthe end of the file).\n\n\t\tLinus\n---\ndiff --git a/apply.c b/apply.c\nindex 0ed9d13..905bf34 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -1333,6 +1333,7 @@ static int apply_line(char *output, cons\n \n static int apply_one_fragment(struct buffer_desc *desc, struct fragment *frag)\n {\n+\tint match_end;\n \tchar *buf = desc->buffer;\n \tconst char *patch = frag->patch;\n \tint offset, size = frag->size;\n@@ -1395,10 +1396,20 @@ #endif\n \tnewlines = new;\n \tleading = frag->leading;\n \ttrailing = frag->trailing;\n+\n+\t/*\n+\t * If we don't have any trailing data in the patch,\n+\t * we want to match the final ending '\\0' byte in\n+\t * the file too..\n+\t */\n+\tmatch_end = !trailing;\n+\n \tlines = 0;\n \tpos = frag->newpos;\n \tfor (;;) {\n \t\toffset = find_offset(buf, desc->size, oldlines, oldsize, pos, &lines);\n+\t\tif (match_end && offset + oldsize != desc->size)\n+\t\t\toffset = -1;\n \t\tif (offset >= 0) {\n \t\t\tint diff = newsize - oldsize;\n \t\t\tunsigned long size = desc->size + diff;\n@@ -1428,6 +1439,10 @@ #endif\n \t\t/* Am I at my context limits? */\n \t\tif ((leading <= p_context) && (trailing <= p_context))\n \t\t\tbreak;\n+\t\tif (match_end) {\n+\t\t\tmatch_end = 0;\n+\t\t\tcontinue;\n+\t\t}\n \t\t/* Reduce the number of context lines\n \t\t * Reduce both leading and trailing if they are equal\n \t\t * otherwise just reduce the larger context.\n"},{"id":"20601","messageId":"Pine.LNX.4.64.0605231916210.5623@g5.osdl.org","threadId":"4270","inReplyTo":"Pine.LNX.4.64.0605231905470.5623@g5.osdl.org","subject":"Re: [PATCH] Add a test-case for git-apply trying to add an ending line","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-05-24T02:17:44Z","receivedAt":"2006-05-24T02:17:44Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 23 May 2006, Linus Torvalds wrote:\n>\n> +\t/*\n> +\t * If we don't have any trailing data in the patch,\n> +\t * we want to match the final ending '\\0' byte in\n> +\t * the file too..\n> +\t */\n\nBtw, ignore the comment. I was thinking of doing the matching differently \n(just make the source buffer include a '\\0' at the end, and forcing that \nto match), but once I actually wrote it, it ended up being much easier to \njust check the offset/size difference.\n\nSo that \"final ending '\\0' byte in the file\" part of the comment is just \nnonsense.\n\n\t\tLinus\n"},{"id":"20607","messageId":"7v8xosqaqm.fsf@assigned-by-dhcp.cox.net","threadId":"4270","inReplyTo":"Pine.LNX.4.64.0605231905470.5623@g5.osdl.org","subject":"Re: [PATCH] Add a test-case for git-apply trying to add an ending line","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-05-24T04:59:13Z","receivedAt":"2006-05-24T04:59:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@osdl.org> writes:\n\n> On Tue, 23 May 2006, Junio C Hamano wrote:\n>\n>> The issue is if we can reliably tell if there is such an EOF\n>> context by looking at the diff.  Not having the same number of\n>> lines that starts with ' ' in the hunk is not really a nice way\n>> of doing so (you could make a unified diff that does not have\n>> trailing context at all), and I do not offhand think of a good\n>> way to do so.\n>\n> We can. Something like this should do it.\n>\n> (The same thing could be done for \"match_beginning\", perhaps).\n\nBut this is exactly what I said I had trouble with in the above.\n\nIn the extreme case, wouldn't this make git apply to refuse to\napply a self generated patch with 0-line context?  IOW,\n\n        $ git checkout -- foo ;# reset to indexed version\n\t$ edit foo\n        $ git diff -U0 >P.diff\n        $ git checkout -- foo ;# reset to indexed version\n        $ git apply <P.diff\n\nwould fail, even though it _should_ cleanly apply.\n\nI'd admit that trying to apply a patch without context like the\nabove example _is_ insane, and I realize that I am making this\nproblem unsolvable by refusing the heuristics to consider that\nthe patch is anchored at the end when we do not see any trailing\ncontext.  But somehow it feels wrong...\n"},{"id":"20627","messageId":"b0943d9e0605240632t4189f8d0yfdbab111c83f0426@mail.gmail.com","threadId":"4270","inReplyTo":"7v8xosqaqm.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Add a test-case for git-apply trying to add an ending line","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2006-05-24T13:32:01Z","receivedAt":"2006-05-24T13:32:01Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"On 24/05/06, Junio C Hamano <junkio@cox.net> wrote:\n> I'd admit that trying to apply a patch without context like the\n> above example _is_ insane, and I realize that I am making this\n> problem unsolvable by refusing the heuristics to consider that\n> the patch is anchored at the end when we do not see any trailing\n> context.  But somehow it feels wrong...\n\nThe reason I sent you this test is that GNU patch fails to apply the\ndiff but git-apply succeeds (and I thought git-apply is more\nrestrictive).\n\nWhen there are context lines either before or after the \"+\" line, it\nshould be OK to assume that the diff has context and therefore the EOF\nshould be considered.\n\nIf there are no context lines at all, the diff is either without\ncontext or it is meant to patch an empty file. The latter is safer and\nprobably valid for most of the cases but if you have a patch without\ncontext, you could explicitely pass the -C0 option to git-apply.\n\n-- \nCatalin\n"},{"id":"20632","messageId":"Pine.LNX.4.64.0605240745590.5623@g5.osdl.org","threadId":"4270","inReplyTo":"7v8xosqaqm.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Add a test-case for git-apply trying to add an ending line","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-05-24T14:49:46Z","receivedAt":"2006-05-24T14:49:46Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 23 May 2006, Junio C Hamano wrote:\n\n> Linus Torvalds <torvalds@osdl.org> writes:\n> \n> > On Tue, 23 May 2006, Junio C Hamano wrote:\n> >\n> >> The issue is if we can reliably tell if there is such an EOF\n> >> context by looking at the diff.  Not having the same number of\n> >> lines that starts with ' ' in the hunk is not really a nice way\n> >> of doing so (you could make a unified diff that does not have\n> >> trailing context at all), and I do not offhand think of a good\n> >> way to do so.\n> >\n> > We can. Something like this should do it.\n> >\n> > (The same thing could be done for \"match_beginning\", perhaps).\n> \n> But this is exactly what I said I had trouble with in the above.\n\nWell, not quite. You said \"not the same number of lines\", and I say \"no \nending context\". Very different.\n\nMy patch actually is totally self-consistent: not having any context at \nthe end of a unified diff really means that it is the end of the file (ie, \nthe \"end of file\" there _is_ the context). And if you want to apply files \nwithout context, you should use \"-Cx\", and my patch does that too - if you \nasked for \"relaxed context checking\", it will re-try without the \"only at \nend\" check thanks to the\n\n\tif (match_end) {\n\t\tmatch_end = 0;\n\t\tcontinue;\n\t}\n\nso it all should work.\n\nNot that I _tested_ it, of course ;)\n\n\t\tLinus\n"}]}