Junio C Hamano <gits...@pobox.com> wrote: > Phil Hord <ho...@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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html