Re: [PATCH v3 3/9] tree: increase test coverage for tree.c

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 17, 2022, 13:36 UTC
Message-ID: <xmqqy1te1sii.fsf@gitster.g>
In-Reply-To: <9a07c6932f4c7ef844df1fc4f5b6b9feb1810135.1665973401.git.gitgitgadget@gmail.com>

"Heather Lapointe via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 18 quoted lines

> diff --git a/t/t1023-tree-read-tree-at.sh b/t/t1023-tree-read-tree-at.sh
> new file mode 100755
> index 00000000000..9e5ce3abb4b
> --- /dev/null
> +++ b/t/t1023-tree-read-tree-at.sh
> @@ -0,0 +1,65 @@
> +#!/bin/sh
> +
> +# tests for tree.c (not read-tree.c)
> +test_description='Test read_tree / read_tree_at'
> +. ./test-lib.sh
> +
> +test_expect_success 'read_tree basic' '
> +	rm -rf walk_tree_basic &&
> +	git init walk_tree_basic &&
> +	(
> +		cd walk_tree_basic &&
> +		set -x &&

Do we need this, when we have '-x' option alongside with '-v', '-i', and '-d' available in the test harness already?

> +		mkdir -p dir1/dirA &&
> +		mkdir -p dir1/dirB &&
> +		mkdir -p dir2 &&

Can't we have these three done by the same single "mkdir -p" process?

> +		echo "file1" > file1.txt &&
> +		echo "file2" > file2.txt &&

Lose the SP between redirection operator ">" and its target, i.e.

		echo file1 >file1.txt

cf. Documentation/CodingGuidelines

Also you do not necessarily have to have dq around a single token.

Show 17 quoted lines

> +		# uncommitted
> +		echo "file3" > file3.txt &&
> +
> +		echo "file1A1" > dir1/dirA/file1.txt &&
> +		git add file1.txt file2.txt dir1/dirA/file1.txt &&
> +		git commit -m "initial commit" &&
> +
> +		test-tool tree-read-tree-at . > walk1.txt &&
> +		grep " file1.txt" walk1.txt &&
> +		! grep " file3.txt" walk1.txt &&
> +		! grep " dir1/dirB" walk1.txt &&
> +		grep " dir1/dirA/file1.txt" walk1.txt
> +	)
> +'
> +
> +test_expect_success 'read_tree submodules' '
> +	rm -rf walk_tree_submodules &&

Curious why the above does not clean "submodule1", too. After all, all the "rm -rf" we saw in this script above are removing what its earlier steps would never have created (but will create), just in case. Why not do the same?

If the pattern is to "remove what we will need to create immediately before we actually try to create, just in case", then shouldn't the above be removing "submodule1", and we should have another "rm -rf" for "walk_tree_submodules" immediately before we do "git init" on it several lines below?

Show 30 quoted lines

> +	git init submodule1 &&
> +	(
> +		cd submodule1 &&
> +		mkdir -p dir1/dirA &&
> +		echo "dir2/sub1/file1.txt" > file1.txt &&
> +		echo "dir2/sub1/file1A1.txt" > dir1/dirA/file1.txt &&
> +		git add file1.txt dir1/dirA/file1.txt &&
> +		git commit -m "initial commit"
> +	) &&
> +	git init walk_tree_submodules &&
> +	(
> +		cd walk_tree_submodules &&
> +
> +		mkdir -p dir2 &&
> +		echo "file1" > file1.txt &&
> +		echo "dir2/file2" > dir2/file2.txt &&
> +		git add file1.txt dir2/file2.txt &&
> +		git commit -m "initial commit" &&
> +
> +		git submodule add ../submodule1 dir2/sub1 &&
> +		git commit -m "add submodule1" &&
> +
> +		test-tool tree-read-tree-at . > walk2.txt &&
> +		grep " file1.txt" walk2.txt &&
> +		grep " dir2/sub1/file1.txt" walk2.txt &&
> +		grep " dir2/sub1/dir1/dirA/file1.txt" walk2.txt
> +	)
> +'
> +
> +test_done