Re: [PATCH v17 05/14] ref-filter: introduce match_atom_name()

[Matthieu Moy]

Karthik Nayak  writes:

> Introduce match_atom_name() which helps in checking if a particular
> atom is the atom we're looking for and if it has a value attached to
> it or not.
>
> Use it instead of starts_with() for checking the value of %(color:...)
> atom. Write a test for the same.
>
> Mentored-by: Christian Couder 
> Mentored-by: Matthieu Moy 
> Thanks-to: Junio C Hamano 
> Signed-off-by: Karthik Nayak 
> ---
>  ref-filter.c   | 23 +--
>  t/t6302-for-each-ref-filter.sh |  4 
>  2 files changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index a993216..70d36fe 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -189,6 +189,22 @@ static void pop_stack_element(struct 
> ref_formatting_stack **stack)
>   *stack = prev;
>  }
>  
> +static int match_atom_name(const char *name, const char *atom_name, const 
> char **val)
> +{
> + const char *body;
> +
> + if (!skip_prefix(name, atom_name, ))
> + return 0; /* doesn't even begin with "atom_name" */
> + if (!body[0] || !body[1]) {
> + *val = NULL; /* %(atom_name) and no customization */

The logic is still hard to follow. If I read correctly, this function
accepts "%(colorX)" the same ways as "%(color)" here. I first thought it
was a bug, but it doesn't seem to be since %(colorX) would have been
rejected earlier.

It would be a bug if colorX was actually a valid atom name though: you
would be returning 1 for match_atom_name(name, "color") when
name=="colorX". So, I would say this "we can accept one extra character
because some earlier code rejected it before" is too hard to follow for
reviwers and too fragile.

OTOH, you are now accepting %(atom:) as a synonym to %(atom), and it's
not clear whether this is a deliberate decition.

> + return 1;
> + }
> + if (body[0] != ':')
> + return 0; /* "atom_namefoo" is not "atom_name" or 
> "atom_name:..." */
> + *val = body + 1; /* "atom_name:val" */
> + return 1;
> +}

Reversing the logic like this would be better IMHO:

static int match_atom_name(const char *name, const char *atom_name, const char 
**val)
{
const char *body;
if (!skip_prefix(name, atom_name, )) {
/* doesn't even begin with "atom_name" */
return 0;
}
if (!body[0]) {
/* "atom_name" and no customization */
*val = NULL;
return 1;
}
if (body[0] != ':')
/* "atom_namefoo" is not "atom_name" or "atom_name:..." */
return 0;
if (!body[1]) {
/* "atom_name:" */
*val = NULL;
return 1;
}
/* "atom_name:... */
*val = body + 1;
return 1;
}

=> each case appears very clearly, and we check body[0] != ':' before
testing !body[1], so %(colorX) is rejected before noticing the '\0'
after the 'X'.

"atom_name:" appears explicitly. If we want to reject it, we know which
code to change.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v17 05/14] ref-filter: introduce match_atom_name()

[Karthik Nayak]

