Junio C Hamano <gitster@xxxxxxxxx> writes:
> Karthik Nayak <karthik.188@xxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html