Re: [PATCH v5 01/11] ref-filter: introduce 'ref_formatting_state'

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Jul 27, 2015, 12:42 UTC
Message-ID: <vpqmvyhai1k.fsf@anie.imag.fr>
In-Reply-To: <1437982035-6658-1-git-send-email-Karthik.188@gmail.com>

Karthik Nayak <karthik.188@gmail.com> writes:

Show 7 quoted lines

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

Show 5 quoted lines

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