On Thu, Sep 10, 2015 at 10:26 PM, Junio C Hamano  wrote:
> Karthik Nayak  writes:
>
>> Introduce match_atom_name() which helps in checking if a particular
>> atom is the atom we're looking for and if it has a value attached to
>> it or not.
>>
>> Use it instead of starts_with() for checking the value of %(color:...)
>> atom. Write a test for the same.
>>
>> Mentored-by: Christian Couder 
>> Mentored-by: Matthieu Moy 
>> Thanks-to: Junio C Hamano 
>> Signed-off-by: Karthik Nayak 
>> ---
>>  ref-filter.c   | 23 +--
>>  t/t6302-for-each-ref-filter.sh |  4 
>>  2 files changed, 25 insertions(+), 2 deletions(-)
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index a993216..70d36fe 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -189,6 +189,22 @@ static void pop_stack_element(struct 
>> ref_formatting_stack **stack)
>>   *stack = prev;
>>  }
>>
>> +static int match_atom_name(const char *name, const char *atom_name, const 
>> char **val)
>> +{
>> + const char *body;
>> +
>> + if (!skip_prefix(name, atom_name, ))
>> + return 0; /* doesn't even begin with "atom_name" */
>> + if (!body[0] || !body[1]) {
>> + *val = NULL; /* %(atom_name) and no customization */
>
> Why do we check body[1] here?  I do not understand why you are not
> checking !body[0] alone nothing else in this if condition.
>
> For (atom_name="align", name="aligna"), should the function say that
> "%(aligna)" is an "%(align)" with no customization?
>

The check was for checking if there is anything after the colon,
Matthieu's modified version seems better though.

>
> Why use the helper only for this one?  Aren't existing calls to
> starts_with() in the same if/else if/... cascade all potential bugs
> that the new helper function is meant to help fixing?  For example,
> the very fist one in the cascade:
>
> if (starts_with(name, "refname"))
> refname = ref->refname;
>
> is correct *ONLY* when name is "refname" or "refname:" followed by
> something, and it should skip "refnamex" when such a new atom is
> added to valid_atom[] list, i.e. a bug waiting to happen.  I think
> the new helper is designed to prevent such a bug from happening.
>

Yes definitely, but it would work with only refname, whereas
the color atom had no check before this patch, I didn't want to introduce
those patches in this series.

-- 
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v17 05/14] ref-filter: introduce match_atom_name()

[Junio C Hamano]

Matthieu Moy  writes:

> OTOH, you are now accepting %(atom:) as a synonym to %(atom), and it's
> not clear whether this is a deliberate decition.

I would say so.  When the caller wants to reject %(atom:), the
caller can tell it by checking val[0] == '\0' and reject that.

So it is better if you did not do this:

>   if (!body[1]) {
>   /* "atom_name:" */
>   *val = NULL;
>   return 1;
>   }

which robs that information from the caller.  It should be
sufficient to just drop the check that allows "colorx" when
expecting "color" without making any other change, I would think.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v17 05/14] ref-filter: introduce match_atom_name()

[Matthieu Moy]

Junio C Hamano  writes:

> Karthik Nayak  writes:
>
>> -} else if (starts_with(name, "color:")) {
>> +} else if (match_atom_name(name, "color", )) {
>
> Why use the helper only for this one?  Aren't existing calls to
> starts_with() in the same if/else if/... cascade all potential bugs
> that the new helper function is meant to help fixing?  For example,
> the very fist one in the cascade:
>
>   if (starts_with(name, "refname"))
>   refname = ref->refname;
>
> is correct *ONLY* when name is "refname" or "refname:" followed by
> something, and it should skip "refnamex" when such a new atom is
> added to valid_atom[] list, i.e. a bug waiting to happen.  I think
> the new helper is designed to prevent such a bug from happening.

I fully agree, but I also think that this should be a separate topic.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v17 05/14] ref-filter: introduce match_atom_name()

[Junio C Hamano]

Karthik Nayak  writes:

