{"thread":{"id":"30958","subject":"[RFC/PATCH] stash: Delete MERGE_RR before stash apply","startedAt":"2012-07-05T13:48:22Z","lastAt":"2012-08-17T18:02:41Z","messageCount":13,"participants":["Phil Hord","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"194653","messageId":"CABURp0pNsRQgbf7_iYc-xVaySa9-gGiA++Lw4-WgSCQ4QGCXsA@mail.gmail.com","threadId":"30958","inReplyTo":null,"subject":"[RFC/PATCH] stash: Delete MERGE_RR before stash apply","fromName":"Phil Hord","fromEmail":"phil.hord@gmail.com","sentAt":"2012-07-05T13:48:22Z","receivedAt":"2012-07-05T13:48:22Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"The presence of a GIT_DIR/MERGE_RR file indicates we\nwere resolving a merge which had rerere candidates for\nrecording.  But the file does not get deleted after\nall resolutions are completed.  This is ok for most\ncases because the file will get replaced when the\nnext merge happens.  But stash apply does not use\na merge that supports rerere, and so the old\nMERGE_RR does not get replaced with a current one.\n\nThis then confuses mergetool who thinks a rerere\noperation is in play when it is not.\n\nFix this by ensuring there is no leftover\nMERGE_RR file whenever we are beginning a git stash\napply.\n\nSigned-off-by: Phil Hord <hordp@cisco.com>\n---\n\nThis feels like a hack and like it is in the wrong location.\nHowever, the change is minimal and should fix my\nspecific problem[1].\n\n[1] http://comments.gmane.org/gmane.comp.version-control.git/200178\n\n git-stash.sh | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex 4e2c7f8..0b96e2f 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -409,6 +409,9 @@ apply_stash () {\n\n        assert_stash_like \"$@\"\n\n+       test -f \"$GIT_DIR/MERGE_RR\" &&\n+               git rerere clear\n+\n        git update-index -q --refresh || die \"$(gettext \"unable to\nrefresh index\")\"\n\n        # current index state\n-- \n1.7.11.1.161.g0f17059.dirty\n"},{"id":"194660","messageId":"7v7guiruxo.fsf@alter.siamese.dyndns.org","threadId":"30958","inReplyTo":"CABURp0pNsRQgbf7_iYc-xVaySa9-gGiA++Lw4-WgSCQ4QGCXsA@mail.gmail.com","subject":"Re: [RFC/PATCH] stash: Delete MERGE_RR before stash apply","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-05T17:15:15Z","receivedAt":"2012-07-05T17:15:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phil Hord <phil.hord@gmail.com> writes:\n\n> The presence of a GIT_DIR/MERGE_RR file indicates we\n> were resolving a merge which had rerere candidates for\n> recording.  But the file does not get deleted after\n> all resolutions are completed.  This is ok for most\n> cases because the file will get replaced when the\n> next merge happens.  But stash apply does not use\n> a merge that supports rerere, and so the old\n> MERGE_RR does not get replaced with a current one.\n\nThanks for digging to the real cause.  It does use merge-recursive\nbackend directly, and as a backend, it probably is correct that it\ndoes not invoke rerere itself.\n\nIn your patch, you are removing the state before you check and\nnotice that the user is in the middle of a merge and give control\nback with \"Cannot apply a stash in the middle of a merge\".  Wouldn't\nit be nicer to the user if you didn't remove the rerere state when\nthis happens (i.e. the user mistakenly said \"stash apply\" after a\nconflicted merge), as that rerere state likely is from that merge\nthat produced the conflicted state?\n\nWould an obvious alternative of running \"git rerere\" ourselves after\nrunning \"git merge-recursive\" in this script work?\n\n git-stash.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex 4e2c7f8..bbefdf6 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -469,6 +469,7 @@ apply_stash () {\n \telse\n \t\t# Merge conflict; keep the exit status from merge-recursive\n \t\tstatus=$?\n+\t\tgit rerere\n \t\tif test -n \"$INDEX_OPTION\"\n \t\tthen\n \t\t\tgettextln \"Index was not unstashed.\" >&2\n"},{"id":"194724","messageId":"CABURp0rOfr=c-TcF7O6Ms=2iRA3SC9ckt_ukeozBS5vGqaKZaQ@mail.gmail.com","threadId":"30958","inReplyTo":"7v7guiruxo.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH] stash: Delete MERGE_RR before stash apply","fromName":"Phil Hord","fromEmail":"phil.hord@gmail.com","sentAt":"2012-07-06T15:53:23Z","receivedAt":"2012-07-06T15:53:23Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"On Thu, Jul 5, 2012 at 1:15 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Phil Hord <phil.hord@gmail.com> writes:\n>\n>> The presence of a GIT_DIR/MERGE_RR file indicates we\n>> were resolving a merge which had rerere candidates for\n>> recording.  But the file does not get deleted after\n>> all resolutions are completed.  This is ok for most\n>> cases because the file will get replaced when the\n>> next merge happens.  But stash apply does not use\n>> a merge that supports rerere, and so the old\n>> MERGE_RR does not get replaced with a current one.\n>\n> Thanks for digging to the real cause.\n\nWell, it only took me a year.\nhttp://permalink.gmane.org/gmane.comp.version-control.git/177224\n\n> It does use merge-recursive\n> backend directly, and as a backend, it probably is correct that it\n> does not invoke rerere itself.\n>\n> In your patch, you are removing the state before you check and\n> notice that the user is in the middle of a merge and give control\n> back with \"Cannot apply a stash in the middle of a merge\".  Wouldn't\n> it be nicer to the user if you didn't remove the rerere state when\n> this happens (i.e. the user mistakenly said \"stash apply\" after a\n> conflicted merge), as that rerere state likely is from that merge\n> that produced the conflicted state?\n>\n> Would an obvious alternative of running \"git rerere\" ourselves after\n> running \"git merge-recursive\" in this script work?\n>\n>  git-stash.sh | 1 +\n>  1 file changed, 1 insertion(+)\n>\n> diff --git a/git-stash.sh b/git-stash.sh\n> index 4e2c7f8..bbefdf6 100755\n> --- a/git-stash.sh\n> +++ b/git-stash.sh\n> @@ -469,6 +469,7 @@ apply_stash () {\n>         else\n>                 # Merge conflict; keep the exit status from merge-recursive\n>                 status=$?\n> +               git rerere\n>                 if test -n \"$INDEX_OPTION\"\n>                 then\n>                         gettextln \"Index was not unstashed.\" >&2\n\nYes, except it needs \"git rerere clear\".  \"git rerere\" is not enough\nto cause the clean-up to occur.\n\nI'll roll a patch.\n\nPhil\n"},{"id":"194753","messageId":"7v7gugodzh.fsf@alter.siamese.dyndns.org","threadId":"30958","inReplyTo":"CABURp0rOfr=c-TcF7O6Ms=2iRA3SC9ckt_ukeozBS5vGqaKZaQ@mail.gmail.com","subject":"Re: [RFC/PATCH] stash: Delete MERGE_RR before stash apply","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-06T20:01:54Z","receivedAt":"2012-07-06T20:01:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phil Hord <phil.hord@gmail.com> writes:\n\n>> Would an obvious alternative of running \"git rerere\" ourselves after\n>> running \"git merge-recursive\" in this script work?\n>>\n>>  git-stash.sh | 1 +\n>>  1 file changed, 1 insertion(+)\n>>\n>> diff --git a/git-stash.sh b/git-stash.sh\n>> index 4e2c7f8..bbefdf6 100755\n>> --- a/git-stash.sh\n>> +++ b/git-stash.sh\n>> @@ -469,6 +469,7 @@ apply_stash () {\n>>         else\n>>                 # Merge conflict; keep the exit status from merge-recursive\n>>                 status=$?\n>> +               git rerere\n>>                 if test -n \"$INDEX_OPTION\"\n>>                 then\n>>                         gettextln \"Index was not unstashed.\" >&2\n>\n> Yes, except it needs \"git rerere clear\".  \"git rerere\" is not enough\n> to cause the clean-up to occur.\n\nIntuitively, it feels wrong to run \"rerere clear\" after an operation\nthat potentially can create conflicts in the index and in the\nworking tree.\n\nThe point in the codepath where the above \"git rerere\" appears is\nimmediately after we run merge-recursive backend.  Because the\nbackend does not invoke rerere itself (which by the way probably is\nthe correct thing not to; I haven't thought things through, though),\nwe invoke it ourselves there, so that the user can ask rerere to\nreplay an earlier conflict resolution.  Why can it be a good thing\nto discard potentially useful information with \"git rerere clear\"?\n\nI just tried this sequence (manually without any patch).\n\n    $ git init empty && cd empty\n    $ for i in a b c d e; do echo $i; done >file\n    $ git add file && git commit -m initial\n    $ for i in a b C d e; do echo $i; done >file ;# c to C\n    $ git stash\n\n    $ for i in a B c d e; do echo $i; done >file ;# b to B\n    $ git commit -a -m second\n\n    $ mkdir .git/rr-cache ;# enable rerere\n    $ git stash apply ;# conflicts\n    $ git rerere ;# records preimage\n\n    $ for i in a B C d e; do echo $i; done >file ;# c to C\n    $ git commit -a -m third ;# records resolution\n    $ git reset --hard HEAD^\n\n    $ git stash apply ;# conflicts\n    $ git rerere ;# replays\n\nI think the above \"how about this\" patch is equivalent to the two\n\"git rerere\" invocations I made manually with my experiment, and it\nseems to improve the end user experience (please try it yourself).\n\nWhat am I missing???\n"},{"id":"194762","messageId":"CABURp0oXhZ5ysm4b3Z=7o+2TB+3wFdMjj4oEwxafApjD4c7ozA@mail.gmail.com","threadId":"30958","inReplyTo":"7v7gugodzh.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH] stash: Delete MERGE_RR before stash apply","fromName":"Phil Hord","fromEmail":"phil.hord@gmail.com","sentAt":"2012-07-06T23:18:30Z","receivedAt":"2012-07-06T23:18:30Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"On Fri, Jul 6, 2012 at 4:01 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Phil Hord <phil.hord@gmail.com> writes:\n>\n>>> Would an obvious alternative of running \"git rerere\" ourselves after\n>>> running \"git merge-recursive\" in this script work?\n>>>\n>>>  git-stash.sh | 1 +\n>>>  1 file changed, 1 insertion(+)\n>>>\n>>> diff --git a/git-stash.sh b/git-stash.sh\n>>> index 4e2c7f8..bbefdf6 100755\n>>> --- a/git-stash.sh\n>>> +++ b/git-stash.sh\n>>> @@ -469,6 +469,7 @@ apply_stash () {\n>>>         else\n>>>                 # Merge conflict; keep the exit status from merge-recursive\n>>>                 status=$?\n>>> +               git rerere\n>>>                 if test -n \"$INDEX_OPTION\"\n>>>                 then\n>>>                         gettextln \"Index was not unstashed.\" >&2\n>>\n>> Yes, except it needs \"git rerere clear\".  \"git rerere\" is not enough\n>> to cause the clean-up to occur.\n>\n> Intuitively, it feels wrong to run \"rerere clear\" after an operation\n> that potentially can create conflicts in the index and in the\n> working tree.\n>\n> The point in the codepath where the above \"git rerere\" appears is\n> immediately after we run merge-recursive backend.  Because the\n> backend does not invoke rerere itself (which by the way probably is\n> the correct thing not to; I haven't thought things through, though),\n> we invoke it ourselves there, so that the user can ask rerere to\n> replay an earlier conflict resolution.  Why can it be a good thing\n> to discard potentially useful information with \"git rerere clear\"?\n>\n> I just tried this sequence (manually without any patch).\n>\n>     $ git init empty && cd empty\n>     $ for i in a b c d e; do echo $i; done >file\n>     $ git add file && git commit -m initial\n>     $ for i in a b C d e; do echo $i; done >file ;# c to C\n>     $ git stash\n>\n>     $ for i in a B c d e; do echo $i; done >file ;# b to B\n>     $ git commit -a -m second\n>\n>     $ mkdir .git/rr-cache ;# enable rerere\n>     $ git stash apply ;# conflicts\n>     $ git rerere ;# records preimage\n>\n>     $ for i in a B C d e; do echo $i; done >file ;# c to C\n>     $ git commit -a -m third ;# records resolution\n>     $ git reset --hard HEAD^\n>\n>     $ git stash apply ;# conflicts\n>     $ git rerere ;# replays\n>\n> I think the above \"how about this\" patch is equivalent to the two\n> \"git rerere\" invocations I made manually with my experiment, and it\n> seems to improve the end user experience (please try it yourself).\n>\n> What am I missing???\n\nI thought the reason rerere was not supported was because of some\nlimitation in the type of merge being completed.  I didn't think it\nwas possible to make rerere actually work with this situation.\n\nI understand now.  I'll try again.\n\nPhil\n"},{"id":"194773","messageId":"1341693962-17090-1-git-send-email-hordp@cisco.com","threadId":"30958","inReplyTo":"CABURp0oXhZ5ysm4b3Z=7o+2TB+3wFdMjj4oEwxafApjD4c7ozA@mail.gmail.com","subject":"[PATCH v2 0/2] stash: invoke rerere in case of conflict","fromName":"Phil Hord","fromEmail":"hordp@cisco.com","sentAt":"2012-07-07T20:46:00Z","receivedAt":"2012-07-07T20:46:00Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"[PATCH v2 1/2] test: git-stash conflict sets up rerere\nA rewritten failing test to verify rerere is used during \n'stash apply' conflicts.\n\n[PATCH v2 2/2] stash: invoke rerere in case of conflict\nThe fix (written entirely by JCH) and switched the test\nto expect success.\n"},{"id":"194774","messageId":"1341693962-17090-2-git-send-email-hordp@cisco.com","threadId":"30958","inReplyTo":"1341693962-17090-1-git-send-email-hordp@cisco.com","subject":"[PATCH v2 1/2] test: git-stash conflict sets up rerere","fromName":"Phil Hord","fromEmail":"hordp@cisco.com","sentAt":"2012-07-07T20:46:01Z","receivedAt":"2012-07-07T20:46:01Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"Add a failing test to confirm a conflicted stash apply\ninvokes rerere to record the conflicts and resolve the\nthe files it can.  In this failing state, mergetool may\nbe confused by a left-over state from previous rerere\nactivity.\n\nAlso, the next test expected us to finish up with a reset,\nwhich is impossible to do if we fail (as we must) and it's\nan unreasonable expectation anyway.  Begin the next test\nwith a reset of his own instead.\n\nSigned-off-by: Phil Hord <hordp@cisco.com>\n---\n t/t7610-mergetool.sh | 40 ++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 40 insertions(+)\n\ndiff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\nindex f5e16fc..c9f2fdc 100755\n--- a/t/t7610-mergetool.sh\n+++ b/t/t7610-mergetool.sh\n@@ -55,6 +55,16 @@ test_expect_success 'setup' '\n     git rm file12 &&\n     git commit -m \"branch1 changes\" &&\n \n+    git checkout -b stash1 master &&\n+    echo stash1 change file11 >file11 &&\n+    git add file11 &&\n+    git commit -m \"stash1 changes\" &&\n+\n+    git checkout -b stash2 master &&\n+    echo stash2 change file11 >file11 &&\n+    git add file11 &&\n+    git commit -m \"stash2 changes\" &&\n+\n     git checkout master &&\n     git submodule update -N &&\n     echo master updated >file1 &&\n@@ -193,7 +203,37 @@ test_expect_success 'mergetool skips resolved paths when rerere is active' '\n     git reset --hard\n '\n \n+test_expect_failure 'conflicted stash sets up rerere'  '\n+    git config rerere.enabled true &&\n+    git checkout stash1 &&\n+    echo \"Conflicting stash content\" >file11 &&\n+    git stash &&\n+    \n+    git checkout --detach stash2 &&\n+    test_must_fail git stash apply &&\n+    \n+    test -e .git/MERGE_RR &&\n+    test -n \"$(git ls-files -u)\" &&\n+    conflicts=\"$(git rerere remaining)\" &&\n+    test \"$conflicts\" = \"file11\" &&\n+    output=\"$(git mergetool --no-prompt)\" &&\n+    test \"$output\" != \"No files need merging\" &&\n+\n+    git commit -am \"save the stash resolution\" &&\n+\n+    git reset --hard stash2 &&\n+    test_must_fail git stash apply &&\n+\n+    test -e .git/MERGE_RR &&\n+    test -n \"$(git ls-files -u)\" &&\n+    conflicts=\"$(git rerere remaining)\" &&\n+    test -z \"$conflicts\" &&\n+    output=\"$(git mergetool --no-prompt)\" &&\n+    test \"$output\" = \"No files need merging\"\n+'\n+\n test_expect_success 'mergetool takes partial path' '\n+    git reset --hard\n     git config rerere.enabled false &&\n     git checkout -b test12 branch1 &&\n     git submodule update -N &&\n-- \n1.7.11.1.213.gb567ea5.dirty\n"},{"id":"194775","messageId":"1341693962-17090-3-git-send-email-hordp@cisco.com","threadId":"30958","inReplyTo":"1341693962-17090-1-git-send-email-hordp@cisco.com","subject":"[PATCH v2 2/2] stash: invoke rerere in case of conflict","fromName":"Phil Hord","fromEmail":"hordp@cisco.com","sentAt":"2012-07-07T20:46:02Z","receivedAt":"2012-07-07T20:46:02Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"'stash apply' directly calls a backend merge function\nwhich does not automatically invoke rerere.  This\nconfuses mergetool when leftover rerere state is left\nbehind from previous merges.\n\nInvoke rerere explicitly when we encounter a conflict\nduring stash apply\n\nChange the test for this flaw to expect success.\n\nSigned-off-by: Phil Hord <hordp@cisco.com>\nMentored-by: Junio C Hamano <gitster@pobox.com>\n---\n git-stash.sh         | 1 +\n t/t7610-mergetool.sh | 2 +-\n 2 files changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex 4e2c7f8..bbefdf6 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -469,6 +469,7 @@ apply_stash () {\n \telse\n \t\t# Merge conflict; keep the exit status from merge-recursive\n \t\tstatus=$?\n+\t\tgit rerere\n \t\tif test -n \"$INDEX_OPTION\"\n \t\tthen\n \t\t\tgettextln \"Index was not unstashed.\" >&2\ndiff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\nindex c9f2fdc..419cc38 100755\n--- a/t/t7610-mergetool.sh\n+++ b/t/t7610-mergetool.sh\n@@ -203,7 +203,7 @@ test_expect_success 'mergetool skips resolved paths when rerere is active' '\n     git reset --hard\n '\n \n-test_expect_failure 'conflicted stash sets up rerere'  '\n+test_expect_success 'conflicted stash sets up rerere'  '\n     git config rerere.enabled true &&\n     git checkout stash1 &&\n     echo \"Conflicting stash content\" >file11 &&\n-- \n1.7.11.1.213.gb567ea5.dirty\n"},{"id":"194797","messageId":"7v8vetmzgz.fsf@alter.siamese.dyndns.org","threadId":"30958","inReplyTo":"1341693962-17090-2-git-send-email-hordp@cisco.com","subject":"Re: [PATCH v2 1/2] test: git-stash conflict sets up rerere","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-09T02:37:32Z","receivedAt":"2012-07-09T02:37:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phil Hord <hordp@cisco.com> writes:\n\n> Add a failing test to confirm a conflicted stash apply invokes\n> rerere to record the conflicts and resolve the the files it can.\n\nOK.\n\n> In this failing state, mergetool may be confused by a left-over\n> state from previous rerere activity.\n\nIt is unclear to me what relevance this has to this patch.  Does it\nhave this sequence:\n\n    \"previous rerere activity\" (whatever that is)\n    test_must_fail git stash apply &&\n    git mergetool\n\nand demonstrate that \"git mergetool\" fails because there is a wrong\nrerere state in the repository after \"git stash apply\" returns?\n\nOr perhaps you are relying on the state that is left by the previous\ntest piece?\n\n> Also, the next test expected us to finish up with a reset, which\n> is impossible to do if we fail (as we must) and it's an\n> unreasonable expectation anyway.  Begin the next test with a reset\n> of his own instead.\n\nYes, it is always a good discipline to start a new test piece from a\nknown state.\n\n> @@ -193,7 +203,37 @@ test_expect_success 'mergetool skips resolved paths when rerere is active' '\n>      git reset --hard\n>  '\n>  \n> +test_expect_failure 'conflicted stash sets up rerere'  '\n> +    git config rerere.enabled true &&\n> +    git checkout stash1 &&\n> +    echo \"Conflicting stash content\" >file11 &&\n> +    git stash &&\n> +    \n> +    git checkout --detach stash2 &&\n> +    test_must_fail git stash apply &&\n> +    \n> +    test -e .git/MERGE_RR &&\n> +    test -n \"$(git ls-files -u)\" &&\n> +    conflicts=\"$(git rerere remaining)\" &&\n\nChecking that the index is conflicted with \"ls-files -u\" and asking\nthe public API \"git rerere remaining\" to see what paths rerere\nthinks it did not touch, like you do in the second and third lines,\nare very sensible, but it is probably not a good idea for this test\nto check implementation details with \"test -f .git/MERGE_RR\".\n\n> +    test \"$conflicts\" = \"file11\" &&\n> +    output=\"$(git mergetool --no-prompt)\" &&\n> +    test \"$output\" != \"No files need merging\" &&\n> +\n> +    git commit -am \"save the stash resolution\" &&\n> +\n> +    git reset --hard stash2 &&\n> +    test_must_fail git stash apply &&\n> +\n> +    test -e .git/MERGE_RR &&\n> +    test -n \"$(git ls-files -u)\" &&\n> +    conflicts=\"$(git rerere remaining)\" &&\n\nLikewise.\n\n> +    test -z \"$conflicts\" &&\n> +    output=\"$(git mergetool --no-prompt)\" &&\n> +    test \"$output\" = \"No files need merging\"\n> +'\n> +\n>  test_expect_success 'mergetool takes partial path' '\n> +    git reset --hard\n>      git config rerere.enabled false &&\n>      git checkout -b test12 branch1 &&\n>      git submodule update -N &&\n"},{"id":"194812","messageId":"CABURp0rrTvQcjHLsVwXC5hfBOuRmFG4AzSj6ZQwM=MXn0FAUCQ@mail.gmail.com","threadId":"30958","inReplyTo":"7v8vetmzgz.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 1/2] test: git-stash conflict sets up rerere","fromName":"Phil Hord","fromEmail":"phil.hord@gmail.com","sentAt":"2012-07-09T14:41:32Z","receivedAt":"2012-07-09T14:41:32Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n> Phil Hord <hordp@cisco.com> writes:\n>\n> > Add a failing test to confirm a conflicted stash apply invokes\n> > rerere to record the conflicts and resolve the the files it can.\n>\n> OK.\n>\n> > In this failing state, mergetool may be confused by a left-over\n> > state from previous rerere activity.\n>\n> It is unclear to me what relevance this has to this patch.  Does it\n> have this sequence:\n>\n>     \"previous rerere activity\" (whatever that is)\n>     test_must_fail git stash apply &&\n>     git mergetool\n>\n> and demonstrate that \"git mergetool\" fails because there is a wrong\n> rerere state in the repository after \"git stash apply\" returns?\n>\n> Or perhaps you are relying on the state that is left by the previous\n> test piece?\n\nThe previous edition of this patch explicitly created the condition\nwhich would confuse mergetool by creating a conflict and resolving it.\n The mergetool confusion is what I was testing and is what lead to\nthis patch series.\n\nBut I have since learned that rerere _can_ be effective after a stash\nconflict, but it was not invoked automatically. This series aims to\nfix that instead of simply forcing rerere to clean up in the stash\nconflict case.\n\nI left the concern in the commit message because this is more than a\nmissing feature; under certain conditions, it is a bug.  But I could\nhave reworded it to be more clear.\n\nI am not relying on a prior condition to exist.  In fact the 'git\nreset' at the start of this test will clear the previous rerere state\nthat I am testing for in this test.\n\nIn the previous edition, the test was this:\n  Verify mergetool is (not) confused by unclean rerere behavior:\n        1. Set up a normal merge-conflict with rerere so that MERGE_RR exists\n        2. Set up a conflicted stash-pop\n        3. Confirm/test the aberrant behavior of mergetool\n\nIn this edition, the aim of the test is different:\n  Verify rerere is (not) called by a conflict stash-apply:\n        1. Set up a conflicted stash-pop\n        2. Confirm/test whether rerere tracks the result\n\n> > Also, the next test expected us to finish up with a reset, which\n> > is impossible to do if we fail (as we must) and it's an\n> > unreasonable expectation anyway.  Begin the next test with a reset\n> > of his own instead.\n>\n> Yes, it is always a good discipline to start a new test piece from a\n> known state.\n>\n> > @@ -193,7 +203,37 @@ test_expect_success 'mergetool skips resolved paths when rerere is active' '\n> >      git reset --hard\n> >  '\n> >\n> > +test_expect_failure 'conflicted stash sets up rerere'  '\n> > +    git config rerere.enabled true &&\n> > +    git checkout stash1 &&\n> > +    echo \"Conflicting stash content\" >file11 &&\n> > +    git stash &&\n> > +\n> > +    git checkout --detach stash2 &&\n> > +    test_must_fail git stash apply &&\n> > +\n> > +    test -e .git/MERGE_RR &&\n> > +    test -n \"$(git ls-files -u)\" &&\n> > +    conflicts=\"$(git rerere remaining)\" &&\n>\n> Checking that the index is conflicted with \"ls-files -u\" and asking\n> the public API \"git rerere remaining\" to see what paths rerere\n> thinks it did not touch, like you do in the second and third lines,\n> are very sensible, but it is probably not a good idea for this test\n> to check implementation details with \"test -f .git/MERGE_RR\".\n\nI tend to agree.  However, it is the presence of .git/MERGE_RR which\nwill cause mergetool to take the 'rerere remaining' path.  I wanted to\nensure that mergetool is going to go the way I expected.  In light of\nthe later actions in this test, that is probably overkill.  I can\nremove it.\n\n> > +    test \"$conflicts\" = \"file11\" &&\n> > +    output=\"$(git mergetool --no-prompt)\" &&\n> > +    test \"$output\" != \"No files need merging\" &&\n> > +\n> > +    git commit -am \"save the stash resolution\" &&\n> > +\n> > +    git reset --hard stash2 &&\n> > +    test_must_fail git stash apply &&\n> > +\n> > +    test -e .git/MERGE_RR &&\n> > +    test -n \"$(git ls-files -u)\" &&\n> > +    conflicts=\"$(git rerere remaining)\" &&\n>\n> Likewise.\n\nAnd ditto.\n\n\n> > +    test -z \"$conflicts\" &&\n> > +    output=\"$(git mergetool --no-prompt)\" &&\n> > +    test \"$output\" = \"No files need merging\"\n> > +'\n> > +\n> >  test_expect_success 'mergetool takes partial path' '\n> > +    git reset --hard\n> >      git config rerere.enabled false &&\n> >      git checkout -b test12 branch1 &&\n> >      git submodule update -N &&\n\nSo, the next roll will remove the tests for MERGE_RR and will be more\nexplicit about the potential for mergetool confusion and/or the fact\nthat it is not explicitly tested here.\n\nI'll wait a little longer for any further comments.\n\nPhil\n"},{"id":"197127","messageId":"7vehn6zedz.fsf@alter.siamese.dyndns.org","threadId":"30958","inReplyTo":"CABURp0rrTvQcjHLsVwXC5hfBOuRmFG4AzSj6ZQwM=MXn0FAUCQ@mail.gmail.com","subject":"Re: [PATCH v2 1/2] test: git-stash conflict sets up rerere","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-16T22:00:08Z","receivedAt":"2012-08-16T22:00:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phil Hord <phil.hord@gmail.com> writes:\n\n> So, the next roll will remove the tests for MERGE_RR and will be more\n> explicit about the potential for mergetool confusion and/or the fact\n> that it is not explicitly tested here.\n>\n> I'll wait a little longer for any further comments.\n\nMild ping to a seemingly stalled topic.\n"},{"id":"197200","messageId":"CABURp0rdZ2N6exE0gpJncSnTzGB0Hbr+RuO4G5F6zb=z8Qb0Sg@mail.gmail.com","threadId":"30958","inReplyTo":"7vehn6zedz.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 1/2] test: git-stash conflict sets up rerere","fromName":"Phil Hord","fromEmail":"phil.hord@gmail.com","sentAt":"2012-08-17T17:52:07Z","receivedAt":"2012-08-17T17:52:07Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"On Thu, Aug 16, 2012 at 6:00 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Phil Hord <phil.hord@gmail.com> writes:\n>\n>> So, the next roll will remove the tests for MERGE_RR and will be more\n>> explicit about the potential for mergetool confusion and/or the fact\n>> that it is not explicitly tested here.\n>>\n>> I'll wait a little longer for any further comments.\n>\n> Mild ping to a seemingly stalled topic.\n\nI was going to say the same thing.\n\nPatch v3 series is here:\nhttp://permalink.gmane.org/gmane.comp.version-control.git/201282\nhttp://permalink.gmane.org/gmane.comp.version-control.git/201283\n\nI assume these were missed because I sent them during some critical\npoint in the release cycle.  But maybe it was because I missed some\nstep in the submission-revision protocol.  If the latter, please let\nme know and I will try to learn to do better next time.\n\nPhil ~ resent due to missed ReplyAll button\n"},{"id":"197202","messageId":"7vy5ldtn0e.fsf@alter.siamese.dyndns.org","threadId":"30958","inReplyTo":"CABURp0rdZ2N6exE0gpJncSnTzGB0Hbr+RuO4G5F6zb=z8Qb0Sg@mail.gmail.com","subject":"Re: [PATCH v2 1/2] test: git-stash conflict sets up rerere","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-17T18:02:41Z","receivedAt":"2012-08-17T18:02:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phil Hord <phil.hord@gmail.com> writes:\n\n> On Thu, Aug 16, 2012 at 6:00 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Phil Hord <phil.hord@gmail.com> writes:\n>>\n>>> So, the next roll will remove the tests for MERGE_RR and will be more\n>>> explicit about the potential for mergetool confusion and/or the fact\n>>> that it is not explicitly tested here.\n>>>\n>>> I'll wait a little longer for any further comments.\n>>\n>> Mild ping to a seemingly stalled topic.\n>\n> I was going to say the same thing.\n>\n> Patch v3 series is here:\n> http://permalink.gmane.org/gmane.comp.version-control.git/201282\n> http://permalink.gmane.org/gmane.comp.version-control.git/201283\n>\n> I assume these were missed because I sent them during some critical\n> point in the release cycle.  But maybe it was because I missed some\n> step in the submission-revision protocol.  If the latter, please let\n> me know and I will try to learn to do better next time.\n\nThanks; they were just lost in the noise of the traffic.  Will\nreplace and requeue.\n"}]}