Karthik Nayak <karthik.188@xxxxxxxxx> writes:
> +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.
> -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.
> @@ -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.
*/
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);
--
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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html