Re: [PATCH v2 3/3] t1301: do not change $CWD in "shared=all" test case

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 28, 2022, 13:18 UTC
Message-ID: <221128.86ilizkybr.gmgdl@evledraar.gmail.com>
In-Reply-To: <20221128130323.8914-4-worldhello.net@gmail.com>

On Mon, Nov 28 2022, Jiang Xin wrote:

Show 48 quoted lines

> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>
> In test case "shared=all", the working directory is permanently changed
> to the "sub" directory. This leads to a strange behavior that the
> temporary repositories created by subsequent test cases are all in this
> "sub" directory, such as "sub/new", "sub/child.git". If we bypass this
> test case, all subsequent test cases will have different working
> directory.
>
> Besides, all subsequent test cases assuming they are in the "sub"
> directory do not run any destructive operations in their parent
> directory (".."), and will not make damage out side of $TRASH_DIRECTORY.
>
> So it is a safe change for us to run the test case "shared=all" in
> current repository instead of creating and changing to "sub".
>
> For the next test case, we no longer run it in the "sub" repository
> which is initialized from an empty template, we should not assume the
> path ".git/info" is missing. So add option "-p" to mkdir.
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> ---
>  t/t1301-shared-repo.sh | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
> index 1225abbb6d..fd10c139f5 100755
> --- a/t/t1301-shared-repo.sh
> +++ b/t/t1301-shared-repo.sh
> @@ -46,8 +46,6 @@ do
>  done
>  
>  test_expect_success 'shared=all' '
> -	mkdir sub &&
> -	cd sub &&
>  	git init --template= --shared=all &&
>  	test 2 = $(git config core.sharedrepository)
>  '
> @@ -57,7 +55,7 @@ test_expect_success POSIXPERM 'update-server-info honors core.sharedRepository'
>  	git add a1 &&
>  	test_tick &&
>  	git commit -m a1 &&
> -	mkdir .git/info &&
> +	mkdir -p .git/info &&
>  	umask 0277 &&
>  	git update-server-info &&
>  	actual="$(ls -l .git/info/refs)" &&

I think this approach goes against the effort to implicitly stop relying on templates. See 3d3874d537a (Merge branch 'ab/test-without-templates', 2022-07-18) for commits related to that.

I think better thing to do here is to squash this in:
	
	diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
	index 0b3722aa149..b7222b7bc07 100755
	--- a/t/t1301-shared-repo.sh
	+++ b/t/t1301-shared-repo.sh
	@@ -8,6 +8,7 @@ test_description='Test shared repository initialization'
	 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
	 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
	 
	+TEST_CREATE_REPO_NO_TEMPLATE=1
	 . ./test-lib.sh
	 
	 # Remove a default ACL from the test dir if possible.
	@@ -55,7 +56,7 @@ test_expect_success POSIXPERM 'update-server-info honors core.sharedRepository'
	 	git add a1 &&
	 	test_tick &&
	 	git commit -m a1 &&
	-	mkdir -p .git/info &&
	+	mkdir .git/info &&
	 	umask 0277 &&
	 	git update-server-info &&
	 	actual="$(ls -l .git/info/refs)" &&

I.e. before we'd not reply on the template, as we created a directory manually, but now we're using the standard templated "git init", so AFAICT the first hunk here could be taken, and this could be squashed into the second hunk instead:

	diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
	index 0b3722aa149..d4315b5ef5a 100755
	--- a/t/t1301-shared-repo.sh
	+++ b/t/t1301-shared-repo.sh
	@@ -55,7 +55,6 @@ test_expect_success POSIXPERM 'update-server-info honors core.sharedRepository'
	 	git add a1 &&
	 	test_tick &&
	 	git commit -m a1 &&
	-	mkdir -p .git/info &&
	 	umask 0277 &&
	 	git update-server-info &&
	 	actual="$(ls -l .git/info/refs)" &&

I.e. before your change we went from knowing that we're crafting a custom repo, to saying that we're unsure what we're doing by using the "mkdir -p".

But can't we just use "TEST_CREATE_REPO_NO_TEMPLATE=1" then, and avoid the "cd sub?"