Re: [PATCH 02/10] branch: refactor width computation

[Karthik Nayak <karthik.188@gmail.com>]

On Tue, Aug 11, 2015 at 7:28 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Aug 4, 2015 at 9:01 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Remove unnecessary variables from ref_list and ref_item which were
>> used for width computation. This is to make ref_item similar to
>> ref-filter's ref_array_item. This will ensure a smooth port of
>> branch.c to use ref-filter APIs in further patches.
>>
>> Previously the maxwidth was computed when inserting the refs into the
>> ref_list. Now, we obtain the entire ref_list and then compute
>> maxwidth.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> diff --git a/builtin/branch.c b/builtin/branch.c
>> index 4fc8beb..b058b74 100644
>> --- a/builtin/branch.c
>> +++ b/builtin/branch.c
>> @@ -557,16 +552,21 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
>> -static int calc_maxwidth(struct ref_list *refs)
>> +static int calc_maxwidth(struct ref_list *refs, int remote_bonus)
>>  {
>> -       int i, w = 0;
>> +       int i, max = 0;
>>         for (i = 0; i < refs->index; i++) {
>> +               struct ref_item *it = &refs->list[i];
>> +               int w = utf8_strwidth(it->name);
>> +
>>                 if (refs->list[i].ignore)
>>                         continue;
>
> Nit: Unnecessary work. You compute the width and then immediately
> throw it away when 'ignore' is true.
>
> Also, you use 'it' elsewhere rather than 'refs->list[i]' but not this
> line, which makes it seem out of place. I would have expected to see:
>
>     if (it->ignore)
>         continue;
>
> (Despite the noisier diff, the end result will be more consistent.)
>

Ah right, will put that after the check :D

Yea, that seems out of place. Thanks

-- 
Regards,
Karthik Nayak