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

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

[Jon Seymour]

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

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

[Jon Seymour]

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(-)

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

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

[Jon Seymour]

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(-)

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

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

[Jon Seymour]

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"
+}
+
 #
 # if this function returns, then:
 #   s is set to the stash commit
-- 
1.7.2.1.51.g82c0c0

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

[Jon Seymour]

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(-)

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

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

[Jon Seymour]

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(-)

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

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

[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.

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

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

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

[Jon Seymour]

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(-)

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

[PATCH v3 8/9] t3903-stash.sh: 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

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

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

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

[Jon Seymour]

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

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

[Johannes Sixt]

On Sonntag, 8. August 2010, Jon Seymour wrote:
> +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

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

[Jon Seymour]

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

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

[Jon Seymour]

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:
> 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
>
>

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

[Jon Seymour]

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 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(-)

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

[Jon Seymour]

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(-)

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

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

[Jon Seymour]

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(-)

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

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

[Jon Seymour]

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"
+}
+
 #
 # if this function returns, then:
 #   s is set to the stash commit
-- 
1.6.5.rc3.8.g8ba5e

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

[Jon Seymour]

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(-)

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

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

[Jon Seymour]

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(-)

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

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

[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.

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

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

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

[Jon Seymour]

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(-)

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

[PATCH v4 8/9] t3903-stash.sh: 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

| 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(-)

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

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

[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 |   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).
 
-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

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

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

> 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...

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

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

> 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?

> -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

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

[Jon Seymour]

On Fri, Aug 13, 2010 at 10:49 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

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

[Jon Seymour]

| Apologies - missed the list in my first reply.


On Fri, Aug 13, 2010 at 10:49 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

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

[Jon Seymour]

On Fri, Aug 13, 2010 at 10:49 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.