Re: [PATCH v4 1/8] branch: refactor width computation

From: Karthik Nayak <karthik.188@gmail.com>
Date: Sep 13, 2015, 12:23 UTC
Message-ID: <CAOLa=ZQoEsJ_pHZpsOWJaCVhku0Ai0Xkc=ON+LR+qGspDnszqQ@mail.gmail.com>
In-Reply-To: <vpqio7ev8o6.fsf@anie.imag.fr>

On Sun, Sep 13, 2015 at 5:21 PM, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

Show 21 quoted lines

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> @@ -667,26 +675,22 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
>>                       clear_commit_marks(item->commit, ALL_REV_FLAGS);
>>               }
>>               clear_commit_marks(filter, ALL_REV_FLAGS);
>> -
>> -             if (verbose)
>> -                     ref_list.maxwidth = calc_maxwidth(&ref_list);
>>       }
>> +     if (verbose)
>> +             maxwidth = calc_maxwidth(&ref_list, strlen(remote_prefix));
>
> I don't understand this hunk. To give a bit more context, the closing
> brace corresponds to:
>
>         if (merge_filter != NO_FILTER) {
>
> Hence this patch gets the two lines out of this "if". Actually, I don't
> understand how it could work previously. Wasn't this "calc_maxwidth"
> needed regardless of merge_filter from the beginning?

Previously, we used to compute and store each item's width in append_ref() and change the ref_list.maxwidth only if we find a new maxwidth (all within append_ref()). Although the maxwidth variable is only needed when we use the verbose option this is computed otherwise also (without the usage of calc_maxwidth()). When using the merge option with verbose the maxwidth would need to be recalculated. Hence we compute the maxwidth again using calc_maxwidth() within the if block.

Currently we only compute and store maxwidth if required, after obtaining required refs.

>
> In any case, that remark is not an objection on your patch, but I'd like
> to understand.
>

Happy to explain.

-- 
Regards,
Karthik Nayak