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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 15, 2012, 17:12 UTC
Message-ID: <7vvcn810ml.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1329303808-16989-1-git-send-email-zbyszek@in.waw.pl>

Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> 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.

Show 10 quoted lines

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

Show 8 quoted lines

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

Show 8 quoted lines

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

Show 5 quoted lines

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

Show 11 quoted lines

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

Show 8 quoted lines

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

Show 5 quoted lines

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

Show 13 quoted lines

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