Re: [PATCH 1/8] t1091: use check_files to reduce boilerplate

From: Junio C Hamano <gitster@pobox.com>
To: "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, me@ttaylorr.com, peff@peff.net,
	newren@gmail.com, Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH 1/8] t1091: use check_files to reduce boilerplate
Date: Thu, 16 Jan 2020 13:40:38 -0800	[thread overview]
Message-ID: <xmqq8sm7gjvt.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <9f7791ae5ee8df9d84d25407a014fb2c63e09b38.1579029963.git.gitgitgadget@gmail.com> (Derrick Stolee via GitGitGadget's message of "Tue, 14 Jan 2020 19:25:55 +0000")

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Derrick Stolee <dstolee@microsoft.com>
>
> When testing the sparse-checkout feature, we need to compare the
> contents of the working-directory against some expected output.
> Using here-docs was useful in the beginning, but became repetetive
> as the test script grew.
>
> Create a check_files helper to make the tests simpler and easier
> to extend. It also reduces instances of bad here-doc whitespace.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  t/t1091-sparse-checkout-builtin.sh | 215 ++++++++++-------------------
>  1 file changed, 71 insertions(+), 144 deletions(-)
>
> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> index ff7f8f7a1f..20caefe155 100755
> --- a/t/t1091-sparse-checkout-builtin.sh
> +++ b/t/t1091-sparse-checkout-builtin.sh
> @@ -12,6 +12,13 @@ list_files() {
>  	(cd "$1" && printf '%s\n' *)
>  }
>  
> +check_files() {
> +	DIR=$1
> +	printf "%s\n" $2 >expect &&
> +	list_files $DIR >actual  &&

It is unclear if the script is being deliberate or sloppy.

It turns out that not quoting $2 is deliberate (i.e. it wants to
pass more than one words in $2, have them split at $IFS and show
each of them on a separate line), at the same time not quoting $DIR
is simply sloppy.

And it is totally unnecessary to confuse readers like this.

Unless you plan to extend this helper further, I think this would be
much less burdensome to the readers:

        check_files () {
                list_files "$1" >actual  &&
                shift &&
                printf "%s\n" "$@" >expect &&
                test_cmp expect actual
        }

This ...

>  	test_cmp expect repo/.git/info/sparse-checkout &&
> -	list_files repo >dir  &&
> -	cat >expect <<-EOF &&
> -		a
> -		folder1
> -		folder2
> -	EOF
> -	test_cmp expect dir
> +	check_files repo "a folder1 folder2"

... is a kind of change that the log message advertises, which is a
very nice rewrite.

And ...

>  test_expect_success 'clone --sparse' '
>  	git clone --sparse repo clone &&
>  	git -C clone sparse-checkout list >actual &&
> -	cat >expect <<-EOF &&
> -		/*
> -		!/*/
> +	cat >expect <<-\EOF &&
> +	/*
> +	!/*/
>  	EOF

... this is a style-fix that is another nice rewrite but in a
different category.  I wonder if they should be done in separate
commits.

Other than that, makes sense.

Thanks.