[PATCH v5 02/11] ref-filter: make `color` use `ref_formatting_state`
Make the `color` atom a pseudo atom and ensure that it uses
`ref_formatting_state`.
---
ref-filter.c | 19 ++-
ref-filter.h | 4 +++-
2 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index a3625e8..cc25c85 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -662,6 +662,8 @@ static void populate_value(struct ref_array_item *ref)
if (color_parse(name + 6, color) < 0)
die(_("unable to parse format"));
v->s = xstrdup(color);
+ v->pseudo_atom = 1;
+ v->color = 1;
continue;
} else if (!strcmp(name, "flag")) {
char buf[256], *cp = buf;
@@ -1195,6 +1197,11 @@ void ref_array_sort(struct ref_sorting *sorting, struct
ref_array *array)
static void ref_formatting(struct ref_formatting_state *state,
struct atom_value *v, struct strbuf *value)
{
+ if (state->color) {
+ strbuf_addstr(value, state->color);
+ free(state->color);
+ state->color = NULL;
+ }
strbuf_addf(value, "%s", v->s);
}
@@ -1266,6 +1273,13 @@ static void emit(const char *cp, const char *ep)
}
}
+static void apply_pseudo_state(struct ref_formatting_state *state,
+ struct atom_value *v)
+{
+ if (v->color)
+ state->color = (char *)v->s;
+}
+
void show_ref_array_item(struct ref_array_item *info, const char *format, int
quote_style)
{
const char *cp, *sp, *ep;
@@ -1281,7 +1295,10 @@ void show_ref_array_item(struct ref_array_item *info,
const char *format, int qu
if (cp < sp)
emit(cp, sp);
get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep),
&atomv);
- print_value(&state, atomv);
+ if (atomv->pseudo_atom)
+ apply_pseudo_state(&state, atomv);
+ else
+ print_value(&state, atomv);
}
if (*cp) {
sp = cp + strlen(cp);
diff --git a/ref-filter.h b/ref-filter.h
index f24e3ef..7687879 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -19,11 +19,13 @@
struct atom_value {
const char *s;
unsigned long ul; /* used for sorting when not FIELD_STR */
- unsigned int pseudo_atom : 1; /* atoms which aren't placeholders for
ref attributes */
+ unsigned int pseudo_atom : 1, /* atoms which aren't placeholders for
ref attributes */
+ color : 1;
};
struct ref_formatting_state {
int quote_style;
+ char *color;
};
struct ref_sorting {
--
2.4.6
--
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 v5 02/11] ref-filter: make `color` use `ref_formatting_state`
Karthik Nayak writes:
> Make the `color` atom a pseudo atom and ensure that it uses
> `ref_formatting_state`.
Actually, I think this is an incorrect change.
Previously, %(color:...) was taking effect immediately, and after this
patch, it takes effect starting from the next atom.
Try this:
git for-each-ref --format '%(color:red)red %(color:reset)normal'
Before your patch, I get 'red' as red, and 'normal' as normal. After
your patch, I get no color at all, since %(color:...) just stores
information that is never used because I have no real atom.
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -19,11 +19,13 @@
> struct atom_value {
> const char *s;
> unsigned long ul; /* used for sorting when not FIELD_STR */
> - unsigned int pseudo_atom : 1; /* atoms which aren't placeholders for
> ref attributes */
> + unsigned int pseudo_atom : 1, /* atoms which aren't placeholders for
> ref attributes */
> + color : 1;
> };
As a consequence of the remark above, I think the name and comment of
the field are misleading. There are 3 kinds of atoms:
* Placeholders for ref attributes
* Atoms that take action immediately, but that are not ref attributes
like %(color)
* Atoms that only act as modifiers for the next atom. Perhaps they could
be called "prefix atoms" or "modifier atoms".
--
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 v5 02/11] ref-filter: make `color` use `ref_formatting_state`
Karthik Nayak writes:
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1195,6 +1197,11 @@ void ref_array_sort(struct ref_sorting *sorting,
> struct ref_array *array)
> static void ref_formatting(struct ref_formatting_state *state,
> struct atom_value *v, struct strbuf *value)
> {
> + if (state->color) {
> + strbuf_addstr(value, state->color);
> + free(state->color);
> + state->color = NULL;
> + }
> strbuf_addf(value, "%s", v->s);
> }
>
> @@ -1266,6 +1273,13 @@ static void emit(const char *cp, const char *ep)
> }
> }
>
> +static void apply_pseudo_state(struct ref_formatting_state *state,
> +struct atom_value *v)
> +{
> + if (v->color)
> + state->color = (char *)v->s;
> +}
> +
> void show_ref_array_item(struct ref_array_item *info, const char *format,
> int quote_style)
> {
> const char *cp, *sp, *ep;
It's not clear enough in the code and history that these these two
functions are symmetrical.
You can find better names. 'apply_pseudo_state' seems wrong it two ways:
it does not _apply_ the state, but it stores it. And it's a "pseudo-atom
related state", but not a "pseudo-state".
How about
ref_formatting -> apply_formatting_state
apply_pseudo_state -> store_formatting_state
?
Actually, I would even call these functions right from
show_ref_array_item, so that the result look like this:
if (atomv->pseudo_atom)
store_formatting_state(&state, atomv);
else {
apply_formatting_state(&state, atomv);
reset_formatting_state(&state);
print_value(&state, atomv);
}
In the history, if you are to introduce a dumb version of ref_formatting
in PATCH 1, I think you should also introduce a dumb (actually, totally
empty) version of apply_pseudo_state. Then, further patches would just
add a few lines in each function, and ...
> @@ -1281,7 +1295,10 @@ void show_ref_array_item(struct ref_array_item *info,
> const char *format, int qu
> if (cp < sp)
> emit(cp, sp);
> get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep),
> &atomv);
> - print_value(&state, atomv);
> + if (atomv->pseudo_atom)
> + apply_pseudo_state(&state, atomv);
> + else
> + print_value(&state, atomv);
> }
... this hunk would belong to PATCH 1.
--
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 v5 02/11] ref-filter: make `color` use `ref_formatting_state`
Matthieu Moy writes: > Karthik Nayak writes: > >> Make the `color` atom a pseudo atom and ensure that it uses >> `ref_formatting_state`. > > Actually, I think this is an incorrect change. > > Previously, %(color:...) was taking effect immediately, and after this > patch, it takes effect starting from the next atom. > ... > As a consequence of the remark above, I think the name and comment of > the field are misleading. There are 3 kinds of atoms: > > * Placeholders for ref attributes > > * Atoms that take action immediately, but that are not ref attributes > like %(color) > > * Atoms that only act as modifiers for the next atom. Perhaps they could > be called "prefix atoms" or "modifier atoms". My fault. I briefly thought that it may be simpler to treat %(color) just as a different way to express a literal that cannot be typed from the keyboard, but your "different kind of atom" is a much better way to think about it. What is necessary is that, just like the updated "print_value()" knows about the formatting state, "emit()" needs to be told about the same formatting state. Some of these "state affecting" atoms may take effect on what is output by "emit()" (e.g. "color" is obviously an example, the hypotethical one that counts the current output column and uses that knowledge to "align" the output to certain columns, instead of "right pad to make the next column 30-columns wide" one, which is in this series, is another). Thanks for finding this, and Karthik, sorry for an incomplete suggestion based on a faulty thinking. -- 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 v5 02/11] ref-filter: make `color` use `ref_formatting_state`
On Mon, Jul 27, 2015 at 6:17 PM, Matthieu Moy
wrote:
> Karthik Nayak writes:
>
>> Make the `color` atom a pseudo atom and ensure that it uses
>> `ref_formatting_state`.
>
> Actually, I think this is an incorrect change.
>
> Previously, %(color:...) was taking effect immediately, and after this
> patch, it takes effect starting from the next atom.
>
> Try this:
>
> git for-each-ref --format '%(color:red)red %(color:reset)normal'
>
> Before your patch, I get 'red' as red, and 'normal' as normal. After
> your patch, I get no color at all, since %(color:...) just stores
> information that is never used because I have no real atom.
>
>> --- a/ref-filter.h
>> +++ b/ref-filter.h
>> @@ -19,11 +19,13 @@
>> struct atom_value {
>> const char *s;
>> unsigned long ul; /* used for sorting when not FIELD_STR */
>> - unsigned int pseudo_atom : 1; /* atoms which aren't placeholders for
>> ref attributes */
>> + unsigned int pseudo_atom : 1, /* atoms which aren't placeholders for
>> ref attributes */
>> + color : 1;
>> };
>
> As a consequence of the remark above, I think the name and comment of
> the field are misleading. There are 3 kinds of atoms:
>
> * Placeholders for ref attributes
>
> * Atoms that take action immediately, but that are not ref attributes
> like %(color)
>
> * Atoms that only act as modifiers for the next atom. Perhaps they could
> be called "prefix atoms" or "modifier atoms".
>
Thats right, Junio and I missed out the second part and overlapped it
over with the
third part. Thanks for seeing it through.
--
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 v5 02/11] ref-filter: make `color` use `ref_formatting_state`
On Mon, Jul 27, 2015 at 6:36 PM, Matthieu Moy
wrote:
> Karthik Nayak writes:
>
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -1195,6 +1197,11 @@ void ref_array_sort(struct ref_sorting *sorting,
>> struct ref_array *array)
>> static void ref_formatting(struct ref_formatting_state *state,
>> struct atom_value *v, struct strbuf *value)
>> {
>> + if (state->color) {
>> + strbuf_addstr(value, state->color);
>> + free(state->color);
>> + state->color = NULL;
>> + }
>> strbuf_addf(value, "%s", v->s);
>> }
>>
>> @@ -1266,6 +1273,13 @@ static void emit(const char *cp, const char *ep)
>> }
>> }
>>
>> +static void apply_pseudo_state(struct ref_formatting_state *state,
>> +struct atom_value *v)
>> +{
>> + if (v->color)
>> + state->color = (char *)v->s;
>> +}
>> +
>> void show_ref_array_item(struct ref_array_item *info, const char *format,
>> int quote_style)
>> {
>> const char *cp, *sp, *ep;
>
> It's not clear enough in the code and history that these these two
> functions are symmetrical.
>
> You can find better names. 'apply_pseudo_state' seems wrong it two ways:
> it does not _apply_ the state, but it stores it. And it's a "pseudo-atom
> related state", but not a "pseudo-state".
>
> How about
>
> ref_formatting -> apply_formatting_state
> apply_pseudo_state -> store_formatting_state
>
> ?
Yes, your suggested naming scheme is better. Ill adopt this.
>
> Actually, I would even call these functions right from
> show_ref_array_item, so that the result look like this:
>
> if (atomv->pseudo_atom)
> store_formatting_state(&state, atomv);
> else {
> apply_formatting_state(&state, atomv);
> reset_formatting_state(&state);
> print_value(&state, atomv);
> }
This would eliminate that extra strbuf in print_value() wouldn't it,
but this would also mean
that we replace the value in atomv to hold the new formatted value.
Sounds good to me.
Thanks :)
>
> In the history, if you are to introduce a dumb version of ref_formatting
> in PATCH 1, I think you should also introduce a dumb (actually, totally
> empty) version of apply_pseudo_state. Then, further patches would just
> add a few lines in each function, and ...
>
>> @@ -1281,7 +1295,10 @@ void show_ref_array_item(struct ref_array_item *info,
>> const char *format, int qu
>> if (cp < sp)
>> emit(cp, sp);
>> get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep),
>> &atomv);
>> - print_value(&state, atomv);
>> + if (atomv->pseudo_atom)
>> + apply_pseudo_state(&state, atomv);
>> + else
>> + print_value(&state, atomv);
>> }
>
> ... this hunk would belong to PATCH 1.
>
I'll add a placeholder for this in the PATCH 1. Thanks :D
--
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 v5 02/11] ref-filter: make `color` use `ref_formatting_state`
On Mon, Jul 27, 2015 at 7:58 PM, Junio C Hamano wrote: > Matthieu Moy writes: > >> Karthik Nayak writes: >> >>> Make the `color` atom a pseudo atom and ensure that it uses >>> `ref_formatting_state`. >> >> Actually, I think this is an incorrect change. >> >> Previously, %(color:...) was taking effect immediately, and after this >> patch, it takes effect starting from the next atom. >> ... >> As a consequence of the remark above, I think the name and comment of >> the field are misleading. There are 3 kinds of atoms: >> >> * Placeholders for ref attributes >> >> * Atoms that take action immediately, but that are not ref attributes >> like %(color) >> >> * Atoms that only act as modifiers for the next atom. Perhaps they could >> be called "prefix atoms" or "modifier atoms". > > My fault. > > I briefly thought that it may be simpler to treat %(color) just as a > different way to express a literal that cannot be typed from the > keyboard, but your "different kind of atom" is a much better way to > think about it. Yes even I agree with this. > > What is necessary is that, just like the updated "print_value()" > knows about the formatting state, "emit()" needs to be told about > the same formatting state. Some of these "state affecting" atoms > may take effect on what is output by "emit()" (e.g. "color" is > obviously an example, the hypotethical one that counts the current > output column and uses that knowledge to "align" the output to > certain columns, instead of "right pad to make the next column > 30-columns wide" one, which is in this series, is another). This depends on whether these modifier atoms should effect only succeeding atoms or any string. Since there is a conversation about this topic on the other patch, let's continue this discussion there. > > Thanks for finding this, and Karthik, sorry for an incomplete > suggestion based on a faulty thinking. I don't see why you have to be sorry, you were trying to help :) -- 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
