[PATCH v5 4/8] detached-stash: refactor git stash pop implementation

[PATCH v5 4/8] detached-stash: refactor git stash pop implementation

[Jon Seymour]

git stash pop is abstracted into its own implementation function - pop_stash.

The behaviour is changed so that git stash pop fails early if the
the specified stash reference does not exist or does not refer to
an extant entry in the reflog of the reference stash.

This fixes the case where the apply succeeds, but the drop fails.
Previously this caused caused git stash pop to exit with a non-zero exit code
and a dirty tree.

Now, git stash pop fails with a non-zero exit code, but the working
tree is not modified.

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 git-stash.sh |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/git-stash.sh b/git-stash.sh
index 750f360..ac4c0f6 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -423,6 +423,13 @@ apply_stash () {
 	fi
 }
 
+pop_stash() {
+	assert_stash_ref "$@"
+
+	apply_stash "$@" &&
+	drop_stash "$@"
+}
+
 drop_stash () {
 	assert_stash_ref "$@"
 
@@ -498,10 +505,7 @@ drop)
 	;;
 pop)
 	shift
-	if apply_stash "$@"
-	then
-		drop_stash "$applied_stash"
-	fi
+	pop_stash "$@"
 	;;
 branch)
 	shift
-- 
1.7.2.1.95.g4fabf

[PATCH v5 5/8] detached-stash: simplify git stash branch

[Jon Seymour]

This patch teaches git stash branch to tolerate stash-like arguments.

In particular, a stash is only required if an argument isn't specified
and the stash is only dropped if a stash entry reference was
specified or implied.

The implementation has been simplified by taking advantage of
assert_stash_like() and the variables established by
parse_flags_and_rev().

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 git-stash.sh |   17 +++++++----------
 1 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/git-stash.sh b/git-stash.sh
index ac4c0f6..ff1edc9 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -441,20 +441,17 @@ drop_stash () {
 }
 
 apply_to_branch () {
-	have_stash || die 'Nothing to apply'
-
 	test -n "$1" || die 'No branch name specified'
 	branch=$1
+	shift 1
 
-	if test -z "$2"
-	then
-		set x "$ref_stash@{0}"
-	fi
-	stash=$2
+	set -- --index "$@"
+	assert_stash_like "$@"
+
+	git checkout -b $branch $REV^ &&
+	apply_stash "$@"
 
-	git checkout -b $branch $stash^ &&
-	apply_stash --index $stash &&
-	drop_stash $stash
+	test -z "$IS_STASH_REF" || drop_stash "$@"
 }
 
 PARSE_CACHE='--not-parsed'
-- 
1.7.2.1.95.g4fabf

[PATCH v5 6/8] detached-stash: simplify git stash show

[Jon Seymour]

This commit refactors git stash show to make use of the assert_stash_like function.

git show now dies if the presented argument is non-stash-like.

Previous behaviour was to tolerate commits that were not even stash-like.

Previously, git stash show would accept stash-like arguments, but
only if there was a stash on the stack.

Now, git stash accepts stash-like arguments always and only fails
if no stash-like argument is specified and there is no stash stack.

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 git-stash.sh |   14 ++------------
 1 files changed, 2 insertions(+), 12 deletions(-)

diff --git a/git-stash.sh b/git-stash.sh
index ff1edc9..7ce818b 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -210,19 +210,9 @@ list_stash () {
 }
 
 show_stash () {
-	have_stash || die 'No stash found'
-
-	flags=$(git rev-parse --no-revs --flags "$@")
-	if test -z "$flags"
-	then
-		flags=--stat
-	fi
-
-	w_commit=$(git rev-parse --quiet --verify --default $ref_stash "$@") &&
-	b_commit=$(git rev-parse --quiet --verify "$w_commit^") ||
-		die "'$*' is not a stash"
+	assert_stash_like "$@"
 
-	git diff $flags $b_commit $w_commit
+	git diff ${FLAGS:---stat} $b_commit $w_commit
 }
 
 #
