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

> Junio C Hamano <gitster@pobox.com> writes:
>
>> 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?
>

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