{"thread":{"id":"14323","subject":"[PATCH] builtin-rerere: fix conflict markers parsing","startedAt":"2008-07-07T12:42:48Z","lastAt":"2008-07-08T10:42:05Z","messageCount":9,"participants":["Olivier Marin","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"82440","messageId":"1215434568-30456-1-git-send-email-dkr+ml.git@free.fr","threadId":"14323","inReplyTo":null,"subject":"[PATCH] builtin-rerere: fix conflict markers parsing","fromName":"Olivier Marin","fromEmail":"dkr+ml.git@free.fr","sentAt":"2008-07-07T12:42:48Z","receivedAt":"2008-07-07T12:42:48Z","isPatch":true,"sender":{"key":"dkr+ml.git@free.fr","avatar":null},"body":"From: Olivier Marin <dkr@freesurf.fr>\n\nWhen a conflicting file contains a line that begin with \"=======\", rerere\nfailed to parse conflict markers. This result to a wrong preimage file and\nan unexpected error for the user.\n\nThis patch enforce parsing rules so that markers match in the right order\nand update tests to match the above fix.\n\nSigned-off-by: Olivier Marin <dkr@freesurf.fr>\n---\n\n This happend to me with a conflict in Documentation/git-remote.txt.\n\n The bug seems to have always been there but nobody noticed probably because\n before a1b32fdc3d1d05395f186bfa06e92174519dab8d parsing errors were ignored\n and (I think) it does not affect the way git rerere works.\n\n builtin-rerere.c  |    7 ++++---\n t/t4200-rerere.sh |   26 ++++++++++++++++++++------\n 2 files changed, 24 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin-rerere.c b/builtin-rerere.c\nindex 839b26e..e618862 100644\n--- a/builtin-rerere.c\n+++ b/builtin-rerere.c\n@@ -112,11 +112,12 @@ static int handle_file(const char *path,\n \tstrbuf_init(&one, 0);\n \tstrbuf_init(&two,  0);\n \twhile (fgets(buf, sizeof(buf), f)) {\n-\t\tif (!prefixcmp(buf, \"<<<<<<< \"))\n+\t\tif (hunk == 0 && !prefixcmp(buf, \"<<<<<<< \"))\n \t\t\thunk = 1;\n-\t\telse if (!prefixcmp(buf, \"=======\"))\n+\t\telse if (hunk == 1 && !prefixcmp(buf, \"=======\") &&\n+\t\t\t isspace(buf[7]))\n \t\t\thunk = 2;\n-\t\telse if (!prefixcmp(buf, \">>>>>>> \")) {\n+\t\telse if (hunk == 2 && !prefixcmp(buf, \">>>>>>> \")) {\n \t\t\tif (strbuf_cmp(&one, &two) > 0)\n \t\t\t\tstrbuf_swap(&one, &two);\n \t\t\thunk_no++;\ndiff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh\nindex a64727d..cf10557 100755\n--- a/t/t4200-rerere.sh\n+++ b/t/t4200-rerere.sh\n@@ -9,6 +9,8 @@ test_description='git rerere\n . ./test-lib.sh\n \n cat > a1 << EOF\n+Some title\n+==========\n Whether 'tis nobler in the mind to suffer\n The slings and arrows of outrageous fortune,\n Or to take arms against a sea of troubles,\n@@ -24,6 +26,8 @@ git commit -q -a -m initial\n \n git checkout -b first\n cat >> a1 << EOF\n+Some title\n+==========\n To die, to sleep;\n To sleep: perchance to dream: ay, there's the rub;\n For in that sleep of death what dreams may come\n@@ -35,7 +39,7 @@ git commit -q -a -m first\n \n git checkout -b second master\n git show first:a1 |\n-sed -e 's/To die, t/To die! T/' > a1\n+sed -e 's/To die, t/To die! T/' -e 's/Some title/Some Title/' > a1\n echo \"* END *\" >>a1\n git commit -q -a -m second\n \n@@ -55,14 +59,14 @@ test_expect_success 'conflicting merge' '\n \n sha1=$(sed -e 's/\t.*//' .git/rr-cache/MERGE_RR)\n rr=.git/rr-cache/$sha1\n-test_expect_success 'recorded preimage' \"grep ======= $rr/preimage\"\n+test_expect_success 'recorded preimage' \"grep ^=======$ $rr/preimage\"\n \n test_expect_success 'rerere.enabled works, too' '\n \trm -rf .git/rr-cache &&\n \tgit config rerere.enabled true &&\n \tgit reset --hard &&\n \t! git merge first &&\n-\tgrep ======= $rr/preimage\n+\tgrep ^=======$ $rr/preimage\n '\n \n test_expect_success 'no postimage or thisimage yet' \\\n@@ -71,7 +75,7 @@ test_expect_success 'no postimage or thisimage yet' \\\n test_expect_success 'preimage has right number of lines' '\n \n \tcnt=$(sed -ne \"/^<<<<<<</,/^>>>>>>>/p\" $rr/preimage | wc -l) &&\n-\ttest $cnt = 9\n+\ttest $cnt = 13\n \n '\n \n@@ -80,13 +84,23 @@ git show first:a1 > a1\n cat > expect << EOF\n --- a/a1\n +++ b/a1\n-@@ -6,17 +6,9 @@\n+@@ -1,4 +1,4 @@\n+-Some Title\n++Some title\n+ ==========\n+ Whether 'tis nobler in the mind to suffer\n+ The slings and arrows of outrageous fortune,\n+@@ -8,21 +8,11 @@\n  The heart-ache and the thousand natural shocks\n  That flesh is heir to, 'tis a consummation\n  Devoutly to be wish'd.\n -<<<<<<<\n+-Some Title\n+-==========\n -To die! To sleep;\n -=======\n+ Some title\n+ ==========\n  To die, to sleep;\n ->>>>>>>\n  To sleep: perchance to dream: ay, there's the rub;\n@@ -124,7 +138,7 @@ test_expect_success 'another conflicting merge' '\n '\n \n git show first:a1 | sed 's/To die: t/To die! T/' > expect\n-test_expect_success 'rerere kicked in' \"! grep ======= a1\"\n+test_expect_success 'rerere kicked in' \"! grep ^=======$ a1\"\n \n test_expect_success 'rerere prefers first change' 'test_cmp a1 expect'\n \n-- \n1.5.6.2.346.gddd7f\n"},{"id":"82442","messageId":"alpine.DEB.1.00.0807071400180.18205@racer","threadId":"14323","inReplyTo":"1215434568-30456-1-git-send-email-dkr+ml.git@free.fr","subject":"Re: [PATCH] builtin-rerere: fix conflict markers parsing","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-07T13:02:20Z","receivedAt":"2008-07-07T13:02:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 7 Jul 2008, Olivier Marin wrote:\n\n> From: Olivier Marin <dkr@freesurf.fr>\n> \n> When a conflicting file contains a line that begin with \"=======\", rerere\n> failed to parse conflict markers. This result to a wrong preimage file and\n> an unexpected error for the user.\n> \n> This patch enforce parsing rules so that markers match in the right order\n> and update tests to match the above fix.\n\nSo what about\n\n\t<<<<<<< This hunk contains =====\n\tanythin\n\t=======\n\n\tHello\n\t=======\n\tsomethin else\n\t>>>>>>> problem!\n\n\nIf you fix it, I think you should do it properly, and analyze the index.\n\nCiao,\nDscho\n"},{"id":"82446","messageId":"48722038.1010203@free.fr","threadId":"14323","inReplyTo":"alpine.DEB.1.00.0807071400180.18205@racer","subject":"Re: [PATCH] builtin-rerere: fix conflict markers parsing","fromName":"Olivier Marin","fromEmail":"dkr+ml.git@free.fr","sentAt":"2008-07-07T13:55:04Z","receivedAt":"2008-07-07T13:55:04Z","isPatch":true,"sender":{"key":"dkr+ml.git@free.fr","avatar":null},"body":"Johannes Schindelin a écrit :\n> \n> So what about\n> \n> \t<<<<<<< This hunk contains =====\n> \tanythin\n> \t=======\n> \n> \tHello\n> \t=======\n> \tsomethin else\n> \t>>>>>>> problem!\n> \n> \n> If you fix it, I think you should do it properly, and analyze the index.\n\nIf I read the code correctly, this case is not a problem at all because what\nis important is the content between <<< and >>> : even if you match the wrong\n=== marker, you will match the first one only, then parsing will success and\npreimage file will be OK. Also because we always match in the same order the\nsha1 will be the same.\n\nAnyway, I do not know how to match the right === marker.\n\nOlivier.\n"},{"id":"82447","messageId":"alpine.DEB.1.00.0807071505470.18205@racer","threadId":"14323","inReplyTo":"48722038.1010203@free.fr","subject":"Re: [PATCH] builtin-rerere: fix conflict markers parsing","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-07T14:06:31Z","receivedAt":"2008-07-07T14:06:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 7 Jul 2008, Olivier Marin wrote:\n\n> Johannes Schindelin a écrit :\n> > \n> > So what about\n> > \n> > \t<<<<<<< This hunk contains =====\n> > \tanythin\n> > \t=======\n> > \n> > \tHello\n> > \t=======\n> > \tsomethin else\n> > \t>>>>>>> problem!\n> > \n> > \n> > If you fix it, I think you should do it properly, and analyze the index.\n> \n> If I read the code correctly, this case is not a problem at all because what\n> is important is the content between <<< and >>> : even if you match the wrong\n> === marker, you will match the first one only, then parsing will success and\n> preimage file will be OK. Also because we always match in the same order the\n> sha1 will be the same.\n> \n> Anyway, I do not know how to match the right === marker.\n\nOkay, but then the obvious question is: what do you do about \"<<<<<<\" \nlines that are not a marker?\n\nSame remark as before: if you fix rerere, why not do it properly?\n\nCiao,\nDscho\n"},{"id":"82456","messageId":"48722BD9.4090406@free.fr","threadId":"14323","inReplyTo":"alpine.DEB.1.00.0807071505470.18205@racer","subject":"Re: [PATCH] builtin-rerere: fix conflict markers parsing","fromName":"Olivier Marin","fromEmail":"dkr+ml.git@free.fr","sentAt":"2008-07-07T14:44:41Z","receivedAt":"2008-07-07T14:44:41Z","isPatch":true,"sender":{"key":"dkr+ml.git@free.fr","avatar":null},"body":"Johannes Schindelin a écrit :\n> \n> Okay, but then the obvious question is: what do you do about \"<<<<<<\" \n> lines that are not a marker?\n\nThe answer is the same.\n\n> Same remark as before: if you fix rerere, why not do it properly?\n\nIf you have a better fix send a patch or at least give me some clues.\n\nOlivier.\n"},{"id":"82464","messageId":"alpine.DEB.1.00.0807071627020.18205@racer","threadId":"14323","inReplyTo":"48722BD9.4090406@free.fr","subject":"Re: [PATCH] builtin-rerere: fix conflict markers parsing","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-07T15:29:35Z","receivedAt":"2008-07-07T15:29:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 7 Jul 2008, Olivier Marin wrote:\n\n> Johannes Schindelin a écrit :\n> > \n> > Okay, but then the obvious question is: what do you do about \"<<<<<<\" \n> > lines that are not a marker?\n> \n> The answer is the same.\n\nI think not.  You say you want to do something about the ambiguity, but \nfact is: the conflict markers are ambiguous.  They always have been, will \never be, and I do not even have to argue for it.  Or do I?\n\n> > Same remark as before: if you fix rerere, why not do it properly?\n> \n> If you have a better fix send a patch or at least give me some clues.\n\nDid I not say that the index has enough information?  Or at least hint at \nit?\n\nOf course, we could run with your solution.  But that only fixes a corner \ncase, right?\n\nSo a proper fix will be needed eventually anyway.\n\nAnd no, I will not work on it: this is not my itch, and my time is being \nstolen too much already, these days.\n\nCiao,\nDscho\n"},{"id":"82474","messageId":"7vy74d4mr8.fsf@gitster.siamese.dyndns.org","threadId":"14323","inReplyTo":"alpine.DEB.1.00.0807071400180.18205@racer","subject":"Re: [PATCH] builtin-rerere: fix conflict markers parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-07T17:39:23Z","receivedAt":"2008-07-07T17:39:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> So what about\n>\n> \t<<<<<<< This hunk contains =====\n> \tanythin\n> \t=======\n>\n> \tHello\n> \t=======\n> \tsomethin else\n> \t>>>>>>> problem!\n>\n>\n> If you fix it, I think you should do it properly, and analyze the index.\n\nI do not know offhand if analyzing the index is the right solution, but\nyour point is very valid.  You need to know which ====== is the real one\nto be able to properly flip sides of the conflict.\n\nI however think detecting that we have this ambiguous hunk is easy, and\npunting gracefully and not re-resolving in such a case is million times\nbetter than producing random results that the users need to be worried\nabout.\n"},{"id":"82567","messageId":"7vwsjwvmlk.fsf_-_@gitster.siamese.dyndns.org","threadId":"14323","inReplyTo":"7vy74d4mr8.fsf@gitster.siamese.dyndns.org","subject":"Re* [PATCH] builtin-rerere: fix conflict markers parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-08T07:52:55Z","receivedAt":"2008-07-08T07:52:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n>\n>> So what about\n>>\n>> \t<<<<<<< This hunk contains =====\n>> \tanythin\n>> \t=======\n>>\n>> \tHello\n>> \t=======\n>> \tsomethin else\n>> \t>>>>>>> problem!\n>> ...\n> I however think detecting that we have this ambiguous hunk is easy, and\n> punting gracefully and not re-resolving in such a case is million times\n> better than producing random results that the users need to be worried\n> about.\n\nI am wondering if a patch like this on top of your patch may make things\neven safer.  The idea is the same as the earlier a1b32fd (git-rerere:\ndetect unparsable conflicts, 2008-06-22) to fail rerere unless the markers\nare unambiguous.\n\nThanks to your isspace(buf[7]), it is slightly less likely that this\nsafety triggers on false positives.\n\nThoughts?\n\n-- >8 --\nrerere: punt and do not resolve if conflict markers are ambiguous\n\nEspecially because we are introducing rerere.autoupdate configuration\n(which is off by default for safety) that automatically stages the\nresolution made by rerere, it is necessary to make sure that we do not\nautoresolve when there is any ambiguity.\n\n builtin-rerere.c |   17 +++++++++++++----\n 1 files changed, 13 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-rerere.c b/builtin-rerere.c\nindex e618862..69c3a52 100644\n--- a/builtin-rerere.c\n+++ b/builtin-rerere.c\n@@ -112,12 +112,17 @@ static int handle_file(const char *path,\n \tstrbuf_init(&one, 0);\n \tstrbuf_init(&two,  0);\n \twhile (fgets(buf, sizeof(buf), f)) {\n-\t\tif (hunk == 0 && !prefixcmp(buf, \"<<<<<<< \"))\n+\t\tif (!prefixcmp(buf, \"<<<<<<< \")) {\n+\t\t\tif (hunk)\n+\t\t\t\tgoto bad;\n \t\t\thunk = 1;\n-\t\telse if (hunk == 1 && !prefixcmp(buf, \"=======\") &&\n-\t\t\t isspace(buf[7]))\n+\t\t} else if (!prefixcmp(buf, \"=======\") && isspace(buf[7])) {\n+\t\t\tif (hunk != 1)\n+\t\t\t\tgoto bad;\n \t\t\thunk = 2;\n-\t\telse if (hunk == 2 && !prefixcmp(buf, \">>>>>>> \")) {\n+\t\t} else if (!prefixcmp(buf, \">>>>>>> \")) {\n+\t\t\tif (hunk != 2)\n+\t\t\t\tgoto bad;\n \t\t\tif (strbuf_cmp(&one, &two) > 0)\n \t\t\t\tstrbuf_swap(&one, &two);\n \t\t\thunk_no++;\n@@ -143,6 +148,10 @@ static int handle_file(const char *path,\n \t\t\tstrbuf_addstr(&two, buf);\n \t\telse if (out)\n \t\t\tfputs(buf, out);\n+\t\tcontinue;\n+\tbad:\n+\t\thunk = 99; /* force error exit */\n+\t\tbreak;\n \t}\n \tstrbuf_release(&one);\n \tstrbuf_release(&two);\n"},{"id":"82582","messageId":"4873447D.5090208@free.fr","threadId":"14323","inReplyTo":"7vwsjwvmlk.fsf_-_@gitster.siamese.dyndns.org","subject":"Re: Re* [PATCH] builtin-rerere: fix conflict markers parsing","fromName":"Olivier Marin","fromEmail":"dkr+ml.git@free.fr","sentAt":"2008-07-08T10:42:05Z","receivedAt":"2008-07-08T10:42:05Z","isPatch":true,"sender":{"key":"dkr+ml.git@free.fr","avatar":null},"body":"Junio C Hamano a écrit :\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> I am wondering if a patch like this on top of your patch may make things\n> even safer.  The idea is the same as the earlier a1b32fd (git-rerere:\n> detect unparsable conflicts, 2008-06-22) to fail rerere unless the markers\n> are unambiguous.\n> \n> Thanks to your isspace(buf[7]), it is slightly less likely that this\n> safety triggers on false positives.\n> \n> Thoughts?\n\nMy main concern was the error message that most users will not understand\nafter a \"git rebase --continue\", for example. So, I tried to remove it and\nlet things work as before because rerere seems to work even with ambiguous\ncases.\n\nBut I think your patch is the right thing to do: safe is better.\n\nOlivier.\n"}]}