Re: [PATCH v4 06/12] ref-filter: introduce color_atom_parser()

[Karthik Nayak]

On Sun, Feb 7, 2016 at 1:13 PM, Eric Sunshine  wrote:
> On Sat, Feb 6, 2016 at 10:20 AM, Karthik Nayak  wrote:
>> On Fri, Feb 5, 2016 at 3:55 AM, Eric Sunshine  
>> wrote:
>>> On Sun, Jan 31, 2016 at 12:42 PM, Karthik Nayak  
>>> wrote:
 +static void color_atom_parser(struct used_atom *atom, const char 
 *color_value)
 +{
 +   if (!color_value)
 +   die(_("expected format: %%(color:)"));
 +   if (color_parse(color_value, atom->u.color) < 0)
 +   die(_("invalid color value: %s"), atom->u.color);
>>>
>>> Shouldn't this be:
>>>
>>> die(_("invalid color value: %s"), color_value);
>>>
>>> ?
>>
>> Oops. You're right, it should.
>> Also the error is reported already in color_parse(...), so seems duplicated.
>>
>> git for-each-ref  --format="%(color:sfadf)%(align:middle,30)%(refname)%(end)"
>> error: invalid color value: sfadf
>> fatal: invalid color value: sfadf
>>
>> What would be an ideal way around this?
>
> According to f6c5a29 (color_parse: do not mention variable name in
> error message, 2014-10-07), the doubled diagnostic messages are
> intentional, so I don't think you need to do anything about it in this
> series. If you want to make the "fatal" message a bit more helpful,
> you could add a %(color:) annotation, like this:
>
> die(_("unrecognized color: %%(color:%s)"), color_value);
>
> which would give you the slightly more helpful:
>
> error: invalid color value: sfadf
> fatal: unrecognized color: %(color:sfadf)

That seems to be a good way around the repetitive message.
will add 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 v4 06/12] ref-filter: introduce color_atom_parser()

[Karthik Nayak]

On Fri, Feb 5, 2016 at 3:55 AM, Eric Sunshine  wrote:
> On Sun, Jan 31, 2016 at 12:42 PM, Karthik Nayak  wrote:
>> Introduce color_atom_parser() which will parse a "color" atom and
>> store its color in the "used_atom" structure for further usage in
>> populate_value().
>>
>> Signed-off-by: Karthik Nayak 
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> @@ -29,10 +29,21 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } 
>> cmp_type;
>>  static struct used_atom {
>> const char *name;
>> cmp_type type;
>> +   union {
>> +   char color[COLOR_MAXLEN];
>> +   } u;
>>  } *used_atom;
>>  static int used_atom_cnt, need_tagged, need_symref;
>>  static int need_color_reset_at_eol;
>>
>> +static void color_atom_parser(struct used_atom *atom, const char 
>> *color_value)
>> +{
>> +   if (!color_value)
>> +   die(_("expected format: %%(color:)"));
>> +   if (color_parse(color_value, atom->u.color) < 0)
>> +   die(_("invalid color value: %s"), atom->u.color);
>
> Shouldn't this be:
>
> die(_("invalid color value: %s"), color_value);
>
> ?

Oops. You're right, it should.
Also the error is reported already in color_parse(...), so seems duplicated.

e.g.

git for-each-ref  --format="%(color:sfadf)%(align:middle,30)%(refname)%(end)"
error: invalid color value: sfadf
fatal: invalid color value: sfadf

What would be an ideal way around this?

-- 
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 v4 06/12] ref-filter: introduce color_atom_parser()

[Christian Couder]

