Re: [PATCH v4 01/10] ref-filter: add option to align atoms to the left

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 24, 2015, 23:00 UTC
Message-ID: <xmqqpp3hjh3r.fsf@gitster.dls.corp.google.com>
In-Reply-To: <xmqqwpxpjk6p.fsf@gitster.dls.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 22 quoted lines

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> -	if (!ref->value) {
>> -		populate_value(ref);
>> +	/*
>> +	 * If the atom is a pseudo_atom then we re-populate the value
>> +	 * into the ref_formatting_state stucture.
>> +	 */
>> +	if (!ref->value || ref->value[atom].pseudo_atom) {
>> +		populate_value(state, ref);
>>  		fill_missing_values(ref->value);
>
> I am not sure why you need to do this.  populate_value() and
> fill_missing_values() are fairly heavy-weight operations that are
> expected to grab everything necessary from scratch, and that is why
> we ensure that we do not call them more than once for each "ref"
> with by guarding the calls with "if (!ref->value)".
>
> This change is breaking that basic arrangement, and worse yet, it
> forces us re-read everything about that ref, leaking old ref->value.
>
> Why could this be a good idea?

I think populate_value() should not take state; that is the root cause of this mistake.

The flow should be:

    - verify_format() looks at the format string and enumerates all
      atoms that will ever be used in the output by calling
      parse_atom() and letting it to fill used_atom[];

    - when ref->value is not yet populated, populate_value() is
      called, just once.  This uses the enumeration in used_atom[]
      and stores computed value to refs->value[atom];

    - show_ref() repeatedly calls find_next() to find the next
      reference to %(...), emits everything before it, and then
      uses the atom value (i.e. ref->value[atom]).

I would expect that the atom value for pseudos like color and align to be parsed and stored in ref->value in populate_value() when it is called for the first time for each ref _just once_.

"color:blue" may choose to store "blue" as v->s, and "align:4" may choose to do "v->ul = 4".

And the code that uses these values should look more like:

	for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
		struct atom_value *atomv;

		ep = strchr(sp, ')');
		if (cp < sp)
			emit(cp, sp);
		get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
		if (atomv->is_pseudo)
                	apply_pseudo_state(&state, atomv);
		else
			print_value(&state, atomv);
	}

where apply_pseudo_state() would switch on what kind of pseudo the atom is and update the state accordingly, i.e. the "state" munging code you added to populate_value() in this patch.