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

[Junio C Hamano <gitster@pobox.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.

>
> 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' '