{"thread":{"id":"24613","subject":"[PATCH v2 0/3] *** SUBJECT HERE ***","startedAt":"2010-08-03T10:36:12Z","lastAt":"2010-08-05T20:13:32Z","messageCount":9,"participants":["Jon Seymour","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"147033","messageId":"1280831775-30759-1-git-send-email-jon.seymour@gmail.com","threadId":"24613","inReplyTo":null,"subject":"[PATCH v2 0/3] *** SUBJECT HERE ***","fromName":"Jon Seymour","fromEmail":"jon.seymour@gmail.com","sentAt":"2010-08-03T10:36:12Z","receivedAt":"2010-08-03T10:36:12Z","isPatch":true,"sender":{"key":"jon.seymour@gmail.com","avatar":"https://avatars.githubusercontent.com/u/207131?v=4"},"body":"This series fixes git stash branch so that it is more tolerant of\nstash-like commits created with git stash create.\n\nIt particular, it doesn't require there to be a stash stack if a\nstash-like argument is specified and it doesn't attempt to drop\nnon-stash references after applying the stash.\n\nThis series replaces my previous patch that just included a test \nthat demonstrated the existance of the issue.\n\n  stash: It looks like a stash, but doesn't quack like a stash...\n  stash: Allow git stash branch to process commits that look like\n    stashes but are not stash references.\n  stash: modify tests to reflect stash branch fixes.\n\n git-stash.sh     |   10 ++++++++--\n t/t3903-stash.sh |   28 ++++++++++++++++++++++++++++\n 2 files changed, 36 insertions(+), 2 deletions(-)\n\n-- \n1.7.2.1.111.g8fc90\n"},{"id":"147034","messageId":"1280831775-30759-2-git-send-email-jon.seymour@gmail.com","threadId":"24613","inReplyTo":"1280831775-30759-1-git-send-email-jon.seymour@gmail.com","subject":"[PATCH v2 1/3] stash: It looks like a stash, but doesn't quack like a stash...","fromName":"Jon Seymour","fromEmail":"jon.seymour@gmail.com","sentAt":"2010-08-03T10:36:13Z","receivedAt":"2010-08-03T10:36:13Z","isPatch":true,"sender":{"key":"jon.seymour@gmail.com","avatar":"https://avatars.githubusercontent.com/u/207131?v=4"},"body":"In particular, a stash created with git stash create cannot be used as\nan argument to git stash branch because of two separate reasons.\n\n1. a pre-condition assumes that there is always a stash on the stack when git stash branch is called,\nwhich is not necessarily true\n\n2. the cleanup code assumes the specified stash is a stash reference, rather than an arbitrary commit.\n\nThe test_expect_failure tests in this patch demonstrate these issues.\n\n\nSigned-off-by: Jon Seymour <jon.seymour@gmail.com>\n---\n t/t3903-stash.sh |   30 ++++++++++++++++++++++++++++++\n 1 files changed, 30 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 62e208a..4d8b6ad 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -378,4 +378,34 @@ test_expect_failure 'stash file to directory' '\n \ttest foo = \"$(cat file/file)\"\n '\n \n+test_expect_failure 'stash branch from arbitrary stash ref when there are no stash references' '\n+\tgit stash clear &&\n+\ttest_when_finished \"git reset --hard HEAD\" &&\n+\tgit reset --hard &&\n+\techo foo >> file &&\n+\tgit tag stash-tag $(git stash create) &&\n+\ttest_when_finished \"git tag -d stash-tag\" &&\n+\tgit reset --hard &&\n+\tgit stash branch stash-branch $(git rev-parse stash-tag) &&\n+\ttest_when_finished \"git reset --hard HEAD && git checkout master && git branch -D stash-branch\" &&\n+\ttest $(git ls-files --modified | wc -l) -eq 1\n+'\n+\n+test_expect_failure 'stash branch from arbitrary stash ref fails even if there is a stash' '\n+\tgit stash clear && {\n+\t\tgit branch -D stash-branch || true\n+\t}\n+\ttest_when_finished \"git reset --hard HEAD\" &&\n+\tgit reset --hard &&\n+\techo foo >> file &&\n+\tgit stash &&\n+\techo bar >> file &&\n+\tgit tag stash-tag $(git stash create) &&\n+\ttest_when_finished \"git tag -d stash-tag\" &&\n+\tgit reset --hard &&\n+\tgit stash branch stash-branch $(git rev-parse stash-tag) &&\n+\ttest_when_finished \"git reset --hard HEAD && git checkout master && git branch -D stash-branch\" &&\n+\ttest $(git ls-files --modified | wc -l) -eq 1\n+'\n+\n test_done\n-- \n1.7.2.1.111.g8fc90\n"},{"id":"147036","messageId":"1280831775-30759-3-git-send-email-jon.seymour@gmail.com","threadId":"24613","inReplyTo":"1280831775-30759-1-git-send-email-jon.seymour@gmail.com","subject":"[PATCH v2 2/3] stash: Allow git stash branch to process commits that look like stashes but are not stash references.","fromName":"Jon Seymour","fromEmail":"jon.seymour@gmail.com","sentAt":"2010-08-03T10:36:14Z","receivedAt":"2010-08-03T10:36:14Z","isPatch":true,"sender":{"key":"jon.seymour@gmail.com","avatar":"https://avatars.githubusercontent.com/u/207131?v=4"},"body":"This patch allows git stash branch to work with stash-like commits created by git stash create.\n\nTwo changes were required:\n\n* relax the pre-condition so that a stash stack is required if and only if a stash argument is not specified\n* don't attempt to drop a stash argument that doesn't look like a stash reference.\n\n\nSigned-off-by: Jon Seymour <jon.seymour@gmail.com>\n---\n git-stash.sh |   10 ++++++++--\n 1 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex 1d95447..432ddae 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -225,6 +225,12 @@ show_stash () {\n \tgit diff $flags $b_commit $w_commit\n }\n \n+if_stash_ref() {\n+\tref=\"$1\"\n+\tshift\n+\ttest \"${ref#stash}\" = \"${ref}\" -a \"${ref#$ref_stash}\" = \"${ref}\" || \"$@\"\n+}\n+\n apply_stash () {\n \tapplied_stash=\n \tunstash_index=\n@@ -359,20 +365,20 @@ drop_stash () {\n }\n \n apply_to_branch () {\n-\thave_stash || die 'Nothing to apply'\n \n \ttest -n \"$1\" || die 'No branch name specified'\n \tbranch=$1\n \n \tif test -z \"$2\"\n \tthen\n+\t\thave_stash || die 'Nothing to apply'\n \t\tset x \"$ref_stash@{0}\"\n \tfi\n \tstash=$2\n \n \tgit checkout -b $branch $stash^ &&\n \tapply_stash --index $stash &&\n-\tdrop_stash $stash\n+\tif_stash_ref \"$stash\" drop_stash \"$stash\"\n }\n \n # The default command is \"save\" if nothing but options are given\n-- \n1.7.2.1.111.g8fc90\n"},{"id":"147035","messageId":"1280831775-30759-4-git-send-email-jon.seymour@gmail.com","threadId":"24613","inReplyTo":"1280831775-30759-1-git-send-email-jon.seymour@gmail.com","subject":"[PATCH v2 3/3] stash: modify tests to reflect stash branch fixes.","fromName":"Jon Seymour","fromEmail":"jon.seymour@gmail.com","sentAt":"2010-08-03T10:36:15Z","receivedAt":"2010-08-03T10:36:15Z","isPatch":true,"sender":{"key":"jon.seymour@gmail.com","avatar":"https://avatars.githubusercontent.com/u/207131?v=4"},"body":"These tests now pass so they are now updated to reflect this.\n\n\nSigned-off-by: Jon Seymour <jon.seymour@gmail.com>\n---\n t/t3903-stash.sh |    8 +++-----\n 1 files changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 4d8b6ad..e5f248e 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -378,7 +378,7 @@ test_expect_failure 'stash file to directory' '\n \ttest foo = \"$(cat file/file)\"\n '\n \n-test_expect_failure 'stash branch from arbitrary stash ref when there are no stash references' '\n+test_expect_success 'stash branch with no stashes on stack and stash-like reference as argument' '\n \tgit stash clear &&\n \ttest_when_finished \"git reset --hard HEAD\" &&\n \tgit reset --hard &&\n@@ -391,10 +391,8 @@ test_expect_failure 'stash branch from arbitrary stash ref when there are no sta\n \ttest $(git ls-files --modified | wc -l) -eq 1\n '\n \n-test_expect_failure 'stash branch from arbitrary stash ref fails even if there is a stash' '\n-\tgit stash clear && {\n-\t\tgit branch -D stash-branch || true\n-\t}\n+test_expect_success 'stash branch with stashes on stack and stash-like reference as argument' '\n+\tgit stash clear &&\n \ttest_when_finished \"git reset --hard HEAD\" &&\n \tgit reset --hard &&\n \techo foo >> file &&\n-- \n1.7.2.1.111.g8fc90\n"},{"id":"147142","messageId":"7v1vaeeyzq.fsf@alter.siamese.dyndns.org","threadId":"24613","inReplyTo":"1280831775-30759-2-git-send-email-jon.seymour@gmail.com","subject":"Re: [PATCH v2 1/3] stash: It looks like a stash, but doesn't quack like a stash...","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-08-04T23:31:37Z","receivedAt":"2010-08-04T23:31:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jon Seymour <jon.seymour@gmail.com> writes:\n\n> In particular, a stash created with git stash create cannot be used as\n> an argument to git stash branch because of two separate reasons.\n>\n> 1. a pre-condition assumes that there is always a stash on the stack when git stash branch is called,\n> which is not necessarily true\n>\n> 2. the cleanup code assumes the specified stash is a stash reference, rather than an arbitrary commit.\n\nHmm, I don't use the command myself so I ended up reading the description\nof \"stash branch\".  To me it is clear that it wants to use a stash entry,\nnot just an arbitrary stash-looking commit, from \"... then drops the\n<stash>\".\n\nSo I wouldn't call these tests \"expect-failure\"; rather, I would suggest\nswapping the order of patches so that they test the new feature that\nallows \"git stash branch\" to take an arbitrary stash-looking commit.\n\nThe documentation also needs to be updated to make the \"then drops\" part\nconditional (perhaps \"then drops...if the stash was on the list\").\n\nAre there any other stash subcommand that ought to be able to act on a\nstash-looking commit but doesn't, or is \"branch\" the only one?\n"},{"id":"147145","messageId":"7vwrs6djie.fsf@alter.siamese.dyndns.org","threadId":"24613","inReplyTo":"1280831775-30759-3-git-send-email-jon.seymour@gmail.com","subject":"Re: [PATCH v2 2/3] stash: Allow git stash branch to process commits that look like stashes but are not stash references.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-08-04T23:51:21Z","receivedAt":"2010-08-04T23:51:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jon Seymour <jon.seymour@gmail.com> writes:\n\n> This patch allows git stash branch to work with stash-like commits created by git stash create.\n>\n> Two changes were required:\n>\n> * relax the pre-condition so that a stash stack is required if and only if a stash argument is not specified\n> * don't attempt to drop a stash argument that doesn't look like a stash reference.\n>\n>\n> Signed-off-by: Jon Seymour <jon.seymour@gmail.com>\n\nPlease wrap very long lines.\n\n> diff --git a/git-stash.sh b/git-stash.sh\n> index 1d95447..432ddae 100755\n> --- a/git-stash.sh\n> +++ b/git-stash.sh\n> @@ -225,6 +225,12 @@ show_stash () {\n>  \tgit diff $flags $b_commit $w_commit\n>  }\n>  \n> +if_stash_ref() {\n> +\tref=\"$1\"\n> +\tshift\n> +\ttest \"${ref#stash}\" = \"${ref}\" -a \"${ref#$ref_stash}\" = \"${ref}\" || \"$@\"\n> +}\n\nThe interface to this function looks a rather bad taste to me; wouldn't it\nlook more natural if the callers can say:\n\n\tif stash_ref $it\n        then\n        \tdo this\n\tfi\n\nYour criteria used here is that the given parameter does not begin with\n\"stash\" nor \"refs/stash\".  If it begins with either of these two strings,\nthe \"test\" fails and \"$@\" is run.  Wouldn't this produce a false hit if\nyou kept a handcrafted stash-looking commit with a tag \"stash-42\" or\nsomething?\n\nIt may make more sense to give \"stash drop\" an option to be silent if\nthe given parameter is not on the list to begin with, perhaps?\n"},{"id":"147147","messageId":"AANLkTikA0_Og4bzB8AHo3s2cLCvf6pc9=wC4w_8emuxU@mail.gmail.com","threadId":"24613","inReplyTo":"7vwrs6djie.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/3] stash: Allow git stash branch to process commits that look like stashes but are not stash references.","fromName":"Jon Seymour","fromEmail":"jon.seymour@gmail.com","sentAt":"2010-08-05T05:23:19Z","receivedAt":"2010-08-05T05:23:19Z","isPatch":true,"sender":{"key":"jon.seymour@gmail.com","avatar":"https://avatars.githubusercontent.com/u/207131?v=4"},"body":"Junio,\n\nThanks for the feedback. I'll rework along the lines you suggest. If\nit makes sense to make the other stash commands tolerant of non-stash\nentry references I'll add tests, support and documentation for that.\n\njon.\n\nOn Thu, August 2010 at 9:51 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jon  Seymour <jon.seymour@gmail.com> writes:\n>\n>> This patch allows git stash branch to work with stash-like commits created by git stash create.\n>>\n>> Two changes were required:\n>>\n>> * relax the pre-condition so that a stash stack is required if and only if a stash argument is not specified\n>> * don't attempt to drop a stash argument that doesn't look like a stash reference.\n>>\n>>\n>> Signed-off-by: Jon Seymour <jon.seymour@gmail.com>\n>\n> Please wrap very long lines.\n>\n>> diff --git a/git-stash.sh b/git-stash.sh\n>> index 1d95447..432ddae 100755\n>> --- a/git-stash.sh\n>> +++ b/git-stash.sh\n>> @@ -225,6 +225,12 @@ show_stash () {\n>>       git diff $flags $b_commit $w_commit\n>>  }\n>>\n>> +if_stash_ref() {\n>> +     ref=\"$1\"\n>> +     shift\n>> +     test \"${ref#stash}\" = \"${ref}\" -a \"${ref#$ref_stash}\" = \"${ref}\" || \"$@\"\n>> +}\n>\n> The interface to this function looks a rather bad taste to me; wouldn't it\n> look more natural if the callers can say:\n>\n>        if stash_ref $it\n>        then\n>                do this\n>        fi\n>\n> Your criteria used here is that the given parameter does not begin with\n> \"stash\" nor \"refs/stash\".  If it begins with either of these two strings,\n> the \"test\" fails and \"$@\" is run.  Wouldn't this produce a false hit if\n> you kept a handcrafted stash-looking commit with a tag \"stash-42\" or\n> something?\n>\n> It may make more sense to give \"stash drop\" an option to be silent if\n> the given parameter is not on the list to begin with, perhaps?\n>\n>\n"},{"id":"147151","messageId":"AANLkTi=FbF3X83uLjBDoCYUCuig3udTzZCXuE=1_w-Ld@mail.gmail.com","threadId":"24613","inReplyTo":"AANLkTikA0_Og4bzB8AHo3s2cLCvf6pc9=wC4w_8emuxU@mail.gmail.com","subject":"Re: [PATCH v2 2/3] stash: Allow git stash branch to process commits that look like stashes but are not stash references.","fromName":"Jon Seymour","fromEmail":"jon.seymour@gmail.com","sentAt":"2010-08-05T07:50:12Z","receivedAt":"2010-08-05T07:50:12Z","isPatch":true,"sender":{"key":"jon.seymour@gmail.com","avatar":"https://avatars.githubusercontent.com/u/207131?v=4"},"body":"One question about test patches. Are you ok with test_expect_failure\ntests that document the expected failure of a feature yet to be\ndeveloped, followed by the feature, followed by the patch that makes\nthe tests into test_expect_success tests, or would you prefer to see\nthe pre- and post- test patches rolled into a single test that is\ndelivered after the feature patch?\n\nOn Thu, Aug 5, 2010 at 3:23 PM, Jon Seymour <jon.seymour@gmail.com> wrote:\n> Junio,\n>\n> Thanks for the feedback. I'll rework along the lines you suggest. If\n> it makes sense to make the other stash commands tolerant of non-stash\n> entry references I'll add tests, support and documentation for that.\n>\n> jon.\n>\n> On Thu, August 2010 at 9:51 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Jon  Seymour <jon.seymour@gmail.com> writes:\n>>\n>>> This patch allows git stash branch to work with stash-like commits created by git stash create.\n>>>\n>>> Two changes were required:\n>>>\n>>> * relax the pre-condition so that a stash stack is required if and only if a stash argument is not specified\n>>> * don't attempt to drop a stash argument that doesn't look like a stash reference.\n>>>\n>>>\n>>> Signed-off-by: Jon Seymour <jon.seymour@gmail.com>\n>>\n>> Please wrap very long lines.\n>>\n>>> diff --git a/git-stash.sh b/git-stash.sh\n>>> index 1d95447..432ddae 100755\n>>> --- a/git-stash.sh\n>>> +++ b/git-stash.sh\n>>> @@ -225,6 +225,12 @@ show_stash () {\n>>>       git diff $flags $b_commit $w_commit\n>>>  }\n>>>\n>>> +if_stash_ref() {\n>>> +     ref=\"$1\"\n>>> +     shift\n>>> +     test \"${ref#stash}\" = \"${ref}\" -a \"${ref#$ref_stash}\" = \"${ref}\" || \"$@\"\n>>> +}\n>>\n>> The interface to this function looks a rather bad taste to me; wouldn't it\n>> look more natural if the callers can say:\n>>\n>>        if stash_ref $it\n>>        then\n>>                do this\n>>        fi\n>>\n>> Your criteria used here is that the given parameter does not begin with\n>> \"stash\" nor \"refs/stash\".  If it begins with either of these two strings,\n>> the \"test\" fails and \"$@\" is run.  Wouldn't this produce a false hit if\n>> you kept a handcrafted stash-looking commit with a tag \"stash-42\" or\n>> something?\n>>\n>> It may make more sense to give \"stash drop\" an option to be silent if\n>> the given parameter is not on the list to begin with, perhaps?\n>>\n>>\n>\n"},{"id":"147241","messageId":"7vtyn8ddhv.fsf@alter.siamese.dyndns.org","threadId":"24613","inReplyTo":"AANLkTi=FbF3X83uLjBDoCYUCuig3udTzZCXuE=1_w-Ld@mail.gmail.com","subject":"Re: [PATCH v2 2/3] stash: Allow git stash branch to process commits that look like stashes but are not stash references.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-08-05T20:13:32Z","receivedAt":"2010-08-05T20:13:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jon Seymour <jon.seymour@gmail.com> writes:\n\n> One question about test patches. Are you ok with test_expect_failure\n> tests that document the expected failure of a feature yet to be\n> developed, followed by the feature, followed by the patch that makes\n> the tests into test_expect_success tests, or would you prefer to see\n> the pre- and post- test patches rolled into a single test that is\n> delivered after the feature patch?\n\nI think a single patch that implements the feature and at the same time\nprotects it with new tests from getting broken by others would be the best\nform.\n"}]}