| Message ID | 20220124205342.41450-1-chooglen@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ceaf037f617eb774bb8a451c1779dd9b8b12152a |
| Headers | show |
| Series | stash: strip "refs/heads/" with skip_prefix | expand |
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.
Junio C Hamano <gitster@pobox.com> writes: > 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 :))
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 &&
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(-) base-commit: 89bece5c8c96f0b962cfc89e63f82d603fd60bed