threads / patch / 24665

v3, 9 partsstash: modifiy handling of stash-like references

Subject: [PATCH v3 0/9] stash: modifiy handling of stash-like references

## tl;dr

30 messages between Aug 8, 2010 and Aug 14, 2010. Diffs are folded; open one to read it.

replies: 29people: 3as markdown or json

Jon Seymour· Aug 8, 2010, 04:46 UTC · lore

This series teaches git stash branch and git stash show to be more tolerant of stash-like references that are not actually stash entry references, while teaching git stash pop and git stash drop to be less tolerant of stash-like references that are not actually stash entry references.

Junio: I did not adopt your suggestion to make git stash drop more tolerant
of invalid stash references since failing early might help a user
to spot an incorrect assumption about the state of their working tree
and stash stack - silently handling this case might hide such an incorrect
assumption. However, if you would prefer git stash drop to behave that
way, I'll rework the series accordingly.

The first 4 commits refactor the existing code without significantly changing existing behaviour.

The next 4 commits modify existing behaviour to be more (or less) tolerant of stash-like references, as required.

The last 2 commits update the tests and documentation to reflect the modified behaviour.

Jon Seymour (9):
  stash: refactor - create pop_stash function
  stash: extract stash-like check into its own function
  stash: introduce is_stash_ref and assert_stash_ref functions.
  stash: teach git stash pop to fail early if the argument is not a
    stash ref
  stash: teach git stash drop to fail early if the specified revision
    is not a stash reference
  stash: teach git stash branch to tolerate stash-like arguments
  stash: teach git stash show to always tolerate stash-like arguments.
  t3903-stash.sh: tests of git stash with stash-like arguments
  Documentation: git stash branch now tolerates non-stash references.

 Documentation/git-stash.txt |    4 +-
 git-stash.sh                |   92 +++++++++++++++++++++++++++++++------------
 t/t3903-stash.sh            |   81 +++++++++++++++++++++++++++++++++++++
 3 files changed, 149 insertions(+), 28 deletions(-)

-- 
1.7.2.1.51.g82c0c0

Jon Seymour· Aug 8, 2010, 04:46 UTC · re: Jon Seymour · lore

[PATCH v3 1/9] stash: refactor - create pop_stash function

We plan to edit pop_stash later, so let's make it a first class command by giving its own implementation function.

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

Show changes to git-stash.sh +8 −4

diff --git a/git-stash.sh b/git-stash.sh
index 1d95447..31ea333 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -323,6 +323,13 @@ apply_stash () {
 	fi
 }
 
+pop_stash() {
+	if apply_stash "$@"
+	then
+		drop_stash "$applied_stash"
+	fi
+}
+
 drop_stash () {
 	have_stash || die 'No stash entries to drop'
 
@@ -422,10 +429,7 @@ drop)
 	;;
 pop)
 	shift
-	if apply_stash "$@"
-	then
-		drop_stash "$applied_stash"
-	fi
+	pop_stash "$@"
 	;;
 branch)
 	shift

-- 
1.7.2.1.51.g82c0c0

Jon Seymour· Aug 8, 2010, 04:46 UTC · re: Jon Seymour · lore

[PATCH v3 2/9] stash: extract stash-like check into its own function

A similar code block is used in three places; this change factors the stash-like check into a separate function so that it can be used in these and other places.

Note to reviewers: The existing code in apply_stash appears to assume w_tree, b_tree and i_tree are all trees as opposed to tree-ish (which is what they actually are).

This refactoring does not change the existing derivation, but one wonders whether this difference might be subverting the intent of the test which compares c_tree with i_tree.

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

Show changes to git-stash.sh +30 −16

diff --git a/git-stash.sh b/git-stash.sh
index 31ea333..dbb7944 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -218,13 +218,30 @@ show_stash () {
 		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
 }
 
