Re: [PATCH v4 3/4] cache-tree: subdirectory tests

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 7, 2014, 19:15 UTC
Message-ID: <xmqqegxxc75w.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1404619619-4774-3-git-send-email-dturner@twitter.com>

David Turner <dturner@twopensource.com> writes:

Show 25 quoted lines

> Add tests to confirm that invalidation of subdirectories nether over-
> nor under-invalidates.
>
> Signed-off-by: David Turner <dturner@twitter.com>
> ---
>  t/t0090-cache-tree.sh | 28 +++++++++++++++++++++++++---
>  1 file changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
> index 98fb1ab..8437c5f 100755
> --- a/t/t0090-cache-tree.sh
> +++ b/t/t0090-cache-tree.sh
> @@ -21,10 +21,13 @@ test_shallow_cache_tree () {
>  	cmp_cache_tree expect
>  }
>  
> +# Test that the cache-tree for a given directory is invalid.
> +# If no directory is given, check that the root is invalid
>  test_invalid_cache_tree () {
> -	echo "invalid                                   (0 subtrees)" >expect &&
> -	printf "SHA #(ref)  (%d entries, 0 subtrees)\n" $(git ls-files|wc -l) >>expect &&
> -	cmp_cache_tree expect
> +	test-dump-cache-tree >actual &&
> +	sed -e "s/$_x40/SHA/" -e "s/[0-9]* subtrees//g" <actual >filtered &&
> +	expect=$(printf "invalid                                  $1 ()\n") &&

It would be saner to do 'printf "string %s more string" "$1"' than embedding caller-supplied "$1" inside the format specifier.

> +	fgrep "$expect" filtered

We'd actually want to see fewer uses of 'fgrep' in the tests for two reasons.

Is having an entry that is invalidated the only thing we care about in this test? Shouldn't the caller expect "These subtrees and nothing else must be invalidated", in which case the helper should check not just the expected "invalid dir1/" appears in the output but no other unexpected "invalid somethingelse/" appears (and this "no other unexpected output" makes use of grep family in tests like this less desirable).

In other words, wouldn't it be better to do the helper along the lines of:

	test_invalidated_cache_tree () {
        	if test $# != 0
		then
	        	printf "invalid %s ()\n" "" "$@"
                fi >expect &&
		test-dump-cache-tree |
                sed -n -e '/^invalid /p' >actual &&
                test_cmp expect actual
	}

and use

	test_invalidated_cache_tree dir1

when we expect only dir1 and dir2 (but not dir2 or anything else) is invalidated?

Thanks.