[PATCH v3 0/7] diff stat calculation adjustments

[PATCH v3 0/7] diff stat calculation adjustments

[Lucian Poston]

I broke the patch series up into smaller patches. Hopefully this makes the
intent of the changes easier to follow.

Zbigniew's tests were added as PATCH 1/7.

The order of the options->stat_graph_width test was reverted back to the
original order as per Zbigniew's feedback. There remains an issue where
name_width (or graph_width) could be set to a value greater than
stat_name_width (or stat_graph_width). This patch doesn't address the issue --
v2 had a partial fix, but it resulted in other problems anyway.

Lucian Poston (6):
  Add output_prefix_length to diff_options
  Adjust stat width calculations to take --graph output into account
  t4052: Adjust --graph --stat output for prefixes
  t4052: Add tests to illustrate issues with restrictive COLUMNS
  Prevent graph_width of stat width from falling below min
  t4052: Adjust --stat output for minimum graph_width

Zbigniew Jędrzejewski-Szmek (1):
  t4052: test --stat output with --graph

 diff.c                 |    8 ++-
 diff.h                 |    1 +
 graph.c                |    3 +
 t/t4052-stat-output.sh |  122 ++++++++++++++++++++++++++++++++++++++++++++++--
 4 files changed, 128 insertions(+), 6 deletions(-)

-- 
1.7.3.4

[PATCH v3 1/7] t4052: test --stat output with --graph

[Lucian Poston]

From: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>

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>
Signed-off-by: Lucian Poston <lucian.poston@gmail.com>
---
 t/t4052-stat-output.sh |   78 +++++++++++++++++++++++++++++++++++++++++++++--
 1 files 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.3.4

[PATCH v3 2/7] Add output_prefix_length to diff_options

[Lucian Poston]

Add output_prefix_length to diff_options. Initialize the value to 0 and only
set it when graph.c:diff_output_prefix_callback() is called.

Signed-off-by: Lucian Poston <lucian.poston@gmail.com>
---
 diff.h  |    1 +
 graph.c |    3 +++
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/diff.h b/diff.h
index cb68743..19d762f 100644
--- a/diff.h
+++ b/diff.h
@@ -150,6 +150,7 @@ struct diff_options {
 	diff_format_fn_t format_callback;
 	void *format_callback_data;
 	diff_prefix_fn_t output_prefix;
+	int output_prefix_length;
 	void *output_prefix_data;
 };
 
diff --git a/graph.c b/graph.c
index 7358416..7e0a099 100644
--- a/graph.c
+++ b/graph.c
@@ -194,8 +194,10 @@ static struct strbuf *diff_output_prefix_callback(struct diff_options *opt, void
 	struct git_graph *graph = data;
 	static struct strbuf msgbuf = STRBUF_INIT;
 
+	assert(opt);
 	assert(graph);
 
+	opt->output_prefix_length = graph->width;
 	strbuf_reset(&msgbuf);
 	graph_padding_line(graph, &msgbuf);
 	return &msgbuf;
@@ -245,6 +247,7 @@ struct git_graph *graph_init(struct rev_info *opt)
 	 */
 	opt->diffopt.output_prefix = diff_output_prefix_callback;
 	opt->diffopt.output_prefix_data = graph;
+	opt->diffopt.output_prefix_length = 0;
 
 	return graph;
 }
-- 
1.7.3.4

[PATCH v3 3/7] Adjust stat width calculations to take --graph output into account

[Lucian Poston]

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.

Signed-off-by: Lucian Poston <lucian.poston@gmail.com>
---
 diff.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/diff.c b/diff.c
index 377ec1e..0b61cf7 100644
--- a/diff.c
+++ b/diff.c
@@ -1445,7 +1445,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
 	 */
 
 	if (options->stat_width == -1)
-		width = term_columns();
+		width = term_columns() - options->output_prefix_length;
 	else
 		width = options->stat_width ? options->stat_width : 80;
 
-- 
1.7.3.4

[PATCH v3 4/7] t4052: Adjust --graph --stat output for prefixes

[Lucian Poston]