-- 
1.7.2.1.95.g4fabf

[PATCH v5 7/8] detached-stash: tests of git stash with stash-like arguments

[Jon Seymour]

Adds new tests which check that:
* git stash branch handles a stash-like argument when there is a stash stack
* git stash branch handles a stash-like argument when there is not a stash stack
* git stash show handles a stash-like argument when there is a stash stack
* git stash show handles a stash-like argument when there is not a stash stack
* git stash drop fails early if the specified argument is not a stash reference
* git stash pop fails early if the specified argument is not a stash reference
* git stash * fails early if the reference supplied is bogus
* git stash fails early with stash@{n} where n >= length of stash log

| Amended per advice from Johannes Sixt to avoid burying stash create failures.

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t3903-stash.sh |  140 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 140 insertions(+), 0 deletions(-)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 62e208a..efc7e7f 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -378,4 +378,144 @@ test_expect_failure 'stash file to directory' '
 	test foo = "$(cat file/file)"
 '
 
+test_expect_success 'stash branch - no stashes on stack, stash-like argument' '
+	git stash clear &&
+	test_when_finished "git reset --hard HEAD" &&
+	git reset --hard &&
+	echo foo >> file &&
+	STASH_ID=$(git stash create) &&
+	git reset --hard &&
+	git stash branch stash-branch ${STASH_ID} &&
+	test_when_finished "git reset --hard HEAD && git checkout master && git branch -D stash-branch" &&
+	test $(git ls-files --modified | wc -l) -eq 1
+'
+
+test_expect_success 'stash branch - stashes on stack, stash-like argument' '
+	git stash clear &&
+	test_when_finished "git reset --hard HEAD" &&
+	git reset --hard &&
+	echo foo >> file &&
+	git stash &&
+	test_when_finished "git stash drop" &&
+	echo bar >> file &&
+	STASH_ID=$(git stash create) &&
+	git reset --hard &&
+	git stash branch stash-branch ${STASH_ID} &&
+	test_when_finished "git reset --hard HEAD && git checkout master && git branch -D stash-branch" &&
+	test $(git ls-files --modified | wc -l) -eq 1
+'
+
+test_expect_success 'stash show - stashes on stack, stash-like argument' '
+	git stash clear &&
+	test_when_finished "git reset --hard HEAD" &&
+	git reset --hard &&
+	echo foo >> file &&
+	git stash &&
+	test_when_finished "git stash drop" &&
+	echo bar >> file &&
+	STASH_ID=$(git stash create) &&
+	git reset --hard &&
+	git stash show ${STASH_ID}
+'
+test_expect_success 'stash show - no stashes on stack, stash-like argument' '
+	git stash clear &&
+	test_when_finished "git reset --hard HEAD" &&
+	git reset --hard &&
+	echo foo >> file &&
+	STASH_ID=$(git stash create) &&
+	git reset --hard &&
+	git stash show ${STASH_ID}
+'
+
+test_expect_success 'stash drop - fail early if specified stash is not a stash reference' '
+	git stash clear &&
+	test_when_finished "git reset --hard HEAD && git stash clear" &&
+	git reset --hard &&
+	echo foo > file &&
+	git stash &&
+	echo bar > file &&
+	git stash &&
+	! git stash drop $(git rev-parse stash@{0}) &&
+	git stash pop &&
+	test bar = "$(cat file)" &&
+	git reset --hard HEAD
+'
+
+test_expect_success 'stash pop - fail early if specified stash is not a stash reference' '
+	git stash clear &&
+	test_when_finished "git reset --hard HEAD && git stash clear" &&
+	git reset --hard &&
+	echo foo > file &&
+	git stash &&
+	echo bar > file &&
+	git stash &&
+	! git stash pop $(git rev-parse stash@{0}) &&
+	git stash pop &&
+	test bar = "$(cat file)" &&
+	git reset --hard HEAD
+'
+
+test_expect_success 'stash drop - bad stash ref' '
+	git stash clear &&
+	echo bar5 > file &&
+	echo bar6 > file2 &&
+	git add file2 &&
+	git stash &&
+	! git rev-parse --quiet --verify does-not-exist &&
+	! git stash drop does-not-exist &&
+	! git rev-parse --quiet --verify does-not-exist &&
+	! git stash drop does-not-exist@{0} &&
+	git stash drop
+'
+
+test_expect_success 'stash pop - bad stash ref' '
+	git stash clear &&
+	echo bar5 > file &&
+	echo bar6 > file2 &&
+	git add file2 &&
+	git stash &&
+	! git rev-parse --quiet --verify does-not-exist &&
+	! git stash pop does-not-exist &&
+	! git rev-parse --quiet --verify does-not-exist &&
+	! git stash pop does-not-exist@{0} &&
+	git stash drop
+'
+
+test_expect_success 'stash apply - bad stash ref' '
+	git stash clear &&
+	echo bar5 > file &&
+	echo bar6 > file2 &&
+	git add file2 &&
+	git stash &&
+	! git rev-parse --quiet --verify does-not-exist &&
+	! git stash apply does-not-exist &&
+	! git rev-parse --quiet --verify does-not-exist &&
+	! git stash apply does-not-exist@{0} &&
+	git stash drop
+'
+
+test_expect_success 'stash show - bad stash ref' '
+	git stash clear &&
+	echo bar5 > file &&
+	echo bar6 > file2 &&
+	git add file2 &&
+	git stash &&
+	! git rev-parse --quiet --verify does-not-exist &&
+	! git stash show does-not-exist &&
+	! git rev-parse --quiet --verify does-not-exist &&
+	! git stash show does-not-exist@{0} &&
+	git stash drop
+'
+
+test_expect_success 'stash drop - invalid stash reference' '
+	git stash clear &&
+	! git stash stash@{0} &&
+	echo bar5 > file &&
+	echo bar6 > file2 &&
+	git add file2 &&
+	git stash &&
+	! git drop stash@{1} &&
+	git stash drop
+'
+
 test_done
