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

[Junio C Hamano <gitster@pobox.com>]

Lucian Poston <lucian.poston@gmail.com> writes:

> The recent change to compute the width of diff --stat did not take into
> consideration the output from --graph. The consequence is that when both
> options are used, e.g. in 'log --stat --graph', the lines are too long.
>
> Adjust stat width calculations to take --graph output into account.
>
> Prevent graph width of diff-stat from falling below minimum.
>
> Signed-off-by: Lucian Poston <lucian.poston@gmail.com>
> ---
>  diff.c |   72 ++++++++++++++++++++++++++++++++++++++++++++++-----------------
>  1 files changed, 52 insertions(+), 20 deletions(-)
>
> 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;
>  	}
>  
>  	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).

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.

>  	 *
>  	 * 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.

>  	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?

> @@ -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;

> +		/*
> +		 * 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;
> +		}
>  	}

Yikes. It took me three minutes to realize that the only thing you did in
this hunk was to move the "if stat-graph-width is set" logic down.  Not
your fault, but this is one of the times I wish our diff generation logic
matched "corresponding" block of lines better.