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

Karthik Nayak [off-list ref] 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.

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/

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.

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