From: Karthik Nayak <karthik.188@gmail.com>
Date: Wed, 02 Sep 2015 13:12:20 GMT
Subject: Re: [PATCH v15 05/13] ref-filter: implement an `align` atom
Message-ID: <CAOLa=ZQxWO6+_N6TKgZ09HVGiNNDy29p6kU56Y2TX8Y+kYTcsA@mail.gmail.com>
In-Reply-To: <vpqvbbttdjm.fsf@anie.imag.fr>

On Wed, Sep 2, 2015 at 2:15 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>
>> @@ -163,9 +174,28 @@ static void quote_formatting(struct strbuf *s, const char *str, int quote_style)
>>       }
>>  }
>>
>> +static void align_handler(struct ref_formatting_stack *stack)
>
> Perhaps name it end_align_handler, to make the difference with
> align_atom_handler clear.
>
> Also, I think moving this function to be right next to
> align_atom_handler in the code would make this more readable.
>

will do.

>> @@ -754,6 +816,42 @@ static void populate_value(struct ref_array_item *ref)
>>                       else
>>                               v->s = " ";
>>                       continue;
>> +             } else if (skip_prefix(name, "align", &valp)) {
>> +                     struct align *align = &v->align;
>> +                     struct strbuf **s;
>> +
>> +                     if (valp[0] != ':')
>> +                             die(_("format: usage %%(align:<width>,<position>)"));
>> +                     else
>> +                             valp++;
>
> I agree with Junio that skip_prefix(name, "align:" ...) is simpler for
> the same thing.

Correct me if mistaken, but Junio wanted the first skip_prefix to
check for "align:" rather than "align", but that would mean we don't have
the error printing.

Are you sure we want to skip that?

>
>> +     if (state.stack->prev)
>> +             die(_("format: `end` atom missing"));
>
> Perhaps spell it %(end) instead of just end.
>

Yes, will do

-- 
Regards,
Karthik Nayak