Re: [PATCH v2 1/3] t1301: fix wrong template dir for git-init

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

On Mon, Nov 28 2022, Jiang Xin wrote:

Show 30 quoted lines

> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>
> The template dir prepared in test case "forced modes" is not used as
> expected because a wrong template dir is provided to "git init". This is
> because the $CWD for "git-init" command is a sibling directory alongside
> the template directory. Change it to the right template directory and
> add a protection test using "test_path_is_file".
>
> The wrong template directory was introduced by mistake in commit
> e1df7fe43f (init: make --template path relative to $CWD, 2019-05-10).
>
> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> ---
>  t/t1301-shared-repo.sh | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
> index 93a2f91f8a..7578e75d77 100755
> --- a/t/t1301-shared-repo.sh
> +++ b/t/t1301-shared-repo.sh
> @@ -140,7 +140,8 @@ test_expect_success POSIXPERM 'forced modes' '
>  	(
>  		cd new &&
>  		umask 002 &&
> -		git init --shared=0660 --template=templates &&
> +		git init --shared=0660 --template=../templates &&
> +		test_path_is_file .git/hooks/post-update &&
>  		>frotz &&
>  		git add frotz &&
>  		git commit -a -m initial &&

This fix looks fishy to me. The code you're changing looks like it was buggy, but this looks like it's sweeping under the rug the fact that "templates" never did anything at this point.

So I'm not saying you should squash this in, but if you do so you'll see
that we only ever used this later.
	
	diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
	index d4315b5ef5a..106ccc5704e 100755
	--- a/t/t1301-shared-repo.sh
	+++ b/t/t1301-shared-repo.sh
	@@ -129,15 +129,12 @@ test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' '
	 '
	 
	 test_expect_success POSIXPERM 'forced modes' '
	-	mkdir -p templates/hooks &&
	-	echo update-server-info >templates/hooks/post-update &&
	-	chmod +x templates/hooks/post-update &&
	 	echo : >random-file &&
	 	mkdir new &&
	 	(
	 		cd new &&
	 		umask 002 &&
	-		git init --shared=0660 --template=templates &&
	+		git init --shared=0660 &&
	 		>frotz &&
	 		git add frotz &&
	 		git commit -a -m initial &&
	@@ -181,6 +178,10 @@ test_expect_success POSIXPERM 'remote init does not use config from cwd' '
	 test_expect_success POSIXPERM 're-init respects core.sharedrepository (local)' '
	 	git config core.sharedrepository 0666 &&
	 	umask 0022 &&
	+	mkdir -p templates/hooks &&
	+	echo update-server-info >templates/hooks/post-update &&
	+	chmod +x templates/hooks/post-update &&
	+
	 	echo whatever >templates/foo &&
	 	git init --template=templates &&
	 	echo "-rw-rw-rw-" >expect &&

From a glance isn't the real fix here to adjust the "post-update hook must be 0770" case? I.e. it's conflating "I saw the right permissions" with "I didn't see this line at all", isn't it?

Thus if we take this squash above we're not setting up the post-update hook at all, so it's "not broken", but if we ever screw up our test setup again it'll be broken again...

No?