threads / bug / 56872

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

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

## tl;dr

5 messages between Nov 10, 2021 and Feb 24, 2022.

replies: 4people: 4as markdown or json

Daniel Hahler· Nov 10, 2021, 18:55 UTC · lore

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

Jeff King· Nov 10, 2021, 20:53 UTC · re: Daniel Hahler · lore

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

On Wed, Nov 10, 2021 at 07:55:06PM +0100, Daniel Hahler wrote:
Show 23 quoted lines
> 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
Glen Choo· Jan 24, 2022, 20:53 UTC · re: Jeff King · lore

[PATCH] stash: strip "refs/heads/" with skip_prefix

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 <kraymer@gmail.com>
Reported-by: Daniel Hahler <git@thequod.de>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Glen Choo <chooglen@google.com>
---
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
Junio C Hamano· Jan 25, 2022, 07:13 UTC · re: Glen Choo · lore

Re: [PATCH] stash: strip "refs/heads/" with skip_prefix

Glen Choo <chooglen@google.com> 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.
Glen Choo· Feb 24, 2022, 07:13 UTC · re: Junio C Hamano · lore

Re: [PATCH] stash: strip "refs/heads/" with skip_prefix

Junio C Hamano <gitster@pobox.com> writes:
Show 13 quoted lines
> Glen Choo <chooglen@google.com> 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.

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 :))

← back to recent threads