{"thread":{"id":"13010","subject":"bug in git-apply","startedAt":"2008-04-06T23:53:34Z","lastAt":"2008-04-07T07:12:46Z","messageCount":4,"participants":["Karl Hasselström","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"73790","messageId":"20080406235334.GA8483@diana.vm.bytemark.co.uk","threadId":"13010","inReplyTo":null,"subject":"bug in git-apply","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-04-06T23:53:34Z","receivedAt":"2008-04-06T23:53:34Z","isPatch":false,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"StGit's test suite has started failing, and it's due to a change in\ngit that I'm pretty sure is a bug. I built a small test case using\njust git for easier testing:\n\n mkdir foo\n cd foo\n\n git init\n touch xx\n git add xx\n git commit -m 'first'\n\n git checkout -b branch\n\n echo aaa >> xx\n git add xx\n git commit -m 'second'\n\n git checkout master\n\n echo bbb >> xx\n git add xx\n git commit -m 'third'\n\n git diff branch^ branch > p\n git apply < p && echo \"this should not succeed!\"\n\nBasically, we have a file containing \"aaa\\n\", and a patch that tries\nto take the file from \"\" to \"bbb\\n\". A recent enough git-apply will\naccept this patch without complaining, and put \"aaa\\nbbb\\n\" in the\nfile.\n\nI bisected, and the culprit was this commit:\n\ncommit dc41976a3eed1d9c93fdc08c448bab969db4e0ec\nAuthor: Junio C Hamano \nDate: Sat Jan 19 01:58:34 2008 -0800\n\n builtin-apply.c: push match-beginning/end logic down\n\n This moves the logic to force match at the beginning and/or at\n the end of the buffer to the actual function that finds the\n match from its caller. This is a necessary preparation for the\n next step to allow matching disregarding certain differences,\n such as whitespace changes.\n\n We probably could optimize this even more by taking advantage of\n the fact that match_beginning and match_end forces the match to\n be at an exact location (anchored at the beginning and/or the\n end), but that's for another commit.\n\n Signed-off-by: Junio C Hamano \n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"},{"id":"73794","messageId":"7vej9isc3y.fsf@gitster.siamese.dyndns.org","threadId":"13010","inReplyTo":"20080406235334.GA8483@diana.vm.bytemark.co.uk","subject":"Re: bug in git-apply","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-04-07T01:11:13Z","receivedAt":"2008-04-07T01:11:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karl Hasselström writes:\n\n> I bisected, and the culprit was this commit:\n>\n> commit dc41976a3eed1d9c93fdc08c448bab969db4e0ec\n\nI think the issue is much older than that. The faulty one is 4be6096\n(apply --unidiff-zero: loosen sanity checks for --unidiff=0 patches,\n2006-09-17), which incorrectly loosened the match_beginning/match_end\ncomputation too much, whose botch may have been hidden by what the\nsurrounding code did.\n\nAnd 65aadb9 (apply: force matching at the beginning., 2006-05-24) is\nequally wrong in that it tried to take hints from leading context lines,\nto decide if the hunk must match at the beginning, but in this case we\ncan just look at the line number in the hunk to decide.\n\nHow about doing this?\n\n\n builtin-apply.c | 29 ++++++++++++++++-------------\n t/t4104-apply-boundary.sh | 13 +++++++++++++\n 2 files changed, 29 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex b5f78ac..abe73a0 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -1937,21 +1937,24 @@ static int apply_one_fragment(struct image *img, struct fragment *frag,\n \ttrailing = frag->trailing;\n \n \t/*\n-\t * If we don't have any leading/trailing data in the patch,\n-\t * we want it to match at the beginning/end of the file.\n+\t * A hunk to change lines at the beginning would begin with\n+\t * @@ -1,L +N,M @@\n \t *\n-\t * But that would break if the patch is generated with\n-\t * --unified=0; sane people wouldn't do that to cause us\n-\t * trouble, but we try to please not so sane ones as well.\n+\t * And a hunk to add to an empty file would begin with\n+\t * @@ -0,0 +N,M @@\n+\t *\n+\t * In other words, a hunk that is (frag->oldpos <= 1) with or\n+\t * without leading context must match at the beginning.\n \t */\n-\tif (unidiff_zero) {\n-\t\tmatch_beginning = (!leading && !frag->oldpos);\n-\t\tmatch_end = 0;\n-\t}\n-\telse {\n-\t\tmatch_beginning = !leading && (frag->oldpos == 1);\n-\t\tmatch_end = !trailing;\n-\t}\n+\tmatch_beginning = frag->oldpos <= 1;\n+\n+\t/*\n+\t * A hunk without trailing lines must match at the end.\n+\t * However, we simply cannot tell if a hunk must match end\n+\t * from the lack of trailing lines if the patch was generated\n+\t * with unidiff without any context.\n+\t */\n+\tmatch_end = !unidiff_zero && !trailing;\n \n \tpos = frag->newpos ? (frag->newpos - 1) : 0;\n \tpreimage.buf = oldlines;\ndiff --git a/t/t4104-apply-boundary.sh b/t/t4104-apply-boundary.sh\nindex 64f34e3..0007535 100755\n--- a/t/t4104-apply-boundary.sh\n+++ b/t/t4104-apply-boundary.sh\n@@ -112,4 +112,17 @@ do\n \t'\n done\n \n+test_expect_success 'two lines' '\n+\n+\t>file &&\n+\tgit add file &&\n+\techo aaa >file &&\n+\tgit diff >patch &&\n+\tgit add file &&\n+\techo bbb >file &&\n+\tgit add file &&\n+\ttest_must_fail git apply patch\n+\n+'\n+\n test_done\n"},{"id":"73798","messageId":"20080407070449.GA13171@diana.vm.bytemark.co.uk","threadId":"13010","inReplyTo":"7vej9isc3y.fsf@gitster.siamese.dyndns.org","subject":"Re: bug in git-apply","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-04-07T07:04:49Z","receivedAt":"2008-04-07T07:04:49Z","isPatch":false,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2008-04-06 18:11:13 -0700, Junio C Hamano wrote:\n\n> How about doing this?\n\nWell, it does fix the stgit test suite failure, so I'm all for it!\n(Though I see you've already pushed it to master.) Thanks.\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"},{"id":"73800","messageId":"7vzls6qgsx.fsf@gitster.siamese.dyndns.org","threadId":"13010","inReplyTo":"20080407070449.GA13171@diana.vm.bytemark.co.uk","subject":"Re: bug in git-apply","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-04-07T07:12:46Z","receivedAt":"2008-04-07T07:12:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karl Hasselström writes:\n\n> On 2008-04-06 18:11:13 -0700, Junio C Hamano wrote:\n>\n>> How about doing this?\n>\n> Well, it does fix the stgit test suite failure, so I'm all for it!\n> (Though I see you've already pushed it to master.) Thanks.\n\nThis actually is a fix for maint; even though your test case does not\nlet you observe the breakage directly, only because must-match-at-end\ncheck prevents the single-liner test hunk from applying cleanly.\n"}]}