threads / patch / 45482

patchrefs.c: use skip_prefix() in prettify_refname()

Subject: [PATCH] refs.c: use skip_prefix() in prettify_refname()

## tl;dr

11 messages between Mar 23, 2017 and Mar 23, 2017. Diffs are folded; open one to read it.

replies: 10people: 5as markdown or json

SZEDER Gábor· Mar 23, 2017, 15:50 UTC · lore
This eliminates three magic numbers.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 refs.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
Show changes to refs.c +5 −5
diff --git a/refs.c b/refs.c
index e7606716d..0272e332c 100644
--- a/refs.c
+++ b/refs.c
@@ -366,11 +366,11 @@ int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data)
 
 const char *prettify_refname(const char *name)
 {
-	return name + (
-		starts_with(name, "refs/heads/") ? 11 :
-		starts_with(name, "refs/tags/") ? 10 :
-		starts_with(name, "refs/remotes/") ? 13 :
-		0);
+	if (skip_prefix(name, "refs/heads/", &name) ||
+	    skip_prefix(name, "refs/tags/", &name) ||
+	    skip_prefix(name, "refs/remotes/", &name))
+		; /* nothing */
+	return name;
 }
 
 static const char *ref_rev_parse_rules[] = {

-- 
2.12.1.485.g1616aa492
René Scharfe· Mar 23, 2017, 19:18 UTC · re: SZEDER Gábor · lore

Re: [PATCH] refs.c: use skip_prefix() in prettify_refname()

Am 23.03.2017 um 16:50 schrieb SZEDER Gábor:
Show 25 quoted lines
> This eliminates three magic numbers.
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  refs.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index e7606716d..0272e332c 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -366,11 +366,11 @@ int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data)
>
>  const char *prettify_refname(const char *name)
>  {
> -	return name + (
> -		starts_with(name, "refs/heads/") ? 11 :
> -		starts_with(name, "refs/tags/") ? 10 :
> -		starts_with(name, "refs/remotes/") ? 13 :
> -		0);
> +	if (skip_prefix(name, "refs/heads/", &name) ||
> +	    skip_prefix(name, "refs/tags/", &name) ||
> +	    skip_prefix(name, "refs/remotes/", &name))
> +		; /* nothing */
> +	return name;
Nice, but why add the "if" when it's doing nothing?
René
Jeff King· Mar 23, 2017, 19:23 UTC · re: René Scharfe · lore

Re: [PATCH] refs.c: use skip_prefix() in prettify_refname()

On Thu, Mar 23, 2017 at 08:18:26PM +0100, René Scharfe wrote:
Show 28 quoted lines
> Am 23.03.2017 um 16:50 schrieb SZEDER Gábor:
> > This eliminates three magic numbers.
> > 
> > Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> > ---
> >  refs.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/refs.c b/refs.c
> > index e7606716d..0272e332c 100644
> > --- a/refs.c
> > +++ b/refs.c
> > @@ -366,11 +366,11 @@ int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data)
> > 
> >  const char *prettify_refname(const char *name)
> >  {
> > -	return name + (
> > -		starts_with(name, "refs/heads/") ? 11 :
> > -		starts_with(name, "refs/tags/") ? 10 :
> > -		starts_with(name, "refs/remotes/") ? 13 :
> > -		0);
> > +	if (skip_prefix(name, "refs/heads/", &name) ||
> > +	    skip_prefix(name, "refs/tags/", &name) ||
> > +	    skip_prefix(name, "refs/remotes/", &name))
> > +		; /* nothing */
> > +	return name;
> 
> Nice, but why add the "if" when it's doing nothing?
It's short-circuiting in the conditional.
-Peff
Stefan Beller· Mar 23, 2017, 19:31 UTC · re: Jeff King · lore

Re: [PATCH] refs.c: use skip_prefix() in prettify_refname()

On Thu, Mar 23, 2017 at 12:23 PM, Jeff King <peff@peff.net> wrote:
Show 32 quoted lines
> On Thu, Mar 23, 2017 at 08:18:26PM +0100, René Scharfe wrote:
>
>> Am 23.03.2017 um 16:50 schrieb SZEDER Gábor:
>> > This eliminates three magic numbers.
>> >
>> > Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
>> > ---
>> >  refs.c | 10 +++++-----
>> >  1 file changed, 5 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/refs.c b/refs.c
>> > index e7606716d..0272e332c 100644
>> > --- a/refs.c
>> > +++ b/refs.c
>> > @@ -366,11 +366,11 @@ int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data)
>> >
>> >  const char *prettify_refname(const char *name)
>> >  {
>> > -   return name + (
>> > -           starts_with(name, "refs/heads/") ? 11 :
>> > -           starts_with(name, "refs/tags/") ? 10 :
>> > -           starts_with(name, "refs/remotes/") ? 13 :
>> > -           0);
>> > +   if (skip_prefix(name, "refs/heads/", &name) ||
>> > +       skip_prefix(name, "refs/tags/", &name) ||
>> > +       skip_prefix(name, "refs/remotes/", &name))
>> > +           ; /* nothing */
>> > +   return name;
>>
>> Nice, but why add the "if" when it's doing nothing?
>
> It's short-circuiting in the conditional.
You do not need a conditional to short circuit things.
|| works outside an if, too. ;)

