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

["Zbigniew Jędrzejewski-Szmek" <zbyszek@in.waw.pl>]

On 04/12/2012 09:47 AM, Lucian Poston wrote:
> On Fri, Mar 23, 2012 at 03:12, Zbigniew Jędrzejewski-Szmek
> <zbyszek@in.waw.pl>  wrote:
>> On 03/23/2012 06:54 AM, Lucian Poston wrote:
>>
>>> diff --git a/diff.c b/diff.c
>>> index 377ec1e..31ba10c 100644
>>> --- a/diff.c
>>> +++ b/diff.c
>>> @@ -1383,6 +1383,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
>>>        int width, name_width, graph_width, number_width = 4, count;
>>>        const char *reset, *add_c, *del_c;
>>>        const char *line_prefix = "";
>>> +     int line_prefix_length = 0;
>>> +     int reserved_character_count;
>>>        int extra_shown = 0;
>>>        struct strbuf *msg = NULL;
>>>
>>> @@ -1392,6 +1394,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
>>>        if (options->output_prefix) {
>>>                msg = options->output_prefix(options, options->output_prefix_data);
>>>                line_prefix = msg->buf;
>>> +             line_prefix_length = options->output_prefix_length;
>>>        }
>> Hi,
>> line_prefix will only be used once. And options->output_prefix_length will
>> be 0 if !options->output_prefix, so line_prefix variable can be eliminated
>> and options->output_prefix_length used directly instead.
>
> Rather than adding the line_prefix_length variable, the next patch
> will use options->output_prefix_length directly.
>
>>>        count = options->stat_count ? options->stat_count : data->nr;
>>> @@ -1427,37 +1430,46 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
>>>         * We have width = stat_width or term_columns() columns total.
>>>         * We want a maximum of min(max_len, stat_name_width) for the name part.
>>>         * We want a maximum of min(max_change, stat_graph_width) for the +- part.
>>> -      * We also need 1 for " " and 4 + decimal_width(max_change)
>>> -      * for " | NNNN " and one the empty column at the end, altogether
>>> +      * 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).
>>>         *
>>> -      * If there's not enough space, we will use the smaller of
>>> -      * stat_name_width (if set) and 5/8*width for the filename,
>>> -      * and the rest for constant elements + graph part, but no more
>>> -      * than stat_graph_width for the graph part.
>>> -      * (5/8 gives 50 for filename and 30 for the constant parts + graph
>>> -      * for the standard terminal size).
>>> +      * Additionally, there may be a line_prefix, which reduces the available
>>> +      * width by line_prefix_length.
>>> +      *
>>> +      * If there's not enough space, we will use the smaller of stat_name_width
>>> +      * (if set) and 5/8*width for the filename, and the rest for the graph
>>> +      * part, but no more than stat_graph_width for the graph part.
>>> +      * Assuming the line prefix is empty, on a standard 80 column terminal
>>> +      * this ratio results in 50 characters for the filename and 20 characters
>>> +      * for the graph (plus the 10 reserved characters).
>>>         *
>>>         * 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;
>>>
>>>        if (options->stat_width == -1)
>>>                width = term_columns();
>>>        else
>>>                width = options->stat_width ? options->stat_width : 80;
>>>
>>> +     width -= line_prefix_length;
>>> +
>>>        if (options->stat_graph_width == -1)
>>>                options->stat_graph_width = diff_stat_graph_width;
>>>
>>>        /*
>>> -      * Guarantee 3/8*16==6 for the graph part
>>> -      * and 5/8*16==10 for the filename part
>>> +      * Guarantees at least 6 characters for the graph part [16 * 3/8]
>>> +      * and at least 10 for the filename part [16 * 5/8]
>>>         */
>>> -     if (width<    16 + 6 + number_width)
>>> -             width = 16 + 6 + number_width;
>>> +     if (width<    16 + reserved_character_count)
>>> +             width = 16 + reserved_character_count;
>>>
>>>        /*
>>>         * First assign sizes that are wanted, ignoring available width.
>>> @@ -1472,16 +1484,36 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
>>>        /*
>>>         * Adjust adjustable widths not to exceed maximum width
>>>         */
>>
>> In this part below, you add gratuitous braces around single line if-blocks.
>> This makes the code (and the diff) longer with no gain.
>
> I prefer gratuitous braces, particularly when conditionals are nested
> as they are here. It helps later when maintaining the code if someone
> wants to add a debug statement or comment out a line.
>
> I'll remove braces from single line conditionals to keep with the
> existing conventions.
Thanks.

