threads / patch / 25271

patchstash drops the stash even if creating the branch fails because it already exists

Subject: [PATCH] stash drops the stash even if creating the branch fails because it already exists

## tl;dr

6 messages between Sep 28, 2010 and Sep 29, 2010. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Tomas Carnecky· Sep 28, 2010, 11:25 UTC · lore

This bug was disovered by someone on IRC when he tried to 'git stash branch <branch> <stash>' while <branch> already existed. In that case the stash is dropped even though it isn't applied on any branch, so the stash is effectively lost. I think that shouldn't happen, so here is a test.

---
 t/t3903-stash.sh |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

Show changes to t/t3903-stash.sh +11 −0

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 9ed2396..0f6b2e4 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -545,4 +545,15 @@ test_expect_success 'invalid ref of the form stash@{n}, n >= N' '
 	git stash drop
 '
 
+test_expect_failure 'stash branch should not drop the stash if the branch exists' '
+	git stash clear &&
+	echo foo > file &&
+	git add file &&
+	git commit -m initial &&
+	echo bar > file &&
+	git stash &&
+	test_must_fail git stash branch master stash@{0} &&
+	git rev-parse stash@{0} --
+'
+
 test_done

-- 
1.7.3.3.gd2416

Jon Seymour· Sep 28, 2010, 13:19 UTC · re: Tomas Carnecky · lore

[PATCH 0/2] stash: test and fix git stash branch regression

This series fixes another (sorry!) regression I introduced with my detached-stash series.

I have included Tomas' test, so that my tweak to the test script applies cleanly.

Jon Seymour (1):
  stash: fix git stash branch regression when branch creation fails

Tomas Carnecky (1):
  stash drops the stash even if creating the branch fails because it
    already exists

 git-stash.sh     |    6 +++---
 t/t3903-stash.sh |   11 +++++++++++
 2 files changed, 14 insertions(+), 3 deletions(-)

-- 
1.7.3.4.g787b.dirty

Jon Seymour· Sep 28, 2010, 13:19 UTC · re: Tomas Carnecky · lore

[PATCH 1/2] stash drops the stash even if creating the branch fails because it already exists

From: Tomas Carnecky <tom@dbservice.com>

This bug was disovered by someone on IRC when he tried to 'git stash branch <branch> <stash>'
while <branch> already existed. In that case the stash is dropped even though it isn't
applied on any branch, so the stash is effectively lost. I think that shouldn't happen,
so here is a test.
---
 t/t3903-stash.sh |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

Show changes to t/t3903-stash.sh +11 −0

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 9ed2396..0f6b2e4 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -545,4 +545,15 @@ test_expect_success 'invalid ref of the form stash@{n}, n >= N' '
 	git stash drop
 '
 
+test_expect_failure 'stash branch should not drop the stash if the branch exists' '
+	git stash clear &&
+	echo foo > file &&
+	git add file &&
+	git commit -m initial &&
+	echo bar > file &&
+	git stash &&
+	test_must_fail git stash branch master stash@{0} &&
+	git rev-parse stash@{0} --
+'
+
 test_done

-- 
1.7.3.4.g787b.dirty

Tomas Carnecky· Sep 28, 2010, 13:21 UTC · re: Jon Seymour · lore

Re: [PATCH 1/2] stash drops the stash even if creating the branch fails because it already exists

On 9/28/10 3:19 PM, Jon Seymour wrote:

Show 6 quoted lines

> From: Tomas Carnecky <tom@dbservice.com>
> 
> This bug was disovered by someone on IRC when he tried to 'git stash branch <branch> <stash>'
> while <branch> already existed. In that case the stash is dropped even though it isn't
> applied on any branch, so the stash is effectively lost. I think that shouldn't happen,
> so here is a test.

This line was missing from my original patch, sorry about that:
Signed-off-by: Tomas Carnecky <tom@dbservice.com>

Show 24 quoted lines