-- 
1.7.2.1.95.g4fabf

Re: [PATCH v5 7/8] detached-stash: tests of git stash with stash-like arguments

[Jon Seymour]

I'll need to re-issue this patch. The later tests execute
'successfully', but don't properly implement the intent. In
particular, the condition:

! git stash stash@{0}

should really be:

! git stash drop stash@{0}.

Will re-roll a version 6, perhaps with a variant of Junio's git
rev-parse tweak...

jon.

On Wed, Aug 18, 2010 at 11:48 PM, Jon Seymour <jon.seymour@gmail.com> wrote:
> Adds new tests which check that:
> * git stash branch handles a stash-like argument when there is a stash stack
> * git stash branch handles a stash-like argument when there is not a stash stack
> * git stash show handles a stash-like argument when there is a stash stack
> * git stash show handles a stash-like argument when there is not a stash stack
> * git stash drop fails early if the specified argument is not a stash reference
> * git stash pop fails early if the specified argument is not a stash reference
> * git stash * fails early if the reference supplied is bogus
> * git stash fails early with stash@{n} where n >= length of stash log
>
> | Amended per advice from Johannes Sixt to avoid burying stash create failures.
>
> Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  t/t3903-stash.sh |  140 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 140 insertions(+), 0 deletions(-)
>
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 62e208a..efc7e7f 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -378,4 +378,144 @@ test_expect_failure 'stash file to directory' '
>        test foo = "$(cat file/file)"
>  '
>
> +test_expect_success 'stash branch - no stashes on stack, stash-like argument' '
> +       git stash clear &&
> +       test_when_finished "git reset --hard HEAD" &&
> +       git reset --hard &&
> +       echo foo >> file &&
> +       STASH_ID=$(git stash create) &&
> +       git reset --hard &&
> +       git stash branch stash-branch ${STASH_ID} &&
> +       test_when_finished "git reset --hard HEAD && git checkout master && git branch -D stash-branch" &&
> +       test $(git ls-files --modified | wc -l) -eq 1
> +'
> +
> +test_expect_success 'stash branch - stashes on stack, stash-like argument' '
> +       git stash clear &&
> +       test_when_finished "git reset --hard HEAD" &&
> +       git reset --hard &&
> +       echo foo >> file &&
> +       git stash &&
> +       test_when_finished "git stash drop" &&
> +       echo bar >> file &&
> +       STASH_ID=$(git stash create) &&
> +       git reset --hard &&
> +       git stash branch stash-branch ${STASH_ID} &&
> +       test_when_finished "git reset --hard HEAD && git checkout master && git branch -D stash-branch" &&
> +       test $(git ls-files --modified | wc -l) -eq 1
> +'
> +
> +test_expect_success 'stash show - stashes on stack, stash-like argument' '
> +       git stash clear &&
> +       test_when_finished "git reset --hard HEAD" &&
> +       git reset --hard &&
> +       echo foo >> file &&
> +       git stash &&
> +       test_when_finished "git stash drop" &&
> +       echo bar >> file &&
> +       STASH_ID=$(git stash create) &&
> +       git reset --hard &&
> +       git stash show ${STASH_ID}
> +'
> +test_expect_success 'stash show - no stashes on stack, stash-like argument' '
> +       git stash clear &&
> +       test_when_finished "git reset --hard HEAD" &&
> +       git reset --hard &&
> +       echo foo >> file &&
> +       STASH_ID=$(git stash create) &&
> +       git reset --hard &&
> +       git stash show ${STASH_ID}
> +'
> +
> +test_expect_success 'stash drop - fail early if specified stash is not a stash reference' '
> +       git stash clear &&
> +       test_when_finished "git reset --hard HEAD && git stash clear" &&
> +       git reset --hard &&
> +       echo foo > file &&
> +       git stash &&
> +       echo bar > file &&
> +       git stash &&
> +       ! git stash drop $(git rev-parse stash@{0}) &&
> +       git stash pop &&
> +       test bar = "$(cat file)" &&
> +       git reset --hard HEAD
> +'
> +
> +test_expect_success 'stash pop - fail early if specified stash is not a stash reference' '
> +       git stash clear &&
> +       test_when_finished "git reset --hard HEAD && git stash clear" &&
> +       git reset --hard &&
> +       echo foo > file &&
> +       git stash &&
> +       echo bar > file &&
> +       git stash &&
> +       ! git stash pop $(git rev-parse stash@{0}) &&
> +       git stash pop &&
> +       test bar = "$(cat file)" &&
> +       git reset --hard HEAD
> +'
> +
> +test_expect_success 'stash drop - bad stash ref' '
> +       git stash clear &&
> +       echo bar5 > file &&
> +       echo bar6 > file2 &&
> +       git add file2 &&
> +       git stash &&
> +       ! git rev-parse --quiet --verify does-not-exist &&
> +       ! git stash drop does-not-exist &&
> +       ! git rev-parse --quiet --verify does-not-exist &&
> +       ! git stash drop does-not-exist@{0} &&
> +       git stash drop
> +'
> +
> +test_expect_success 'stash pop - bad stash ref' '
> +       git stash clear &&
> +       echo bar5 > file &&
> +       echo bar6 > file2 &&
> +       git add file2 &&
> +       git stash &&
> +       ! git rev-parse --quiet --verify does-not-exist &&
> +       ! git stash pop does-not-exist &&
> +       ! git rev-parse --quiet --verify does-not-exist &&
> +       ! git stash pop does-not-exist@{0} &&
> +       git stash drop
> +'
> +
> +test_expect_success 'stash apply - bad stash ref' '
> +       git stash clear &&
> +       echo bar5 > file &&
> +       echo bar6 > file2 &&
> +       git add file2 &&
> +       git stash &&
> +       ! git rev-parse --quiet --verify does-not-exist &&
> +       ! git stash apply does-not-exist &&
> +       ! git rev-parse --quiet --verify does-not-exist &&
> +       ! git stash apply does-not-exist@{0} &&
> +       git stash drop
> +'
> +
> +test_expect_success 'stash show - bad stash ref' '
> +       git stash clear &&
> +       echo bar5 > file &&
> +       echo bar6 > file2 &&
> +       git add file2 &&
> +       git stash &&
> +       ! git rev-parse --quiet --verify does-not-exist &&
> +       ! git stash show does-not-exist &&
> +       ! git rev-parse --quiet --verify does-not-exist &&
> +       ! git stash show does-not-exist@{0} &&
> +       git stash drop
> +'
> +
> +test_expect_success 'stash drop - invalid stash reference' '
> +       git stash clear &&
> +       ! git stash stash@{0} &&
> +       echo bar5 > file &&
> +       echo bar6 > file2 &&
> +       git add file2 &&
> +       git stash &&
> +       ! git drop stash@{1} &&
> +       git stash drop
> +'
> +
>  test_done
> --
> 1.7.2.1.95.g4fabf
>
>