On Sat, Feb 6, 2016 at 4:20 PM, Karthik Nayak  wrote:
> On Fri, Feb 5, 2016 at 3:55 AM, Eric Sunshine  wrote:
>> On Sun, Jan 31, 2016 at 12:42 PM, Karthik Nayak  
>> wrote:
>>> Introduce color_atom_parser() which will parse a "color" atom and
>>> store its color in the "used_atom" structure for further usage in
>>> populate_value().
>>>
>>> Signed-off-by: Karthik Nayak 
>>> ---
>>> diff --git a/ref-filter.c b/ref-filter.c
>>> @@ -29,10 +29,21 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } 
>>> cmp_type;
>>>  static struct used_atom {
>>> const char *name;
>>> cmp_type type;
>>> +   union {
>>> +   char color[COLOR_MAXLEN];
>>> +   } u;
>>>  } *used_atom;
>>>  static int used_atom_cnt, need_tagged, need_symref;
>>>  static int need_color_reset_at_eol;
>>>
>>> +static void color_atom_parser(struct used_atom *atom, const char 
>>> *color_value)
>>> +{
>>> +   if (!color_value)
>>> +   die(_("expected format: %%(color:)"));
>>> +   if (color_parse(color_value, atom->u.color) < 0)
>>> +   die(_("invalid color value: %s"), atom->u.color);
>>
>> Shouldn't this be:
>>
>> die(_("invalid color value: %s"), color_value);
>>
>> ?
>
> Oops. You're right, it should.
> Also the error is reported already in color_parse(...), so seems duplicated.
>
> e.g.
>
> git for-each-ref  --format="%(color:sfadf)%(align:middle,30)%(refname)%(end)"
> error: invalid color value: sfadf
> fatal: invalid color value: sfadf
>
> What would be an ideal way around this?

Maybe it has already been discussed a lot and I missed the discussion,
but if possible the argument, the parameter or the atom itself might
just be ignored with a warning instead of dying when an atom argument,
format or parameter is not recognized, because in the next Git
versions we might want to add new arguments, formats and parameter and
it would be sad if old versions of Git die when those new things are
passed to them.
--
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 v4 06/12] ref-filter: introduce color_atom_parser()

[Eric Sunshine]

On Sat, Feb 6, 2016 at 10:51 AM, Christian Couder
 wrote:
> On Sat, Feb 6, 2016 at 4:20 PM, Karthik Nayak  wrote:
>> Also the error is reported already in color_parse(...), so seems duplicated.
>>
>> git for-each-ref  --format="%(color:sfadf)%(align:middle,30)%(refname)%(end)"
>> error: invalid color value: sfadf
>> fatal: invalid color value: sfadf
>>
>> What would be an ideal way around this?
>
> Maybe it has already been discussed a lot and I missed the discussion,
> but if possible the argument, the parameter or the atom itself might
> just be ignored with a warning instead of dying when an atom argument,
> format or parameter is not recognized, because in the next Git
> versions we might want to add new arguments, formats and parameter and
> it would be sad if old versions of Git die when those new things are
> passed to them.

The current behavior of die()ing is inherited from existing code which
Karthik refactored to create ref-filter.c, so it is not a new issue,
and old versions of Git are already afflicted. Whether die()ing is
desirable or not is unrelated to the current series which is primarily
aimed at optimizing and slightly generalizing ref-filter.c.
--
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 v4 06/12] ref-filter: introduce color_atom_parser()

[Eric Sunshine]

On Sat, Feb 6, 2016 at 10:20 AM, Karthik Nayak  wrote:
> On Fri, Feb 5, 2016 at 3:55 AM, Eric Sunshine  wrote:
>> On Sun, Jan 31, 2016 at 12:42 PM, Karthik Nayak  
>> wrote:
>>> +static void color_atom_parser(struct used_atom *atom, const char 
>>> *color_value)
>>> +{
>>> +   if (!color_value)
>>> +   die(_("expected format: %%(color:)"));
>>> +   if (color_parse(color_value, atom->u.color) < 0)
>>> +   die(_("invalid color value: %s"), atom->u.color);
>>
>> Shouldn't this be:
>>
>> die(_("invalid color value: %s"), color_value);
>>
>> ?
>
> Oops. You're right, it should.
> Also the error is reported already in color_parse(...), so seems duplicated.
>
> git for-each-ref  --format="%(color:sfadf)%(align:middle,30)%(refname)%(end)"
> error: invalid color value: sfadf
> fatal: invalid color value: sfadf
>
> What would be an ideal way around this?

According to f6c5a29 (color_parse: do not mention variable name in
error message, 2014-10-07), the doubled diagnostic messages are
intentional, so I don't think you need to do anything about it in this
series. If you want to make the "fatal" message a bit more helpful,
you could add a %(color:) annotation, like this:

die(_("unrecognized color: %%(color:%s)"), color_value);

which would give you the slightly more helpful:

error: invalid color value: sfadf
fatal: unrecognized color: %(color:sfadf)
--
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 v4 06/12] ref-filter: introduce color_atom_parser()

