Re: [PATCH v5 01/11] ref-filter: introduce 'ref_formatting_state'
From: Karthik Nayak <hidden>
Date: 2016-06-15 23:05:53
On Mon, Jul 27, 2015 at 6:12 PM, Matthieu Moy [off-list ref] wrote:
Karthik Nayak [off-list ref] writes:quoted
+static void ref_formatting(struct ref_formatting_state *state, + struct atom_value *v, struct strbuf *value) { - struct strbuf sb = STRBUF_INIT; - switch (quote_style) { + strbuf_addf(value, "%s", v->s); +}You're taking 'state' as argument, but you're not using it in the function for now. Perhaps add a temporary comment like: static void ref_formatting(...) { /* Formatting according to 'state' will be applied here */ strbuf_addf(...) } Or perhaps it's OK like this.
I thought it'd be OK since it doesn't have any adverse effect, but I added the comment you suggested nonetheless.
quoted
-static void print_value(struct atom_value *v, int quote_style) +static void print_value(struct ref_formatting_state *state, struct atom_value *v)Changing the position of the v parameter makes the patch a bit harder to read. I would have written in this order: static void print_value(struct atom_value *v, struct ref_formatting_state *state) So the patch reads as "encapsulate quote_style in a struct" more straightforwardly.
I need to be more careful about things like this, thanks.
quoted
@@ -1257,6 +1269,10 @@ static void emit(const char *cp, const char *ep) void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style) { const char *cp, *sp, *ep; + struct ref_formatting_state state;I still found it a bit hard to read, and I would have appreciated a comment here, like /* * Some (pseudo) atom have no immediate side effect, but only * affect the next atom. Store the relevant information from * these atoms in the 'state' variable for use when displaying * the next atom. */
This seems good, will add this.
quoted hunk ↗ jump to hunk
With this in mind, it becomes more obvious that you also need to reset the state after using it, which you forgot to do. See: $ ./git for-each-ref --format '%(padright:30)|%(refname)|%(refname)|' refs/tags/v2.4.\* |refs/tags/v2.4.0 |refs/tags/v2.4.0 | |refs/tags/v2.4.0-rc0 |refs/tags/v2.4.0-rc0 | |refs/tags/v2.4.0-rc1 |refs/tags/v2.4.0-rc1 | |refs/tags/v2.4.0-rc2 |refs/tags/v2.4.0-rc2 | |refs/tags/v2.4.0-rc3 |refs/tags/v2.4.0-rc3 | |refs/tags/v2.4.1 |refs/tags/v2.4.1 | |refs/tags/v2.4.2 |refs/tags/v2.4.2 | |refs/tags/v2.4.3 |refs/tags/v2.4.3 | |refs/tags/v2.4.4 |refs/tags/v2.4.4 | |refs/tags/v2.4.5 |refs/tags/v2.4.5 | |refs/tags/v2.4.6 |refs/tags/v2.4.6 | I think only the first column should have padding, not the second. You can fix this with a patch like this:--- a/ref-filter.c +++ b/ref-filter.c@@ -1431,6 +1431,14 @@ static void apply_pseudo_state(struct ref_formatting_state *state, state->ifexists = v->s; } +static void reset_formatting_state(struct ref_formatting_state *state) +{ + int quote_style = state->quote_style; + memset(state, 0, sizeof(*state)); + state->quote_style = quote_style; +} + + /* * If 'lines' is greater than 0, print that many lines from the given * object_id 'oid'.@@ -1492,8 +1500,11 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv); if (atomv->pseudo_atom) apply_pseudo_state(&state, atomv); - else + else { print_value(&state, atomv); + reset_formatting_state(&state); + } + } if (*cp) { sp = cp + strlen(cp);
If you saw the last patch series, i had them reset individually, I missed that by mistake. But what you're doing seems good, will integrate this. thanks :D -- Regards, Karthik Nayak