{"thread":{"id":"45482","subject":"[PATCH] refs.c: use skip_prefix() in prettify_refname()","startedAt":"2017-03-23T15:50:22Z","lastAt":"2017-03-23T20:06:31Z","messageCount":11,"participants":["SZEDER Gábor","René Scharfe","Jeff King","Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"315102","messageId":"20170323155012.6148-1-szeder.dev@gmail.com","threadId":"45482","inReplyTo":null,"subject":"[PATCH] refs.c: use skip_prefix() in prettify_refname()","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-03-23T15:50:12Z","receivedAt":"2017-03-23T15:50:22Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"This eliminates three magic numbers.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n refs.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex e7606716d..0272e332c 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -366,11 +366,11 @@ int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data)\n \n const char *prettify_refname(const char *name)\n {\n-\treturn name + (\n-\t\tstarts_with(name, \"refs/heads/\") ? 11 :\n-\t\tstarts_with(name, \"refs/tags/\") ? 10 :\n-\t\tstarts_with(name, \"refs/remotes/\") ? 13 :\n-\t\t0);\n+\tif (skip_prefix(name, \"refs/heads/\", &name) ||\n+\t    skip_prefix(name, \"refs/tags/\", &name) ||\n+\t    skip_prefix(name, \"refs/remotes/\", &name))\n+\t\t; /* nothing */\n+\treturn name;\n }\n \n static const char *ref_rev_parse_rules[] = {\n-- \n2.12.1.485.g1616aa492\n\n"},{"id":"315152","messageId":"4382e211-63f6-811f-6f33-1cf5d2c087a8@web.de","threadId":"45482","inReplyTo":"20170323155012.6148-1-szeder.dev@gmail.com","subject":"Re: [PATCH] refs.c: use skip_prefix() in prettify_refname()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-03-23T19:18:26Z","receivedAt":"2017-03-23T19:18:41Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 23.03.2017 um 16:50 schrieb SZEDER Gábor:\n> This eliminates three magic numbers.\n>\n> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n> ---\n>  refs.c | 10 +++++-----\n>  1 file changed, 5 insertions(+), 5 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index e7606716d..0272e332c 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -366,11 +366,11 @@ int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data)\n>\n>  const char *prettify_refname(const char *name)\n>  {\n> -\treturn name + (\n> -\t\tstarts_with(name, \"refs/heads/\") ? 11 :\n> -\t\tstarts_with(name, \"refs/tags/\") ? 10 :\n> -\t\tstarts_with(name, \"refs/remotes/\") ? 13 :\n> -\t\t0);\n> +\tif (skip_prefix(name, \"refs/heads/\", &name) ||\n> +\t    skip_prefix(name, \"refs/tags/\", &name) ||\n> +\t    skip_prefix(name, \"refs/remotes/\", &name))\n> +\t\t; /* nothing */\n> +\treturn name;\n\nNice, but why add the \"if\" when it's doing nothing?\n\nRené\n"},{"id":"315153","messageId":"20170323192313.ytr56pjpnillnh63@sigill.intra.peff.net","threadId":"45482","inReplyTo":"4382e211-63f6-811f-6f33-1cf5d2c087a8@web.de","subject":"Re: [PATCH] refs.c: use skip_prefix() in prettify_refname()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-23T19:23:13Z","receivedAt":"2017-03-23T19:24:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 23, 2017 at 08:18:26PM +0100, René Scharfe wrote:\n\n> Am 23.03.2017 um 16:50 schrieb SZEDER Gábor:\n> > This eliminates three magic numbers.\n> > \n> > Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n> > ---\n> >  refs.c | 10 +++++-----\n> >  1 file changed, 5 insertions(+), 5 deletions(-)\n> > \n> > diff --git a/refs.c b/refs.c\n> > index e7606716d..0272e332c 100644\n> > --- a/refs.c\n> > +++ b/refs.c\n> > @@ -366,11 +366,11 @@ int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data)\n> > \n> >  const char *prettify_refname(const char *name)\n> >  {\n> > -\treturn name + (\n> > -\t\tstarts_with(name, \"refs/heads/\") ? 11 :\n> > -\t\tstarts_with(name, \"refs/tags/\") ? 10 :\n> > -\t\tstarts_with(name, \"refs/remotes/\") ? 13 :\n> > -\t\t0);\n> > +\tif (skip_prefix(name, \"refs/heads/\", &name) ||\n> > +\t    skip_prefix(name, \"refs/tags/\", &name) ||\n> > +\t    skip_prefix(name, \"refs/remotes/\", &name))\n> > +\t\t; /* nothing */\n> > +\treturn name;\n> \n> Nice, but why add the \"if\" when it's doing nothing?\n\nIt's short-circuiting in the conditional.\n\n-Peff\n"},{"id":"315157","messageId":"CAGZ79kZ-yP=6bKnDg8zKjebDkDdQ4ZEur0sC+CCcv5OTdEC3RA@mail.gmail.com","threadId":"45482","inReplyTo":"20170323192313.ytr56pjpnillnh63@sigill.intra.peff.net","subject":"Re: [PATCH] refs.c: use skip_prefix() in prettify_refname()","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-23T19:31:18Z","receivedAt":"2017-03-23T19:31:24Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Mar 23, 2017 at 12:23 PM, Jeff King <peff@peff.net> wrote:\n> On Thu, Mar 23, 2017 at 08:18:26PM +0100, René Scharfe wrote:\n>\n>> Am 23.03.2017 um 16:50 schrieb SZEDER Gábor:\n>> > This eliminates three magic numbers.\n>> >\n>> > Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n>> > ---\n>> >  refs.c | 10 +++++-----\n>> >  1 file changed, 5 insertions(+), 5 deletions(-)\n>> >\n>> > diff --git a/refs.c b/refs.c\n>> > index e7606716d..0272e332c 100644\n>> > --- a/refs.c\n>> > +++ b/refs.c\n>> > @@ -366,11 +366,11 @@ int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data)\n>> >\n>> >  const char *prettify_refname(const char *name)\n>> >  {\n>> > -   return name + (\n>> > -           starts_with(name, \"refs/heads/\") ? 11 :\n>> > -           starts_with(name, \"refs/tags/\") ? 10 :\n>> > -           starts_with(name, \"refs/remotes/\") ? 13 :\n>> > -           0);\n>> > +   if (skip_prefix(name, \"refs/heads/\", &name) ||\n>> > +       skip_prefix(name, \"refs/tags/\", &name) ||\n>> > +       skip_prefix(name, \"refs/remotes/\", &name))\n>> > +           ; /* nothing */\n>> > +   return name;\n>>\n>> Nice, but why add the \"if\" when it's doing nothing?\n>\n> It's short-circuiting in the conditional.\n\nYou do not need a conditional to short circuit things.\n|| works outside an if, too. ;)\n\nAnyway, maybe it's worth spelling it out with an if .. else if\ncascade for readability?\n\nAfter your comment of short-circuiting this code is pretty clear,\nso maybe just a small comment would do?\n\nThanks,\nStefan\n\n>\n> -Peff\n"},{"id":"315158","messageId":"xmqqvaqzbxfx.fsf@gitster.mtv.corp.google.com","threadId":"45482","inReplyTo":"20170323192313.ytr56pjpnillnh63@sigill.intra.peff.net","subject":"Re: [PATCH] refs.c: use skip_prefix() in prettify_refname()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-23T19:33:06Z","receivedAt":"2017-03-23T19:33:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Mar 23, 2017 at 08:18:26PM +0100, René Scharfe wrote:\n>\n>> Am 23.03.2017 um 16:50 schrieb SZEDER Gábor:\n>> > This eliminates three magic numbers.\n>> > \n>> > Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n>> > ---\n>> >  refs.c | 10 +++++-----\n>> >  1 file changed, 5 insertions(+), 5 deletions(-)\n>> > \n>> > diff --git a/refs.c b/refs.c\n>> > index e7606716d..0272e332c 100644\n>> > --- a/refs.c\n>> > +++ b/refs.c\n>> > @@ -366,11 +366,11 @@ int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data)\n>> > \n>> >  const char *prettify_refname(const char *name)\n>> >  {\n>> > -\treturn name + (\n>> > -\t\tstarts_with(name, \"refs/heads/\") ? 11 :\n>> > -\t\tstarts_with(name, \"refs/tags/\") ? 10 :\n>> > -\t\tstarts_with(name, \"refs/remotes/\") ? 13 :\n>> > -\t\t0);\n>> > +\tif (skip_prefix(name, \"refs/heads/\", &name) ||\n>> > +\t    skip_prefix(name, \"refs/tags/\", &name) ||\n>> > +\t    skip_prefix(name, \"refs/remotes/\", &name))\n>> > +\t\t; /* nothing */\n>> > +\treturn name;\n>> \n>> Nice, but why add the \"if\" when it's doing nothing?\n>\n> It's short-circuiting in the conditional.\n\nI think René meant this:\n\n     /* just for side effects */\n     skip_prefix(name, \"refs/heads/\", &name) ||\n     skip_prefix(name, \"refs/tags/\", &name) ||\n     skip_prefix(name, \"refs/remotes/\", &name);\n\n     return name;\n\nwhich still short-sircuits, even though I do think it looks\nstrange; \"correct but strange\".\n"},{"id":"315160","messageId":"4424eeeb-7eea-ed41-c031-ba7915cb93ab@web.de","threadId":"45482","inReplyTo":"xmqqvaqzbxfx.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] refs.c: use skip_prefix() in prettify_refname()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-03-23T19:36:29Z","receivedAt":"2017-03-23T19:37:28Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 23.03.2017 um 20:33 schrieb Junio C Hamano:\n> Jeff King <peff@peff.net> writes:\n>\n>> On Thu, Mar 23, 2017 at 08:18:26PM +0100, René Scharfe wrote:\n>>\n>>> Am 23.03.2017 um 16:50 schrieb SZEDER Gábor:\n>>>> This eliminates three magic numbers.\n>>>>\n>>>> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n>>>> ---\n>>>>  refs.c | 10 +++++-----\n>>>>  1 file changed, 5 insertions(+), 5 deletions(-)\n>>>>\n>>>> diff --git a/refs.c b/refs.c\n>>>> index e7606716d..0272e332c 100644\n>>>> --- a/refs.c\n>>>> +++ b/refs.c\n>>>> @@ -366,11 +366,11 @@ int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data)\n>>>>\n>>>>  const char *prettify_refname(const char *name)\n>>>>  {\n>>>> -\treturn name + (\n>>>> -\t\tstarts_with(name, \"refs/heads/\") ? 11 :\n>>>> -\t\tstarts_with(name, \"refs/tags/\") ? 10 :\n>>>> -\t\tstarts_with(name, \"refs/remotes/\") ? 13 :\n>>>> -\t\t0);\n>>>> +\tif (skip_prefix(name, \"refs/heads/\", &name) ||\n>>>> +\t    skip_prefix(name, \"refs/tags/\", &name) ||\n>>>> +\t    skip_prefix(name, \"refs/remotes/\", &name))\n>>>> +\t\t; /* nothing */\n>>>> +\treturn name;\n>>>\n>>> Nice, but why add the \"if\" when it's doing nothing?\n>>\n>> It's short-circuiting in the conditional.\n>\n> I think René meant this:\n>\n>      /* just for side effects */\n>      skip_prefix(name, \"refs/heads/\", &name) ||\n>      skip_prefix(name, \"refs/tags/\", &name) ||\n>      skip_prefix(name, \"refs/remotes/\", &name);\n>\n>      return name;\n>\n> which still short-sircuits, even though I do think it looks\n> strange; \"correct but strange\".\n\nYes.  At least to me it looks less strange than the same lines wrapped \nin \"if ... /* nothing */\".\n\nRené\n"},{"id":"315162","messageId":"20170323193919.twwfwdoulo2hjz24@sigill.intra.peff.net","threadId":"45482","inReplyTo":"xmqqvaqzbxfx.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] refs.c: use skip_prefix() in prettify_refname()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-23T19:39:19Z","receivedAt":"2017-03-23T19:39:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 23, 2017 at 12:33:06PM -0700, Junio C Hamano wrote:\n\n> >> Nice, but why add the \"if\" when it's doing nothing?\n> >\n> > It's short-circuiting in the conditional.\n> \n> I think René meant this:\n> \n>      /* just for side effects */\n>      skip_prefix(name, \"refs/heads/\", &name) ||\n>      skip_prefix(name, \"refs/tags/\", &name) ||\n>      skip_prefix(name, \"refs/remotes/\", &name);\n> \n>      return name;\n> \n> which still short-sircuits, even though I do think it looks\n> strange; \"correct but strange\".\n\nAnd it causes the compiler to complain that the value is not used.\n\n-Peff\n"},{"id":"315163","messageId":"xmqqr31nbx3h.fsf@gitster.mtv.corp.google.com","threadId":"45482","inReplyTo":"4424eeeb-7eea-ed41-c031-ba7915cb93ab@web.de","subject":"Re: [PATCH] refs.c: use skip_prefix() in prettify_refname()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-23T19:40:34Z","receivedAt":"2017-03-23T19:40:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n>> I think René meant this:\n>>\n>>      /* just for side effects */\n>>      skip_prefix(name, \"refs/heads/\", &name) ||\n>>      skip_prefix(name, \"refs/tags/\", &name) ||\n>>      skip_prefix(name, \"refs/remotes/\", &name);\n>>\n>>      return name;\n>>\n>> which still short-sircuits, even though I do think it looks\n>> strange; \"correct but strange\".\n>\n> Yes.  At least to me it looks less strange than the same lines wrapped\n> in \"if ... /* nothing */\".\n\nYup, after looking at it again, it does not look so \"strange\" to me.\nI probably should have said \"unusual but correct\", not \"correct but\nstrange\".\n"},{"id":"315165","messageId":"xmqqmvcbbvy3.fsf@gitster.mtv.corp.google.com","threadId":"45482","inReplyTo":"20170323193919.twwfwdoulo2hjz24@sigill.intra.peff.net","subject":"Re: [PATCH] refs.c: use skip_prefix() in prettify_refname()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-23T20:05:24Z","receivedAt":"2017-03-23T20:06:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Mar 23, 2017 at 12:33:06PM -0700, Junio C Hamano wrote:\n>\n>> >> Nice, but why add the \"if\" when it's doing nothing?\n>> >\n>> > It's short-circuiting in the conditional.\n>> \n>> I think René meant this:\n>> \n>>      /* just for side effects */\n>>      skip_prefix(name, \"refs/heads/\", &name) ||\n>>      skip_prefix(name, \"refs/tags/\", &name) ||\n>>      skip_prefix(name, \"refs/remotes/\", &name);\n>> \n>>      return name;\n>> \n>> which still short-sircuits, even though I do think it looks\n>> strange; \"correct but strange\".\n>\n> And it causes the compiler to complain that the value is not used.\n\nAhh.  OK.\n\nIn any case, I've queued the original with \"if\", which shouldn't\nhave that problem ;-)\n"},{"id":"315166","messageId":"CAM0VKjkAMrePks4JikNvzs_v71YFidKF7yj0aNVugZ-nzx1kig@mail.gmail.com","threadId":"45482","inReplyTo":"20170323193919.twwfwdoulo2hjz24@sigill.intra.peff.net","subject":"Re: [PATCH] refs.c: use skip_prefix() in prettify_refname()","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-03-23T20:06:04Z","receivedAt":"2017-03-23T20:06:10Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Mar 23, 2017 at 8:39 PM, Jeff King <peff@peff.net> wrote:\n> On Thu, Mar 23, 2017 at 12:33:06PM -0700, Junio C Hamano wrote:\n>\n>> >> Nice, but why add the \"if\" when it's doing nothing?\n>> >\n>> > It's short-circuiting in the conditional.\n>>\n>> I think René meant this:\n>>\n>>      /* just for side effects */\n>>      skip_prefix(name, \"refs/heads/\", &name) ||\n>>      skip_prefix(name, \"refs/tags/\", &name) ||\n>>      skip_prefix(name, \"refs/remotes/\", &name);\n>>\n>>      return name;\n\nThat was my first version indeed.  Unfortunately:\n\n> And it causes the compiler to complain that the value is not used.\n\nExactly.  So that 'if' was a way to shut up the compiler complaining\nabout the unused result of the ||-chain.  Perhaps not the best way to\ndo that, but I couldn't come up with any better.\n\nI had a note about it attached to the commit, but then run\n'format-patch' without '--notes'...  oh, well.\nI guess I should have wrote that in the commit message to begin with,\nshouldn't I?  Or in an in-code comment.\n"},{"id":"315167","messageId":"83a38f4d-a80e-e41c-a563-05585377ac13@web.de","threadId":"45482","inReplyTo":"20170323193919.twwfwdoulo2hjz24@sigill.intra.peff.net","subject":"Re: [PATCH] refs.c: use skip_prefix() in prettify_refname()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-03-23T20:06:09Z","receivedAt":"2017-03-23T20:06:31Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 23.03.2017 um 20:39 schrieb Jeff King:\n> On Thu, Mar 23, 2017 at 12:33:06PM -0700, Junio C Hamano wrote:\n>\n>>>> Nice, but why add the \"if\" when it's doing nothing?\n>>>\n>>> It's short-circuiting in the conditional.\n>>\n>> I think René meant this:\n>>\n>>      /* just for side effects */\n>>      skip_prefix(name, \"refs/heads/\", &name) ||\n>>      skip_prefix(name, \"refs/tags/\", &name) ||\n>>      skip_prefix(name, \"refs/remotes/\", &name);\n>>\n>>      return name;\n>>\n>> which still short-sircuits, even though I do think it looks\n>> strange; \"correct but strange\".\n>\n> And it causes the compiler to complain that the value is not used.\n\nAh, that explains it, thanks.  The \"if\" is strange, but wrapping the \nexpression in (void)(...) isn't better.\n\nClang doesn't warn about the code above by the way.\n\nRené\n"}]}