>>> -     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;
>>> +                     }
>>> +             }
>> This extra test is not necessary. Above, after /* Guarantees at least 6 characters
>> for the graph part [16 * 3/8] ... */, this should already by so that
>> (width * 3/8 - reserved_character_count) is at least 6.
>
> Ahh, this is because the calculations go haywire when the number of
> columns is small. I briefly mentioned it here:
> http://thread.gmane.org/gmane.comp.version-control.git/193694/focus=193744
>
> graph_width actually can have a negative value under certain
> conditions, and this check compensates for that edge case. My earlier
> patches took a less conservative approach and adjusted the
> calculations so that the value of graph_width would be at least 6, but
> it caused several tests to regress. Since the intention of the
> original graph_width calculation was place a lower bound of 6 on its
> value, I simply enforce that here without affecting the general cases,
> which will remain unmodified in order to prevent test regressions.
>
>>> +
>>> +             /*
>>> +              * If the remaining unreserved space will not accomodate the
>>> +              * filenames, adjust name_width to use all available remaining space.
>>> +              * Otherwise, assign any extra space to graph_width.
>>> +              */
>>> +             if (name_width>    width - reserved_character_count - graph_width) {
>>> +                     name_width = width - reserved_character_count - graph_width;
>>> +             } else {
>>> +                     graph_width = width - reserved_character_count - name_width;
>>> +             }
>>> +
>>> +             /*
>>> +              * If stat-graph-width was specified, limit graph_width to its value.
>>> +              */
>>>                if (options->stat_graph_width&&
>>> -                 graph_width>    options->stat_graph_width)
>>> +                             graph_width>    options->stat_graph_width) {
>>>                        graph_width = options->stat_graph_width;
>>> -             if (name_width>    width - number_width - 6 - graph_width)
>>> -                     name_width = width - number_width - 6 - graph_width;
>>> -             else
>>> -                     graph_width = width - number_width - 6 - name_width;
>>> +             }
>> Here, the order of the two tests
>> (1) if (options->stat_graph_width&&  graph_width>  options->stat_graph_width)
>> (2) if (name_width>  width - number_width - 6 - graph_width)
>> is reversed. This is not OK, because this means that
>> options->stat_graph_width will be used unconditionally, while
>> before it was subject to limiting by total width.
>
> If options->stat_graph_width is specified, it should always limit the
> value of graph_width, correct? Since (1) is the last test, it can only
> decrease the value of graph_width, which would already be limited by
> the total width.
Right, but the way the tests are ordered now, we could end up decreasing
name_width first (after (2)) and then graph_width (after (1)), actually
using less than full width.

> I just noticed that name_width isn't being limited to stat_name_width,
> if it is specified. I'll add a check for that.
Sounds good.

>> The tests:
>> After the new tests are added, I see:
>>
>> ok 53 - format-patch ignores COLUMNS (long filename)
>> ok 54 - diff respects COLUMNS (long filename)
>> ok 55 - show respects COLUMNS (long filename)
>> ok 56 - log respects COLUMNS (long filename)
>> ok 57 - show respects 80 COLUMNS (long filename)<=======
>> ok 58 - log respects 80 COLUMNS (long filename)<-------
>> ok 59 - show respects 80 COLUMNS (long filename)<=======
>> ok 60 - log respects 80 COLUMNS (long filename)<-------
>>
>> So some tests descriptions are duplicated. Also it would be
>> nice to test with --graph in more places. I'm attaching a
>> replacement patch which adds more tests. It should go *before*
>> your series, and your series should  tweak the tests to pass,
>> showing what changed.
>
> Thanks, I'll add these.

Regards,
Zbyszek