On Wed, Aug 12, 2015 at 12:22 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Karthik Nayak <karthik.188@xxxxxxxxx> writes:
>
>> struct atom_value{
>
> Obviously not a problem with this step, but you need a SP before the
> open brace.
>
Will add.
>> @@ -692,6 +704,26 @@ static void populate_value(struct ref_array_item *ref)
>> else
>> v->s = " ";
>> continue;
>> + } else if (skip_prefix(name, "align:", &valp)) {
>> + struct align *align = xmalloc(sizeof(struct align));
>> +
>> + if (skip_prefix(valp, "left,", &valp))
>> + align->position = ALIGN_LEFT;
>> + else if (skip_prefix(valp, "right,", &valp))
>> + align->position = ALIGN_RIGHT;
>> + else if (skip_prefix(valp, "middle,", &valp))
>> + align->position = ALIGN_MIDDLE;
>> + else
>> + die(_("improper format entered align:%s"), valp);
>> + if (strtoul_ui(valp, 10, &align->width))
>> + die(_("positive width expected align:%s"), valp);
>
> Minor nits on the design. %(align:<width>[,<position>]) would let
> us write %(align:16)...%(end) and use the "default position", which
> may be beneficial if one kind of alignment is prevalent (I guess all
> the internal users left-align?) %(align:<position>,<width>) forces
> users to spell both out all the time.
>
Isn't that better? I mean It sets a format which the others eventually
can follow
%(atom:suboption,value).
For example: %(objectname:abbrev,size)
Changing this would cause inconsistency according to me.
>> @@ -1198,7 +1230,9 @@ void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)
>>
>> struct ref_formatting_state {
>> struct strbuf output;
>> + struct align *align;
>> int quote_style;
>> + unsigned int end : 1;
>> };
>
> Mental note: it is not clear why you need 'end' field in the state.
> Perhaps it is an indication that the division of labor is poorly
> designed between the helper that updates the formatting state and
> the other helper that reflects the formatting state to the final
> string.
>
This goes with what you've said below!
>> @@ -1262,12 +1296,31 @@ static void append_non_atom(const char *cp, const char *ep, struct ref_formattin
>>
>> static void set_formatting_state(struct atom_value *atomv, struct ref_formatting_state *state)
>> {
>> - /* Based on the atomv values, the formatting state is set */
>> + if (atomv->align) {
>> + state->align = atomv->align;
>> + atomv->align = NULL;
>> + }
>> + if (atomv->end)
>> + state->end = 1;
>> +}
>> +
>> +static int align_ref_strbuf(struct ref_formatting_state *state, struct strbuf *final)
>> +{
>> + if (state->align && state->end) {
>
> ... and I think that is what I see. If this function knows that we
> are processing %(end), i.e. perform-state-formatting is called for
> each atom and receives atomv, there wouldn't have to be a code like
> this.
>
Agreed, your suggestion below eradicates this
>> + struct align *align = state->align;
>> + strbuf_utf8_align(final, align->position, align->width, state->output.buf);
>> + strbuf_reset(&state->output);
>> + state->align = NULL;
>> + return 1;
>> + } else if (state->align)
>> + return 1;
>> + return 0;
>> }
>>
>> static void perform_state_formatting(struct ref_formatting_state *state, struct strbuf *final)
>> {
>> - /* More formatting options to be eventually added */
>> + if (align_ref_strbuf(state, final))
>> + return;
>
> At the design level, I have a strong suspicion that it is a wrong
> way to go. It piles more "if (this state bit was left by the
> previous atom) then do this" on this function and will make an
> unmanageable mess.
Hmm, yeah makes sense.
>
> You have a dictionary of all possible atoms somewhere. Why not hook
> a pointer to the "handler" function (or two) to each element in it,
> instead of duplicating "this one is special" information down to
> individual atom instantiations (i.e. atomv) as atomv.modifier_atom
> bit, an dstructure the caller more like this?
>
> get_ref_atom_value(info, parse_ref_filter_atom, &atomv);
> if (atomv->pre_handler)
> atomv->pre_handler(atomv, &state);
> format_quote_value(atomv, &state);
> if (atomv->post_handler)
> atomv->post_handler(atomv, &state);
>
> Actually, each atom could just have a single handler; an atom like
> %(refname:short) whose sole effect is to append atomv->s to the
> state buffer can point a function to do so in its handler.
>
> On the other hand, align atom's handler would push a new state on
> the stack, marking that it is the one to handle diverted output.
>
> align_atom_handler(atomv, state)
> {
> struct format_state *new = push_new_state(state);
> strbuf_init(&new->output);
> new->atend = align_handler;
> new->return_to = atomv; /* or whatever that holds width,pos */
> }
>
> Then end atom's handler would pop the state from the stack, and the
> processing to be done
>
> end_atom_handler(atomv, state)
> {
> state->atend(state);
> pop_state(state);
> }
>
> and the called align_handler would be something like:
>
> align_handler(state)
> {
> struct strbuf aligned = STRBUF_INIT;
> struct format_state *return_to = state->prev;
> struct atom_value *atomv = state->return_to;
>
> strbuf_utf8_align(&aligned,
> atomv->align.pos, atomv->align.width,
> state->output.buf);
> strbuf_addbuf(&return_to->output, &aligned);
> strbuf_release(&aligned);
> }
>
> With an arrangement like that, the body of the loop in
> show_ref_array_item() could be as simple and regular as:
>
> get_ref_atom_value(info, parse_ref_filter_atom, &atomv);
> atomv->handler(atomv, &state);
>
> without any new "ah, this %(end) is special so we need a new
> mechanism to pass information between set_formatting_state and
> perform_formatting" logic introduced every time you add new things.
>
This I agree is a way better scheme, This could also eventually help cleanup
populate_value() which is a huge function at the moment.
I like your suggestion, I'll work on this, Thanks a lot :)
--
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