Re: [PATCH 1/3 v5] diff --stat: tests for long filenames and big change counts

Zbigniew Jędrzejewski-Szmek [off-list ref] writes:

Eleven tests for various combinations of a long filename and/or big
change count and ways to specify widths for diff --stat.
---

Sign-off? 

Tests added in previous version of 'diff --stat: use full terminal width'
are extracted into a separate patch. The tests are usefull independently
of that patch anyway.

Thanks.

+# 120 character name
+name=aaaaaaaaaa
+name=$name$name$name$name$name$name$name$name$name$name$name$name
+test_expect_success 'preparation' "
+	>\"$name\" &&
+	git add \"$name\" &&
+	git commit -m message &&
+	echo a >\"$name\" &&
+	git commit -m message \"$name\"
+"

Just for future reference.

The last parameter to test_expect_success shell function is `eval`ed by
the shell and $name is visible inside it; you do not have to use double
quote around it and then use backquote to quote the inner double quote.

+cat >expect <<'EOF'
+ ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa |    1 +
+EOF
+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
+'

Hrm, this does not seem to pass, making the result of applying [1/3] fail;
I see that the elided name is shown much shorter than the above expects.

Perhaps this test found a bug in a "very long name, small changes" corner
case?  If that is the case, we'd mark it as test_expect_failure, and
explain the tests that expect failure demonstrates a buggy behaviour, e.g.

	When a pathname is so long that it cannot fit on the column, the
	current code truncates it to make sure that the graph part has at
	least N columns (enough room to show a meaningful graph).  If the
	actual change is small (e.g. only one line changed), this results
	in the final output that is shorter than the width we aim for.  A
	couple of new tests marked with test_expect_failure demonstrate
	this bug.

in the log message (note that I am not sure if that is the nature of the
bug, or what the actual value of N is).

And then a later patch [2/3] that updates the allocation between name and
graph will turn test_expect_failure to test_expect_success; that will make
it clear that your update fixed the bug.

+cat >expect <<'EOF'
+ ...aaaaaaaaaaaaaaaaaaaaaaaaaa |    1 +
+EOF
+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
+'

Likewise.

+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
+'

Likewise.

+test_expect_success 'format patch --stat=...,name-width with long name' '
+	git format-patch --stat=60,29 --stdout -1 >output &&
+	grep " | " output >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'format patch --stat-name-width with long name' '
+	git format-patch --stat-name-width=29 --stdout -1 >output &&
+	grep " | " output >actual &&
+	test_cmp expect actual
+'

Likewise.

+test_expect_success 'preparation' '

There was another "preparation" in this script already, which originally
threw me off while chasing which part of the test is failing. Can you
reword/retitle this one?

+cat >expect <<'EOF'
+ 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
+'

This test shouldn't be added in [1/3] because "cap the graph to 40-col" is
a new feature that is introduced in the later step.

+test_expect_success 'format patch ignores COLUMNS' '
+	COLUMNS=200 git format-patch --stat --stdout -1 >output
+	grep " | " output >actual &&
+	test_cmp expect actual
+'

This is a good test to have in [1/3], but the expectation should not cap the
graph part to 40 columns.  The patch that updates diff.c to implement the
cap in [2/3] should have an update to the expectation, whose diff hunk may
look liks this:

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

That would make the effect of the patch clearer.

+cat >expect <<'EOF'
+ ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++
+EOF
+test_expect_success 'format patch --stat=width with big change and long name' '
+	cp abcd aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&
+	git add aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&
+	git commit -m message &&
+	git format-patch --stat-width=60 --stdout -1 >output &&
+	grep " | " output >actual &&
+	test_cmp expect actual
+'
+
 test_done

The same comment as the previous one.  Because you change the allocation
to the graph part in your patch to diff.c, which hasn't happened in [1/3]
yet, this should expect the existing behaviour (narrower graph) in this
step, and then be updated to expect the output shown above in the [2/3]
patch that changes the implementation in diff.c.