On Wed, Sep 2, 2015 at 2:49 AM, Junio C Hamano <[email protected]> wrote:
> Karthik Nayak <[email protected]> writes:
>
>> We have an `at_end` function for each element of the stack which is to
>> be called when the `end` atom is encountered. Using this we implement
>> the aling_handler() for the `align` atom, this aligns the final strbuf
>
> align_handler().
Will change.
>
>> struct ref_formatting_stack {
>> struct ref_formatting_stack *prev;
>> struct strbuf output;
>> + void (*at_end)(struct ref_formatting_stack *stack);
>> + void *cb_data;
>> };
>
> s/cb_data/at_end_data/ or something, as this is not really about a
> function callback. Imagine a fictional future where you add a new
> functions at_middle---the readers cannot tell what cb_data is about
> at that point.
>
Makes sense will change.
>> +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"));
>
> Not a show-stopper, but we may need some wordsmithing for "a
> supporting atom" here; an end-user would not know what it is.
>
Probably something like "format: `end` atom should only be
used with modifier atoms".
>> + current->at_end(current);
>> +
>> + /*
>> + * Perform quote formatting when the stack element is that of
>> + * a modifier atom and right above the first stack element.
>> + */
>> + if (!state->stack->prev->prev) {
>> + quote_formatting(&s, current->output.buf, state->quote_style);
>> + strbuf_swap(¤t->output, &s);
>> + }
>> + strbuf_release(&s);
>> + pop_stack_element(&state->stack);
>> +}
>
> Nice.
>
>> @@ -687,6 +748,7 @@ static void populate_value(struct ref_array_item *ref)
>> int deref = 0;
>> const char *refname;
>> const char *formatp;
>> + const char *valp;
>> struct branch *branch = NULL;
>>
>> v->handler = append_atom;
>> @@ -754,6 +816,42 @@ static void populate_value(struct ref_array_item *ref)
>> else
>> v->s = " ";
>> continue;
>> + } else if (skip_prefix(name, "align", &valp)) {
>
> This looked as if you are willing to take %(align) in addition to
> %(align:...), but...
>
>> + struct align *align = &v->align;
>> + struct strbuf **s;
>> +
>> + if (valp[0] != ':')
>> + die(_("format: usage
>> %%(align:<width>,<position>)"));
>
> ... apparently that is not what is happening. Why not skip "align:"
> with colon as the prefix, then?
>
Cause we wanted to provide an error for usage of "%(ailgn)" without any
subvalues as such.
>> + else
>> + valp++;
>> + s = strbuf_split_str(valp, ',', 0);
>> +
>> + /* If the position is given trim the ',' from the
>> first strbuf */
>> + if (s[1])
>> + strbuf_setlen(s[0], s[0]->len - 1);
>> +
>> + if (strtoul_ui(s[0]->buf, 10, &align->width))
>> + die(_("positive width expected align:%s"),
>> s[0]->buf);
>> +
>> + if (!s[1])
>> + align->position = ALIGN_LEFT;
>> + else if (!strcmp(s[1]->buf, "left"))
>> + align->position = ALIGN_LEFT;
>> + else if (!strcmp(s[1]->buf, "right"))
>> + align->position = ALIGN_RIGHT;
>> + else if (!strcmp(s[1]->buf, "middle"))
>> + align->position = ALIGN_MIDDLE;
>> + else
>> + die(_("improper format entered align:%s"),
>> s[1]->buf);
>
> This does not reject %(align:40,left,junk), no? Before "s[1] does
> not exist so default to left align", you would want
>
> if (s[2])
> die("align:width,position followed by garbage: ,%s",
> s[2]->buf);
>
Yea we should probably do that.
> I have a few observations; these are not necessarily we would want
> to change in the scope of this series, though.
>
> - The design of strbuf_split_buf API feels screwy. A variant of
> this function that strips the terminator at the end would be what
> most callers would want. Granted, leaving the terminator in the
> resulting buffer does let the caller tell if the input ended with
> an incomplete line that lacked the final terminator, but for all
> s[i] for 0 <= i < N-1 where s[N] is the first element that is
> NULL, they must end with the terminator---otherwise the elements
> would not have split into the array in the first place. "By
> keeping the terminator, you can tell which one of the possible
> terminators was used" could be a valid rationale for the API if
> the function allowed more than one terminators, but that does not
> apply here, either.
>
> - I would have expected the above code to look more like this:
>
> width = -1; position = ALIGN_LEFT;
> s = strbuf_split_str(valp, ',', 0);
> while (*s) {
> if (s[1])
> strbuf_setlen(*s, *s->len - 1);
> if (!strtoul_ui(*s->buf, 10, &width))
> ; /* parsed width successfully */
> else if (!strcmp(*s->buf, "left"))
> position = ALIGN_LEFT;
> else if ...
> else
> die("unknown parameter: %s", *s->buf);
> s++;
> }
> if (width < 0)
> ... perhaps set to the default width, or
> ... call die() complaining that you did not see
> ... an explicit width specified
>
> Doing the code that way, it would be more obvious that a way to
> extend the parser to accept forms like
>
> %(align:width=40,position=left)
>
> is by adding "keyword=value" parser before the fallbacks for
> short-hand, i.e. "if looks like number" and everything else.
>
I'll keep this in mind, probably work on this later after this series is done.
Thanks
--
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
