[BUG] git-stash does not handle branch name with slash correctly

The default (commit) message when creating a stash strips the beginning of
branch names if they contain a slash,
e.g. "WIP on 3.2.x: …" instead of "WIP on stable/3.2.x: …"

 From builtin/stash.c (in do_create_stash):

	branch_ref = resolve_ref_unsafe("HEAD", 0, NULL, &flags);
	if (flags & REF_ISSYMREF)
		branch_name = strrchr(branch_ref, '/') + 1;


Whereas git-legacy-stash has this (in create_stash):

	if branch=$(git symbolic-ref -q HEAD)
	then
		branch=${branch#refs/heads/}
	else
		branch='(no branch)'
	fi
	msg=$(printf '%s: %s' "$branch" "$head")

I think it should also strip only "refs/heads/" or use another method that
keeps the branch name intact.

(I have noticed this with a script/function that warns me when trying to
pop a stash to another branch than where it was stashed from initially,
which parses out the (original) branch name from this message.)


[System Info]
git version:
git version 2.33.1

On Wed, Nov 10, 2021 at 07:55:06PM +0100, Daniel Hahler wrote:

The default (commit) message when creating a stash strips the beginning of
branch names if they contain a slash,
e.g. "WIP on 3.2.x: …" instead of "WIP on stable/3.2.x: …"

From builtin/stash.c (in do_create_stash):

	branch_ref = resolve_ref_unsafe("HEAD", 0, NULL, &flags);
	if (flags & REF_ISSYMREF)
		branch_name = strrchr(branch_ref, '/') + 1;


Whereas git-legacy-stash has this (in create_stash):

	if branch=$(git symbolic-ref -q HEAD)
	then
		branch=${branch#refs/heads/}
	else
		branch='(no branch)'
	fi
	msg=$(printf '%s: %s' "$branch" "$head")

I think it should also strip only "refs/heads/" or use another method that
keeps the branch name intact.

Yes, the C behavior just seems wrong (and came as part of the C rewrite,
so doesn't seem intentional). Something like:

diff --git a/builtin/stash.c b/builtin/stash.c
index d441481d68..70dcb15cb7 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c

@@ -1334,7 +1334,7 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b
 
 	branch_ref = resolve_ref_unsafe("HEAD", 0, NULL, &flags);
 	if (flags & REF_ISSYMREF)
-		branch_name = strrchr(branch_ref, '/') + 1;
+		skip_prefix(branch_ref, "refs/heads/", &branch_name);
 	head_short_sha1 = find_unique_abbrev(&head_commit->object.oid,
 					     DEFAULT_ABBREV);
 	strbuf_addf(&msg, "%s: %s ", branch_name, head_short_sha1);

seems like the right fix. Do you want to try to work that into a patch
with a test?

-Peff

When generating a message for a stash, "git stash" only records the
part of the branch name to the right of the last "/". e.g. if HEAD is at
"foo/bar/baz", "git stash" generates a message prefixed with "WIP on
baz:" instead of "WIP on foo/bar/baz:".

Fix this by using skip_prefix() to skip "refs/heads/" instead of looking
for the last instance of "/".

Reported-by: Kraymer <redacted>
Reported-by: Daniel Hahler <redacted>
Helped-by: Jeff King [off-list ref]
Signed-off-by: Glen Choo <redacted>
---
I prepared this fix before checking the mailing list for any bug
reports; turns out that there are at least two existing reports.

My fix happens to be exactly the same as what Peff suggested, with the
additional test that he asked for.

 builtin/stash.c  |  2 +-
 t/t3903-stash.sh | 11 +++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index 1ef2017c59..01f072a2fb 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c

@@ -1332,7 +1332,7 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b
 
 	branch_ref = resolve_ref_unsafe("HEAD", 0, NULL, &flags);
 	if (flags & REF_ISSYMREF)
-		branch_name = strrchr(branch_ref, '/') + 1;
+		skip_prefix(branch_ref, "refs/heads/", &branch_name);
 	head_short_sha1 = find_unique_abbrev(&head_commit->object.oid,
 					     DEFAULT_ABBREV);
 	strbuf_addf(&msg, "%s: %s ", branch_name, head_short_sha1);

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 686747e55a..bf83fb940e 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh

@@ -1042,6 +1042,17 @@ test_expect_success 'create stores correct message' '
 	test_cmp expect actual
 '
 
+test_expect_success 'create when branch name has /' '
+	test_when_finished "git checkout main" &&
+	git checkout -b some/topic &&
+	>foo &&
+	git add foo &&
+	STASH_ID=$(git stash create "create test message") &&
+	echo "On some/topic: create test message" >expect &&
+	git show --pretty=%s -s ${STASH_ID} >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'create with multiple arguments for the message' '
 	>foo &&
 	git add foo &&

base-commit: 89bece5c8c96f0b962cfc89e63f82d603fd60bed
-- 
2.33.GIT

Glen Choo [off-list ref] writes:

 	branch_ref = resolve_ref_unsafe("HEAD", 0, NULL, &flags);
 	if (flags & REF_ISSYMREF)
-		branch_name = strrchr(branch_ref, '/') + 1;
+		skip_prefix(branch_ref, "refs/heads/", &branch_name);

The branch_name variable is initialized to a constant string "(no branch)",
so if HEAD is poihnting elsewhere (which you could do manually),
skip_prefix() would fail and leave branch_name intact, which would
give us the desirable outcome, too.

Looking good.

Junio C Hamano [off-list ref] writes:

Glen Choo [off-list ref] writes:

quoted

 	branch_ref = resolve_ref_unsafe("HEAD", 0, NULL, &flags);
 	if (flags & REF_ISSYMREF)
-		branch_name = strrchr(branch_ref, '/') + 1;
+		skip_prefix(branch_ref, "refs/heads/", &branch_name);

The branch_name variable is initialized to a constant string "(no branch)",
so if HEAD is poihnting elsewhere (which you could do manually),
skip_prefix() would fail and leave branch_name intact, which would
give us the desirable outcome, too.

Looking good.

Hm, did we ever pick this up? I dug through the old "What's Cooking"
mails and didn't find any mention of this.

Admittedly, this dropped off my radar until performance review season
reminded me of this. Though now that I say this, it sounds like I want
this for the sake of performance review :p

(Which is not the case btw, I just want to scratch my own itch :))