Adjust tests to verify that the commit history graph tree is taken into
consideration when the diff stat output width is calculated.

Signed-off-by: Lucian Poston <lucian.poston@gmail.com>
---
 t/t4052-stat-output.sh |   34 +++++++++++++++++++++++++---------
 1 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh
index da14984..d748e5e 100755
--- a/t/t4052-stat-output.sh
+++ b/t/t4052-stat-output.sh
@@ -89,7 +89,7 @@ cat >expect200 <<'EOF'
  abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 EOF
 cat >expect200-graph <<'EOF'
-|  abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+|  abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 EOF
 while read verb expect cmd args
 do
@@ -117,7 +117,7 @@ cat >expect40 <<'EOF'
  abcd | 1000 ++++++++++++++++++++++++++
 EOF
 cat >expect40-graph <<'EOF'
-|  abcd | 1000 ++++++++++++++++++++++++++
+|  abcd | 1000 ++++++++++++++++++++++++
 EOF
 while read verb expect cmd args
 do
@@ -127,12 +127,6 @@ do
 		test_cmp "$expect" actual
 	'
 
-	test_expect_success "$cmd $verb statGraphWidth config" '
-		git -c diff.statGraphWidth=26 $cmd $args >output
-		grep " | " output >actual &&
-		test_cmp "$expect" actual
-	'
-
 	test "$cmd" != diff || continue
 
 	test_expect_success "$cmd --graph $verb not enough COLUMNS (big change)" '
@@ -140,6 +134,28 @@ do
 		grep " | " output >actual &&
 		test_cmp "$expect-graph" actual
 	'
+done <<\EOF
+ignores expect80 format-patch -1 --stdout
+respects expect40 diff HEAD^ HEAD --stat
+respects expect40 show --stat
+respects expect40 log -1 --stat
+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 statGraphWidth config" '
+		git -c diff.statGraphWidth=26 $cmd $args >output
+		grep " | " output >actual &&
+		test_cmp "$expect" actual
+	'
+
+	test "$cmd" != diff || continue
 
 	test_expect_success "$cmd --graph $verb statGraphWidth config" '
 		git -c diff.statGraphWidth=26 $cmd $args --graph >output
@@ -244,7 +260,7 @@ cat >expect200 <<'EOF'
  aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 EOF
 cat >expect200-graph <<'EOF'
-|  aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+|  aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 EOF
 while read verb expect cmd args
 do
-- 
1.7.3.4

[PATCH v3 5/7] t4052: Add tests to illustrate issues with restrictive COLUMNS

[Lucian Poston]

Signed-off-by: Lucian Poston <lucian.poston@gmail.com>
---
 t/t4052-stat-output.sh |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh
index d748e5e..6cac95b 100755
--- a/t/t4052-stat-output.sh
+++ b/t/t4052-stat-output.sh
@@ -284,6 +284,34 @@ respects expect200 show --stat
 respects expect200 log -1 --stat
 EOF
 
+cat >expect1 <<'EOF'
+ ...aaaaaaaaaaaaaa | 1000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+EOF
+cat >expect1-graph <<'EOF'
+|  ...aaaaaaaaaaaaaa | 1000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+EOF
+while read verb expect cmd args
+do
+	test_expect_success "$cmd $verb prefix greater than COLUMNS (big change)" '
+		COLUMNS=1 git $cmd $args >output
+		grep " | " output >actual &&
+		test_cmp "$expect" actual
+	'
+
+	test "$cmd" != diff || continue
+
+	test_expect_success "$cmd --graph $verb prefix greater than COLUMNS (big change)" '
+		COLUMNS=1 git $cmd $args --graph >output
+		grep " | " output >actual &&
+		test_cmp "$expect-graph" actual
+	'
+done <<\EOF
+ignores expect80 format-patch -1 --stdout
+respects expect1 diff HEAD^ HEAD --stat
+respects expect1 show --stat
+respects expect1 log -1 --stat
+EOF
+
 cat >expect <<'EOF'
  abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 EOF
-- 
1.7.3.4

