Re: [PATCH v2 1/2] test: git-stash conflict sets up rerere
Junio C Hamano <gitster@pobox.com> wrote:
Show 22 quoted lines
> 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 mergetoolIn 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 resultShow 30 quoted lines
> > 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.
Show 14 quoted lines
> > + 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.
Show 10 quoted lines
> > + 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
Phil Hord