[PATCH v2 0/3] *** SUBJECT HERE ***

[PATCH v2 0/3] *** SUBJECT HERE ***

[Jon Seymour]

This series fixes git stash branch so that it is more tolerant of
stash-like commits created with git stash create.

It particular, it doesn't require there to be a stash stack if a
stash-like argument is specified and it doesn't attempt to drop
non-stash references after applying the stash.

This series replaces my previous patch that just included a test 
that demonstrated the existance of the issue.

  stash: It looks like a stash, but doesn't quack like a stash...
  stash: Allow git stash branch to process commits that look like
    stashes but are not stash references.
  stash: modify tests to reflect stash branch fixes.

 git-stash.sh     |   10 ++++++++--
 t/t3903-stash.sh |   28 ++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 2 deletions(-)

-- 
1.7.2.1.111.g8fc90

[PATCH v2 1/3] stash: It looks like a stash, but doesn't quack like a stash...

[Jon Seymour]

In particular, a stash created with git stash create cannot be used as
an argument to git stash branch because of two separate reasons.

1. a pre-condition assumes that there is always a stash on the stack when git stash branch is called,
which is not necessarily true

2. the cleanup code assumes the specified stash is a stash reference, rather than an arbitrary commit.

The test_expect_failure tests in this patch demonstrate these issues.


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

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 62e208a..4d8b6ad 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -378,4 +378,34 @@ test_expect_failure 'stash file to directory' '
 	test foo = "$(cat file/file)"
 '
 
+test_expect_failure 'stash branch from arbitrary stash ref when there are no stash references' '
+	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_failure 'stash branch from arbitrary stash ref fails even if there is a stash' '
+	git stash clear && {
+		git branch -D stash-branch || true
+	}
+	test_when_finished "git reset --hard HEAD" &&
+	git reset --hard &&
+	echo foo >> file &&
+	git stash &&
+	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_done
-- 
1.7.2.1.111.g8fc90

Re: [PATCH v2 1/3] stash: It looks like a stash, but doesn't quack like a stash...

[Junio C Hamano]

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

> In particular, a stash created with git stash create cannot be used as
> an argument to git stash branch because of two separate reasons.
>
> 1. a pre-condition assumes that there is always a stash on the stack when git stash branch is called,
> which is not necessarily true
>
> 2. the cleanup code assumes the specified stash is a stash reference, rather than an arbitrary commit.

Hmm, I don't use the command myself so I ended up reading the description
of "stash branch".  To me it is clear that it wants to use a stash entry,
not just an arbitrary stash-looking commit, from "... then drops the
<stash>".

So I wouldn't call these tests "expect-failure"; rather, I would suggest
swapping the order of patches so that they test the new feature that
allows "git stash branch" to take an arbitrary stash-looking commit.

The documentation also needs to be updated to make the "then drops" part
conditional (perhaps "then drops...if the stash was on the list").

Are there any other stash subcommand that ought to be able to act on a
stash-looking commit but doesn't, or is "branch" the only one?

[PATCH v2 2/3] stash: Allow git stash branch to process commits that look like stashes but are not stash references.

[Jon Seymour]

This patch allows git stash branch to work with stash-like commits created by git stash create.

Two changes were required:

* relax the pre-condition so that a stash stack is required if and only if a stash argument is not specified
* don't attempt to drop a stash argument that doesn't look like a stash reference.


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

diff --git a/git-stash.sh b/git-stash.sh
index 1d95447..432ddae 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -225,6 +225,12 @@ show_stash () {
 	git diff $flags $b_commit $w_commit
 }
 
+if_stash_ref() {
+	ref="$1"
+	shift
+	test "${ref#stash}" = "${ref}" -a "${ref#$ref_stash}" = "${ref}" || "$@"
+}
+
 apply_stash () {
 	applied_stash=
 	unstash_index=
@@ -359,20 +365,20 @@ 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
+		have_stash || die 'Nothing to apply'
 		set x "$ref_stash@{0}"
 	fi
 	stash=$2
 
 	git checkout -b $branch $stash^ &&
 	apply_stash --index $stash &&
-	drop_stash $stash
+	if_stash_ref "$stash" drop_stash "$stash"
 }
 
 # The default command is "save" if nothing but options are given
