On Sun, Aug 30, 2015 at 3:10 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Sun, Aug 30, 2015 at 9:38 AM, Karthik Nayak <karthik.188@xxxxxxxxx> wrote:
>> On Sun, Aug 30, 2015 at 8:57 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>>> On Sat, Aug 29, 2015 at 10:12 AM, Karthik Nayak <karthik.188@xxxxxxxxx> wrote:
>>>> +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.
>>
>>> 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...)
>
I was sure about how %(align)...%(end) was to be quoted at the end.
>From Junio's comment it seems all top level atoms need to be quoted at the end
so with regards to this, the new changes aren't needed and the old
changes will hold.
>> 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.
>
same as above.
>> 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.
>
Shouldn't be needed, was mistaken, sticking to the old logic.
>> quote_formatting(&s, current->output.buf, state->quote_style);
>> strbuf_reset(¤t->output);
>> strbuf_addbuf(¤t->output, &s);
>> }
>> strbuf_release(&s);
>> pop_stack_element(&state->stack);
>> }
>>>> @@ -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?
It just simply ignores them currently, which is kinda bad, as the user
is given no
warning, and the atom is ineffective.
--
Regards,
Karthik Nayak
--
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