Re: [PATCH v3 1/9] ref-filter: add option to align atoms to the left

From: Karthik Nayak <karthik.188@gmail.com>
Date: Jul 20, 2015, 15:06 UTC
Message-ID: <CAOLa=ZRp0WXKj7au_eDk=80ivsgsNWEyO9JBy_rwzOXmnQwS_A@mail.gmail.com>
In-Reply-To: <CAPig+cRXsmi=UxRr-3rnt919d86jD6uMuTqdDxCComYLXk1TYw@mail.gmail.com>

On Mon, Jul 20, 2015 at 5:19 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 21 quoted lines

> On Sat, Jul 18, 2015 at 3:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Add a new atom "align" and support %(align:X) where X is a number.
>> This will align the preceeding atom value to the left followed by
>> spaces for a total length of X characters. If X is less than the item
>> size, the entire atom value is printed.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 7561727..b81a08d 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -53,6 +55,7 @@ static struct {
>>         { "flag" },
>>         { "HEAD" },
>>         { "color" },
>> +       { "align" },
>
> Not a new issue, but some compilers (Solaris?) complain about the
> trailing comma.
>

Ok will check.

Show 18 quoted lines

>>  };
>>
>>  /*
>> @@ -687,6 +690,17 @@ static void populate_value(struct ref_array_item *ref)
>>                         else
>>                                 v->s = " ";
>>                         continue;
>> +               } else if (starts_with(name, "align:")) {
>> +                       const char *valp = NULL;
>> +
>> +                       skip_prefix(name, "align:", &valp);
>> +                       if (!valp[0])
>> +                               die(_("No value given with 'align='"));
>
> The parser expects "align:", but the error message talks about
> "align=". Also, current trend is to drop capitalization from the error
> message.
>

Thanks will change.

Show 13 quoted lines

>> +                       strtoul_ui(valp, 10, &ref->align_value);
>> +                       if (ref->align_value < 1)
>> +                               die(_("Value should be greater than zero"));
>
> Drop capitalization. Also, the user seeing this message won't
> necessarily know to which value this refers. Better would be to
> provide context ("'align:' value should be..."), and even better would
> be to show the actual value at fault:
>
>     die(_("value should be greater than zero: align:%u\n",
>         ref_align_value);
>
> or something.

Makes sense, thanks :)

Show 18 quoted lines

>
>> +                       v->s = "";
>> +                       continue;
>>                 } else
>>                         continue;
>>
>> @@ -1254,17 +1268,38 @@ static void emit(const char *cp, const char *ep)
>>         }
>>  }
>>
>> +static void assign_formating(struct ref_array_item *ref, int parsed_atom, struct atom_value *v)
>> +{
>> +       if (v->s[0] && ref->align_value) {
>
> Mental note: v->s[0] is not NUL ('\0').
>
> Also, in this code base, this is typically written *v->s rather than v->s[0].
>

My bad, got confused with that :)

>> +               unsigned int len = 0;
>> +               len = utf8_strwidth(v->s);
>
> You initialize 'len' to 0 but then immediately re-assign it.

Will change.

Show 9 quoted lines

>
>> +               if (ref->align_value > len) {
>> +                       struct strbuf buf = STRBUF_INIT;
>> +                       strbuf_addstr(&buf, v->s);
>> +                       if (!v->s[0])
>> +                               free((char *)v->s);
>
> We know from the "mental note" above that v->s[0] is not NUL ('\0'),
> so this 'if' statement can never be true, thus is dead code.

Yes, my bad. Will change.

Show 7 quoted lines

>
>> +                       strbuf_addchars(&buf, ' ', ref->align_value - len);
>> +                       v->s = strbuf_detach(&buf, NULL);
>> +               }
>> +               ref->align_value = 0;
>> +       }
>> +}

Thanks.

-- 
Regards,
Karthik Nayak