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

[Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>]

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

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

?

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

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

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