Re: [PATCH v5 7/8] detached-stash: tests of git stash with stash-like arguments

[Johannes Sixt]

On Donnerstag, 19. August 2010, Jon Seymour wrote:
> I'll need to re-issue this patch. The later tests execute
> 'successfully', but don't properly implement the intent. In
> particular, the condition:
>
> ! git stash stash@{0}
>
> should really be:
>
> ! git stash drop stash@{0}.

Better write this as

  test_must_fail git stash drop stash@{0}

-- Hannes

[PATCH v5 8/8] detached-stash: update Documentation

[Jon Seymour]

Update the documentation to indicate that git stash branch only attempts
to drop the specified stash if it looks like stash reference.

Also changed the synopsis to more clearly indicate which commands require
a stash entry reference as opposed to merely a stash-like commit.

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 Documentation/git-stash.txt |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 473889a..8728f7a 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -104,18 +104,22 @@ tree's changes, but also the index's ones. However, this can fail, when you
 have conflicts (which are stored in the index, where you therefore can no
 longer apply the changes as they were originally).
 +
-When no `<stash>` is given, `stash@\{0}` is assumed.
+When no `<stash>` is given, `stash@\{0}` is assumed, otherwise `<stash>` must
+be a reference of the form `stash@\{<revision>}`.
 
 apply [--index] [-q|--quiet] [<stash>]::
 
