Re: [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account

From: Lucian Poston <lucian.poston@gmail.com>
Date: Apr 12, 2012, 08:35 UTC
Message-ID: <CACz_eyetzT3AFg1w3CbQegPLHfH0inwcYv5yhbffjog0cBqwug@mail.gmail.com>
In-Reply-To: <7vy5qrtcca.fsf@alter.siamese.dyndns.org>

On Fri, Mar 23, 2012 at 11:13, Junio C Hamano <gitster@pobox.com> wrote:

> Please do not do reflowing of the text in the same patch as modifying the
> logic.  It is unreadable for the purpose of finding out what you really
> changed.

I will un-reflow the text. :]

Show 19 quoted lines

>
>>        *
>>        * In other words: stat_width limits the maximum width, and
>>        * stat_name_width fixes the maximum width of the filename,
>>        * and is also used to divide available columns if there
>>        * aren't enough.
>>        */
>> +     reserved_character_count = 6 + number_width;
>
> As far as I can tell, this introduces a variable that is set (and is meant
> to be set) only at a single place, namely, here, and used throughout the
> rest of the function. But it invites later patches to mistakenly update
> the variable.  I do not see the merit of it.
>
> If you wanted to have a symbolic name for (6+number_width), #define would
> have served better.
>
> Also as we see in the later part of the review, this name is probably way
> too long to be useful.  We need a shorter and sweeter name to call it.

I'll remove it.

Show 18 quoted lines

>>       if (options->stat_width == -1)
>>               width = term_columns();
>>       else
>>               width = options->stat_width ? options->stat_width : 80;
>>
>> +     width -= line_prefix_length;
>> +
>
> Isn't this wrong?  This is not a rhetoric question, iow, I am not
> declaring that this is wrong --- I just cannot see why the above is a good
> change, as I do not see a sound reasoning behind it.
>
> When the user said "--stat-width=80", she means that the diffstat part
> (name and bargraph) is to extend 80 places, and she does not expect it to
> be reduced by the width of the ancestry graph.  If the user wanted to clip
> the entire width, she would have used COLUMNS=80 instead.
>
> Am I missing something?

You're right, the prefix length shouldn't be subtracted when --stat-width is specified.

Show 27 quoted lines

>> @@ -1472,16 +1484,36 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
>>       /*
>>        * Adjust adjustable widths not to exceed maximum width
>>        */
>> -     if (name_width + number_width + 6 + graph_width > width) {
>> -             if (graph_width > width * 3/8 - number_width - 6)
>> -                     graph_width = width * 3/8 - number_width - 6;
>> +     if (reserved_character_count + name_width + graph_width > width) {
>> +             /*
>> +              * Reduce graph_width to be at most 3/8 of the unreserved space and no
>> +              * less than 6, which leaves at least 5/8 for the filename.
>> +              */
>> +             if (graph_width > width * 3/8 - reserved_character_count) {
>> +                     graph_width = width * 3/8 - reserved_character_count;
>> +                     if (graph_width < 6) {
>> +                             graph_width = 6;
>> +                     }
>> +             }
>
> What is this about?  reserved_character_count already knows about the
> magic number 6 and here you have another magic number 6.  How are they
> related with each other?
>
> In other words, shouldn't the added code be more like this?
>
>        if (graph_width < reserved_character_count - number_width)
>                graph_width = reserved_character_count - number_width;

There are two magic number 6's. From previous comments that explain how reserved_character_count is calculated:

        * Each line needs space for the following characters:
        *   - 1 for the initial " "
        *   - 4 + decimal_width(max_change) for " | NNNN "
        *   - 1 for the empty column at the end,
        * Altogether, the reserved_character_count totals
        * 6 + decimal_width(max_change).

In the case of deriving reserved_character_count, 6 arises because 1+4+1.

The second magic number 6 is the minimum value for graph_width. The intention of the original stat width calculation was to give the filename portion 5/8ths of the total width and give the graph portion 3/8ths of the total width. With 80 columns, that works out to 50 for filename and 30 for the graph (plus reserved characters). With 16 columns, that works out to be 10 and 6. I assume 5 and 3 would be too small, so 10 and 6 were probably chosen as the minimum values by the previous author(s).

So now you ask why I added the if (graph_width < 6) conditional?

Show 6 quoted lines

> +             if (graph_width > width * 3/8 - reserved_character_count) {
> +                     graph_width = width * 3/8 - reserved_character_count;
> +                     if (graph_width < 6) {
> +                             graph_width = 6;
> +                     }
> +             }

The calculation in the graph_width assignment (and the prior conditional) does not guarantee graph_width is at least 6. The calculation should be ((width - reserved_character_count) * 3/8), instead of (width * 3/8 - reserved_character_count). But as we saw in my initial patch, adjusting this calculation causes test regressions. Therefore, I added a conditional to catch the edge case where graph_width is less than 6.

Assuming the $COLUMNS is 26 or less, graph_width will actually come out to -1, iirc.