[PATCH v3 6/7] Prevent graph_width of diff stat from falling below min

[Lucian Poston]

Signed-off-by: Lucian Poston <lucian.poston@gmail.com>
---
 diff.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/diff.c b/diff.c
index 0b61cf7..f08b41a 100644
--- a/diff.c
+++ b/diff.c
@@ -1473,8 +1473,12 @@ 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)
+		if (graph_width > width * 3/8 - number_width - 6) {
 			graph_width = width * 3/8 - number_width - 6;
+			if (graph_width < 6)
+				graph_width = 6;
+		}
+
 		if (options->stat_graph_width &&
 		    graph_width > options->stat_graph_width)
 			graph_width = options->stat_graph_width;
-- 
1.7.3.4

[PATCH v3 7/7] t4052: Adjust --stat output for minimum graph_width

[Lucian Poston]

Signed-off-by: Lucian Poston <lucian.poston@gmail.com>
---
 t/t4052-stat-output.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh
index 6cac95b..ced32db 100755
--- a/t/t4052-stat-output.sh
+++ b/t/t4052-stat-output.sh
@@ -285,10 +285,10 @@ respects expect200 log -1 --stat
 EOF
 
 cat >expect1 <<'EOF'
- ...aaaaaaaaaaaaaa | 1000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+ ...aaaaaaa | 1000 ++++++
 EOF
 cat >expect1-graph <<'EOF'
-|  ...aaaaaaaaaaaaaa | 1000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+|  ...aaaaaaa | 1000 ++++++
 EOF
 while read verb expect cmd args
 do
-- 
1.7.3.4

Re: [PATCH v3 5/7] t4052: Add tests to illustrate issues with restrictive COLUMNS

[Junio C Hamano]

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

> Signed-off-by: Lucian Poston <lucian.poston@gmail.com>
> ---

Sorry, the patch text itself does not tell me what it wants to "illustate"
at all.

I _guess_ that you are saying that the output for expect1 and
expect1-graph should be limited to very narrow width, not like the
humongous output like we see in the added test vector, and it will be
fixed with your patch 6/7.

If that is the case, please structure this part of the series like this:

	5/7: add test vector to illustrate what the correct output should
	     be, but mark the test with "test_expect_failure".

        6/7: update the code to fix it, and flip the "test_expect_failure"
             in 5/7 to "test_expect_success".

And 7/7 will become unneeded.

Thanks.

>  t/t4052-stat-output.sh |   28 ++++++++++++++++++++++++++++
>  1 files changed, 28 insertions(+), 0 deletions(-)
>
> diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh
> index d748e5e..6cac95b 100755
> --- a/t/t4052-stat-output.sh
> +++ b/t/t4052-stat-output.sh
> @@ -284,6 +284,34 @@ respects expect200 show --stat
>  respects expect200 log -1 --stat
>  EOF
>  
> +cat >expect1 <<'EOF'
> + ...aaaaaaaaaaaaaa | 1000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 ++
>  +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> +EOF
> +cat >expect1-graph <<'EOF'
> +|  ...aaaaaaaaaaaaaa | 1000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 ++
>  +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> +EOF
> +while read verb expect cmd args
> +do
> +	test_expect_success "$cmd $verb prefix greater than COLUMNS (big change)" '
> +		COLUMNS=1 git $cmd $args >output
> +		grep " | " output >actual &&
> +		test_cmp "$expect" actual
> +	'
> +
> +	test "$cmd" != diff || continue
> +
> +	test_expect_success "$cmd --graph $verb prefix greater than COLUMNS (big change)" '
> +		COLUMNS=1 git $cmd $args --graph >output
> +		grep " | " output >actual &&
> +		test_cmp "$expect-graph" actual
> +	'
> +done <<\EOF
> +ignores expect80 format-patch -1 --stdout
> +respects expect1 diff HEAD^ HEAD --stat
> +respects expect1 show --stat
> +respects expect1 log -1 --stat
> +EOF
> +
>  cat >expect <<'EOF'
>   abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  EOF

Re: [PATCH v3 4/7] t4052: Adjust --graph --stat output for prefixes