-	Like `pop`, but do not remove the state from the stash list.
+	Like `pop`, but do not remove the state from the stash list. Unlike `pop`,
+	`<stash>` may be any commit that looks like a commit created by
+	`stash save` or `stash create`.
 
 branch <branchname> [<stash>]::
 
 	Creates and checks out a new branch named `<branchname>` starting from
 	the commit at which the `<stash>` was originally created, applies the
-	changes recorded in `<stash>` to the new working tree and index, then
-	drops the `<stash>` if that completes successfully. When no `<stash>`
+	changes recorded in `<stash>` to the new working tree and index.
+	If that succeeds, and `<stash>` is a reference of the form
+	`stash@{<revision>}`, it then drops the `<stash>`. When no `<stash>`
 	is given, applies the latest one.
 +
 This is useful if the branch on which you ran `git stash save` has
@@ -132,7 +136,9 @@ clear::
 drop [-q|--quiet] [<stash>]::
 
 	Remove a single stashed state from the stash list. When no `<stash>`
-	is given, it removes the latest one. i.e. `stash@\{0}`
+	is given, it removes the latest one. i.e. `stash@\{0}`, otherwise
+	`<stash>` must a valid stash log reference of the form
+	`stash@\{<revision>}`.
 
 create::
 
-- 
1.7.2.1.95.g4fabf