> Introduce match_atom_name() which helps in checking if a particular
> atom is the atom we're looking for and if it has a value attached to
> it or not.
>
> Use it instead of starts_with() for checking the value of %(color:...)
> atom. Write a test for the same.
>
> Mentored-by: Christian Couder 
> Mentored-by: Matthieu Moy 
> Thanks-to: Junio C Hamano 
> Signed-off-by: Karthik Nayak 
> ---
>  ref-filter.c   | 23 +--
>  t/t6302-for-each-ref-filter.sh |  4 
>  2 files changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index a993216..70d36fe 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -189,6 +189,22 @@ static void pop_stack_element(struct 
> ref_formatting_stack **stack)
>   *stack = prev;
>  }
>  
> +static int match_atom_name(const char *name, const char *atom_name, const 
> char **val)
> +{
> + const char *body;
> +
> + if (!skip_prefix(name, atom_name, ))
> + return 0; /* doesn't even begin with "atom_name" */
> + if (!body[0] || !body[1]) {
> + *val = NULL; /* %(atom_name) and no customization */

Why do we check body[1] here?  I do not understand why you are not
checking !body[0] alone nothing else in this if condition.

For (atom_name="align", name="aligna"), should the function say that
"%(aligna)" is an "%(align)" with no customization?

> + return 1;
> + }
> + if (body[0] != ':')
> + return 0; /* "atom_namefoo" is not "atom_name" or 
> "atom_name:..." */
> + *val = body + 1; /* "atom_name:val" */
> + return 1;
> +}
> +
>  /*
>   * In a format string, find the next occurrence of %(atom).
>   */
> @@ -687,6 +703,7 @@ static void populate_value(struct ref_array_item *ref)
>   int deref = 0;
>   const char *refname;
>   const char *formatp;
> + const char *valp;
>   struct branch *branch = NULL;
>  
>   v->handler = append_atom;
> @@ -721,10 +738,12 @@ static void populate_value(struct ref_array_item *ref)
>   refname = branch_get_push(branch, NULL);
>   if (!refname)
>   continue;
> - } else if (starts_with(name, "color:")) {
> + } else if (match_atom_name(name, "color", )) {

Why use the helper only for this one?  Aren't existing calls to
starts_with() in the same if/else if/... cascade all potential bugs
that the new helper function is meant to help fixing?  For example,
the very fist one in the cascade:

if (starts_with(name, "refname"))
refname = ref->refname;

is correct *ONLY* when name is "refname" or "refname:" followed by
something, and it should skip "refnamex" when such a new atom is
added to valid_atom[] list, i.e. a bug waiting to happen.  I think
the new helper is designed to prevent such a bug from happening.

>   char color[COLOR_MAXLEN] = "";
>  
> - if (color_parse(name + 6, color) < 0)
> + if (!valp)
> + die(_("expected format: %%(color:)"));
> + if (color_parse(valp, color) < 0)
>   die(_("unable to parse format"));
>   v->s = xstrdup(color);
>   continue;
> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> index 505a360..c4f0378 100755
> --- a/t/t6302-for-each-ref-filter.sh
> +++ b/t/t6302-for-each-ref-filter.sh
> @@ -81,4 +81,8 @@ test_expect_success 'filtering with --contains' '
>   test_cmp expect actual
>  '
>  
> +test_expect_success '%(color) must fail' '
> + test_must_fail git for-each-ref --format="%(color)%(refname)"
> +'
> +
>  test_done
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v17 05/14] ref-filter: introduce match_atom_name()

[Junio C Hamano]

Karthik Nayak  writes:

> The check was for checking if there is anything after the colon,

Why do you even care?  If %(color) expects more specific
customization by adding colon followed by specific data after it,
i.e. %(color:something), %(color:) should clearly be that "%(color)"
thing with no customization---if it is "not enough customization" or
"leaving everything default" depends on each atom, match_atom_name()
is not a good place to make that policy decision (i.e. Mattheiu's
rewrite to clear *valp to NULL when %(color:) is seen).  Instead,
point *val to body + 1 just as usual, and let the caller tell
between *val being NULL (not even any colon) and *val pointing at a
NUL byte (nothing after colon) if it cares.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v17 05/14] ref-filter: introduce match_atom_name()

[Karthik Nayak]

On Thu, Sep 10, 2015 at 10:58 PM, Junio C Hamano  wrote:
> Karthik Nayak  writes:
>
>> The check was for checking if there is anything after the colon,
>
> Why do you even care?  If %(color) expects more specific
> customization by adding colon followed by specific data after it,
> i.e. %(color:something), %(color:) should clearly be that "%(color)"
> thing with no customization---if it is "not enough customization" or
> "leaving everything default" depends on each atom, match_atom_name()
> is not a good place to make that policy decision (i.e. Mattheiu's
> rewrite to clear *valp to NULL when %(color:) is seen).  Instead,
> point *val to body + 1 just as usual, and let the caller tell
> between *val being NULL (not even any colon) and *val pointing at a
> NUL byte (nothing after colon) if it cares.

