Re: [PATCH 2/3 v5] diff --stat: use the full terminal width

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

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

> Use as many columns as necessary for the filenames and up to 40
> columns for the graph.

I started to wonder if it is a good idea to split this step further into
at least two patches:

 - using term_columns() to set the default 'stat-width' instead of
   hardcoded 80, and do nothing else.  As you discovered, this step will
   not have to touch any test expectations.

 - update the logic to split the stat-width into name part and graph
   part. I think as a side-effect this will fix the corner case bugs the
   new tests in your [1/3] seem to have discovered, and the fix will be
   visible to the part that will update t/t4014 in this step.

> ... I've taken the middle way:
> number_width=4 is hardcoded, but the fprintf is reverted to the previous
> version. I think that this way the code is most readable, independently
> if later changes.

Sensible.

> I haven't done this, because $COLUMNS and the actual terminal width is always
> ignored in tests.

As long as the tests are not affected by ioctl(1) that is OK. I am not
expecting us to be automating the test of that codepath (unless somebody
has clever ideas perhaps using pty, but I suspect that would be a separate
patch anyway).

After writing the attached patch that goes on top of this patch to be
squashed, I am starting to think that "maximum 40 columns for the graph"
may be a mild regression for a project with a shallow hierarchy, namely,
this part.

 cat >expect <<'EOF'
- abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+ abcd | 1000 ++++++++++++++++++++++++++++++++++++++++
 EOF

A bug used to waste space by allocating more than necessary as the minimum
number of columns given for the graph part, even when the change turns out
to be just one line, requiring only one '+' in the graph.  The bug was fixed
by this patch not to waste space that way.  But now with this "maximum 40"
limit, we can see that it wastes the space even when the stat-width is 80.

Perhaps the maximum for garph_width should be raised to something like
"min(80, stat_width) - name_width"?

-- >8 --
Subject: [PATCH] (squash to the previous -- replace the last line of the
 log with the following)

The effect of this change is visible in the patch to t4014 that fixes a
few tests marked with test_expect_failure, and the change to shorten the
maximum graph width to 40 columns.
---
 t/t4014-format-patch.sh |   15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 0376186..88ccc5a 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -908,7 +908,7 @@ test_expect_success 'preparation' '
 cat >expect <<'EOF'
  ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa |    1 +
 EOF
-test_expect_failure 'format patch graph width defaults to 80 columns' '
+test_expect_success 'format patch graph width defaults to 80 columns' '
 	git format-patch --stat --stdout -1 >output &&
 	grep " | " output >actual &&
 	test_cmp expect actual
@@ -917,13 +917,13 @@ test_expect_failure 'format patch graph width defaults to 80 columns' '
 cat >expect <<'EOF'
  ...aaaaaaaaaaaaaaaaaaaaaaaaaa |    1 +
 EOF
-test_expect_failure 'format patch --stat=width with long name' '
+test_expect_success 'format patch --stat=width with long name' '
 	git format-patch --stat=40 --stdout -1 >output &&
 	grep " | " output >actual &&
 	test_cmp expect actual
 '
 
-test_expect_failure 'format patch --stat-width=width works with long name' '
+test_expect_success 'format patch --stat-width=width works with long name' '
 	git format-patch --stat-width=40 --stdout -1 >output &&
 	grep " | " output >actual &&
 	test_cmp expect actual
@@ -954,8 +954,13 @@ test_expect_success 'preparation for big change tests' '
 '
 
 cat >expect <<'EOF'
- abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+ abcd | 1000 ++++++++++++++++++++++++++++++++++++++++
 EOF
+test_expect_success 'format patch graph part width is 40 columns' '
+	git format-patch --stat --stdout -1 >output &&
+	grep " | " output >actual &&
+	test_cmp expect actual
+'
 
 test_expect_success 'format patch ignores COLUMNS' '
 	COLUMNS=200 git format-patch --stat --stdout -1 >output
@@ -979,7 +984,7 @@ test_expect_success 'format patch --stat-width=width with big change' '
 '
 
 cat >expect <<'EOF'
- ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++
+ ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++
 EOF
 test_expect_success 'format patch --stat=width with big change and long name' '
 	cp abcd aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&
-- 
1.7.9.1.237.g00b59