{"thread":{"id":"33533","subject":"[PATCH] stash: tighten IS_STASH_LIKE logic","startedAt":"2013-04-18T07:38:37Z","lastAt":"2013-04-19T18:54:23Z","messageCount":6,"participants":["Ramkumar Ramachandra","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"214686","messageId":"1366270717-19929-1-git-send-email-artagnon@gmail.com","threadId":"33533","inReplyTo":null,"subject":"[PATCH] stash: tighten IS_STASH_LIKE logic","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-04-18T07:38:37Z","receivedAt":"2013-04-18T07:38:37Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Currently, 'git stash show' and 'git stash apply' can show/apply any\nmerge commit, as the test for setting IS_STASH_LIKE simply asserts if\nthe commit is a merge.  Improve the situation by asserting if the\nindex_commit and the worktree_commit are based off the same commit, by\nchecking that $REV^1 is equal to $REV^2^1.\n\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n git-stash.sh     |  3 ++-\n t/t3903-stash.sh | 11 +++++++++++\n 2 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex bbefdf6..d0428a8 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -366,13 +366,14 @@ parse_flags_and_rev()\n \t}\n \n \ti_commit=$(git rev-parse --quiet --verify $REV^2 2>/dev/null) &&\n-\tset -- $(git rev-parse $REV $REV^1 $REV: $REV^1: $REV^2: 2>/dev/null) &&\n+\tset -- $(git rev-parse $REV $REV^1 $REV: $REV^1: $REV^2: $REV^2^1 2>/dev/null) &&\n \ts=$1 &&\n \tw_commit=$1 &&\n \tb_commit=$2 &&\n \tw_tree=$3 &&\n \tb_tree=$4 &&\n \ti_tree=$5 &&\n+\ttest $b_commit = $6 &&\n \tIS_STASH_LIKE=t &&\n \ttest \"$ref_stash\" = \"$(git rev-parse --symbolic-full-name \"${REV%@*}\")\" &&\n \tIS_STASH_REF=t\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 5dfbda7..11bcd72 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -637,4 +637,15 @@ test_expect_success 'stash where working directory contains \"HEAD\" file' '\n \ttest_cmp output expect\n '\n \n+test_expect_success 'show refuses to show any random merge commit' '\n+\tgit stash clear &&\n+\tgit reset --hard &&\n+\tgit checkout -b quux &&\n+\ttest_commit bar &&\n+\tgit checkout - &&\n+\ttest_commit foo &&\n+\tgit merge quux &&\n+\ttest_must_fail git stash show HEAD\n+'\n+\n test_done\n-- \n1.8.2.1.423.g4fb5c0a.dirty\n"},{"id":"214766","messageId":"7vk3nz4h20.fsf@alter.siamese.dyndns.org","threadId":"33533","inReplyTo":"1366270717-19929-1-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH] stash: tighten IS_STASH_LIKE logic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-18T22:20:23Z","receivedAt":"2013-04-18T22:20:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n> Currently, 'git stash show' and 'git stash apply' can show/apply any\n> merge commit, as the test for setting IS_STASH_LIKE simply asserts if\n> the commit is a merge.  Improve the situation by asserting if the\n> index_commit and the worktree_commit are based off the same commit, by\n> checking that $REV^1 is equal to $REV^2^1.\n\nWhen was the last time you tried to run \"git stash apply next\"?\n\nThe current code parses the ancestors and trees because the real\nwork needs them, and the 'does this look like a stash?' is a side\neffect, but reading it^2^1 and comparing with it^1 is a pure\noverhead for the sake of checking.  It looks like a futile mental\nmasturbation to solve theoretical non-issue to me.\n\nIs it worth it?\n"},{"id":"214808","messageId":"CALkWK0m_Nt8SFdjGKWWHtBJ+HVsk095ukjZ0N=KBt5q+or5XFQ@mail.gmail.com","threadId":"33533","inReplyTo":"7vk3nz4h20.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] stash: tighten IS_STASH_LIKE logic","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-04-19T05:57:04Z","receivedAt":"2013-04-19T05:57:04Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> [...]\n\nI'm curious.  Why are you going back on what you said just one day\nago?  What changed?\n\nIn a previous email, you wrote:\n> You are free to try to think of a way to tighten the implemention to\n> reject a random two-or-three parent merge commit that is not a\n> product of \"stash create\".  People already have looked at this code\n> since it was written, and didn't find a reasonable way to tighten it\n> without triggering false negatives, so I wouldn't be surprised if\n> anybody tried it again today and failed.\n\nSo, my patch is not a \"reasonable\" way to achieve this?\n\n> When was the last time you tried to run \"git stash apply next\"?\n\nMy patch is not solving an end-user problem.  Think of it as a source\ncode comment: to answer the question \"what kind of commit does stash\ncreate make?\", the reader simple has to look at when IS_STASH_LIKE is\nset.  It's helping make the source code clearer.  Previously,\nIS_STASH_LIKE might as well have been named IS_MERGE_COMMIT, and\nnothing would've changed.  The reader will wonder what IS_MERGE_COMMIT\nhas to do with stashes, so we named it IS_STASH_LIKE.  This is another\nminor improvement in the same spirit.\n\n> Is it worth it?\n\nIs it worth what?  What are we losing?\n"},{"id":"214853","messageId":"7v38um2yvr.fsf@alter.siamese.dyndns.org","threadId":"33533","inReplyTo":"CALkWK0m_Nt8SFdjGKWWHtBJ+HVsk095ukjZ0N=KBt5q+or5XFQ@mail.gmail.com","subject":"Re: [PATCH] stash: tighten IS_STASH_LIKE logic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-19T17:50:32Z","receivedAt":"2013-04-19T17:50:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n> My patch is not solving an end-user problem.  Think of it as a source\n> code comment: to answer the question \"what kind of commit does stash\n> create make?\",\n\nThere already is sufficient comment that explains how a stash commit\nis constructed, isn't it?\n\nI may have forgotten to say this, but we were helped by the logic\nthat makes sure we can read what we need to carry out the operation\nand nothing more in the then-current (which is the same as current)\ncode that was written before we added --include-untracked.  If the\ncheck were enforcing that a stash-like must be a two parent merge,\nwhich may have seemed reasonable back when the stash-like logic was\nwritten, it would have been more painful to add three parent\npossibility while still allowing people to use different vintage of\nGit in the same repository.\n\nInsisting on the i-commit and w-commit to have exactly the same\nparent would mean that somebody who wants to improve create-stash\nhas one less option---even when the easiest and/or cleanest way to\nimprove create-stash for the particular goal were to record these\ntwo commits based on a different parents, the requirement the patch\nis trying to add here will prevent her from doing so and force her\nto work around the pointless (read: not necessary when the current\ncode shows or applies the stash) check.\n\n>> Is it worth it?\n>\n> Is it worth what?  What are we losing?\n\nThe risk of actually closing the door for future developers.  That\nis a downside of this patch, if we were to apply it.\n\nAnd having to spend braincycles to worry about what door we may be\nunintentionally closing.  That's a downside of even discussing this\npatch.\n"},{"id":"214859","messageId":"CALkWK0maM5ocSoSGy8OdL+hNW9mm_s1m1h+xJeCq-JKfTMW0Uw@mail.gmail.com","threadId":"33533","inReplyTo":"7v38um2yvr.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] stash: tighten IS_STASH_LIKE logic","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-04-19T18:22:00Z","receivedAt":"2013-04-19T18:22:00Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> The risk of actually closing the door for future developers.  That\n> is a downside of this patch, if we were to apply it.\n\nOkay, no issues.  Drop it.\n\nConsider documenting the fact that IS_STASH_LIKE is merely a\ncontextual synonym for IS_MERGE_COMMIT somewhere though.\n"},{"id":"214862","messageId":"7vd2tq1hcw.fsf@alter.siamese.dyndns.org","threadId":"33533","inReplyTo":"CALkWK0maM5ocSoSGy8OdL+hNW9mm_s1m1h+xJeCq-JKfTMW0Uw@mail.gmail.com","subject":"Re: [PATCH] stash: tighten IS_STASH_LIKE logic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-19T18:54:23Z","receivedAt":"2013-04-19T18:54:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n> Junio C Hamano wrote:\n>> The risk of actually closing the door for future developers.  That\n>> is a downside of this patch, if we were to apply it.\n>\n> Okay, no issues.  Drop it.\n>\n> Consider documenting the fact that IS_STASH_LIKE is merely a\n> contextual synonym for IS_MERGE_COMMIT somewhere though.\n\nDocumenting what IS_STASH_LIKE means and what it is meant to be used\nfor would add a lot of value, if it is not done already, but I doubt\nit adds much value to document that it _happens to be implemented_\nas \"is it a merge commit?\" in the current version.\n"}]}