[Junio C Hamano]

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

> Adjust tests to verify that the commit history graph tree is taken into
> consideration when the diff stat output width is calculated.
>
> Signed-off-by: Lucian Poston <lucian.poston@gmail.com>
> ---
>  t/t4052-stat-output.sh |   34 +++++++++++++++++++++++++---------
>  1 files changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh
> index da14984..d748e5e 100755
> --- a/t/t4052-stat-output.sh
> +++ b/t/t4052-stat-output.sh
> @@ -89,7 +89,7 @@ cat >expect200 <<'EOF'
>   abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  EOF
>  cat >expect200-graph <<'EOF'
> -|  abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> +|  abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  EOF

This change is easily explainable.  Earlier, the code did not take the "|
" part at the very left end (i.e. ancestry graph) and drew the --stat
output too wide by two columns, but now the code is updated to compensate
for that, because "200" is coming from COLUMNS to clip the whole width,
not just the stat-width part.

>  while read verb expect cmd args
>  do
> @@ -117,7 +117,7 @@ cat >expect40 <<'EOF'
>   abcd | 1000 ++++++++++++++++++++++++++
>  EOF
>  cat >expect40-graph <<'EOF'
> -|  abcd | 1000 ++++++++++++++++++++++++++
> +|  abcd | 1000 ++++++++++++++++++++++++
>  EOF

Likewise.

>  while read verb expect cmd args
>  do
> @@ -127,12 +127,6 @@ do
>  		test_cmp "$expect" actual
>  	'
>  
> -	test_expect_success "$cmd $verb statGraphWidth config" '
> -		git -c diff.statGraphWidth=26 $cmd $args >output
> -		grep " | " output >actual &&
> -		test_cmp "$expect" actual
> -	'
> -

This is unclear.  Why does this need to be removed?

Re: [PATCH v3 2/7] Add output_prefix_length to diff_options

[Junio C Hamano]

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

> Add output_prefix_length to diff_options. Initialize the value to 0 and only
> set it when graph.c:diff_output_prefix_callback() is called.
>
> Signed-off-by: Lucian Poston <lucian.poston@gmail.com>
> ---
>  diff.h  |    1 +
>  graph.c |    3 +++
>  2 files changed, 4 insertions(+), 0 deletions(-)

This patch is a no-op, as nobody pays attention to the value this field
contains.  I think it is better to squash this with 3/7 where you
introduce the only user of this field.

Also when that happens, you would need to adjust the test vector, which
may mean that you need to squash the result with 4/7 as well.

> diff --git a/diff.h b/diff.h
> index cb68743..19d762f 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -150,6 +150,7 @@ struct diff_options {
>  	diff_format_fn_t format_callback;
>  	void *format_callback_data;
>  	diff_prefix_fn_t output_prefix;
> +	int output_prefix_length;
>  	void *output_prefix_data;
>  };
>  
> diff --git a/graph.c b/graph.c
> index 7358416..7e0a099 100644
> --- a/graph.c
> +++ b/graph.c
> @@ -194,8 +194,10 @@ static struct strbuf *diff_output_prefix_callback(struct diff_options *opt, void
>  	struct git_graph *graph = data;
>  	static struct strbuf msgbuf = STRBUF_INIT;
>  
> +	assert(opt);
>  	assert(graph);
>  
> +	opt->output_prefix_length = graph->width;
>  	strbuf_reset(&msgbuf);
>  	graph_padding_line(graph, &msgbuf);
>  	return &msgbuf;
> @@ -245,6 +247,7 @@ struct git_graph *graph_init(struct rev_info *opt)
>  	 */
>  	opt->diffopt.output_prefix = diff_output_prefix_callback;
>  	opt->diffopt.output_prefix_data = graph;
> +	opt->diffopt.output_prefix_length = 0;
>  
>  	return graph;
>  }

Re: [PATCH v3 0/7] diff stat calculation adjustments

[Junio C Hamano]

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

> I broke the patch series up into smaller patches. Hopefully this makes the
> intent of the changes easier to follow.
>
> Zbigniew's tests were added as PATCH 1/7.

