Re: [PATCH v13 04/12] ref-filter: implement an `align` atom

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +static void perform_quote_formatting(struct strbuf *s, const char *str, int quote_style);
>> +
>> +static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
>> +{
>> +     struct ref_formatting_stack *current = state->stack;
>> +     struct strbuf s = STRBUF_INIT;
>> +
>> +     if (!current->at_end)
>> +             die(_("format: `end` atom used without a supporting atom"));
>> +     current->at_end(current);
>> +     /*
>> +      * Whenever we have more than one stack element that means we
>> +      * are using a certain modifier atom. In that case we need to
>> +      * perform quote formatting.
>> +      */
>> +     if (!state->stack->prev->prev) {
>
> The comment and the condition seem to be saying opposite things.
> The code says "If the stack only has one prev that is the very
> initial one, then we do the quoting, i.e. the result of expanding
> the enclosed string in %(start-something)...%(end) is quoted only
> when that appears at the top level", which feels more correct than
> the comment that says "if we are about to pop after seeing the
> first %(end) in %(start)...%(another)...%(end)...%(end) sequence,
> we quote what is between %(another)...%(end)".
>

>> +             perform_quote_formatting(&s, current->output.buf, state->quote_style);
>> +             strbuf_reset(&current->output);
>> +             strbuf_addbuf(&current->output, &s);
>> +     }
>> +     strbuf_release(&s);
>> +     pop_stack_element(&state->stack);
>> +}
>> +
>
>> @@ -1228,29 +1315,33 @@ void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)
>>       qsort(array->items, array->nr, sizeof(struct ref_array_item *), compare_refs);
>>  }
>>
>> -static void append_atom(struct atom_value *v, struct ref_formatting_state *state)
>> +static void perform_quote_formatting(struct strbuf *s, const char *str, int quote_style)
>>  {
>> -     struct strbuf *s = &state->stack->output;
>> -
>> -     switch (state->quote_style) {
>> +     switch (quote_style) {
>>       case QUOTE_NONE:
>> -             strbuf_addstr(s, v->s);
>> +             strbuf_addstr(s, str);
>>               break;
>>       case QUOTE_SHELL:
>> -             sq_quote_buf(s, v->s);
>> +             sq_quote_buf(s, str);
>>               break;
>>       case QUOTE_PERL:
>> -             perl_quote_buf(s, v->s);
>> +             perl_quote_buf(s, str);
>>               break;
>>       case QUOTE_PYTHON:
>> -             python_quote_buf(s, v->s);
>> +             python_quote_buf(s, str);
>>               break;
>>       case QUOTE_TCL:
>> -             tcl_quote_buf(s, v->s);
>> +             tcl_quote_buf(s, str);
>>               break;
>>       }
>>  }
>>
>> +static void append_atom(struct atom_value *v, struct ref_formatting_state *state)
>> +{
>> +     struct strbuf *s = &state->stack->output;
>> +     perform_quote_formatting(s, v->s, state->quote_style);
>
> Hmmm, do we want to unconditionally do the quote here, or only when
> we are not being captured by upcoming %(end) to be consistent with
> the behaviour of end_atom_handler() above?
>
>> @@ -1307,7 +1398,18 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
>>               if (cp < sp)
>>                       append_literal(cp, sp, &state);
>>               get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
>> -             append_atom(atomv, &state);
>> +             /*
>> +              * If the atom is a modifier atom, then call the handler function.
>> +              * Else, if this is the first element on the stack, then we need to
>> +              * format the atom as per the given quote. Else we just add the atom value
>> +              * to the current stack element and handle quote formatting at the end.
>> +              */
>> +             if (atomv->handler)
>> +                     atomv->handler(atomv, &state);
>> +             else if (!state.stack->prev)
>> +                     append_atom(atomv, &state);
>> +             else
>> +                     strbuf_addstr(&state.stack->output, atomv->s);
>
> Ahh, this explains why you are not doing it above, but I do not
> think if this is a good division of labor.
>
> You can see that I expected that "if !state.stack->prev" check to be
> inside append_atom(), and I would imagine future readers would have
> the same expectation when reading this code.  I.e.
>
>         append_atom(struct atom_value *v, struct ref_f_s *state)
>         {
>                 if (state->stack->prev)
>                         strbuf_addstr(&state->stack->output, v->s);
>                 else
>                         quote_format(&state->stack->output, v->s, state->quote_style);
>         }
>
> The end result may be the same, but I do think "append_atom is to
> always quote, so we do an unquoted appending by hand" is a bad way
> to do this.
>
> Moreover, notice that the function signature of append_atom() is
> exactly the same as atomv->handler's.  I wonder if it would be
> easier to understand if you made append_atom() the handler for a
> non-magic atoms, which would let you do the above without any if/else
> and just a single unconditional
>
>         atomv->handler(atomv, &state);