> ---
>  t/t3903-stash.sh |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 9ed2396..0f6b2e4 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -545,4 +545,15 @@ test_expect_success 'invalid ref of the form stash@{n}, n >= N' '
>  	git stash drop
>  '
>  
> +test_expect_failure 'stash branch should not drop the stash if the branch exists' '
> +	git stash clear &&
> +	echo foo > file &&
> +	git add file &&
> +	git commit -m initial &&
> +	echo bar > file &&
> +	git stash &&
> +	test_must_fail git stash branch master stash@{0} &&
> +	git rev-parse stash@{0} --
> +'
> +
>  test_done

Jon Seymour· Sep 29, 2010, 13:53 UTC · re: Tomas Carnecky · lore

Re: [PATCH 1/2] stash drops the stash even if creating the branch fails because it already exists

Junio,

Let me know if you want me to re-roll this series with Tomas' sign-off. I can also add my simplification of the Brian's fix and another fix I have made to have git stash save/create fail early in case the index contains merge conflicts.

jon.

On Tue, Sep 28, 2010 at 11:21 PM, Tomas Carnecky <tom@dbservice.com> wrote:

Show 42 quoted lines

> On 9/28/10 3:19 PM, Jon Seymour wrote:
>> From: Tomas Carnecky <tom@dbservice.com>
>>
>> This bug was disovered by someone on IRC when he tried to 'git stash branch <branch> <stash>'
>> while <branch> already existed. In that case the stash is dropped even though it isn't
>> applied on any branch, so the stash is effectively lost. I think that shouldn't happen,
>> so here is a test.
>
> This line was missing from my original patch, sorry about that:
> Signed-off-by: Tomas Carnecky <tom@dbservice.com>
>
>> ---
>>  t/t3903-stash.sh |   11 +++++++++++
>>  1 files changed, 11 insertions(+), 0 deletions(-)
>>
>> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
>> index 9ed2396..0f6b2e4 100755
>> --- a/t/t3903-stash.sh
>> +++ b/t/t3903-stash.sh
>> @@ -545,4 +545,15 @@ test_expect_success 'invalid ref of the form stash@{n}, n >= N' '
>>       git stash drop
>>  '
>>
>> +test_expect_failure 'stash branch should not drop the stash if the branch exists' '
>> +     git stash clear &&
>> +     echo foo > file &&
>> +     git add file &&
>> +     git commit -m initial &&
>> +     echo bar > file &&
>> +     git stash &&
>> +     test_must_fail git stash branch master stash@{0} &&
>> +     git rev-parse stash@{0} --
>> +'
>> +
>>  test_done
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Jon Seymour· Sep 28, 2010, 13:19 UTC · re: Tomas Carnecky · lore

[PATCH 2/2] stash: fix git stash branch regression when branch creation fails

Tomas Carnecky reported a regression in the behaviour of git stash branch, when the branch creation fails.

This patch fixes that regression by restoring the pre-condition for dropping that previously existed.

This patch assumes Tomas Carnecky's patch has already been applied.

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 git-stash.sh     |    6 +++---
 t/t3903-stash.sh |    2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

Show changes to 2 files +4 −4

git-stash.sh, t/t3903-stash.sh

diff --git a/git-stash.sh b/git-stash.sh
index 23a9ab5..5fb1245 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -432,9 +432,9 @@ apply_to_branch () {
 	assert_stash_like "$@"
 
 	git checkout -b $branch $REV^ &&
-	apply_stash "$@"
-
-	test -z "$IS_STASH_REF" || drop_stash "$@"
+	apply_stash "$@" && {
+		test -z "$IS_STASH_REF" || drop_stash "$@"
+	}
 }
 
 PARSE_CACHE='--not-parsed'
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 0f6b2e4..336e244 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -545,7 +545,7 @@ test_expect_success 'invalid ref of the form stash@{n}, n >= N' '
 	git stash drop
 '
 
-test_expect_failure 'stash branch should not drop the stash if the branch exists' '
+test_expect_success 'stash branch should not drop the stash if the branch exists' '
 	git stash clear &&
 	echo foo > file &&
 	git add file &&

-- 
1.7.3.4.g787b.dirty