Re: [PATCH v4 2/5] stash: Add a test for when apply fails during stash branch
On Tue, Jun 13, 2017 at 12:40 PM, Junio C Hamanowrote: > Joel Teichroeb writes: > >> If the return value of merge recurisve is not checked, the stash could end >> up being dropped even though it was not applied properly > > s/recurisve/recursive/ > >> Signed-off-by: Joel Teichroeb >> --- >> t/t3903-stash.sh | 14 ++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh >> index cc923e6335..5399fb05ca 100755 >> --- a/t/t3903-stash.sh >> +++ b/t/t3903-stash.sh >> @@ -656,6 +656,20 @@ test_expect_success 'stash branch should not drop the >> stash if the branch exists >> git rev-parse stash@{0} -- >> ' >> >> +test_expect_success 'stash branch should not drop the stash if the apply >> fails' ' >> + git stash clear && >> + git reset HEAD~1 --hard && >> + echo foo >file && >> + git add file && >> + git commit -m initial && > > It's not quite intuitive to call a non-root commit "initial" ;-) > >> + echo bar >file && >> + git stash && >> + echo baz >file && > > OK, so 'file' has 'foo' in HEAD, 'bar' in the stash@{0}. > >> + test_when_finished "git checkout master" && >> + test_must_fail git stash branch new_branch stash@{0} && > > Hmph. Do we blindly checkout new_branch out of stash@{0}^1 and > unstash, but because 'file' in the working tree is dirty, we fail to > apply the stash and stop? > > This sounds like a bug to me. Shouldn't we be staying on 'master', > and fail without even creating 'new_branch', when this happens? Good point. The existing behavior is to create new_branch and check it out. I'm not sure what the correct state should be then. Create new_branch, checkout new_branch, fail to apply, checkout master? Should it then delete new_branch? Is there a way instead to test applying the stash before creating the branch without actually applying it? Something like putting merge_recursive into some kind of dry-run mode? > > In any case we should be testing what branch we are on after this > step. What branch should we be on after "git stash branch" fails? > >> + git rev-parse stash@{0} -- >> +' >> + >> test_expect_success 'stash apply shows status same as git status (relative >> to current directory)' ' >> git stash clear && >> echo 1 >subdir/subfile1 &&
Re: [PATCH v4 2/5] stash: Add a test for when apply fails during stash branch
Joel Teichroebwrites: > If the return value of merge recurisve is not checked, the stash could end > up being dropped even though it was not applied properly s/recurisve/recursive/ > Signed-off-by: Joel Teichroeb > --- > t/t3903-stash.sh | 14 ++ > 1 file changed, 14 insertions(+) > > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > index cc923e6335..5399fb05ca 100755 > --- a/t/t3903-stash.sh > +++ b/t/t3903-stash.sh > @@ -656,6 +656,20 @@ test_expect_success 'stash branch should not drop the > stash if the branch exists > git rev-parse stash@{0} -- > ' > > +test_expect_success 'stash branch should not drop the stash if the apply > fails' ' > + git stash clear && > + git reset HEAD~1 --hard && > + echo foo >file && > + git add file && > + git commit -m initial && It's not quite intuitive to call a non-root commit "initial" ;-) > + echo bar >file && > + git stash && > + echo baz >file && OK, so 'file' has 'foo' in HEAD, 'bar' in the stash@{0}. > + test_when_finished "git checkout master" && > + test_must_fail git stash branch new_branch stash@{0} && Hmph. Do we blindly checkout new_branch out of stash@{0}^1 and unstash, but because 'file' in the working tree is dirty, we fail to apply the stash and stop? This sounds like a bug to me. Shouldn't we be staying on 'master', and fail without even creating 'new_branch', when this happens? In any case we should be testing what branch we are on after this step. What branch should we be on after "git stash branch" fails? > + git rev-parse stash@{0} -- > +' > + > test_expect_success 'stash apply shows status same as git status (relative > to current directory)' ' > git stash clear && > echo 1 >subdir/subfile1 &&
[PATCH v4 2/5] stash: Add a test for when apply fails during stash branch
If the return value of merge recurisve is not checked, the stash could end up being dropped even though it was not applied properly Signed-off-by: Joel Teichroeb--- t/t3903-stash.sh | 14 ++ 1 file changed, 14 insertions(+) diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index cc923e6335..5399fb05ca 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -656,6 +656,20 @@ test_expect_success 'stash branch should not drop the stash if the branch exists git rev-parse stash@{0} -- ' +test_expect_success 'stash branch should not drop the stash if the apply fails' ' + git stash clear && + git reset HEAD~1 --hard && + echo foo >file && + git add file && + git commit -m initial && + echo bar >file && + git stash && + echo baz >file && + test_when_finished "git checkout master" && + test_must_fail git stash branch new_branch stash@{0} && + git rev-parse stash@{0} -- +' + test_expect_success 'stash apply shows status same as git status (relative to current directory)' ' git stash clear && echo 1 >subdir/subfile1 && -- 2.13.0