Anyway, maybe it's worth spelling it out with an if .. else if cascade for readability?

After your comment of short-circuiting this code is pretty clear, so maybe just a small comment would do?

Thanks, Stefan

>
> -Peff
Junio C Hamano· Mar 23, 2017, 19:33 UTC · re: Jeff King · lore

Re: [PATCH] refs.c: use skip_prefix() in prettify_refname()

Jeff King <peff@peff.net> writes:
Show 32 quoted lines
> On Thu, Mar 23, 2017 at 08:18:26PM +0100, René Scharfe wrote:
>
>> Am 23.03.2017 um 16:50 schrieb SZEDER Gábor:
>> > This eliminates three magic numbers.
>> > 
>> > Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
>> > ---
>> >  refs.c | 10 +++++-----
>> >  1 file changed, 5 insertions(+), 5 deletions(-)
>> > 
>> > diff --git a/refs.c b/refs.c
>> > index e7606716d..0272e332c 100644
>> > --- a/refs.c
>> > +++ b/refs.c
>> > @@ -366,11 +366,11 @@ int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data)
>> > 
>> >  const char *prettify_refname(const char *name)
>> >  {
>> > -	return name + (
>> > -		starts_with(name, "refs/heads/") ? 11 :
>> > -		starts_with(name, "refs/tags/") ? 10 :
>> > -		starts_with(name, "refs/remotes/") ? 13 :
>> > -		0);
>> > +	if (skip_prefix(name, "refs/heads/", &name) ||
>> > +	    skip_prefix(name, "refs/tags/", &name) ||
>> > +	    skip_prefix(name, "refs/remotes/", &name))
>> > +		; /* nothing */
>> > +	return name;
>> 
>> Nice, but why add the "if" when it's doing nothing?
>
> It's short-circuiting in the conditional.
I think René meant this:
     /* just for side effects */
     skip_prefix(name, "refs/heads/", &name) ||
     skip_prefix(name, "refs/tags/", &name) ||
     skip_prefix(name, "refs/remotes/", &name);
     return name;

which still short-sircuits, even though I do think it looks strange; "correct but strange".

René Scharfe· Mar 23, 2017, 19:36 UTC · re: Junio C Hamano · lore

Re: [PATCH] refs.c: use skip_prefix() in prettify_refname()

Am 23.03.2017 um 20:33 schrieb Junio C Hamano:
Show 46 quoted lines
> Jeff King <peff@peff.net> writes:
>
>> On Thu, Mar 23, 2017 at 08:18:26PM +0100, René Scharfe wrote:
>>
>>> Am 23.03.2017 um 16:50 schrieb SZEDER Gábor:
>>>> This eliminates three magic numbers.
>>>>
>>>> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
>>>> ---
>>>>  refs.c | 10 +++++-----
>>>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/refs.c b/refs.c
>>>> index e7606716d..0272e332c 100644
>>>> --- a/refs.c
>>>> +++ b/refs.c
>>>> @@ -366,11 +366,11 @@ int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data)
>>>>
>>>>  const char *prettify_refname(const char *name)
>>>>  {
>>>> -	return name + (
>>>> -		starts_with(name, "refs/heads/") ? 11 :
>>>> -		starts_with(name, "refs/tags/") ? 10 :
>>>> -		starts_with(name, "refs/remotes/") ? 13 :
>>>> -		0);
>>>> +	if (skip_prefix(name, "refs/heads/", &name) ||
>>>> +	    skip_prefix(name, "refs/tags/", &name) ||
>>>> +	    skip_prefix(name, "refs/remotes/", &name))
>>>> +		; /* nothing */
>>>> +	return name;
>>>
>>> Nice, but why add the "if" when it's doing nothing?
>>
>> It's short-circuiting in the conditional.
>
> I think René meant this:
>
>      /* just for side effects */
>      skip_prefix(name, "refs/heads/", &name) ||
>      skip_prefix(name, "refs/tags/", &name) ||
>      skip_prefix(name, "refs/remotes/", &name);
>
>      return name;
>
> which still short-sircuits, even though I do think it looks
> strange; "correct but strange".

Yes. At least to me it looks less strange than the same lines wrapped in "if ... /* nothing */".

René
Junio C Hamano· Mar 23, 2017, 19:40 UTC · re: René Scharfe · lore

Re: [PATCH] refs.c: use skip_prefix() in prettify_refname()

René Scharfe <l.s.r@web.de> writes:
Show 14 quoted lines
>> I think René meant this:
>>
>>      /* just for side effects */
>>      skip_prefix(name, "refs/heads/", &name) ||
>>      skip_prefix(name, "refs/tags/", &name) ||
>>      skip_prefix(name, "refs/remotes/", &name);
>>
>>      return name;
>>
>> which still short-sircuits, even though I do think it looks
>> strange; "correct but strange".
>
> Yes.  At least to me it looks less strange than the same lines wrapped
> in "if ... /* nothing */".

Yup, after looking at it again, it does not look so "strange" to me. I probably should have said "unusual but correct", not "correct but strange".

Jeff King· Mar 23, 2017, 19:39 UTC · re: Junio C Hamano · lore

Re: [PATCH] refs.c: use skip_prefix() in prettify_refname()

On Thu, Mar 23, 2017 at 12:33:06PM -0700, Junio C Hamano wrote:
Show 15 quoted lines
> >> Nice, but why add the "if" when it's doing nothing?
> >
> > It's short-circuiting in the conditional.
> 
> I think René meant this:
> 
>      /* just for side effects */
>      skip_prefix(name, "refs/heads/", &name) ||
>      skip_prefix(name, "refs/tags/", &name) ||
>      skip_prefix(name, "refs/remotes/", &name);
> 
>      return name;
> 
> which still short-sircuits, even though I do think it looks
> strange; "correct but strange".
And it causes the compiler to complain that the value is not used.
-Peff
Junio C Hamano· Mar 23, 2017, 20:05 UTC · re: Jeff King · lore

Re: [PATCH] refs.c: use skip_prefix() in prettify_refname()

Jeff King <peff@peff.net> writes:
Show 19 quoted lines
> On Thu, Mar 23, 2017 at 12:33:06PM -0700, Junio C Hamano wrote:
>
>> >> Nice, but why add the "if" when it's doing nothing?
>> >
>> > It's short-circuiting in the conditional.
>> 
>> I think René meant this:
>> 
>>      /* just for side effects */
>>      skip_prefix(name, "refs/heads/", &name) ||
>>      skip_prefix(name, "refs/tags/", &name) ||
>>      skip_prefix(name, "refs/remotes/", &name);
>> 
>>      return name;
>> 
>> which still short-sircuits, even though I do think it looks
>> strange; "correct but strange".
>
> And it causes the compiler to complain that the value is not used.
Ahh.  OK.

In any case, I've queued the original with "if", which shouldn't have that problem ;-)

