Re: [PATCH 1/1] Fix --stat width calculations to handle --graph

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mar 20, 2012, 16:17 UTC
Message-ID: <alpine.DEB.1.00.1203201109370.3340@s15462909.onlinehome-server.info>
In-Reply-To: <1332229097-19262-2-git-send-email-lucian.poston@gmail.com>

Hi Lucian,

On Tue, 20 Mar 2012, Lucian Poston wrote:

Show 15 quoted lines

> Adjusted stat width calculations to take into consideration the diff output
> prefix e.g. the graph prefix generated by `git log --graph --stat`.
> 
> This change fixes the line wrapping that occurs when diff stats are large
> enough to be scaled to fit within the terminal's columns. This issue only
> appears when using --stat and --graph together on large diffs.
> 
> Adjusted stat output tests accordingly. The scaled output tests are closer to
> the target 5:3 ratio.
> 
> Added test that verifies the output of --stat --graph is truncated to fit
> within the available terminal $COLUMNS
> 
> Signed-off-by: Lucian Poston <lucian.poston@gmail.com>
> ---

Good. Just a quick question before everything else: are the commit messages cut off/wrapped to the same number of columns? If so, where do they get the indent from? (Sorry for asking, but I figured that you're already deep in the code so you might know of the top of your head.)

Show 32 quoted lines

> diff --git a/diff.c b/diff.c
> index 377ec1e..3a26561 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -1382,7 +1382,9 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
>  	int total_files = data->nr;
>  	int width, name_width, graph_width, number_width = 4, count;
>  	const char *reset, *add_c, *del_c;
> -	const char *line_prefix = "";
> +	const char *line_prefix = "", *line_prefix_iter;
> +	unsigned int line_prefix_length = 0;
> +	unsigned int reserved_character_count;
>  	int extra_shown = 0;
>  	struct strbuf *msg = NULL;
>  
> @@ -1392,6 +1394,18 @@ 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 can contain color codes, so only pipes '|' and
> +		 * spaces ' ' are counted.
> +		 */
> +		line_prefix_iter = line_prefix;
> +		while (*line_prefix_iter != '\0') {
> +			if (*line_prefix_iter == ' ' || *line_prefix_iter == '|') {
> +				line_prefix_length++;
> +			}
> +			line_prefix_iter += 1;
> +		}
>  	}

My 1st reaction was: why is the current indent width not stored in the options? But you're right, the indent is generated dynamically from output_prefix() which is a method of diff_options, so there is little chance to do it differently from your solution.

However, a little nit, since this list is so famous for "just a little nit": I'd prefer to factor-out the indent width measuring, like so:

static int count_pipes_and_spaces(const char *string)
{
	int count;

	for (count = 0; *string; string++)
		if (*string == '|' || *string == ' ')
			count++;

	return count;
}

It's not only that that new function cannot mess with the local variables of show_stats(), it also documents a bit better what the code is supposed to do (and all that without a single /* ... */! Isn't that fab? ;)

As for the complete patch: nicely done. I especially like that it is minimally intrusive and that you took great care of updating the comments -- not something everybody does!

My nits aside: this is good to go.

Ciao, Dscho