Re: [PATCH v5 02/11] ref-filter: make `color` use `ref_formatting_state`

Karthik Nayak [off-list ref] writes:

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

... this hunk would belong to PATCH 1.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

On Mon, Jul 27, 2015 at 6:36 PM, Matthieu Moy
[off-list ref] wrote:

Karthik Nayak [off-list ref] writes:

quoted

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

quoted

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