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

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

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.

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

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

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

>   	}
> 
>   	/*

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.

------- 8< --------
From 348d96dd9ae4a4ffd04aea4497b237a794e37727 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= <zbyszek@in.waw.pl>
Date: Fri, 23 Mar 2012 11:04:21 +0100
Subject: [PATCH] t4052: test --stat output with --graph
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Add tests which show that the width of the --prefix added by --graph
is not taken into consideration when the diff stat output width is
calculated.

Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
---
 t/t4052-stat-output.sh |   78 +++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 74 insertions(+), 4 deletions(-)

diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh
index 328aa8f..da14984 100755
--- a/t/t4052-stat-output.sh
+++ b/t/t4052-stat-output.sh
@@ -82,11 +82,15 @@ test_expect_success 'preparation for big change tests' '
 cat >expect80 <<'EOF'
  abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 EOF
-
+cat >expect80-graph <<'EOF'
+|  abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+EOF
 cat >expect200 <<'EOF'
  abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 EOF
-
+cat >expect200-graph <<'EOF'
+|  abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+EOF
 while read verb expect cmd args
 do
 	test_expect_success "$cmd $verb COLUMNS (big change)" '
@@ -94,6 +98,14 @@ do
 		grep " | " output >actual &&
 		test_cmp "$expect" actual
 	'
+
+	test "$cmd" != diff || continue
+
+	test_expect_success "$cmd --graph $verb COLUMNS (big change)" '
+		COLUMNS=200 git $cmd $args --graph >output
+		grep " | " output >actual &&
+		test_cmp "$expect-graph" actual
+	'
 done <<\EOF
 ignores expect80 format-patch -1 --stdout
 respects expect200 diff HEAD^ HEAD --stat
@@ -104,7 +116,9 @@ EOF
 cat >expect40 <<'EOF'
  abcd | 1000 ++++++++++++++++++++++++++
 EOF
-
+cat >expect40-graph <<'EOF'
+|  abcd | 1000 ++++++++++++++++++++++++++
+EOF
 while read verb expect cmd args
 do
 	test_expect_success "$cmd $verb not enough COLUMNS (big change)" '
@@ -118,6 +132,20 @@ do
 		grep " | " output >actual &&
 		test_cmp "$expect" actual
 	'
+
+	test "$cmd" != diff || continue
+
+	test_expect_success "$cmd --graph $verb not enough COLUMNS (big change)" '
+		COLUMNS=40 git $cmd $args --graph >output
+		grep " | " output >actual &&
+		test_cmp "$expect-graph" actual
+	'
+
+	test_expect_success "$cmd --graph $verb statGraphWidth config" '
+		git -c diff.statGraphWidth=26 $cmd $args --graph >output
+		grep " | " output >actual &&
+		test_cmp "$expect-graph" actual
+	'
 done <<\EOF
 ignores expect80 format-patch -1 --stdout
 respects expect40 diff HEAD^ HEAD --stat
@@ -129,6 +157,9 @@ EOF
 cat >expect <<'EOF'
  abcd | 1000 ++++++++++++++++++++++++++
 EOF
+cat >expect-graph <<'EOF'
+|  abcd | 1000 ++++++++++++++++++++++++++
+EOF
 while read cmd args
 do
 	test_expect_success "$cmd --stat=width with big change" '
@@ -143,11 +174,25 @@ do
 		test_cmp expect actual
 	'
 
-	test_expect_success "$cmd --stat-graph--width with big change" '
+	test_expect_success "$cmd --stat-graph-width with big change" '
 		git $cmd $args --stat-graph-width=26 >output
 		grep " | " output >actual &&
 		test_cmp expect actual
 	'
+
+	test "$cmd" != diff || continue
+
+	test_expect_success "$cmd --stat-width=width --graph with big change" '
+		git $cmd $args --stat-width=40 --graph >output
+		grep " | " output >actual &&
+		test_cmp expect-graph actual
+	'
+
+	test_expect_success "$cmd --stat-graph-width --graph with big change" '
+		git $cmd $args --stat-graph-width=26 --graph >output
+		grep " | " output >actual &&
+		test_cmp expect-graph actual
+	'
 done <<\EOF
 format-patch -1 --stdout
 diff HEAD^ HEAD --stat
@@ -164,6 +209,9 @@ test_expect_success 'preparation for long filename tests' '
 cat >expect <<'EOF'
  ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++
 EOF
+cat >expect-graph <<'EOF'
+|  ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++
+EOF
 while read cmd args
 do
 	test_expect_success "$cmd --stat=width with big change is more balanced" '
@@ -171,6 +219,14 @@ do
 		grep " | " output >actual &&
 		test_cmp expect actual
 	'
+
+	test "$cmd" != diff || continue
+
+	test_expect_success "$cmd --stat=width --graph with big change is balanced" '
+		git $cmd $args --stat-width=60 --graph >output &&
+		grep " | " output >actual &&
+		test_cmp expect-graph actual
+	'
 done <<\EOF
 format-patch -1 --stdout
 diff HEAD^ HEAD --stat
@@ -181,9 +237,15 @@ EOF
 cat >expect80 <<'EOF'
  ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++++
 EOF
+cat >expect80-graph <<'EOF'
+|  ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++++
+EOF
 cat >expect200 <<'EOF'
  aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 EOF
+cat >expect200-graph <<'EOF'
+|  aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+EOF
 while read verb expect cmd args
 do
 	test_expect_success "$cmd $verb COLUMNS (long filename)" '
@@ -191,6 +253,14 @@ do
 		grep " | " output >actual &&
 		test_cmp "$expect" actual
 	'
+
+	test "$cmd" != diff || continue
+
+	test_expect_success "$cmd --graph $verb COLUMNS (long filename)" '
+		COLUMNS=200 git $cmd $args --graph >output
+		grep " | " output >actual &&
+		test_cmp "$expect-graph" actual
+	'
 done <<\EOF
 ignores expect80 format-patch -1 --stdout
 respects expect200 diff HEAD^ HEAD --stat
-- 
1.7.10.rc1.225.gba57e


------- >8 --------