Thanks.  Overall it looks nicely done, except that the structure of the
series may want a few squashing, moving and use of test_expect_failure
that later turns into test_expect_success.

When I learned from you that you will be rerolling, I was expecting to see
the other series, i.e.

 * lp/maint-diff-three-dash-with-graph (2012-03-20) 3 commits
  - t4202: add test for "log --graph --stat -p" separator lines
  - log --graph: fix break in graph lines
  - log --graph --stat: three-dash separator should come after graph lines

but I think this was $gmane/193490 split into three and didn't have
anything controversial.

I'll merge it to 'next'.

Thanks.

Re: [PATCH v3 4/7] t4052: Adjust --graph --stat output for prefixes

[Lucian Poston]

On Mon, Apr 16, 2012 at 11:48, Junio C Hamano <gitster@pobox.com> wrote:
>> @@ -117,7 +117,7 @@ cat >expect40 <<'EOF'
>>   abcd | 1000 ++++++++++++++++++++++++++
>>  EOF
>>  cat >expect40-graph <<'EOF'
>> -|  abcd | 1000 ++++++++++++++++++++++++++
>> +|  abcd | 1000 ++++++++++++++++++++++++
>>  EOF
>>  while read verb expect cmd args
>>  do
>> @@ -127,12 +127,6 @@ do
>>               test_cmp "$expect" actual
>>       '
>>
>> -     test_expect_success "$cmd $verb statGraphWidth config" '
>> -             git -c diff.statGraphWidth=26 $cmd $args >output
>> -             grep " | " output >actual &&
>> -             test_cmp "$expect" actual
>> -     '
>> -
>
> This is unclear.  Why does this need to be removed?

This test was actually only moved. There were 4 test_expect_success in
a while loop -- 2 tested COLUMNS=40, and 2 tested
diff.statGraphWidth=26. I moved the two tests for diff.statGraphWidth
to a new while loop. expect40-graph varies between these two sets of
tests, so I thought it cleaner to separate them so that the
expectation could be defined separately.

Re: [PATCH v3 2/7] Add output_prefix_length to diff_options

[Lucian Poston]

On Mon, Apr 16, 2012 at 11:48, Junio C Hamano <gitster@pobox.com> wrote:
> Lucian Poston <lucian.poston@gmail.com> writes:
>
>> Add output_prefix_length to diff_options. Initialize the value to 0 and only
>> set it when graph.c:diff_output_prefix_callback() is called.
>>
>> Signed-off-by: Lucian Poston <lucian.poston@gmail.com>
>> ---
>>  diff.h  |    1 +
>>  graph.c |    3 +++
>>  2 files changed, 4 insertions(+), 0 deletions(-)
>
> This patch is a no-op, as nobody pays attention to the value this field
> contains.  I think it is better to squash this with 3/7 where you
> introduce the only user of this field.
>
> Also when that happens, you would need to adjust the test vector, which
> may mean that you need to squash the result with 4/7 as well.

Squashed.

Re: [PATCH v3 5/7] t4052: Add tests to illustrate issues with restrictive COLUMNS

[Lucian Poston]

On Mon, Apr 16, 2012 at 11:48, Junio C Hamano <gitster@pobox.com> wrote:
> Lucian Poston <lucian.poston@gmail.com> writes:
>
>> Signed-off-by: Lucian Poston <lucian.poston@gmail.com>
>> ---
>
> Sorry, the patch text itself does not tell me what it wants to "illustate"
> at all.
>
> I _guess_ that you are saying that the output for expect1 and
> expect1-graph should be limited to very narrow width, not like the
> humongous output like we see in the added test vector, and it will be
> fixed with your patch 6/7.
>
> If that is the case, please structure this part of the series like this:
>
>        5/7: add test vector to illustrate what the correct output should
>             be, but mark the test with "test_expect_failure".
>
>        6/7: update the code to fix it, and flip the "test_expect_failure"
>             in 5/7 to "test_expect_success".
>
> And 7/7 will become unneeded.

Done. Thanks.