{"thread":{"id":"60341","subject":"Bug: git stash store can create stash entries that can't be dropped","startedAt":"2023-10-11T08:43:15Z","lastAt":"2023-10-11T20:44:13Z","messageCount":4,"participants":["Erik Cervin Edin","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"483039","messageId":"CA+JQ7M_effxh9BSOhF67N+rsvBVTULe0dWZzp-kq1yOiDq3+hQ@mail.gmail.com","threadId":"60341","inReplyTo":null,"subject":"Bug: git stash store can create stash entries that can't be dropped","fromName":"Erik Cervin Edin","fromEmail":"erik@cervined.in","sentAt":"2023-10-11T08:42:31Z","receivedAt":"2023-10-11T08:43:15Z","isPatch":false,"sender":{"key":"erik@cervined.in","avatar":null},"body":"Thank you for filling out a Git bug report!\nPlease answer the following questions to help us understand your issue.\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\n\n  git stash store HEAD && git stash drop\n\nWhat did you expect to happen? (Expected behavior)\n\nTo either fail storing HEAD or the ability to drop the stash entry, even if it\nwasn't created using\n  git stash create\n\nWhat happened instead? (Actual behavior)\n\nA stash entry is created that cannot be dropped, because it's not\nstash-like commit.\n\n  git stash drop\n  fatal: 'refs/stash@{0}' is not a stash-like commit\n\nWhat's different between what you expected and what actually happened?\n\nA corrupt entry is added to the stash and it cannot be removed, expect probably\n  git stash clear\n\nAnything else you want to add:\n\nAny guidance in modifying the stash reflog so that I can remove the stash entry\nmanually, or other suggestions are appreciated.\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n\n[System Info]\ngit version:\ngit version 2.42.0\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Linux 5.15.90.1-microsoft-standard-WSL2 #1 SMP Fri Jan 27\n02:56:13 UTC 2023 x86_64\ncompiler info: gnuc: 11.4\nlibc info: glibc: 2.35\n$SHELL (typically, interactive shell): /bin/bash\n\n\n[Enabled Hooks]\npre-commit\n"},{"id":"483056","messageId":"xmqqzg0pnmv5.fsf@gitster.g","threadId":"60341","inReplyTo":"CA+JQ7M_effxh9BSOhF67N+rsvBVTULe0dWZzp-kq1yOiDq3+hQ@mail.gmail.com","subject":"Re: Bug: git stash store can create stash entries that can't be dropped","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-11T16:29:50Z","receivedAt":"2023-10-11T16:29:59Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Cervin Edin <erik@cervined.in> writes:\n\n> ... even if it wasn't created using git stash create\n\nI am of two minds.  As \"stash store\" and \"stash create\" were\ninvented as, and have always been ever since, pretty much\nimplementation details of scripted \"stash save\", the user deserves\nwhat they get when they abuse them: garbage-in, garbage-out.\n\n> A stash entry is created that cannot be dropped, because it's not\n> stash-like commit.\n>\n>   git stash drop\n>   fatal: 'refs/stash@{0}' is not a stash-like commit\n\nYes, this is exactly what the user deserves.\n\nHaving said that, I agree that this shows an uneven UI.  The \"drop\"\ncommand cares about what it is dropping and refuses if it is not a\nstash-like thing, so it is understandable to wish \"store\" to also\ncare to the same degree.\n\nIt may be just the matter of doing something silly like this.  Not\neven compile tested, but hopefully it is sufficient to convey the\nidea.\n\n builtin/stash.c  | 6 ++++++\n t/t3903-stash.sh | 4 ++++\n 2 files changed, 10 insertions(+)\n\ndiff --git c/builtin/stash.c w/builtin/stash.c\nindex 1ad496985a..4a6771c9f4 100644\n--- c/builtin/stash.c\n+++ w/builtin/stash.c\n@@ -989,6 +989,12 @@ static int show_stash(int argc, const char **argv, const char *prefix)\n static int do_store_stash(const struct object_id *w_commit, const char *stash_msg,\n \t\t\t  int quiet)\n {\n+\tstruct stash_info info;\n+\tchar revision[GIT_MAX_HEXSZ];\n+\n+\toid_to_hex_r(revision, w_commit);\n+\tassert_stash_like(&info, revision);\n+\n \tif (!stash_msg)\n \t\tstash_msg = \"Created via \\\"git stash store\\\".\";\n \ndiff --git c/t/t3903-stash.sh w/t/t3903-stash.sh\nindex 0b3dfeaea2..30b64260a8 100755\n--- c/t/t3903-stash.sh\n+++ w/t/t3903-stash.sh\n@@ -931,6 +931,10 @@ test_expect_success 'store called with invalid commit' '\n \ttest_must_fail git stash store foo\n '\n \n+test_expect_success 'store called with non-stash commit' '\n+\ttest_must_fail git stash store HEAD\n+'\n+\n test_expect_success 'store updates stash ref and reflog' '\n \tgit stash clear &&\n \tgit reset --hard &&\n"},{"id":"483083","messageId":"CA+JQ7M9j5_1kJ2+E70+EXF6wFyPPovHnz=T26sQ2hOr0cyaO8g@mail.gmail.com","threadId":"60341","inReplyTo":"xmqqzg0pnmv5.fsf@gitster.g","subject":"Re: Bug: git stash store can create stash entries that can't be dropped","fromName":"Erik Cervin Edin","fromEmail":"erik@cervined.in","sentAt":"2023-10-11T19:44:07Z","receivedAt":"2023-10-11T19:44:49Z","isPatch":false,"sender":{"key":"erik@cervined.in","avatar":null},"body":"On Wed, Oct 11, 2023 at 6:29 PM Junio C Hamano <gitster@pobox.com> wrote:\n> I am of two minds.  As \"stash store\" and \"stash create\" were\n> invented as, and have always been ever since, pretty much\n> implementation details of scripted \"stash save\", the user deserves\n> what they get when they abuse them: garbage-in, garbage-out.\n\nFair. I knew what I was doing was probably not kosher. Still, the\ndocumentation says\n\n           git stash store [(-m | --message) <message>] [-q | --quiet] <commit>\n\nand then later clarifying\n\n           Store a given stash created via git stash create (which is\na dangling merge commit) in the stash ref, updating the stash reflog.\nThis is intended to be useful for scripts.\n           It is probably not the command you want to use; see \"push\" above.\n\nI knew git stash store is meant to be used with git stash create. But\nI didn't expect to be able to create a stash entry that I wouldn't be\nable to drop.\n\n> Yes, this is exactly what the user deserves.\n>\n> Having said that, I agree that this shows an uneven UI.  The \"drop\"\n> command cares about what it is dropping and refuses if it is not a\n> stash-like thing, so it is understandable to wish \"store\" to also\n> care to the same degree.\n\nI can see why I deserve the mess I made for myself. But I'm also\ninclined to think that if I've been allowed to make said mess, I\nshould similarly be allowed to clean it up, without having to manually\ndelete the entry from the stash reflog. (In case someone else finds\nthemselves in a similar mess, I just went into the\n.git/logs/refs/stash and deleted line of the offending stash entry)\n\nI think a more user-friendly behavior is to either prevent the user\nfrom making the mess, or not prevent the user from cleaning it up.\nDoing both seems a tad overzealous. Hard for me to say which is\nbetter. For example, it's not immediately clear, to me, why git stash\ndrop cares about the validity of stash entries to be dropped. But\nmaybe there are edge cases where it's good to be defensive here.\n"},{"id":"483087","messageId":"xmqqbkd4lwj0.fsf_-_@gitster.g","threadId":"60341","inReplyTo":"xmqqzg0pnmv5.fsf@gitster.g","subject":"[PATCH] stash: be careful what we store","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-11T20:44:03Z","receivedAt":"2023-10-11T20:44:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"git stash store\" is meant to store what \"git stash create\"\nproduces, as these two are implementation details of the end-user\nfacing \"git stash save\" command.  Even though it is clearly\ndocumented as such, users would try silly things like \"git stash\nstore HEAD\" to render their stash unusable.\n\nWorse yet, because \"git stash drop\" does not allow such a stash\nentry to be removed, \"git stash clear\" would be the only way to\nrecover from such a mishap.  Reuse the logic that allows \"drop\" to\nrefrain from working on such a stash entry to teach \"store\" to avoid\nstoring an object that is not a stash entry in the first place.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n  > It may be just the matter of doing something silly like this.\n  > Not even compile tested, but hopefully it is sufficient to\n  > convey the idea.\n\n  Now it is at least compile-tested.\n\n builtin/stash.c  | 6 ++++++\n t/t3903-stash.sh | 4 ++++\n 2 files changed, 10 insertions(+)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 3a4f9fd566..8073ef4019 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -977,6 +977,12 @@ static int show_stash(int argc, const char **argv, const char *prefix)\n static int do_store_stash(const struct object_id *w_commit, const char *stash_msg,\n \t\t\t  int quiet)\n {\n+\tstruct stash_info info;\n+\tchar revision[GIT_MAX_HEXSZ];\n+\n+\toid_to_hex_r(revision, w_commit);\n+\tassert_stash_like(&info, revision);\n+\n \tif (!stash_msg)\n \t\tstash_msg = \"Created via \\\"git stash store\\\".\";\n \ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 376cc8f4ab..35c8569aea 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -931,6 +931,10 @@ test_expect_success 'store called with invalid commit' '\n \ttest_must_fail git stash store foo\n '\n \n+test_expect_success 'store called with non-stash commit' '\n+\ttest_must_fail git stash store HEAD\n+'\n+\n test_expect_success 'store updates stash ref and reflog' '\n \tgit stash clear &&\n \tgit reset --hard &&\n-- \n2.42.0-345-gaab89be2eb\n\n"}]}