Re: [PATCH v1 3/4] t1301: wrap the statements in the for loop

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 28, 2022, 04:18 UTC
Message-ID: <xmqq35a390ek.fsf@gitster.g>
In-Reply-To: <20221127145130.16155-3-worldhello.net@gmail.com>

Jiang Xin <worldhello.net@gmail.com> writes:

> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> Subject: Re: [PATCH v1 3/4] t1301: wrap the statements in the for loop

That makes it sound as if there weren't a loop and now you wrapped the statement in a loop, but that is not what is happening. You are wrapping the statements in something you are not telling us, and "in the for loop" is there only to explain where the statements in question are found.

	t1301: wrap code to prepare configuration in a separate test

or something?

> Wrap the statements which were introduced in commit 06cbe85503 (Make
> core.sharedRepository more generic, 2008-04-16)) in the for loop in a
> new test case, so if we want to skip some of the test cases, these
> unwrapped statements won't affect the test cases we choose to run.

I am not quite sure why this change is needed for the above, though. If we want to skip u=0660:rw-rw---- test, we can skip the two test_expect_success for the first iteration, which will still run "git config core.sharedrepository" for the first case, and when we test for the next one (i.e. u=0640:rw-r-----), we will overwrite the configuration with the value appropriate for the round.

Now you have three separate tests in an interation of the loop. If you skipped the first one in the iteration (by mistake) and let the other two run, they will run with a wrong configuration and values of $x and $y variables, either unset or leftover from the previous round.

So I am not sure how this patch can be an improvement.

If you wrapped the setting of $x, $y, $u and the config into a helper shell function, e.g.

	prepare_perm_test_variables () {
		u=$1
		x=...
		y=...
		u=...
		git config core.sharedrepository "$u"
	}

before and outside the loop, and make these two tests in the loop to call it upfront, then your users can skip each test and iteration independently while ensuring that the necessary setup is always done correctly, though.

Show 50 quoted lines

>
> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> ---
>  t/t1301-shared-repo.sh | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
> index 1225abbb6d..3ca91bf504 100755
> --- a/t/t1301-shared-repo.sh
> +++ b/t/t1301-shared-repo.sh
> @@ -78,31 +78,28 @@ for u in	0660:rw-rw---- \
>  		0666:rw-rw-rw- \
>  		0664:rw-rw-r--
>  do
> -	x=$(expr "$u" : ".*:\([rw-]*\)") &&
> -	y=$(echo "$x" | sed -e "s/w/-/g") &&
> -	u=$(expr "$u" : "\([0-7]*\)") &&
> -	git config core.sharedrepository "$u" &&
> -	umask 0277 &&
> +	test_expect_success POSIXPERM "set variables from $u" '
> +		x=$(expr "$u" : ".*:\([rw-]*\)") &&
> +		y=$(echo "$x" | sed -e "s/w/-/g") &&
> +		u=$(expr "$u" : "\([0-7]*\)") &&
> +		git config core.sharedrepository "$u"
> +	'
>  
>  	test_expect_success POSIXPERM "shared = $u ($y) ro" '
> -
> +		umask 0277 &&
>  		rm -f .git/info/refs &&
>  		git update-server-info &&
>  		actual="$(test_modebits .git/info/refs)" &&
>  		verbose test "x$actual" = "x-$y"
> -
>  	'
>  
> -	umask 077 &&
>  	test_expect_success POSIXPERM "shared = $u ($x) rw" '
> -
> +		umask 077 &&
>  		rm -f .git/info/refs &&
>  		git update-server-info &&
>  		actual="$(test_modebits .git/info/refs)" &&
>  		verbose test "x$actual" = "x-$x"
> -
>  	'
> -
>  done
>  
>  test_expect_success POSIXPERM 'info/refs respects umask in unshared repo' '