{"thread":{"id":"52346","subject":"[PATCH] name-rev: use skip_prefix() instead of starts_with()","startedAt":"2019-11-26T15:23:39Z","lastAt":"2019-11-26T16:07:40Z","messageCount":2,"participants":["René Scharfe","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"387085","messageId":"23925fba-9413-0596-b21a-f49aac922f88@web.de","threadId":"52346","inReplyTo":null,"subject":"[PATCH] name-rev: use skip_prefix() instead of starts_with()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-11-26T15:23:31Z","receivedAt":"2019-11-26T15:23:39Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Let skip_prefix() advance refname to get rid of two magic numbers.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n builtin/name-rev.c | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/name-rev.c b/builtin/name-rev.c\nindex b0f0776947..c261d661d7 100644\n--- a/builtin/name-rev.c\n+++ b/builtin/name-rev.c\n@@ -161,10 +161,8 @@ static const char *name_ref_abbrev(const char *refname, int shorten_unambiguous)\n {\n \tif (shorten_unambiguous)\n \t\trefname = shorten_unambiguous_ref(refname, 0);\n-\telse if (starts_with(refname, \"refs/heads/\"))\n-\t\trefname = refname + 11;\n-\telse if (starts_with(refname, \"refs/\"))\n-\t\trefname = refname + 5;\n+\telse if (!skip_prefix(refname, \"refs/heads/\", &refname))\n+\t\tskip_prefix(refname, \"refs/\", &refname);\n \treturn refname;\n }\n\n--\n2.24.0\n"},{"id":"387090","messageId":"20191126160737.GC25729@sigill.intra.peff.net","threadId":"52346","inReplyTo":"23925fba-9413-0596-b21a-f49aac922f88@web.de","subject":"Re: [PATCH] name-rev: use skip_prefix() instead of starts_with()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-26T16:07:37Z","receivedAt":"2019-11-26T16:07:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 26, 2019 at 04:23:31PM +0100, René Scharfe wrote:\n\n> diff --git a/builtin/name-rev.c b/builtin/name-rev.c\n> index b0f0776947..c261d661d7 100644\n> --- a/builtin/name-rev.c\n> +++ b/builtin/name-rev.c\n> @@ -161,10 +161,8 @@ static const char *name_ref_abbrev(const char *refname, int shorten_unambiguous)\n>  {\n>  \tif (shorten_unambiguous)\n>  \t\trefname = shorten_unambiguous_ref(refname, 0);\n> -\telse if (starts_with(refname, \"refs/heads/\"))\n> -\t\trefname = refname + 11;\n> -\telse if (starts_with(refname, \"refs/\"))\n> -\t\trefname = refname + 5;\n> +\telse if (!skip_prefix(refname, \"refs/heads/\", &refname))\n> +\t\tskip_prefix(refname, \"refs/\", &refname);\n>  \treturn refname;\n\nAnd this one is correct because we were already mutating the pointer.\nGood.\n\nCollapsing the conditional makes sense. IMHO it might be a little easier\nto follow if we keep else-if non-negated, like:\n\n  if (shorten_unambiguous)\n         refname = shorten_unambiguous(refname, 0);\n  else if (skip_prefix(refname, \"refs/heads/\", &refname))\n         ; /* refname already advanced */\n  else\n         skip_prefix(refname, \"refs/\", &refname);\n\nbut I'm fine with it either way.\n\nAlso, I think we are leaking the result of shorten_unambiguous_refename\nhere (the caller won't free it, as we return a const; but anyway we\nsometimes return a pointer into the existing const string so it wouldn't\nbe safe). That's all outside your patch, obviously.\n\n-Peff\n"}]}