SZEDER Gábor· Mar 23, 2017, 20:06 UTC · re: Jeff King · lore

Re: [PATCH] refs.c: use skip_prefix() in prettify_refname()

On Thu, Mar 23, 2017 at 8:39 PM, Jeff King <peff@peff.net> wrote:
Show 14 quoted lines
> On Thu, Mar 23, 2017 at 12:33:06PM -0700, Junio C Hamano wrote:
>
>> >> Nice, but why add the "if" when it's doing nothing?
>> >
>> > It's short-circuiting in the conditional.
>>
>> I think René meant this:
>>
>>      /* just for side effects */
>>      skip_prefix(name, "refs/heads/", &name) ||
>>      skip_prefix(name, "refs/tags/", &name) ||
>>      skip_prefix(name, "refs/remotes/", &name);
>>
>>      return name;
That was my first version indeed.  Unfortunately:
> And it causes the compiler to complain that the value is not used.

Exactly. So that 'if' was a way to shut up the compiler complaining about the unused result of the ||-chain. Perhaps not the best way to do that, but I couldn't come up with any better.

I had a note about it attached to the commit, but then run 'format-patch' without '--notes'... oh, well. I guess I should have wrote that in the commit message to begin with, shouldn't I? Or in an in-code comment.

René Scharfe· Mar 23, 2017, 20:06 UTC · re: Jeff King · lore

Re: [PATCH] refs.c: use skip_prefix() in prettify_refname()

Am 23.03.2017 um 20:39 schrieb Jeff King:
Show 19 quoted lines
> On Thu, Mar 23, 2017 at 12:33:06PM -0700, Junio C Hamano wrote:
>
>>>> Nice, but why add the "if" when it's doing nothing?
>>>
>>> It's short-circuiting in the conditional.
>>
>> I think René meant this:
>>
>>      /* just for side effects */
>>      skip_prefix(name, "refs/heads/", &name) ||
>>      skip_prefix(name, "refs/tags/", &name) ||
>>      skip_prefix(name, "refs/remotes/", &name);
>>
>>      return name;
>>
>> which still short-sircuits, even though I do think it looks
>> strange; "correct but strange".
>
> And it causes the compiler to complain that the value is not used.

Ah, that explains it, thanks. The "if" is strange, but wrapping the expression in (void)(...) isn't better.

Clang doesn't warn about the code above by the way.
René

← back to recent threads