[RFC/PATCH] stash: Delete MERGE_RR before stash apply

[RFC/PATCH] stash: Delete MERGE_RR before stash apply

[Phil Hord]

The presence of a GIT_DIR/MERGE_RR file indicates we
were resolving a merge which had rerere candidates for
recording.  But the file does not get deleted after
all resolutions are completed.  This is ok for most
cases because the file will get replaced when the
next merge happens.  But stash apply does not use
a merge that supports rerere, and so the old
MERGE_RR does not get replaced with a current one.

This then confuses mergetool who thinks a rerere
operation is in play when it is not.

Fix this by ensuring there is no leftover
MERGE_RR file whenever we are beginning a git stash
apply.

Signed-off-by: Phil Hord <hordp@cisco.com>
---

This feels like a hack and like it is in the wrong location.
However, the change is minimal and should fix my
specific problem[1].

[1] http://comments.gmane.org/gmane.comp.version-control.git/200178

 git-stash.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/git-stash.sh b/git-stash.sh
index 4e2c7f8..0b96e2f 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -409,6 +409,9 @@ apply_stash () {

        assert_stash_like "$@"

+       test -f "$GIT_DIR/MERGE_RR" &&
+               git rerere clear
+
        git update-index -q --refresh || die "$(gettext "unable to
refresh index")"

        # current index state
-- 
1.7.11.1.161.g0f17059.dirty

Re: [RFC/PATCH] stash: Delete MERGE_RR before stash apply

[Junio C Hamano]

Phil Hord <phil.hord@gmail.com> writes:

> The presence of a GIT_DIR/MERGE_RR file indicates we
> were resolving a merge which had rerere candidates for
> recording.  But the file does not get deleted after
> all resolutions are completed.  This is ok for most
> cases because the file will get replaced when the
> next merge happens.  But stash apply does not use
> a merge that supports rerere, and so the old
> MERGE_RR does not get replaced with a current one.

Thanks for digging to the real cause.  It does use merge-recursive
backend directly, and as a backend, it probably is correct that it
does not invoke rerere itself.

In your patch, you are removing the state before you check and
notice that the user is in the middle of a merge and give control
back with "Cannot apply a stash in the middle of a merge".  Wouldn't
it be nicer to the user if you didn't remove the rerere state when
this happens (i.e. the user mistakenly said "stash apply" after a
conflicted merge), as that rerere state likely is from that merge
that produced the conflicted state?

Would an obvious alternative of running "git rerere" ourselves after
running "git merge-recursive" in this script work?

 git-stash.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/git-stash.sh b/git-stash.sh
index 4e2c7f8..bbefdf6 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -469,6 +469,7 @@ apply_stash () {
 	else
 		# Merge conflict; keep the exit status from merge-recursive
 		status=$?
+		git rerere
 		if test -n "$INDEX_OPTION"
 		then
 			gettextln "Index was not unstashed." >&2

Re: [RFC/PATCH] stash: Delete MERGE_RR before stash apply

[Phil Hord]

On Thu, Jul 5, 2012 at 1:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Phil Hord <phil.hord@gmail.com> writes:
>
>> The presence of a GIT_DIR/MERGE_RR file indicates we
>> were resolving a merge which had rerere candidates for
>> recording.  But the file does not get deleted after
>> all resolutions are completed.  This is ok for most
>> cases because the file will get replaced when the
>> next merge happens.  But stash apply does not use
>> a merge that supports rerere, and so the old
>> MERGE_RR does not get replaced with a current one.
>
> Thanks for digging to the real cause.

Well, it only took me a year.
http://permalink.gmane.org/gmane.comp.version-control.git/177224

> It does use merge-recursive
> backend directly, and as a backend, it probably is correct that it
> does not invoke rerere itself.
>
> In your patch, you are removing the state before you check and
> notice that the user is in the middle of a merge and give control
> back with "Cannot apply a stash in the middle of a merge".  Wouldn't
> it be nicer to the user if you didn't remove the rerere state when
> this happens (i.e. the user mistakenly said "stash apply" after a
> conflicted merge), as that rerere state likely is from that merge
> that produced the conflicted state?
>
> Would an obvious alternative of running "git rerere" ourselves after
> running "git merge-recursive" in this script work?
>
>  git-stash.sh | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/git-stash.sh b/git-stash.sh
> index 4e2c7f8..bbefdf6 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -469,6 +469,7 @@ apply_stash () {
>         else
>                 # Merge conflict; keep the exit status from merge-recursive
>                 status=$?
> +               git rerere
>                 if test -n "$INDEX_OPTION"
>                 then
>                         gettextln "Index was not unstashed." >&2

Yes, except it needs "git rerere clear".  "git rerere" is not enough
to cause the clean-up to occur.

I'll roll a patch.

Phil

Re: [RFC/PATCH] stash: Delete MERGE_RR before stash apply

[Junio C Hamano]

Phil Hord <phil.hord@gmail.com> writes:

>> Would an obvious alternative of running "git rerere" ourselves after
>> running "git merge-recursive" in this script work?
>>
>>  git-stash.sh | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/git-stash.sh b/git-stash.sh
>> index 4e2c7f8..bbefdf6 100755
>> --- a/git-stash.sh
>> +++ b/git-stash.sh
>> @@ -469,6 +469,7 @@ apply_stash () {
>>         else
>>                 # Merge conflict; keep the exit status from merge-recursive
>>                 status=$?
>> +               git rerere
>>                 if test -n "$INDEX_OPTION"
>>                 then
>>                         gettextln "Index was not unstashed." >&2
>
> Yes, except it needs "git rerere clear".  "git rerere" is not enough
> to cause the clean-up to occur.

Intuitively, it feels wrong to run "rerere clear" after an operation
that potentially can create conflicts in the index and in the
working tree.

The point in the codepath where the above "git rerere" appears is
immediately after we run merge-recursive backend.  Because the
backend does not invoke rerere itself (which by the way probably is
the correct thing not to; I haven't thought things through, though),
we invoke it ourselves there, so that the user can ask rerere to
replay an earlier conflict resolution.  Why can it be a good thing
to discard potentially useful information with "git rerere clear"?

I just tried this sequence (manually without any patch).

    $ git init empty && cd empty
    $ for i in a b c d e; do echo $i; done >file
    $ git add file && git commit -m initial
    $ for i in a b C d e; do echo $i; done >file ;# c to C
    $ git stash

    $ for i in a B c d e; do echo $i; done >file ;# b to B
    $ git commit -a -m second

    $ mkdir .git/rr-cache ;# enable rerere
    $ git stash apply ;# conflicts
    $ git rerere ;# records preimage

    $ for i in a B C d e; do echo $i; done >file ;# c to C
    $ git commit -a -m third ;# records resolution
    $ git reset --hard HEAD^

    $ git stash apply ;# conflicts
    $ git rerere ;# replays

I think the above "how about this" patch is equivalent to the two
"git rerere" invocations I made manually with my experiment, and it
seems to improve the end user experience (please try it yourself).

What am I missing???

Re: [RFC/PATCH] stash: Delete MERGE_RR before stash apply

[Phil Hord]

On Fri, Jul 6, 2012 at 4:01 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Phil Hord <phil.hord@gmail.com> writes:
>
>>> Would an obvious alternative of running "git rerere" ourselves after
>>> running "git merge-recursive" in this script work?
>>>
>>>  git-stash.sh | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/git-stash.sh b/git-stash.sh
>>> index 4e2c7f8..bbefdf6 100755
>>> --- a/git-stash.sh
>>> +++ b/git-stash.sh
>>> @@ -469,6 +469,7 @@ apply_stash () {
>>>         else
>>>                 # Merge conflict; keep the exit status from merge-recursive
>>>                 status=$?
>>> +               git rerere
>>>                 if test -n "$INDEX_OPTION"
>>>                 then
>>>                         gettextln "Index was not unstashed." >&2
>>
>> Yes, except it needs "git rerere clear".  "git rerere" is not enough
>> to cause the clean-up to occur.
>
> Intuitively, it feels wrong to run "rerere clear" after an operation
> that potentially can create conflicts in the index and in the
> working tree.
>
> The point in the codepath where the above "git rerere" appears is
> immediately after we run merge-recursive backend.  Because the
> backend does not invoke rerere itself (which by the way probably is
> the correct thing not to; I haven't thought things through, though),
> we invoke it ourselves there, so that the user can ask rerere to
> replay an earlier conflict resolution.  Why can it be a good thing
> to discard potentially useful information with "git rerere clear"?
>
> I just tried this sequence (manually without any patch).
>
>     $ git init empty && cd empty
>     $ for i in a b c d e; do echo $i; done >file
>     $ git add file && git commit -m initial
>     $ for i in a b C d e; do echo $i; done >file ;# c to C
>     $ git stash
>
>     $ for i in a B c d e; do echo $i; done >file ;# b to B
>     $ git commit -a -m second
>
>     $ mkdir .git/rr-cache ;# enable rerere
>     $ git stash apply ;# conflicts
>     $ git rerere ;# records preimage
>
>     $ for i in a B C d e; do echo $i; done >file ;# c to C
>     $ git commit -a -m third ;# records resolution
>     $ git reset --hard HEAD^
>
>     $ git stash apply ;# conflicts
>     $ git rerere ;# replays
>
> I think the above "how about this" patch is equivalent to the two
> "git rerere" invocations I made manually with my experiment, and it
> seems to improve the end user experience (please try it yourself).
>
> What am I missing???

I thought the reason rerere was not supported was because of some
limitation in the type of merge being completed.  I didn't think it
was possible to make rerere actually work with this situation.

I understand now.  I'll try again.

Phil

[PATCH v2 0/2] stash: invoke rerere in case of conflict

[Phil Hord]

[PATCH v2 1/2] test: git-stash conflict sets up rerere
A rewritten failing test to verify rerere is used during 
'stash apply' conflicts.

[PATCH v2 2/2] stash: invoke rerere in case of conflict
The fix (written entirely by JCH) and switched the test
to expect success.

[PATCH v2 1/2] test: git-stash conflict sets up rerere

[Phil Hord]

Add a failing test to confirm a conflicted stash apply
invokes rerere to record the conflicts and resolve the
the files it can.  In this failing state, mergetool may
be confused by a left-over state from previous rerere
activity.

Also, the next test expected us to finish up with a reset,
which is impossible to do if we fail (as we must) and it's
an unreasonable expectation anyway.  Begin the next test
with a reset of his own instead.

Signed-off-by: Phil Hord <hordp@cisco.com>
---
 t/t7610-mergetool.sh | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index f5e16fc..c9f2fdc 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -55,6 +55,16 @@ test_expect_success 'setup' '
     git rm file12 &&
     git commit -m "branch1 changes" &&
 
+    git checkout -b stash1 master &&
+    echo stash1 change file11 >file11 &&
+    git add file11 &&
+    git commit -m "stash1 changes" &&
+
+    git checkout -b stash2 master &&
+    echo stash2 change file11 >file11 &&
+    git add file11 &&
+    git commit -m "stash2 changes" &&
+
     git checkout master &&
     git submodule update -N &&
     echo master updated >file1 &&
@@ -193,7 +203,37 @@ test_expect_success 'mergetool skips resolved paths when rerere is active' '
     git reset --hard
 '
 
+test_expect_failure 'conflicted stash sets up rerere'  '
+    git config rerere.enabled true &&
+    git checkout stash1 &&
+    echo "Conflicting stash content" >file11 &&
+    git stash &&
+    
+    git checkout --detach stash2 &&
+    test_must_fail git stash apply &&
+    
+    test -e .git/MERGE_RR &&
+    test -n "$(git ls-files -u)" &&
+    conflicts="$(git rerere remaining)" &&
+    test "$conflicts" = "file11" &&
+    output="$(git mergetool --no-prompt)" &&
+    test "$output" != "No files need merging" &&
+
+    git commit -am "save the stash resolution" &&
+
+    git reset --hard stash2 &&
+    test_must_fail git stash apply &&
+
+    test -e .git/MERGE_RR &&
+    test -n "$(git ls-files -u)" &&
+    conflicts="$(git rerere remaining)" &&
+    test -z "$conflicts" &&
+    output="$(git mergetool --no-prompt)" &&
+    test "$output" = "No files need merging"
+'
+
 test_expect_success 'mergetool takes partial path' '
+    git reset --hard
     git config rerere.enabled false &&
     git checkout -b test12 branch1 &&
     git submodule update -N &&
-- 
1.7.11.1.213.gb567ea5.dirty

Re: [PATCH v2 1/2] test: git-stash conflict sets up rerere

[Junio C Hamano]

Phil Hord <hordp@cisco.com> writes:

> Add a failing test to confirm a conflicted stash apply invokes
> rerere to record the conflicts and resolve the the files it can.

OK.

> In this failing state, mergetool may be confused by a left-over
> state from previous rerere activity.

It is unclear to me what relevance this has to this patch.  Does it
have this sequence:

    "previous rerere activity" (whatever that is)
    test_must_fail git stash apply &&
    git mergetool

and demonstrate that "git mergetool" fails because there is a wrong
rerere state in the repository after "git stash apply" returns?

Or perhaps you are relying on the state that is left by the previous
test piece?

> Also, the next test expected us to finish up with a reset, which
> is impossible to do if we fail (as we must) and it's an
> unreasonable expectation anyway.  Begin the next test with a reset
> of his own instead.

Yes, it is always a good discipline to start a new test piece from a
known state.

> @@ -193,7 +203,37 @@ test_expect_success 'mergetool skips resolved paths when rerere is active' '
>      git reset --hard
>  '
>  
> +test_expect_failure 'conflicted stash sets up rerere'  '
> +    git config rerere.enabled true &&
> +    git checkout stash1 &&
> +    echo "Conflicting stash content" >file11 &&
> +    git stash &&
> +    
> +    git checkout --detach stash2 &&
> +    test_must_fail git stash apply &&
> +    
> +    test -e .git/MERGE_RR &&
> +    test -n "$(git ls-files -u)" &&
> +    conflicts="$(git rerere remaining)" &&

Checking that the index is conflicted with "ls-files -u" and asking
the public API "git rerere remaining" to see what paths rerere
thinks it did not touch, like you do in the second and third lines,
are very sensible, but it is probably not a good idea for this test
to check implementation details with "test -f .git/MERGE_RR".

> +    test "$conflicts" = "file11" &&
> +    output="$(git mergetool --no-prompt)" &&
> +    test "$output" != "No files need merging" &&
> +
> +    git commit -am "save the stash resolution" &&
> +
> +    git reset --hard stash2 &&
> +    test_must_fail git stash apply &&
> +
> +    test -e .git/MERGE_RR &&
> +    test -n "$(git ls-files -u)" &&
> +    conflicts="$(git rerere remaining)" &&

Likewise.

> +    test -z "$conflicts" &&
> +    output="$(git mergetool --no-prompt)" &&
> +    test "$output" = "No files need merging"
> +'
> +
>  test_expect_success 'mergetool takes partial path' '
> +    git reset --hard
>      git config rerere.enabled false &&
>      git checkout -b test12 branch1 &&
>      git submodule update -N &&

Re: [PATCH v2 1/2] test: git-stash conflict sets up rerere

[Phil Hord]

Junio C Hamano <gitster@pobox.com> wrote:
> Phil Hord <hordp@cisco.com> writes:
>
> > Add a failing test to confirm a conflicted stash apply invokes
> > rerere to record the conflicts and resolve the the files it can.
>
> OK.
>
> > In this failing state, mergetool may be confused by a left-over
> > state from previous rerere activity.
>
> It is unclear to me what relevance this has to this patch.  Does it
> have this sequence:
>
>     "previous rerere activity" (whatever that is)
>     test_must_fail git stash apply &&
>     git mergetool
>
> and demonstrate that "git mergetool" fails because there is a wrong
> rerere state in the repository after "git stash apply" returns?
>
> Or perhaps you are relying on the state that is left by the previous
> test piece?

The previous edition of this patch explicitly created the condition
which would confuse mergetool by creating a conflict and resolving it.
 The mergetool confusion is what I was testing and is what lead to
this patch series.

But I have since learned that rerere _can_ be effective after a stash
conflict, but it was not invoked automatically. This series aims to
fix that instead of simply forcing rerere to clean up in the stash
conflict case.

I left the concern in the commit message because this is more than a
missing feature; under certain conditions, it is a bug.  But I could
have reworded it to be more clear.

I am not relying on a prior condition to exist.  In fact the 'git
reset' at the start of this test will clear the previous rerere state
that I am testing for in this test.

In the previous edition, the test was this:
  Verify mergetool is (not) confused by unclean rerere behavior:
        1. Set up a normal merge-conflict with rerere so that MERGE_RR exists
        2. Set up a conflicted stash-pop
        3. Confirm/test the aberrant behavior of mergetool

In this edition, the aim of the test is different:
  Verify rerere is (not) called by a conflict stash-apply:
        1. Set up a conflicted stash-pop
        2. Confirm/test whether rerere tracks the result

> > Also, the next test expected us to finish up with a reset, which
> > is impossible to do if we fail (as we must) and it's an
> > unreasonable expectation anyway.  Begin the next test with a reset
> > of his own instead.
>
> Yes, it is always a good discipline to start a new test piece from a
> known state.
>
> > @@ -193,7 +203,37 @@ test_expect_success 'mergetool skips resolved paths when rerere is active' '
> >      git reset --hard
> >  '
> >
> > +test_expect_failure 'conflicted stash sets up rerere'  '
> > +    git config rerere.enabled true &&
> > +    git checkout stash1 &&
> > +    echo "Conflicting stash content" >file11 &&
> > +    git stash &&
> > +
> > +    git checkout --detach stash2 &&
> > +    test_must_fail git stash apply &&
> > +
> > +    test -e .git/MERGE_RR &&
> > +    test -n "$(git ls-files -u)" &&
> > +    conflicts="$(git rerere remaining)" &&
>
> Checking that the index is conflicted with "ls-files -u" and asking
> the public API "git rerere remaining" to see what paths rerere
> thinks it did not touch, like you do in the second and third lines,
> are very sensible, but it is probably not a good idea for this test
> to check implementation details with "test -f .git/MERGE_RR".

I tend to agree.  However, it is the presence of .git/MERGE_RR which
will cause mergetool to take the 'rerere remaining' path.  I wanted to
ensure that mergetool is going to go the way I expected.  In light of
the later actions in this test, that is probably overkill.  I can
remove it.

> > +    test "$conflicts" = "file11" &&
> > +    output="$(git mergetool --no-prompt)" &&
> > +    test "$output" != "No files need merging" &&
> > +
> > +    git commit -am "save the stash resolution" &&
> > +
> > +    git reset --hard stash2 &&
> > +    test_must_fail git stash apply &&
> > +
> > +    test -e .git/MERGE_RR &&
> > +    test -n "$(git ls-files -u)" &&
> > +    conflicts="$(git rerere remaining)" &&
>
> Likewise.

And ditto.


> > +    test -z "$conflicts" &&
> > +    output="$(git mergetool --no-prompt)" &&
> > +    test "$output" = "No files need merging"
> > +'
> > +
> >  test_expect_success 'mergetool takes partial path' '
> > +    git reset --hard
> >      git config rerere.enabled false &&
> >      git checkout -b test12 branch1 &&
> >      git submodule update -N &&

So, the next roll will remove the tests for MERGE_RR and will be more
explicit about the potential for mergetool confusion and/or the fact
that it is not explicitly tested here.

I'll wait a little longer for any further comments.

Phil

Re: [PATCH v2 1/2] test: git-stash conflict sets up rerere

[Junio C Hamano]

Phil Hord <phil.hord@gmail.com> writes:

> So, the next roll will remove the tests for MERGE_RR and will be more
> explicit about the potential for mergetool confusion and/or the fact
> that it is not explicitly tested here.
>
> I'll wait a little longer for any further comments.

Mild ping to a seemingly stalled topic.

Re: [PATCH v2 1/2] test: git-stash conflict sets up rerere

[Phil Hord]

On Thu, Aug 16, 2012 at 6:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Phil Hord <phil.hord@gmail.com> writes:
>
>> So, the next roll will remove the tests for MERGE_RR and will be more
>> explicit about the potential for mergetool confusion and/or the fact
>> that it is not explicitly tested here.
>>
>> I'll wait a little longer for any further comments.
>
> Mild ping to a seemingly stalled topic.

I was going to say the same thing.

Patch v3 series is here:
http://permalink.gmane.org/gmane.comp.version-control.git/201282
http://permalink.gmane.org/gmane.comp.version-control.git/201283

I assume these were missed because I sent them during some critical
point in the release cycle.  But maybe it was because I missed some
step in the submission-revision protocol.  If the latter, please let
me know and I will try to learn to do better next time.

Phil ~ resent due to missed ReplyAll button

Re: [PATCH v2 1/2] test: git-stash conflict sets up rerere

[Junio C Hamano]

Phil Hord <phil.hord@gmail.com> writes:

> On Thu, Aug 16, 2012 at 6:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Phil Hord <phil.hord@gmail.com> writes:
>>
>>> So, the next roll will remove the tests for MERGE_RR and will be more
>>> explicit about the potential for mergetool confusion and/or the fact
>>> that it is not explicitly tested here.
>>>
>>> I'll wait a little longer for any further comments.
>>
>> Mild ping to a seemingly stalled topic.
>
> I was going to say the same thing.
>
> Patch v3 series is here:
> http://permalink.gmane.org/gmane.comp.version-control.git/201282
> http://permalink.gmane.org/gmane.comp.version-control.git/201283
>
> I assume these were missed because I sent them during some critical
> point in the release cycle.  But maybe it was because I missed some
> step in the submission-revision protocol.  If the latter, please let
> me know and I will try to learn to do better next time.

Thanks; they were just lost in the noise of the traffic.  Will
replace and requeue.

[PATCH v2 2/2] stash: invoke rerere in case of conflict

[Phil Hord]

'stash apply' directly calls a backend merge function
which does not automatically invoke rerere.  This
confuses mergetool when leftover rerere state is left
behind from previous merges.

Invoke rerere explicitly when we encounter a conflict
during stash apply

Change the test for this flaw to expect success.

Signed-off-by: Phil Hord <hordp@cisco.com>
Mentored-by: Junio C Hamano <gitster@pobox.com>
---
 git-stash.sh         | 1 +
 t/t7610-mergetool.sh | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/git-stash.sh b/git-stash.sh
index 4e2c7f8..bbefdf6 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -469,6 +469,7 @@ apply_stash () {
 	else
 		# Merge conflict; keep the exit status from merge-recursive
 		status=$?
+		git rerere
 		if test -n "$INDEX_OPTION"
 		then
 			gettextln "Index was not unstashed." >&2
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index c9f2fdc..419cc38 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -203,7 +203,7 @@ test_expect_success 'mergetool skips resolved paths when rerere is active' '
     git reset --hard
 '
 
-test_expect_failure 'conflicted stash sets up rerere'  '
+test_expect_success 'conflicted stash sets up rerere'  '
     git config rerere.enabled true &&
     git checkout stash1 &&
     echo "Conflicting stash content" >file11 &&
-- 
1.7.11.1.213.gb567ea5.dirty