It is one thing that the user can actually do the check themselves,
but doesn't it make more sense that when we're using colon we expect a
value after it, and something like %(color:) makes no sense when color
specifically needs a value after the colon.

Hence rather than assuming that the no value is given and fallback to
the default
%(color), wouldn't it be better to warn the user that there is no value?

But then again we can leave this decision to the user like you said.

-- 
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v17 05/14] ref-filter: introduce match_atom_name()

[Junio C Hamano]

Karthik Nayak  writes:

> It is one thing that the user can actually do the check themselves,
> but doesn't it make more sense that when we're using colon we expect a
> value after it, and something like %(color:) makes no sense when color
> specifically needs a value after the colon.

If you imagine the format being built by scripts (we are talking
about plumbing feature --format here), I think you will realize that
it perfectly makes sense to allow them to say "%(atom:$modifiation)"
without having to worry about a special case where $modification
happened to end up being empty.  So no, I do not agree with your
statement at all.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v17 05/14] ref-filter: introduce match_atom_name()

[Karthik Nayak]

On Thu, Sep 10, 2015 at 11:15 PM, Junio C Hamano  wrote:
> Karthik Nayak  writes:
>
>> It is one thing that the user can actually do the check themselves,
>> but doesn't it make more sense that when we're using colon we expect a
>> value after it, and something like %(color:) makes no sense when color
>> specifically needs a value after the colon.
>
> If you imagine the format being built by scripts (we are talking
> about plumbing feature --format here), I think you will realize that
> it perfectly makes sense to allow them to say "%(atom:$modifiation)"
> without having to worry about a special case where $modification
> happened to end up being empty.  So no, I do not agree with your
> statement at all.

Ah! that makes sense, thanks :)

-- 
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v17 05/14] ref-filter: introduce match_atom_name()

[Junio C Hamano]

Matthieu Moy  writes:

> Junio C Hamano  writes:
>
>> Karthik Nayak  writes:
>>
>>> -   } else if (starts_with(name, "color:")) {
>>> +   } else if (match_atom_name(name, "color", )) {
>>
>> Why use the helper only for this one?  Aren't existing calls to
>> starts_with() in the same if/else if/... cascade all potential bugs
>> that the new helper function is meant to help fixing?  For example,
>> the very fist one in the cascade:
>>
>>  if (starts_with(name, "refname"))
>>  refname = ref->refname;
>>
>> is correct *ONLY* when name is "refname" or "refname:" followed by
>> something, and it should skip "refnamex" when such a new atom is
>> added to valid_atom[] list, i.e. a bug waiting to happen.  I think
>> the new helper is designed to prevent such a bug from happening.
>
> I fully agree, but I also think that this should be a separate topic.

Yeah, it can be a separate topic.  I am neutral (i.e. I certainly
would not insist that the existing one should be fixed with the
helper in the series, but I cannot quite say that I prefer the fix
to be made outside this topic, either).

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v17 05/14] ref-filter: introduce match_atom_name()

[Matthieu Moy]

Junio C Hamano  writes:

> Matthieu Moy  writes:
>
>> OTOH, you are now accepting %(atom:) as a synonym to %(atom), and it's
>> not clear whether this is a deliberate decition.
>
> I would say so.  When the caller wants to reject %(atom:), the
> caller can tell it by checking val[0] == '\0' and reject that.
>
> So it is better if you did not do this:
>
>>  if (!body[1]) {
>>  /* "atom_name:" */
>>  *val = NULL;
>>  return 1;
>>  }
>
> which robs that information from the caller.

OK. Just dropping this part lets the code fall back to

/* "atom_name:... */
*val = body + 1;
return 1;

right below in my version. It also accepts it (return 1) but lets val
point to an empty string. Makes sense.

And indeed, without this, my code looks a lot like Karthik's one, just
dropping the "|| !body[1]" part in a condition.

In any case, I'd like to see "atom_name:" explicitly mentionned
somewhere in a comment, if only to make it clear that what is done with
it is deliberate (e.g. avoid having someone not following this
conversation later considering this %(atom:) thing to be a bug and try
to fix it).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html