Re: [PATCH v15 05/13] ref-filter: implement an `align` atom

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Sep 2, 2015 at 2:49 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Karthik Nayak <karthik.188@xxxxxxxxx> 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(&current->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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]