Re: [PATCH v14 04/13] ref-filter: implement an `align` atom

On Sun, Aug 30, 2015 at 9:38 AM, Karthik Nayak [off-list ref] wrote:

On Sun, Aug 30, 2015 at 8:57 AM, Eric Sunshine [off-list ref] wrote:

quoted

On Sat, Aug 29, 2015 at 10:12 AM, Karthik Nayak [off-list ref] wrote:

quoted

+static void append_atom(struct atom_value *v, struct ref_formatting_state *state)
+{
+       /*
+        * Quote formatting is only done when the stack has a single
+        * element. Otherwise quote formatting is done on the
+        * element's entire output strbuf when the %(end) atom is
+        * encountered.
+        */
+       if (!state->stack->prev)

With the disclaimer that I wasn't following the quoting discussion
closely: Is this condition going to be sufficient for all cases, such
as an %(if:) atom? That is, if you have:

    %(if:notempty)%(bloop)%(then) --option=%(bloop)%(end)

isn't the intention that, %(bloop) within the %(then) section should
be quoted but not the literal "--option="?

The condition `!state->stack->prev' would be insufficient to handle
this if %(if:) pushes one or more states onto the stack, no? This
implies that you might want an explicit flag for enabling/disabling
quoting rather than relying upon the state of the stack, and that
individual atom handlers would control that flag.

quoted

Or, am I misunderstanding due to not having followed the discussion closely?

Yes this wont be work for what you've said.

To sum up the discussion:
We didn't want atoms within the %(align)....%(end) to be quoted separately
rather the whole aligned buffer to be quoted at the end.

But this does conflict with %(if)...%(end). So it makes sense to change it to
only have checks for %(align) atom being used.

If I understand Junio's response correctly, then this doesn't sound
correct either. Rather than imbuing only %(align:) with special
quoting knowledge, it sounds like quoting should be handled
generically for all top-level atoms and pseudo-atoms, including %(if:)
and others which may come later.

(But perhaps I'm still misunderstanding...)

So probably:

static void append_atom(struct atom_value *v, struct
ref_formatting_state *state)
{
    if (state->stack->at_end == align_handler)

This couples append_atom() far too tightly with the %(align:) atom. If
you really need to do this sort of special-casing, then it probably
would make more sense to have an explicit flag saying whether or not
quoting should be done, rather than tying it specifically to the
%(align:) atom.

However, (again, if I'm understanding Junio's response), your original
`!state->stack->prev' condition might be sufficient after all.

        strbuf_addstr(&state->stack->output, v->s);
    else
        quote_formatting(&state->stack->output, v->s, state->quote_style);
}

and

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 align modifier atom. In that case we need to
     * perform quote formatting.
     */
    if (current->at_end == align_handler) {

Ditto about being too tightly coupled to %(align:). Such logic should
likely be generic for any such atom.

        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);
}

quoted

quoted

@@ -725,6 +818,37 @@ static void populate_value(struct ref_array_item *ref)
                        else
                                v->s = " ";
                        continue;
+               } else if (!strcmp(name, "align"))
+                       die(_("format: incomplete use of the `align` atom"));

Why does %(align) get flagged as a malformation of %(align:), whereas
%(color) does not get flagged as a malformation of %(color:)? Why does
one deserve special treatment but not the other?

Didn't see that, I think its needed to add a check for both like :

else if (!strcmp(name, "align") || !strcmp(name, "color"))
            die(_("format: improper usage of %s atom"), name);

I had a look if any other atoms need a subvalue to operate, couldn't
find any.

Hmm, I'm not convinced that either %(align) or %(color) need to be
called out specially. What is the current behavior when these
"malformations" or any other misspelled atoms are used? Does it error
out? Does it simply ignore them and pass them through to the output
unmolested?