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

Junio C Hamano [off-list ref] wrote:

Phil Hord [off-list ref] writes:

quoted

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

OK.

quoted

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

quoted

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.

quoted

@@ -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.

quoted

+    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.

quoted

+    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