{"thread":{"id":"56872","subject":"[BUG] git-stash does not handle branch name with slash correctly","startedAt":"2021-11-10T19:02:45Z","lastAt":"2022-02-24T07:13:53Z","messageCount":5,"participants":["Daniel Hahler","Jeff King","Glen Choo","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"440860","messageId":"693b55f9-3c0e-5a31-5a43-d6495d689779@hahler.de","threadId":"56872","inReplyTo":null,"subject":"[BUG] git-stash does not handle branch name with slash correctly","fromName":"Daniel Hahler","fromEmail":"git@thequod.de","sentAt":"2021-11-10T18:55:06Z","receivedAt":"2021-11-10T19:02:45Z","isPatch":false,"sender":{"key":"git@thequod.de","avatar":"https://gravatar.com/avatar/7ca36ccd9ac57f02a433e4ecf2e28e6e23abce8b005a408498f516b74232da67?d=mp&s=160"},"body":"The default (commit) message when creating a stash strips the beginning of\nbranch names if they contain a slash,\ne.g. \"WIP on 3.2.x: …\" instead of \"WIP on stable/3.2.x: …\"\n\n From builtin/stash.c (in do_create_stash):\n\n\tbranch_ref = resolve_ref_unsafe(\"HEAD\", 0, NULL, &flags);\n\tif (flags & REF_ISSYMREF)\n\t\tbranch_name = strrchr(branch_ref, '/') + 1;\n\n\nWhereas git-legacy-stash has this (in create_stash):\n\n\tif branch=$(git symbolic-ref -q HEAD)\n\tthen\n\t\tbranch=${branch#refs/heads/}\n\telse\n\t\tbranch='(no branch)'\n\tfi\n\tmsg=$(printf '%s: %s' \"$branch\" \"$head\")\n\nI think it should also strip only \"refs/heads/\" or use another method that\nkeeps the branch name intact.\n\n(I have noticed this with a script/function that warns me when trying to\npop a stash to another branch than where it was stashed from initially,\nwhich parses out the (original) branch name from this message.)\n\n\n[System Info]\ngit version:\ngit version 2.33.1\n"},{"id":"440872","messageId":"YYwxVO7A29FcN+hN@coredump.intra.peff.net","threadId":"56872","inReplyTo":"693b55f9-3c0e-5a31-5a43-d6495d689779@hahler.de","subject":"Re: [BUG] git-stash does not handle branch name with slash correctly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-10T20:53:40Z","receivedAt":"2021-11-10T20:53:49Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 10, 2021 at 07:55:06PM +0100, Daniel Hahler wrote:\n\n> The default (commit) message when creating a stash strips the beginning of\n> branch names if they contain a slash,\n> e.g. \"WIP on 3.2.x: …\" instead of \"WIP on stable/3.2.x: …\"\n> \n> From builtin/stash.c (in do_create_stash):\n> \n> \tbranch_ref = resolve_ref_unsafe(\"HEAD\", 0, NULL, &flags);\n> \tif (flags & REF_ISSYMREF)\n> \t\tbranch_name = strrchr(branch_ref, '/') + 1;\n> \n> \n> Whereas git-legacy-stash has this (in create_stash):\n> \n> \tif branch=$(git symbolic-ref -q HEAD)\n> \tthen\n> \t\tbranch=${branch#refs/heads/}\n> \telse\n> \t\tbranch='(no branch)'\n> \tfi\n> \tmsg=$(printf '%s: %s' \"$branch\" \"$head\")\n> \n> I think it should also strip only \"refs/heads/\" or use another method that\n> keeps the branch name intact.\n\nYes, the C behavior just seems wrong (and came as part of the C rewrite,\nso doesn't seem intentional). Something like:\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex d441481d68..70dcb15cb7 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -1334,7 +1334,7 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b\n \n \tbranch_ref = resolve_ref_unsafe(\"HEAD\", 0, NULL, &flags);\n \tif (flags & REF_ISSYMREF)\n-\t\tbranch_name = strrchr(branch_ref, '/') + 1;\n+\t\tskip_prefix(branch_ref, \"refs/heads/\", &branch_name);\n \thead_short_sha1 = find_unique_abbrev(&head_commit->object.oid,\n \t\t\t\t\t     DEFAULT_ABBREV);\n \tstrbuf_addf(&msg, \"%s: %s \", branch_name, head_short_sha1);\n\nseems like the right fix. Do you want to try to work that into a patch\nwith a test?\n\n-Peff\n"},{"id":"446817","messageId":"20220124205342.41450-1-chooglen@google.com","threadId":"56872","inReplyTo":"YYwxVO7A29FcN+hN@coredump.intra.peff.net","subject":"[PATCH] stash: strip \"refs/heads/\" with skip_prefix","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-01-24T20:53:42Z","receivedAt":"2022-01-24T22:34:08Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"When generating a message for a stash, \"git stash\" only records the\npart of the branch name to the right of the last \"/\". e.g. if HEAD is at\n\"foo/bar/baz\", \"git stash\" generates a message prefixed with \"WIP on\nbaz:\" instead of \"WIP on foo/bar/baz:\".\n\nFix this by using skip_prefix() to skip \"refs/heads/\" instead of looking\nfor the last instance of \"/\".\n\nReported-by: Kraymer <kraymer@gmail.com>\nReported-by: Daniel Hahler <git@thequod.de>\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Glen Choo <chooglen@google.com>\n---\nI prepared this fix before checking the mailing list for any bug\nreports; turns out that there are at least two existing reports.\n\nMy fix happens to be exactly the same as what Peff suggested, with the\nadditional test that he asked for.\n\n builtin/stash.c  |  2 +-\n t/t3903-stash.sh | 11 +++++++++++\n 2 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 1ef2017c59..01f072a2fb 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -1332,7 +1332,7 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b\n \n \tbranch_ref = resolve_ref_unsafe(\"HEAD\", 0, NULL, &flags);\n \tif (flags & REF_ISSYMREF)\n-\t\tbranch_name = strrchr(branch_ref, '/') + 1;\n+\t\tskip_prefix(branch_ref, \"refs/heads/\", &branch_name);\n \thead_short_sha1 = find_unique_abbrev(&head_commit->object.oid,\n \t\t\t\t\t     DEFAULT_ABBREV);\n \tstrbuf_addf(&msg, \"%s: %s \", branch_name, head_short_sha1);\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 686747e55a..bf83fb940e 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1042,6 +1042,17 @@ test_expect_success 'create stores correct message' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'create when branch name has /' '\n+\ttest_when_finished \"git checkout main\" &&\n+\tgit checkout -b some/topic &&\n+\t>foo &&\n+\tgit add foo &&\n+\tSTASH_ID=$(git stash create \"create test message\") &&\n+\techo \"On some/topic: create test message\" >expect &&\n+\tgit show --pretty=%s -s ${STASH_ID} >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'create with multiple arguments for the message' '\n \t>foo &&\n \tgit add foo &&\n\nbase-commit: 89bece5c8c96f0b962cfc89e63f82d603fd60bed\n-- \n2.33.GIT\n\n"},{"id":"446831","messageId":"xmqq7dao1e5m.fsf@gitster.g","threadId":"56872","inReplyTo":"20220124205342.41450-1-chooglen@google.com","subject":"Re: [PATCH] stash: strip \"refs/heads/\" with skip_prefix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-25T07:13:25Z","receivedAt":"2022-01-25T07:16:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo <chooglen@google.com> writes:\n\n>  \tbranch_ref = resolve_ref_unsafe(\"HEAD\", 0, NULL, &flags);\n>  \tif (flags & REF_ISSYMREF)\n> -\t\tbranch_name = strrchr(branch_ref, '/') + 1;\n> +\t\tskip_prefix(branch_ref, \"refs/heads/\", &branch_name);\n\nThe branch_name variable is initialized to a constant string \"(no branch)\",\nso if HEAD is poihnting elsewhere (which you could do manually),\nskip_prefix() would fail and leave branch_name intact, which would\ngive us the desirable outcome, too.\n\nLooking good.\n"},{"id":"449396","messageId":"kl6lsfs84u06.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"56872","inReplyTo":"xmqq7dao1e5m.fsf@gitster.g","subject":"Re: [PATCH] stash: strip \"refs/heads/\" with skip_prefix","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-02-24T07:13:29Z","receivedAt":"2022-02-24T07:13:53Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Glen Choo <chooglen@google.com> writes:\n>\n>>  \tbranch_ref = resolve_ref_unsafe(\"HEAD\", 0, NULL, &flags);\n>>  \tif (flags & REF_ISSYMREF)\n>> -\t\tbranch_name = strrchr(branch_ref, '/') + 1;\n>> +\t\tskip_prefix(branch_ref, \"refs/heads/\", &branch_name);\n>\n> The branch_name variable is initialized to a constant string \"(no branch)\",\n> so if HEAD is poihnting elsewhere (which you could do manually),\n> skip_prefix() would fail and leave branch_name intact, which would\n> give us the desirable outcome, too.\n>\n> Looking good.\n\nHm, did we ever pick this up? I dug through the old \"What's Cooking\"\nmails and didn't find any mention of this.\n\nAdmittedly, this dropped off my radar until performance review season\nreminded me of this. Though now that I say this, it sounds like I want\nthis for the sake of performance review :p\n\n(Which is not the case btw, I just want to scratch my own itch :))\n"}]}