Re: [PATCH v2 1/2] test: git-stash conflict sets up rerere
On Thu, Aug 16, 2012 at 6:00 PM, Junio C Hamano gits...@pobox.com wrote: Phil Hord phil.h...@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 -- 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
Re: [PATCH v2 1/2] test: git-stash conflict sets up rerere
Phil Hord phil.h...@gmail.com writes: On Thu, Aug 16, 2012 at 6:00 PM, Junio C Hamano gits...@pobox.com wrote: Phil Hord phil.h...@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. -- 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
Re: [PATCH v2 1/2] test: git-stash conflict sets up rerere
Phil Hord phil.h...@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. -- 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
Re: [PATCH v2 1/2] test: git-stash conflict sets up rerere
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
Re: [PATCH v2 1/2] test: git-stash conflict sets up rerere
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? 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 -- 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
[PATCH v2 1/2] test: git-stash conflict sets up rerere
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 ho...@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 -- 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
