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

From: Phil Hord <phil.hord@gmail.com>
Date: Jul 9, 2012, 14:41 UTC
Message-ID: <CABURp0rrTvQcjHLsVwXC5hfBOuRmFG4AzSj6ZQwM=MXn0FAUCQ@mail.gmail.com>
In-Reply-To: <7v8vetmzgz.fsf@alter.siamese.dyndns.org>

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

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

And ditto.

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