Re: [GSoC PATCH 2/2] t4074: add test for diffstat width when prefix contains ANSI chars

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

LorenzoPegorari <lorenzo.pegorari2002@gmail.com> writes:

> Add test checking the calculation of the diffstat display width when
> the line_prefix contains ANSI characters.

Hmph, who stuffs the "line_prefix" and with what?

> Signed-off-by: LorenzoPegorari <lorenzo.pegorari2002@gmail.com>
> ---
>  t/meson.build                               |  1 +
>  t/t4074-diff-stat-width-with-line-prefix.sh | 42 +++++++++++++++++++++
>  2 files changed, 43 insertions(+)
>  create mode 100755 t/t4074-diff-stat-width-with-line-prefix.sh

Do we need a brand new test script only to host just two tests?  Is
it too cumbersome to modify existing test scripts that already test
"git log --graph"?

> +setup () {
> +	rm -rf * ".git" &&
> +	git init &&
> +	git config color.diff always
> +}

Why?

> +test_expect_success 'check width with max name-width' '
> +	setup &&

Why?  Shouldn't something like

	test_config color.diff always &&

be sufficient?

> +	touch "${FILE_MAX}" &&

Use of "touch" when you do not care about the timestamp of the
resulting file is misleading.  If you only care about its existence,
do something like

	>"${FILE_MAX}" &&

instead.

> +	git add . &&
> +	git commit -m "init" &&
> +	echo "text" >"${FILE_MAX}" &&
> +	git add . &&
> +	git commit -m "text" &&

OK, so we have two commits, one adds an empty file, followed by
another that adds one line of "text" to the file.

> +	git log --graph --stat >out &&
> +	grep "${FILE} | 1" out
> +'

Use "test_grep" instead of "grep" to make it easier to debug when
things break, perhaps?

It is unclear what we are exactly looking for.  What's the failure
mode and how the miscounting of the width of the line-prefix
contribute to that failure?

We have two commits, "log" without "--reverse" would give the
creation of an empty file i.e., "file | 0" first and then one line
addition to the file i.e., "file | 1 +" next.  We only care about
"${FILE} | 1" existing in the output and we do not bother checking
what comes at the beginning of the line before "${FILE}" or what
comes on the line fter "| 1", because we can detect the breakage we
expect to see only by looking at the middle of the line?  How would
that work?  The test deserves a bit more comment to help readers who
wonder about these things.

	Side note.  I think I know the answer to these questions,
	but this project is not about me ;-) but other future
	contributors also would need help when they encounter this
	test and want to learn what it is testing.

> +test_expect_success 'check width with longer name-width' '
> +	setup &&
> +	touch "${FILE_MAX}+" &&
> +	git add . &&
> +	git commit -m "init" &&
> +	echo "text" >"${FILE_MAX}+" &&
> +	git add . &&
> +	git commit -m "text" &&
> +	git log --graph --stat >out &&
> +	grep "${FILE_LONGER} | 1" out
> +'
> +
> +test_done

Instead of doing the repository construction twice, you can see the
same effect by running "git log --graph --stat" on the same history
with different values exported on COLUMNS environment variable,
which I think would be easier to see and simpler to debug.

Something along the lines of ...

    COLUMNS=80 git log --graph --stat >out &&
    test_decode_color out >out.decoded &&
    test_grep "^<RED>|<RESET> ${FILE_MAX} | 1 <GREEN>+<RESET>$" out.decoded &&

    COLUMNS=79 git log --graph --stat >out &&
    test_decode_color out >out.decoded &&
    test_grep "^<RED>|<RESET> ${FILE_TRUNCATED} | 1 <GREEN>+<RESET>$" out.decoded

... perhaps?

> +# The terminal, during a test, should default to a width of 80 columns
> +FILE_MAX="filename-with-exact-length-to-take-the-max-amount-of-space-in-diffstat"
> +FILE_LONGER="...name-with-exact-length-to-take-the-max-amount-of-space-in-diffstat+"