[Eric Sunshine]

On Sun, Jan 31, 2016 at 12:42 PM, Karthik Nayak  wrote:
> Introduce color_atom_parser() which will parse a "color" atom and
> store its color in the "used_atom" structure for further usage in
> populate_value().
>
> Signed-off-by: Karthik Nayak 
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -29,10 +29,21 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } 
> cmp_type;
>  static struct used_atom {
> const char *name;
> cmp_type type;
> +   union {
> +   char color[COLOR_MAXLEN];
> +   } u;
>  } *used_atom;
>  static int used_atom_cnt, need_tagged, need_symref;
>  static int need_color_reset_at_eol;
>
> +static void color_atom_parser(struct used_atom *atom, const char 
> *color_value)
> +{
> +   if (!color_value)
> +   die(_("expected format: %%(color:)"));
> +   if (color_parse(color_value, atom->u.color) < 0)
> +   die(_("invalid color value: %s"), atom->u.color);

Shouldn't this be:

die(_("invalid color value: %s"), color_value);

?

> +}
--
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

[PATCH v4 06/12] ref-filter: introduce color_atom_parser()

[Karthik Nayak]

Introduce color_atom_parser() which will parse a "color" atom and
store its color in the "used_atom" structure for further usage in
populate_value().

Helped-by: Ramsay Jones 
Helped-by: Eric Sunshine 
Signed-off-by: Karthik Nayak 
---
 ref-filter.c | 25 -
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 92b4419..7adff67 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -29,10 +29,21 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } 
cmp_type;
 static struct used_atom {
const char *name;
cmp_type type;
+   union {
+   char color[COLOR_MAXLEN];
+   } u;
 } *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
 static int need_color_reset_at_eol;
 
+static void color_atom_parser(struct used_atom *atom, const char *color_value)
+{
+   if (!color_value)
+   die(_("expected format: %%(color:)"));
+   if (color_parse(color_value, atom->u.color) < 0)
+   die(_("invalid color value: %s"), atom->u.color);
+}
+
 static struct {
const char *name;
cmp_type cmp_type;
@@ -70,7 +81,7 @@ static struct {
{ "symref" },
{ "flag" },
{ "HEAD" },
-   { "color" },
+   { "color", FIELD_STR, color_atom_parser },
{ "align" },
{ "end" },
 };
@@ -157,6 +168,7 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
used_atom[at].type = valid_atom[i].cmp_type;
if (arg)
arg = used_atom[at].name + (arg - atom) + 1;
+   memset(_atom[at].u, 0, sizeof(used_atom[at].u));
if (valid_atom[i].parser)
valid_atom[i].parser(_atom[at], arg);
if (*atom == '*')
@@ -815,6 +827,7 @@ static void populate_value(struct ref_array_item *ref)
 
/* Fill in specials first */
for (i = 0; i < used_atom_cnt; i++) {
+   struct used_atom *atom = _atom[i];
const char *name = used_atom[i].name;
struct atom_value *v = >value[i];
int deref = 0;
@@ -855,14 +868,8 @@ static void populate_value(struct ref_array_item *ref)
refname = branch_get_push(branch, NULL);
if (!refname)
continue;
-   } else if (match_atom_name(name, "color", )) {
-   char color[COLOR_MAXLEN] = "";
-
-   if (!valp)
-   die(_("expected format: %%(color:)"));
-   if (color_parse(valp, color) < 0)
-   die(_("unable to parse format"));
-   v->s = xstrdup(color);
+   } else if (starts_with(name, "color:")) {
+   v->s = atom->u.color;
continue;
} else if (!strcmp(name, "flag")) {
char buf[256], *cp = buf;
-- 
2.7.0

--
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