+#
+# if this function returns, then:
+#   s is set to the stash commit
+#   w_commit is set to the commit containing the working tree
+#   b_commit is set to the base commit
+#   i_commit is set to the commit containing the index tree
+# otherwise:
+#   the function exits via die
+#
+assert_stash_like() {
+	# stash records the work tree, and is a merge between the
+	# base commit (first parent) and the index tree (second parent).
+	s=$(git rev-parse --revs-only --quiet --verify --default $ref_stash "$@") &&
+	w_commit=$(git rev-parse --quiet --verify "$s:") &&
+	b_commit=$(git rev-parse --quiet --verify "$s^1:") &&
+	i_commit=$(git rev-parse --quiet --verify "$s^2:") ||
+		die "$*: no valid stashed state found"
+}
+
 apply_stash () {
 	applied_stash=
 	unstash_index=
@@ -253,13 +270,11 @@ apply_stash () {
 		applied_stash="$*"
 	fi
 
-	# stash records the work tree, and is a merge between the
-	# base commit (first parent) and the index tree (second parent).
-	s=$(git rev-parse --quiet --verify --default $ref_stash "$@") &&
-	w_tree=$(git rev-parse --quiet --verify "$s:") &&
-	b_tree=$(git rev-parse --quiet --verify "$s^1:") &&
-	i_tree=$(git rev-parse --quiet --verify "$s^2:") ||
-		die "$*: no valid stashed state found"
+	assert_stash_like "$@"
+
+	b_tree=$b_commit
+	w_tree=$w_commit
+	i_tree=$i_commit
 
 	git update-index -q --refresh &&
 	git diff-files --quiet --ignore-submodules ||
@@ -270,6 +285,9 @@ apply_stash () {
 		die 'Cannot apply a stash in the middle of a merge'
 
 	unstashed_index_tree=
+	#
+	# note to reviewers: comparing $c_tree to $i_tree seems unsound, since i_tree is only tree-ish
+	#
 	if test -n "$unstash_index" && test "$b_tree" != "$i_tree" &&
 			test "$c_tree" != "$i_tree"
 	then
@@ -351,12 +369,8 @@ drop_stash () {
 		set x "$ref_stash@{0}"
 		shift
 	fi
-	# Verify supplied argument looks like a stash entry
-	s=$(git rev-parse --verify "$@") &&
-	git rev-parse --verify "$s:"   > /dev/null 2>&1 &&
-	git rev-parse --verify "$s^1:" > /dev/null 2>&1 &&
-	git rev-parse --verify "$s^2:" > /dev/null 2>&1 ||
-		die "$*: not a valid stashed state"
+
+	assert_stash_like "$@"
 
 	git reflog delete --updateref --rewrite "$@" &&
 		say "Dropped $* ($s)" || die "$*: Could not drop stash entry"

-- 
1.7.2.1.51.g82c0c0

Jon Seymour· Aug 8, 2010, 04:46 UTC · re: Jon Seymour · lore

[PATCH v3 3/9] stash: introduce is_stash_ref and assert_stash_ref functions.

Introduce a function, is_stash_ref, which checks whether an argument is valid stash reference (e.g. is of the form ['refs/']stash['@{'something'}])

Introduce a function, assert_stash_ref, which can be used to ensure that certain commands are only called with stash references as arguments.

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

Show changes to git-stash.sh +9 −0

diff --git a/git-stash.sh b/git-stash.sh
index dbb7944..41a9359 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -223,6 +223,15 @@ show_stash () {
 	git diff $flags $b_commit $w_commit
 }
 
+is_stash_ref() {
+	revs="$(git rev-parse --revs-only --symbolic --verify --default ${ref_stash} "$@")" &&
+	test "$ref_stash" = "$(git rev-parse --symbolic-full-name "${revs%@*}")"
+}
+
+assert_stash_ref() {
+	is_stash_ref "$@" || die "$* is not a stash reference"
+}
+
 #
 # if this function returns, then:
 #   s is set to the stash commit

-- 
1.7.2.1.51.g82c0c0

Jon Seymour· Aug 8, 2010, 04:46 UTC · re: Jon Seymour · lore

[PATCH v3 4/9] stash: teach git stash pop to fail early if the argument is not a stash ref

With this change, git stash pop will fail early if a specified revision argument is not a valid stash reference.

This fixes the case where the apply succeeds, but the drop fails which 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 tree is not modified.

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

Show changes to git-stash.sh +4 −4

diff --git a/git-stash.sh b/git-stash.sh
index 41a9359..4f28c09 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -351,10 +351,10 @@ apply_stash () {
 }
 
 pop_stash() {
-	if apply_stash "$@"
-	then
-		drop_stash "$applied_stash"
-	fi
+	assert_stash_ref "$@"
+
+	apply_stash "$@" &&
+	drop_stash "$applied_stash"
 }
 
 drop_stash () {

-- 
1.7.2.1.51.g82c0c0

Jon Seymour· Aug 8, 2010, 04:46 UTC · re: Jon Seymour · lore

[PATCH v3 5/9] stash: teach git stash drop to fail early if the specified revision is not a stash reference

Previously, git stash drop would fail noisily while executing git reflog delete if the specified revision argument was not a stash reference.

Now, git stash drop fails with an error message which more precisely indicates the reason for failure.

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

Show changes to git-stash.sh +1 −0

diff --git a/git-stash.sh b/git-stash.sh
index 4f28c09..86cd446 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -379,6 +379,7 @@ drop_stash () {
 		shift
 	fi
 
+	assert_stash_ref "$@"
 	assert_stash_like "$@"
 
 	git reflog delete --updateref --rewrite "$@" &&

-- 
1.7.2.1.51.g82c0c0

Jon Seymour· Aug 8, 2010, 04:46 UTC · re: Jon Seymour · lore

[PATCH v3 6/9] stash: teach git stash branch to tolerate stash-like arguments

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.

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

Show changes to git-stash.sh +11 −4

diff --git a/git-stash.sh b/git-stash.sh
index 86cd446..97e3749 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -390,20 +390,27 @@ drop_stash () {
 }
 
 apply_to_branch () {
-	have_stash || die 'Nothing to apply'
-
 	test -n "$1" || die 'No branch name specified'
 	branch=$1
 
 	if test -z "$2"
 	then
 		set x "$ref_stash@{0}"
+		have_stash || die 'Nothing to apply'
 	fi
 	stash=$2
 
+	assert_stash_like "$stash"
+
 	git checkout -b $branch $stash^ &&
-	apply_stash --index $stash &&
-	drop_stash $stash
+	apply_stash --index $stash
+
+	if is_stash_ref "$stash"
+	then
+		drop_stash "$stash"
+	else
+		true
+	fi
 }
 
 # The default command is "save" if nothing but options are given

-- 
1.7.2.1.51.g82c0c0

Jon Seymour· Aug 8, 2010, 04:46 UTC · re: Jon Seymour · lore

[PATCH v3 7/9] stash: teach git stash show to always tolerate stash-like arguments.

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 |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

Show changes to git-stash.sh +7 −2

diff --git a/git-stash.sh b/git-stash.sh
index 97e3749..5b875ea 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -210,14 +210,19 @@ 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
 
+	revs=$(git rev-parse --revs-only "$@")
+	if test -z "$revs"
+	then
+		have_stash || die "No stash found"
+		set -- ${ref_stash}@{0}
+	fi
+
 	assert_stash_like "$@"
 
 	git diff $flags $b_commit $w_commit

-- 
1.7.2.1.51.g82c0c0

Jon Seymour· Aug 8, 2010, 04:46 UTC · re: Jon Seymour · lore

[PATCH v3 8/9] t3903-stash.sh: tests of git stash with stash-like arguments

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

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

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

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 62e208a..4910409 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -378,4 +378,85 @@ 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 &&
+	git tag stash-tag $(git stash create) &&
+	test_when_finished "git tag -d stash-tag" &&
+	git reset --hard &&
+	git stash branch stash-branch $(git rev-parse stash-tag) &&
+	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 &&
+	git tag stash-tag $(git stash create) &&
+	test_when_finished "git tag -d stash-tag" &&
+	git reset --hard &&
+	git stash branch stash-branch $(git rev-parse stash-tag) &&
+	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 &&
+	git tag stash-tag $(git stash create) &&
+	test_when_finished "git tag -d stash-tag" &&
+	git reset --hard &&
+	git stash show stash-tag
+'
+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 &&
+	git tag stash-tag $(git stash create) &&
+	test_when_finished "git tag -d stash-tag" &&
+	git reset --hard &&
+	git stash show stash-tag
+'
+
+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_done

-- 
1.7.2.1.51.g82c0c0

Johannes Sixt· Aug 8, 2010, 09:32 UTC · re: Jon Seymour · lore

Re: [PATCH v3 8/9] t3903-stash.sh: tests of git stash with stash-like arguments

On Sonntag, 8. August 2010, Jon Seymour wrote:

Show 6 quoted lines

> +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 &&
> +	git tag stash-tag $(git stash create) &&

To catch failures in $(git stash create), you should rewrite lines like this one as:

	stashid=$(git stash create) &&
	git tag stash-tag $stashid &&

-- Hannes

Jon Seymour· Aug 8, 2010, 12:56 UTC · re: Johannes Sixt · lore

Re: [PATCH v3 8/9] t3903-stash.sh: tests of git stash with stash-like arguments

On Sun, Aug 8, 2010 at 7:32 PM, Johannes Sixt <j6t@kdbg.org> wrote:

Show 7 quoted lines

> On Sonntag, 8. August 2010, Jon Seymour wrote:
> To catch failures in $(git stash create), you should rewrite lines like this
> one as:
>
>        stashid=$(git stash create) &&
>        git tag stash-tag $stashid &&
>

Good point. I've replaced the use of git tag with a variable assignment and dereference and will repost the series with these changes in a day or two.

Regards,

jon.

Jon Seymour· Aug 8, 2010, 04:46 UTC · re: Jon Seymour · lore

[PATCH v3 9/9] Documentation: git stash branch now tolerates non-stash references.

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

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

Show changes to Documentation/git-stash.txt +2 −2

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 473889a..c7b7e4e 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -115,8 +115,8 @@ 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>`
-	is given, applies the latest one.
+	drops the `<stash>` if that completes successfully and `<stash>` looks like a
+	stash the name of a stash list entry. When no `<stash>` is given, applies the latest one.
 +
 This is useful if the branch on which you ran `git stash save` has
 changed enough that `git stash apply` fails due to conflicts. Since

-- 
1.7.2.1.51.g82c0c0

Jon Seymour· Aug 8, 2010, 13:18 UTC · re: Jon Seymour · lore

Re: [PATCH v3 9/9] Documentation: git stash branch now tolerates non-stash references.

The next iteration will amend the synopsis as follows:

-'git stash' show [<stash>] -'git stash' drop [-q|--quiet] [<stash>] -'git stash' ( pop | apply ) [--index] [-q|--quiet] [<stash>] -'git stash' branch <branchname> [<stash>] +'git stash' show [<stash-like>] +'git stash' drop [-q|--quiet] [<stash-entry>] +'git stash' pop [--index] [-q|--quiet] [<stash-entry>] +'git stash' apply [--index] [-q|--quiet] [<stash-like>] +'git stash' branch <branchname> [<stash-like>]

with corresponding changes to the body. This will help clarify when a stash entry is required and when a stash-like commit can be used instead.

On Sun, Aug 8, 2010 at 2:46 PM, Jon Seymour <jon.seymour@gmail.com> wrote:

Show 27 quoted lines

> Update the documentation to indicate that git stash branch only attempts
> to drop the specified stash if it looks like stash reference.
>
> Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
> ---
>  Documentation/git-stash.txt |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
> index 473889a..c7b7e4e 100644
> --- a/Documentation/git-stash.txt
> +++ b/Documentation/git-stash.txt
> @@ -115,8 +115,8 @@ 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>`
> -       is given, applies the latest one.
> +       drops the `<stash>` if that completes successfully and `<stash>` looks like a
> +       stash the name of a stash list entry. When no `<stash>` is given, applies the latest one.
>  +
>  This is useful if the branch on which you ran `git stash save` has
>  changed enough that `git stash apply` fails due to conflicts. Since
> --
> 1.7.2.1.51.g82c0c0
>
>

Jon Seymour· Aug 10, 2010, 00:04 UTC · re: Jon Seymour · lore

[PATCH v4 0/9] stash: modifiy handling of stash-like references

Junio: I did not adopt your suggestion to make git stash drop more tolerant
of invalid stash references since failing early might help a user
to spot an incorrect assumption about the state of their working tree
and stash stack - silently handling this case might hide such an incorrect
assumption. However, if you would prefer git stash drop to behave that
way, I'll rework the series accordingly.

The first 3 commits refactor the existing code without significantly changing existing behaviour.

The next 4 commits modify existing behaviour to be more (or less) tolerant of stash-like references, as required.

The last 2 commits update the tests and documentation to reflect the modified behaviour.

Thanks to Johannes Sixt for suggesting improvements to the tests.

This series in unchanged from v3 except for patch 8 which improves the tests and patch 9 which enhances the documentation to indicate which commands require a stash entry reference and which are happy to use just a stash-like commit.

Jon Seymour (9):
  stash: refactor - create pop_stash function
  stash: extract stash-like check into its own function
  stash: introduce is_stash_ref and assert_stash_ref functions.
  stash: teach git stash pop to fail early if the argument is not a
    stash ref
  stash: teach git stash drop to fail early if the specified revision
    is not a stash reference
  stash: teach git stash branch to tolerate stash-like arguments
  stash: teach git stash show to always tolerate stash-like arguments.
  t3903-stash.sh: tests of git stash with stash-like arguments
  Documentation: git stash branch now tolerates non-stash references.

 Documentation/git-stash.txt |   37 +++++++++--------
 git-stash.sh                |   92 +++++++++++++++++++++++++++++++------------
 t/t3903-stash.sh            |   77 ++++++++++++++++++++++++++++++++++++
 3 files changed, 163 insertions(+), 43 deletions(-)

Jon Seymour· Aug 10, 2010, 00:04 UTC · re: Jon Seymour · lore

[PATCH v4 1/9] stash: refactor - create pop_stash function

We plan to edit pop_stash later, so let's make it a first class command by giving its own implementation function.

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

Show changes to git-stash.sh +8 −4

diff --git a/git-stash.sh b/git-stash.sh
index 1d95447..31ea333 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -323,6 +323,13 @@ apply_stash () {
 	fi
 }
 
+pop_stash() {
+	if apply_stash "$@"
+	then
+		drop_stash "$applied_stash"
+	fi
+}
+
 drop_stash () {
 	have_stash || die 'No stash entries to drop'
 
@@ -422,10 +429,7 @@ drop)
 	;;
 pop)
 	shift
-	if apply_stash "$@"
-	then
-		drop_stash "$applied_stash"
-	fi
+	pop_stash "$@"
 	;;
 branch)
 	shift

-- 
1.6.5.rc3.8.g8ba5e

Jon Seymour· Aug 10, 2010, 00:04 UTC · re: Jon Seymour · lore

[PATCH v4 2/9] stash: extract stash-like check into its own function

A similar code block is used in three places; this change factors the stash-like check into a separate function so that it can be used in these and other places.

Note to reviewers: The existing code in apply_stash appears to assume w_tree, b_tree and i_tree are all trees as opposed to tree-ish (which is what they actually are).

This refactoring does not change the existing derivation, but one wonders whether this difference might be subverting the intent of the test which compares c_tree with i_tree.

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

Show changes to git-stash.sh +30 −16

diff --git a/git-stash.sh b/git-stash.sh
index 31ea333..dbb7944 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -218,13 +218,30 @@ show_stash () {
 		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
 }
 
+#
+# if this function returns, then:
+#   s is set to the stash commit
+#   w_commit is set to the commit containing the working tree
+#   b_commit is set to the base commit
+#   i_commit is set to the commit containing the index tree
+# otherwise:
+#   the function exits via die
+#
+assert_stash_like() {
+	# stash records the work tree, and is a merge between the
+	# base commit (first parent) and the index tree (second parent).
+	s=$(git rev-parse --revs-only --quiet --verify --default $ref_stash "$@") &&
+	w_commit=$(git rev-parse --quiet --verify "$s:") &&
+	b_commit=$(git rev-parse --quiet --verify "$s^1:") &&
+	i_commit=$(git rev-parse --quiet --verify "$s^2:") ||
+		die "$*: no valid stashed state found"
+}
+
 apply_stash () {
 	applied_stash=
 	unstash_index=
@@ -253,13 +270,11 @@ apply_stash () {
 		applied_stash="$*"
 	fi
 
-	# stash records the work tree, and is a merge between the
-	# base commit (first parent) and the index tree (second parent).
-	s=$(git rev-parse --quiet --verify --default $ref_stash "$@") &&
-	w_tree=$(git rev-parse --quiet --verify "$s:") &&
-	b_tree=$(git rev-parse --quiet --verify "$s^1:") &&
-	i_tree=$(git rev-parse --quiet --verify "$s^2:") ||
-		die "$*: no valid stashed state found"
+	assert_stash_like "$@"
+
+	b_tree=$b_commit
+	w_tree=$w_commit
+	i_tree=$i_commit
 
 	git update-index -q --refresh &&
 	git diff-files --quiet --ignore-submodules ||
@@ -270,6 +285,9 @@ apply_stash () {
 		die 'Cannot apply a stash in the middle of a merge'
 
 	unstashed_index_tree=
+	#
+	# note to reviewers: comparing $c_tree to $i_tree seems unsound, since i_tree is only tree-ish
+	#
 	if test -n "$unstash_index" && test "$b_tree" != "$i_tree" &&
 			test "$c_tree" != "$i_tree"
 	then
@@ -351,12 +369,8 @@ drop_stash () {
 		set x "$ref_stash@{0}"
 		shift
 	fi
-	# Verify supplied argument looks like a stash entry
-	s=$(git rev-parse --verify "$@") &&
-	git rev-parse --verify "$s:"   > /dev/null 2>&1 &&
-	git rev-parse --verify "$s^1:" > /dev/null 2>&1 &&
-	git rev-parse --verify "$s^2:" > /dev/null 2>&1 ||
-		die "$*: not a valid stashed state"
+
+	assert_stash_like "$@"
 
 	git reflog delete --updateref --rewrite "$@" &&
 		say "Dropped $* ($s)" || die "$*: Could not drop stash entry"

-- 
1.6.5.rc3.8.g8ba5e

Junio C Hamano· Aug 13, 2010, 00:49 UTC · re: Jon Seymour · lore

Re: [PATCH v4 2/9] stash: extract stash-like check into its own function

Jon Seymour <jon.seymour@gmail.com> writes:

> A similar code block is used in three places; this change
> factors the stash-like check into a separate function so that
> it can be used in these and other places.

"git stash show" used to be able to show a single-parent commit, and we are forbidding that "feature". I'd actually call that a bugfix though ;-)

Show 16 quoted lines

> +# if this function returns, then:
> +#   s is set to the stash commit
> +#   w_commit is set to the commit containing the working tree
> +#   b_commit is set to the base commit
> +#   i_commit is set to the commit containing the index tree
> +# otherwise:
> +#   the function exits via die
> +#
> +assert_stash_like() {
> +	# stash records the work tree, and is a merge between the
> +	# base commit (first parent) and the index tree (second parent).
> +	s=$(git rev-parse --revs-only --quiet --verify --default $ref_stash "$@") &&
> +	w_commit=$(git rev-parse --quiet --verify "$s:") &&
> +	b_commit=$(git rev-parse --quiet --verify "$s^1:") &&
> +	i_commit=$(git rev-parse --quiet --verify "$s^2:") ||
> +		die "$*: no valid stashed state found"

Don't these "rev-parse" calls return tree objects? It is too confusing to call them x_commit if that is the case.

Jon Seymour· Aug 13, 2010, 23:55 UTC · re: Junio C Hamano · lore

Re: [PATCH v4 2/9] stash: extract stash-like check into its own function

On Fri, Aug 13, 2010 at 10:49 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 29 quoted lines

> Jon Seymour <jon.seymour@gmail.com> writes:
>
>> A similar code block is used in three places; this change
>> factors the stash-like check into a separate function so that
>> it can be used in these and other places.
>
> "git stash show" used to be able to show a single-parent commit, and we
> are forbidding that "feature".  I'd actually call that a bugfix though ;-)
>
>> +# if this function returns, then:
>> +#   s is set to the stash commit
>> +#   w_commit is set to the commit containing the working tree
>> +#   b_commit is set to the base commit
>> +#   i_commit is set to the commit containing the index tree
>> +# otherwise:
>> +#   the function exits via die
>> +#
>> +assert_stash_like() {
>> +     # stash records the work tree, and is a merge between the
>> +     # base commit (first parent) and the index tree (second parent).
>> +     s=$(git rev-parse --revs-only --quiet --verify --default $ref_stash "$@") &&
>> +     w_commit=$(git rev-parse --quiet --verify "$s:") &&
>> +     b_commit=$(git rev-parse --quiet --verify "$s^1:") &&
>> +     i_commit=$(git rev-parse --quiet --verify "$s^2:") ||
>> +             die "$*: no valid stashed state found"
>
> Don't these "rev-parse" calls return tree objects?  It is too confusing to
> call them x_commit if that is the case.
>

My bad. You are quite correct. I was a little _too_ eager to remove a redundancy. This also explains my misconceived "Note tor reviewers"! I'll fix in the next round.

Jon Seymour· Aug 10, 2010, 00:04 UTC · re: Jon Seymour · lore

[PATCH v4 3/9] stash: introduce is_stash_ref and assert_stash_ref functions.

Introduce a function, is_stash_ref, which checks whether an argument is valid stash reference (e.g. is of the form ['refs/']stash['@{'something'}])

Introduce a function, assert_stash_ref, which can be used to ensure that certain commands are only called with stash references as arguments.

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

Show changes to git-stash.sh +9 −0

diff --git a/git-stash.sh b/git-stash.sh
index dbb7944..41a9359 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -223,6 +223,15 @@ show_stash () {
 	git diff $flags $b_commit $w_commit
 }
 
+is_stash_ref() {
+	revs="$(git rev-parse --revs-only --symbolic --verify --default ${ref_stash} "$@")" &&
+	test "$ref_stash" = "$(git rev-parse --symbolic-full-name "${revs%@*}")"
+}
+
+assert_stash_ref() {
+	is_stash_ref "$@" || die "$* is not a stash reference"
+}
+
 #
 # if this function returns, then:
 #   s is set to the stash commit

-- 
1.6.5.rc3.8.g8ba5e

Junio C Hamano· Aug 13, 2010, 00:49 UTC · re: Jon Seymour · lore

Re: [PATCH v4 3/9] stash: introduce is_stash_ref and assert_stash_ref functions.

Jon Seymour <jon.seymour@gmail.com> writes:

Show 28 quoted lines

> Introduce a function, is_stash_ref, which checks whether an
> argument is valid stash reference (e.g. is of the form ['refs/']stash['@{'something'}])
>
> Introduce a function, assert_stash_ref, which can be used to ensure
> that certain commands are only called with stash references as
> arguments.
>
> Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
> ---
>  git-stash.sh |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)
>
> diff --git a/git-stash.sh b/git-stash.sh
> index dbb7944..41a9359 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -223,6 +223,15 @@ show_stash () {
>  	git diff $flags $b_commit $w_commit
>  }
>  
> +is_stash_ref() {
> +	revs="$(git rev-parse --revs-only --symbolic --verify --default ${ref_stash} "$@")" &&
> +	test "$ref_stash" = "$(git rev-parse --symbolic-full-name "${revs%@*}")"
> +}
> +
> +assert_stash_ref() {
> +	is_stash_ref "$@" || die "$* is not a stash reference"
> +}

I have to wonder what these functions would do when given more than one args, but I cannot (yet) see the users of this function, which may be doing their own sanity checking to avoid that situation, so...

Jon Seymour· Aug 14, 2010, 00:26 UTC · re: Junio C Hamano · lore

Re: [PATCH v4 3/9] stash: introduce is_stash_ref and assert_stash_ref functions.

| Apologies - missed the list in my first reply.

On Fri, Aug 13, 2010 at 10:49 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 7 quoted lines

> Jon Seymour <jon.seymour@gmail.com> writes:
>
> I have to wonder what these functions would do when given more than one
> args, but I cannot (yet) see the users of this function, which may be
> doing their own sanity checking to avoid that situation, so...
>
>

I did wonder about that myself, but with insufficient curiosity to actually write a test. I'll fix...

jon.

Jon Seymour· Aug 10, 2010, 00:04 UTC · re: Jon Seymour · lore

[PATCH v4 4/9] stash: teach git stash pop to fail early if the argument is not a stash ref

With this change, git stash pop will fail early if a specified revision argument is not a valid stash reference.

This fixes the case where the apply succeeds, but the drop fails which 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 tree is not modified.

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

Show changes to git-stash.sh +4 −4

diff --git a/git-stash.sh b/git-stash.sh
index 41a9359..4f28c09 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -351,10 +351,10 @@ apply_stash () {
 }
 
 pop_stash() {
-	if apply_stash "$@"
-	then
-		drop_stash "$applied_stash"
-	fi
+	assert_stash_ref "$@"
+
+	apply_stash "$@" &&
+	drop_stash "$applied_stash"
 }
 
 drop_stash () {

-- 
1.6.5.rc3.8.g8ba5e

Jon Seymour· Aug 10, 2010, 00:04 UTC · re: Jon Seymour · lore

[PATCH v4 5/9] stash: teach git stash drop to fail early if the specified revision is not a stash reference

Previously, git stash drop would fail noisily while executing git reflog delete if the specified revision argument was not a stash reference.

Now, git stash drop fails with an error message which more precisely indicates the reason for failure.

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

Show changes to git-stash.sh +1 −0

diff --git a/git-stash.sh b/git-stash.sh
index 4f28c09..86cd446 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -379,6 +379,7 @@ drop_stash () {
 		shift
 	fi
 
+	assert_stash_ref "$@"
 	assert_stash_like "$@"
 
 	git reflog delete --updateref --rewrite "$@" &&

-- 
1.6.5.rc3.8.g8ba5e

Jon Seymour· Aug 10, 2010, 00:04 UTC · re: Jon Seymour · lore

[PATCH v4 6/9] stash: teach git stash branch to tolerate stash-like arguments

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.

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

Show changes to git-stash.sh +11 −4

diff --git a/git-stash.sh b/git-stash.sh
index 86cd446..97e3749 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -390,20 +390,27 @@ drop_stash () {
 }
 
 apply_to_branch () {
-	have_stash || die 'Nothing to apply'
-
 	test -n "$1" || die 'No branch name specified'
 	branch=$1
 
 	if test -z "$2"
 	then
 		set x "$ref_stash@{0}"
+		have_stash || die 'Nothing to apply'
 	fi
 	stash=$2
 
+	assert_stash_like "$stash"
+
 	git checkout -b $branch $stash^ &&
-	apply_stash --index $stash &&
-	drop_stash $stash
+	apply_stash --index $stash
+
+	if is_stash_ref "$stash"
+	then
+		drop_stash "$stash"
+	else
+		true
+	fi
 }
 
 # The default command is "save" if nothing but options are given

-- 
1.6.5.rc3.8.g8ba5e

Jon Seymour· Aug 10, 2010, 00:04 UTC · re: Jon Seymour · lore

[PATCH v4 7/9] stash: teach git stash show to always tolerate stash-like arguments.

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 |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

Show changes to git-stash.sh +7 −2

diff --git a/git-stash.sh b/git-stash.sh
index 97e3749..5b875ea 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -210,14 +210,19 @@ 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
 
+	revs=$(git rev-parse --revs-only "$@")
+	if test -z "$revs"
+	then
+		have_stash || die "No stash found"
+		set -- ${ref_stash}@{0}
+	fi
+
 	assert_stash_like "$@"
 
 	git diff $flags $b_commit $w_commit

-- 
1.6.5.rc3.8.g8ba5e

Junio C Hamano· Aug 13, 2010, 00:49 UTC · re: Jon Seymour · lore

Re: [PATCH v4 7/9] stash: teach git stash show to always tolerate stash-like arguments.

Jon Seymour <jon.seymour@gmail.com> writes:

> +	revs=$(git rev-parse --revs-only "$@")

You probably want to also give --no-flags to avoid "show stash --parents" grabbing --parents into $revs variable here.

Jon Seymour· Aug 10, 2010, 00:04 UTC · re: Jon Seymour · lore

[PATCH v4 8/9] t3903-stash.sh: tests of git stash with stash-like arguments

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

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

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

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

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 62e208a..c051b51 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -378,4 +378,81 @@ 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_done

-- 
1.6.5.rc3.8.g8ba5e

Jon Seymour· Aug 10, 2010, 00:04 UTC · re: Jon Seymour · lore

[PATCH v4 9/9] Documentation: git stash branch now tolerates non-stash references.

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 |   37 ++++++++++++++++++++-----------------
 1 files changed, 20 insertions(+), 17 deletions(-)

Show changes to Documentation/git-stash.txt +20 −17

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 473889a..499ac48 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -9,10 +9,11 @@ SYNOPSIS
 --------
 [verse]
 'git stash' list [<options>]
-'git stash' show [<stash>]
-'git stash' drop [-q|--quiet] [<stash>]
-'git stash' ( pop | apply ) [--index] [-q|--quiet] [<stash>]
-'git stash' branch <branchname> [<stash>]
+'git stash' show [<stash-like>]
+'git stash' drop [-q|--quiet] [<stash-entry>]
+'git stash' pop [--index] [-q|--quiet] [<stash-entry>]
+'git stash' apply [--index] [-q|--quiet] [<stash-like>]
+'git stash' branch <branchname> [<stash-like>]
 'git stash' [save [--patch] [-k|--[no-]keep-index] [-q|--quiet] [<message>]]
 'git stash' clear
 'git stash' create
@@ -80,15 +81,15 @@ stash@{1}: On master: 9cc0589... Add git-stash
 The command takes options applicable to the 'git log'
 command to control what is shown and how. See linkgit:git-log[1].
 
-show [<stash>]::
+show [<stash-like>]::
 
 	Show the changes recorded in the stash as a diff between the
-	stashed state and its original parent. When no `<stash>` is given,
+	stashed state and its original parent. When no `<stash-like>` is given,
 	shows the latest one. By default, the command shows the diffstat, but
 	it will accept any format known to 'git diff' (e.g., `git stash show
 	-p stash@\{1}` to view the second most recent stash in patch form).
 
-pop [--index] [-q|--quiet] [<stash>]::
+pop [--index] [-q|--quiet] [<stash-entry>]::
 
 	Remove a single stashed state from the stash list and apply it
 	on top of the current working tree state, i.e., do the inverse
@@ -104,19 +105,21 @@ 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-entry>` is given, `stash@\{0}` is assumed.
 
-apply [--index] [-q|--quiet] [<stash>]::
+apply [--index] [-q|--quiet] [<stash-like>]::
 
-	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`,
+	the `<stash-like>` argument can be any commit that looks like a stash, for
+	example, a stash created with `create`.
 
-branch <branchname> [<stash>]::
+branch <branchname> [<stash-like>]::
 
 	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>`
-	is given, applies the latest one.
+	the commit at which the `<stash-like>` was originally created, applies the
+	changes recorded in `<stash-like>` to the new working tree and index, then
+	drops the `<stash-like>` if that completes successfully and `<stash-like>`
+	is also a stash entry. When no `<stash-like>` is given, applies the latest one.
 +
 This is useful if the branch on which you ran `git stash save` has
 changed enough that `git stash apply` fails due to conflicts. Since
@@ -129,9 +132,9 @@ clear::
 	be subject to pruning, and may be impossible to recover (see
 	'Examples' below for a possible strategy).
 
-drop [-q|--quiet] [<stash>]::
+drop [-q|--quiet] [<stash-entry>]::
 
-	Remove a single stashed state from the stash list. When no `<stash>`
+	Remove a single stashed state from the stash list. When no `<stash-entry>`
 	is given, it removes the latest one. i.e. `stash@\{0}`
 
 create::

-- 
1.6.5.rc3.8.g8ba5e

Junio C Hamano· Aug 13, 2010, 00:49 UTC · re: Jon Seymour · lore

Re: [PATCH v4 9/9] Documentation: git stash branch now tolerates non-stash references.

Jon Seymour <jon.seymour@gmail.com> writes:

Show 44 quoted lines

> 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 |   37 ++++++++++++++++++++-----------------
>  1 files changed, 20 insertions(+), 17 deletions(-)
>
> diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
> index 473889a..499ac48 100644
> --- a/Documentation/git-stash.txt
> +++ b/Documentation/git-stash.txt
> @@ -9,10 +9,11 @@ SYNOPSIS
>  --------
>  [verse]
>  'git stash' list [<options>]
> -'git stash' show [<stash>]
> -'git stash' drop [-q|--quiet] [<stash>]
> -'git stash' ( pop | apply ) [--index] [-q|--quiet] [<stash>]
> -'git stash' branch <branchname> [<stash>]
> +'git stash' show [<stash-like>]
> +'git stash' drop [-q|--quiet] [<stash-entry>]
> +'git stash' pop [--index] [-q|--quiet] [<stash-entry>]
> +'git stash' apply [--index] [-q|--quiet] [<stash-like>]
> +'git stash' branch <branchname> [<stash-like>]
>  'git stash' [save [--patch] [-k|--[no-]keep-index] [-q|--quiet] [<message>]]
>  'git stash' clear
>  'git stash' create
> @@ -80,15 +81,15 @@ stash@{1}: On master: 9cc0589... Add git-stash
>  The command takes options applicable to the 'git log'
>  command to control what is shown and how. See linkgit:git-log[1].
>  
> -show [<stash>]::
> +show [<stash-like>]::
>  
>  	Show the changes recorded in the stash as a diff between the
> -	stashed state and its original parent. When no `<stash>` is given,
> +	stashed state and its original parent. When no `<stash-like>` is given,
>  	shows the latest one. By default, the command shows the diffstat, but
>  	it will accept any format known to 'git diff' (e.g., `git stash show
>  	-p stash@\{1}` to view the second most recent stash in patch form).

Hmm.

Is there anything that describes how <stash-like>, <stash-entry> and <stash> relate to each other? I do not think a regular reader can answer by reading the description after the patch these questions:

 - Is a <stash-entry> a <stash-like>?
 - Is the opposite true?

etc...

Perhaps we can simply call these two-parent merge commits a <stash>, define that these commands as working on a <stash>, add notes to certain subcommands that the <stash> they take must be on the stash ref (as opposed to a freestanding one you can create with "stash create"), and be done with it?

Also what is the error condition? I am assuming that your <stash-entry> is a <stash-like> that is on the reflog of refs/stash, but if you give a <stash-like> that is not a <stash-entry> (iow a freestanding <stash>) to a subcommand that wants to see <stash-entry>, what happens, and does the document describe it as an error?

Show 5 quoted lines

> -pop [--index] [-q|--quiet] [<stash>]::
> +pop [--index] [-q|--quiet] [<stash-entry>]::
>  
>  	Remove a single stashed state from the stash list and apply it
>  	on top of the current working tree state, i.e., do the inverse

Jon Seymour· Aug 14, 2010, 00:30 UTC · re: Junio C Hamano · lore

Re: [PATCH v4 9/9] Documentation: git stash branch now tolerates non-stash references.

On Fri, Aug 13, 2010 at 10:49 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 31 quoted lines

> Jon Seymour <jon.seymour@gmail.com> writes:
>
> Hmm.
>
> Is there anything that describes how <stash-like>, <stash-entry> and
> <stash> relate to each other?  I do not think a regular reader can answer
> by reading the description after the patch these questions:
>
>  - Is a <stash-entry> a <stash-like>?
>  - Is the opposite true?
>
> etc...
>
> Perhaps we can simply call these two-parent merge commits a <stash>,
> define that these commands as working on a <stash>, add notes to certain
> subcommands that the <stash> they take must be on the stash ref (as
> opposed to a freestanding one you can create with "stash create"), and be
> done with it?
>
> Also what is the error condition?  I am assuming that your <stash-entry>
> is a <stash-like> that is on the reflog of refs/stash, but if you give a
> <stash-like> that is not a <stash-entry> (iow a freestanding <stash>) to a
> subcommand that wants to see <stash-entry>, what happens, and does the
> document describe it as an error?
>
>> -pop [--index] [-q|--quiet] [<stash>]::
>> +pop [--index] [-q|--quiet] [<stash-entry>]::
>>
>>       Remove a single stashed state from the stash list and apply it
>>       on top of the current working tree state, i.e., do the inverse
>

Ok, I'll have another crack at it, and address the other issues you have noted in another round.

Thanks for the review.

jon.