-- 
1.7.2.1.111.g8fc90

Re: [PATCH v2 2/3] stash: Allow git stash branch to process commits that look like stashes but are not stash references.

[Junio C Hamano]

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

> This patch allows git stash branch to work with stash-like commits created by git stash create.
>
> Two changes were required:
>
> * relax the pre-condition so that a stash stack is required if and only if a stash argument is not specified
> * don't attempt to drop a stash argument that doesn't look like a stash reference.
>
>
> Signed-off-by: Jon Seymour <jon.seymour@gmail.com>

Please wrap very long lines.

> diff --git a/git-stash.sh b/git-stash.sh
> index 1d95447..432ddae 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -225,6 +225,12 @@ show_stash () {
>  	git diff $flags $b_commit $w_commit
>  }
>  
> +if_stash_ref() {
> +	ref="$1"
> +	shift
> +	test "${ref#stash}" = "${ref}" -a "${ref#$ref_stash}" = "${ref}" || "$@"
> +}

The interface to this function looks a rather bad taste to me; wouldn't it
look more natural if the callers can say:

	if stash_ref $it
        then
        	do this
	fi

Your criteria used here is that the given parameter does not begin with
"stash" nor "refs/stash".  If it begins with either of these two strings,
the "test" fails and "$@" is run.  Wouldn't this produce a false hit if
you kept a handcrafted stash-looking commit with a tag "stash-42" or
something?

It may make more sense to give "stash drop" an option to be silent if
the given parameter is not on the list to begin with, perhaps?

Re: [PATCH v2 2/3] stash: Allow git stash branch to process commits that look like stashes but are not stash references.

[Jon Seymour]

Junio,

Thanks for the feedback. I'll rework along the lines you suggest. If
it makes sense to make the other stash commands tolerant of non-stash
entry references I'll add tests, support and documentation for that.

jon.

On Thu, August 2010 at 9:51 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jon  Seymour <jon.seymour@gmail.com> writes:
>
>> This patch allows git stash branch to work with stash-like commits created by git stash create.
>>
>> Two changes were required:
>>
>> * relax the pre-condition so that a stash stack is required if and only if a stash argument is not specified
>> * don't attempt to drop a stash argument that doesn't look like a stash reference.
>>
>>
>> Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
>
> Please wrap very long lines.
>
>> diff --git a/git-stash.sh b/git-stash.sh
>> index 1d95447..432ddae 100755
>> --- a/git-stash.sh
>> +++ b/git-stash.sh
>> @@ -225,6 +225,12 @@ show_stash () {
>>       git diff $flags $b_commit $w_commit
>>  }
>>
>> +if_stash_ref() {
>> +     ref="$1"
>> +     shift
>> +     test "${ref#stash}" = "${ref}" -a "${ref#$ref_stash}" = "${ref}" || "$@"
>> +}
>
> The interface to this function looks a rather bad taste to me; wouldn't it
> look more natural if the callers can say:
>
>        if stash_ref $it
>        then
>                do this
>        fi
>
> Your criteria used here is that the given parameter does not begin with
> "stash" nor "refs/stash".  If it begins with either of these two strings,
> the "test" fails and "$@" is run.  Wouldn't this produce a false hit if
> you kept a handcrafted stash-looking commit with a tag "stash-42" or
> something?
>
> It may make more sense to give "stash drop" an option to be silent if
> the given parameter is not on the list to begin with, perhaps?
>
>

Re: [PATCH v2 2/3] stash: Allow git stash branch to process commits that look like stashes but are not stash references.

[Jon Seymour]

One question about test patches. Are you ok with test_expect_failure
tests that document the expected failure of a feature yet to be
developed, followed by the feature, followed by the patch that makes
the tests into test_expect_success tests, or would you prefer to see
the pre- and post- test patches rolled into a single test that is
delivered after the feature patch?

