Bug: git stash store can create stash entries that can't be dropped

Bug: git stash store can create stash entries that can't be dropped

[Erik Cervin Edin]

Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.

What did you do before the bug happened? (Steps to reproduce your issue)

  git stash store HEAD && git stash drop

What did you expect to happen? (Expected behavior)

To either fail storing HEAD or the ability to drop the stash entry, even if it
wasn't created using
  git stash create

What happened instead? (Actual behavior)

A stash entry is created that cannot be dropped, because it's not
stash-like commit.

  git stash drop
  fatal: 'refs/stash@{0}' is not a stash-like commit

What's different between what you expected and what actually happened?

A corrupt entry is added to the stash and it cannot be removed, expect probably
  git stash clear

Anything else you want to add:

Any guidance in modifying the stash reflog so that I can remove the stash entry
manually, or other suggestions are appreciated.

Please review the rest of the bug report below.
You can delete any lines you don't wish to share.


[System Info]
git version:
git version 2.42.0
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 5.15.90.1-microsoft-standard-WSL2 #1 SMP Fri Jan 27
02:56:13 UTC 2023 x86_64
compiler info: gnuc: 11.4
libc info: glibc: 2.35
$SHELL (typically, interactive shell): /bin/bash


[Enabled Hooks]
pre-commit

Re: Bug: git stash store can create stash entries that can't be dropped

[Junio C Hamano]

Erik Cervin Edin <erik@cervined.in> writes:

> ... even if it wasn't created using git stash create

I am of two minds.  As "stash store" and "stash create" were
invented as, and have always been ever since, pretty much
implementation details of scripted "stash save", the user deserves
what they get when they abuse them: garbage-in, garbage-out.

> A stash entry is created that cannot be dropped, because it's not
> stash-like commit.
>
>   git stash drop
>   fatal: 'refs/stash@{0}' is not a stash-like commit

Yes, this is exactly what the user deserves.

Having said that, I agree that this shows an uneven UI.  The "drop"
command cares about what it is dropping and refuses if it is not a
stash-like thing, so it is understandable to wish "store" to also
care to the same degree.

It may be just the matter of doing something silly like this.  Not
even compile tested, but hopefully it is sufficient to convey the
idea.

 builtin/stash.c  | 6 ++++++
 t/t3903-stash.sh | 4 ++++
 2 files changed, 10 insertions(+)

diff --git c/builtin/stash.c w/builtin/stash.c
index 1ad496985a..4a6771c9f4 100644
--- c/builtin/stash.c
+++ w/builtin/stash.c
@@ -989,6 +989,12 @@ static int show_stash(int argc, const char **argv, const char *prefix)
 static int do_store_stash(const struct object_id *w_commit, const char *stash_msg,
 			  int quiet)
 {
+	struct stash_info info;
+	char revision[GIT_MAX_HEXSZ];
+
+	oid_to_hex_r(revision, w_commit);
+	assert_stash_like(&info, revision);
+
 	if (!stash_msg)
 		stash_msg = "Created via \"git stash store\".";
 
diff --git c/t/t3903-stash.sh w/t/t3903-stash.sh
index 0b3dfeaea2..30b64260a8 100755
--- c/t/t3903-stash.sh
+++ w/t/t3903-stash.sh
@@ -931,6 +931,10 @@ test_expect_success 'store called with invalid commit' '
 	test_must_fail git stash store foo
 '
 
+test_expect_success 'store called with non-stash commit' '
+	test_must_fail git stash store HEAD
+'
+
 test_expect_success 'store updates stash ref and reflog' '
 	git stash clear &&
 	git reset --hard &&

Re: Bug: git stash store can create stash entries that can't be dropped

[Erik Cervin Edin]

On Wed, Oct 11, 2023 at 6:29 PM Junio C Hamano <gitster@pobox.com> wrote:
> I am of two minds.  As "stash store" and "stash create" were
> invented as, and have always been ever since, pretty much
> implementation details of scripted "stash save", the user deserves
> what they get when they abuse them: garbage-in, garbage-out.

Fair. I knew what I was doing was probably not kosher. Still, the
documentation says

           git stash store [(-m | --message) <message>] [-q | --quiet] <commit>

and then later clarifying

           Store a given stash created via git stash create (which is
a dangling merge commit) in the stash ref, updating the stash reflog.
This is intended to be useful for scripts.
           It is probably not the command you want to use; see "push" above.

I knew git stash store is meant to be used with git stash create. But
I didn't expect to be able to create a stash entry that I wouldn't be
able to drop.

> Yes, this is exactly what the user deserves.
>
> Having said that, I agree that this shows an uneven UI.  The "drop"
> command cares about what it is dropping and refuses if it is not a
> stash-like thing, so it is understandable to wish "store" to also
> care to the same degree.

I can see why I deserve the mess I made for myself. But I'm also
inclined to think that if I've been allowed to make said mess, I
should similarly be allowed to clean it up, without having to manually
delete the entry from the stash reflog. (In case someone else finds
themselves in a similar mess, I just went into the
.git/logs/refs/stash and deleted line of the offending stash entry)

I think a more user-friendly behavior is to either prevent the user
from making the mess, or not prevent the user from cleaning it up.
Doing both seems a tad overzealous. Hard for me to say which is
better. For example, it's not immediately clear, to me, why git stash
drop cares about the validity of stash entries to be dropped. But
maybe there are edge cases where it's good to be defensive here.

[PATCH] stash: be careful what we store

[Junio C Hamano]

"git stash store" is meant to store what "git stash create"
produces, as these two are implementation details of the end-user
facing "git stash save" command.  Even though it is clearly
documented as such, users would try silly things like "git stash
store HEAD" to render their stash unusable.

Worse yet, because "git stash drop" does not allow such a stash
entry to be removed, "git stash clear" would be the only way to
recover from such a mishap.  Reuse the logic that allows "drop" to
refrain from working on such a stash entry to teach "store" to avoid
storing an object that is not a stash entry in the first place.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
  > It may be just the matter of doing something silly like this.
  > Not even compile tested, but hopefully it is sufficient to
  > convey the idea.

  Now it is at least compile-tested.

 builtin/stash.c  | 6 ++++++
 t/t3903-stash.sh | 4 ++++
 2 files changed, 10 insertions(+)

diff --git a/builtin/stash.c b/builtin/stash.c
index 3a4f9fd566..8073ef4019 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -977,6 +977,12 @@ static int show_stash(int argc, const char **argv, const char *prefix)
 static int do_store_stash(const struct object_id *w_commit, const char *stash_msg,
 			  int quiet)
 {
+	struct stash_info info;
+	char revision[GIT_MAX_HEXSZ];
+
+	oid_to_hex_r(revision, w_commit);
+	assert_stash_like(&info, revision);
+
 	if (!stash_msg)
 		stash_msg = "Created via \"git stash store\".";
 
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 376cc8f4ab..35c8569aea 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -931,6 +931,10 @@ test_expect_success 'store called with invalid commit' '
 	test_must_fail git stash store foo
 '
 
+test_expect_success 'store called with non-stash commit' '
+	test_must_fail git stash store HEAD
+'
+
 test_expect_success 'store updates stash ref and reflog' '
 	git stash clear &&
 	git reset --hard &&
-- 
2.42.0-345-gaab89be2eb