Re: [PATCH v6 9/9] status: unit tests for --porcelain=v2

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

Jeff Hostetler <git@jeffhostetler.com> writes:

> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> Test porcelain v2 status format.
>
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
>  t/t7064-wtstatus-pv2.sh | 576 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 576 insertions(+)
>  create mode 100755 t/t7064-wtstatus-pv2.sh
>
> diff --git a/t/t7064-wtstatus-pv2.sh b/t/t7064-wtstatus-pv2.sh
> new file mode 100755
> index 0000000..44a8671
> --- /dev/null
> +++ b/t/t7064-wtstatus-pv2.sh
> @@ -0,0 +1,576 @@
> +#!/bin/sh
> +
> +test_description='git status --porcelain=v2
> +
> +This test exercises porcelain V2 output for git status.'

A general comment on the titles; with retitling of individual tests,
the result has become a lot easier to understand.  I know coming up
with a short and to-the-point description for them is hard, but that
is effort and time well spent and it shows in the result.  Thanks.

> +. ./test-lib.sh
> +
> +OID_EMPTY=e69de29bb2d1d6434b8b29ae775ad8c2e48c5391

It seems that test-lib.sh these days has EMPTY_BLOB defined for your
use.  You can remove this and replace its use (just two lines) with
$EMPTY_BLOB down in the "add -N" test.

> +test_expect_success setup '
> +	test_tick &&
> +	git config --local core.autocrlf false &&

I'd suggest removing "--local".

Existing use of "git config" in the test suite, unless their use is
about testing "git config" itself to validate the operation of the
--local/--global/--system options, do not seem to explicitly say
"--local", which is the default anyway.

> +test_expect_success 'after first commit, make dirt, confirm unstaged changes' '

Did you mean s/dirt/dirty/?  "make and confirm unstaged changes"
would be sufficient.  Because "confirming" is implicit (as these
are all tests), "after the first commit, modify working tree files"
might even be better, perhaps?

> +	echo x >>file_x &&
> +	OID_X1=$(git hash-object -t blob -- file_x) &&
> +	rm file_z &&
> +	H0=$(git rev-parse HEAD) &&
> + ...

> +test_expect_success 'after first commit, stage dirt, confirm staged changes' '

What you "git add" is meant to be good changes, so they are no
longer dirt ;-)  More importantly, because I never heard of "dirt"
used in Git context, it is unclear if it is an untracked file, a
modification that is not meant to be committed immediately, or what.

"after the first commit, fully add changes to the index"?

> +	git add file_x &&
> +	git rm file_z &&
> +	H0=$(git rev-parse HEAD) &&
> +
> +	cat >expect <<-EOF &&
> +	# branch.oid $H0
> +	# branch.head master
> +	1 M. N... 100644 100644 100644 $OID_X $OID_X1 file_x
> +	1 D. N... 100644 000000 000000 $OID_Z $_z40 file_z
> +	? actual
> +	? expect
> +	EOF

> +test_expect_success 'after first commit, also stage rename, confirm 2 path line format' '
> +	git mv file_y renamed_y &&
> +	H0=$(git rev-parse HEAD) &&
> +
> +	q_to_tab >expect <<-EOF &&
> +	# branch.oid $H0
> +	# branch.head master
> +	1 M. N... 100644 100644 100644 $OID_X $OID_X1 file_x
> +	1 D. N... 100644 000000 000000 $OID_Z $_z40 file_z
> +	2 R. N... 100644 100644 100644 $OID_Y $OID_Y R100 renamed_yQfile_y
> +	? actual
> +	? expect
> +	EOF
> +
> +	git status --porcelain=v2 --branch --untracked-files=all >actual &&
> +	test_cmp expect actual
> +'

Do we want to test -z format on this, too?

> ...
> +test_expect_success 'create ignored files, confirm they are not printed' '
> +test_expect_success 'create ignored files, confirm --ignored prints them' '
> ...

These are all good and readably titled. 

> +test_expect_success 'verify upstream fields in branch header' '
> +	git checkout master &&
> +	test_when_finished rm -rf sub_repo &&

Forgot to quote?