On Thu, Aug 5, 2010 at 3:23 PM, Jon Seymour <jon.seymour@gmail.com> wrote:
> Junio,
>
> Thanks for the feedback. I'll rework along the lines you suggest. If
> it makes sense to make the other stash commands tolerant of non-stash
> entry references I'll add tests, support and documentation for that.
>
> jon.
>
> On Thu, August 2010 at 9:51 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Jon  Seymour <jon.seymour@gmail.com> writes:
>>
>>> This patch allows git stash branch to work with stash-like commits created by git stash create.
>>>
>>> Two changes were required:
>>>
>>> * relax the pre-condition so that a stash stack is required if and only if a stash argument is not specified
>>> * don't attempt to drop a stash argument that doesn't look like a stash reference.
>>>
>>>
>>> Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
>>
>> Please wrap very long lines.
>>
>>> diff --git a/git-stash.sh b/git-stash.sh
>>> index 1d95447..432ddae 100755
>>> --- a/git-stash.sh
>>> +++ b/git-stash.sh
>>> @@ -225,6 +225,12 @@ show_stash () {
>>>       git diff $flags $b_commit $w_commit
>>>  }
>>>
>>> +if_stash_ref() {
>>> +     ref="$1"
>>> +     shift
>>> +     test "${ref#stash}" = "${ref}" -a "${ref#$ref_stash}" = "${ref}" || "$@"
>>> +}
>>
>> The interface to this function looks a rather bad taste to me; wouldn't it
>> look more natural if the callers can say:
>>
>>        if stash_ref $it
>>        then
>>                do this
>>        fi
>>
>> Your criteria used here is that the given parameter does not begin with
>> "stash" nor "refs/stash".  If it begins with either of these two strings,
>> the "test" fails and "$@" is run.  Wouldn't this produce a false hit if
>> you kept a handcrafted stash-looking commit with a tag "stash-42" or
>> something?
>>
>> It may make more sense to give "stash drop" an option to be silent if
>> the given parameter is not on the list to begin with, perhaps?
>>
>>
>

Re: [PATCH v2 2/3] stash: Allow git stash branch to process commits that look like stashes but are not stash references.

[Junio C Hamano]

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

> One question about test patches. Are you ok with test_expect_failure
> tests that document the expected failure of a feature yet to be
> developed, followed by the feature, followed by the patch that makes
> the tests into test_expect_success tests, or would you prefer to see
> the pre- and post- test patches rolled into a single test that is
> delivered after the feature patch?

I think a single patch that implements the feature and at the same time
protects it with new tests from getting broken by others would be the best
form.

[PATCH v2 3/3] stash: modify tests to reflect stash branch fixes.

[Jon Seymour]

These tests now pass so they are now updated to reflect this.


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

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 4d8b6ad..e5f248e 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -378,7 +378,7 @@ test_expect_failure 'stash file to directory' '
 	test foo = "$(cat file/file)"
 '
 
-test_expect_failure 'stash branch from arbitrary stash ref when there are no stash references' '
+test_expect_success 'stash branch with no stashes on stack and stash-like reference as argument' '
 	git stash clear &&
 	test_when_finished "git reset --hard HEAD" &&
 	git reset --hard &&
@@ -391,10 +391,8 @@ test_expect_failure 'stash branch from arbitrary stash ref when there are no sta
 	test $(git ls-files --modified | wc -l) -eq 1
 '
 
-test_expect_failure 'stash branch from arbitrary stash ref fails even if there is a stash' '
-	git stash clear && {
-		git branch -D stash-branch || true
-	}
+test_expect_success 'stash branch with stashes on stack and stash-like reference as argument' '
+	git stash clear &&
 	test_when_finished "git reset --hard HEAD" &&
 	git reset --hard &&
 	echo foo >> file &&
-- 
1.7.2.1.111.g8fc90