[PATCH v1 1/4] t1301: fix wrong template dir for git-init

[PATCH v1 1/4] t1301: fix wrong template dir for git-init

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

The template dir parepared 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 &&
-- 
2.39.0.rc0

[PATCH v1 2/4] t1301: use test_when_finished for cleanup

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 t/t1301-shared-repo.sh | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index 7578e75d77..1225abbb6d 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -25,6 +25,7 @@ test_expect_success 'shared = 0400 (faulty permission u-w)' '
 for u in 002 022
 do
 	test_expect_success POSIXPERM "shared=1 does not clear bits preset by umask $u" '
+		test_when_finished "rm -rf sub" &&
 		mkdir sub && (
 			cd sub &&
 			umask $u &&
@@ -42,7 +43,6 @@ do
 			;;
 		esac
 	'
-	rm -rf sub
 done
 
 test_expect_success 'shared=all' '
@@ -132,6 +132,7 @@ test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' '
 '
 
 test_expect_success POSIXPERM 'forced modes' '
+	test_when_finished "rm -rf new" &&
 	mkdir -p templates/hooks &&
 	echo update-server-info >templates/hooks/post-update &&
 	chmod +x templates/hooks/post-update &&
@@ -174,6 +175,7 @@ test_expect_success POSIXPERM 'forced modes' '
 '
 
 test_expect_success POSIXPERM 'remote init does not use config from cwd' '
+	test_when_finished "rm -rf child.git" &&
 	git config core.sharedrepository 0666 &&
 	umask 0022 &&
 	git init --bare child.git &&
@@ -193,7 +195,7 @@ test_expect_success POSIXPERM 're-init respects core.sharedrepository (local)' '
 '
 
 test_expect_success POSIXPERM 're-init respects core.sharedrepository (remote)' '
-	rm -rf child.git &&
+	test_when_finished "rm -rf child.git" &&
 	umask 0022 &&
 	git init --bare --shared=0666 child.git &&
 	test_path_is_missing child.git/foo &&
@@ -204,7 +206,7 @@ test_expect_success POSIXPERM 're-init respects core.sharedrepository (remote)'
 '
 
 test_expect_success POSIXPERM 'template can set core.sharedrepository' '
-	rm -rf child.git &&
+	test_when_finished "rm -rf child.git" &&
 	umask 0022 &&
 	git config core.sharedrepository 0666 &&
 	cp .git/config templates/config &&
-- 
2.39.0.rc0

Re: [PATCH v1 2/4] t1301: use test_when_finished for cleanup

[Junio C Hamano]

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

> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>
> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> ---
>  t/t1301-shared-repo.sh | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
> index 7578e75d77..1225abbb6d 100755
> --- a/t/t1301-shared-repo.sh
> +++ b/t/t1301-shared-repo.sh
> @@ -25,6 +25,7 @@ test_expect_success 'shared = 0400 (faulty permission u-w)' '
>  for u in 002 022
>  do
>  	test_expect_success POSIXPERM "shared=1 does not clear bits preset by umask $u" '
> +		test_when_finished "rm -rf sub" &&
>  		mkdir sub && (
>  			cd sub &&
>  			umask $u &&
> @@ -42,7 +43,6 @@ do
>  			;;
>  		esac
>  	'
> -	rm -rf sub
>  done

This is indeed "we used to clean-up outside the test, but instead
let's use test_when_finished for that".


> @@ -132,6 +132,7 @@ test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' '
>  '
>  
>  test_expect_success POSIXPERM 'forced modes' '
> +	test_when_finished "rm -rf new" &&
>  	mkdir -p templates/hooks &&
>  	echo update-server-info >templates/hooks/post-update &&
>  	chmod +x templates/hooks/post-update &&

But strictly speaking this is different.  We used to leave "new"
after we are done, now we do clean up.  Which is definitely an
improvement in any way ;-)

> @@ -174,6 +175,7 @@ test_expect_success POSIXPERM 'forced modes' '
>  '
>  
>  test_expect_success POSIXPERM 'remote init does not use config from cwd' '
> +	test_when_finished "rm -rf child.git" &&
>  	git config core.sharedrepository 0666 &&
>  	umask 0022 &&
>  	git init --bare child.git &&
> @@ -193,7 +195,7 @@ test_expect_success POSIXPERM 're-init respects core.sharedrepository (local)' '
>  '
>  
>  test_expect_success POSIXPERM 're-init respects core.sharedrepository (remote)' '
> -	rm -rf child.git &&
> +	test_when_finished "rm -rf child.git" &&
>  	umask 0022 &&
>  	git init --bare --shared=0666 child.git &&
>  	test_path_is_missing child.git/foo &&
> @@ -204,7 +206,7 @@ test_expect_success POSIXPERM 're-init respects core.sharedrepository (remote)'
>  '
>  
>  test_expect_success POSIXPERM 'template can set core.sharedrepository' '
> -	rm -rf child.git &&
> +	test_when_finished "rm -rf child.git" &&
>  	umask 0022 &&
>  	git config core.sharedrepository 0666 &&
>  	cp .git/config templates/config &&

These child.git test repositories used to follow "initialize what we
are going to use to a known state before we use" pattern, which is
not wrong per-se, but now we use "clean up the cruft we make after
we are done" pattern, which may arguably be better, simply because
the test that makes cruft should know what cruft it created better
than whatever comes later that may not know.

OK.

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

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

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.

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' '
-- 
2.39.0.rc0

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

[Junio C Hamano]

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

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

[Jiang Xin]

On Mon, Nov 28, 2022 at 12:19 PM Junio C Hamano <gitster@pobox.com> wrote:
> 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.

I agree that this patch is not that necessary as the other 3 patches
and will remove it in next reroll.

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

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

[Jiang Xin]

On Mon, Nov 28, 2022 at 12:19 PM Junio C Hamano <gitster@pobox.com> wrote:
> 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"
>         }

I tried, but found the first test case passed, but the other test case
in the for loop failed. This is because the variable u is changed
twice after prepare_perm_test_variables has been called twice.

So, will drop this patch 3/4.

--
Jiang Xin

[PATCH v1 4/4] t1301: do not change $CWD in "shared=all" test case

[Jiang Xin]

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.

Since the test case "shared=all" and all subsequent will work properly
in the default test repository, we don't need to create and change to
the "sub" directory in the test case "shared=all".

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 t/t1301-shared-repo.sh | 2 --
 1 file changed, 2 deletions(-)

diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index 3ca91bf504..c4f2f72f6b 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)
 '
-- 
2.39.0.rc0

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

[Junio C Hamano]

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

> 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.
>
> Since the test case "shared=all" and all subsequent will work properly
> in the default test repository, we don't need to create and change to
> the "sub" directory in the test case "shared=all".

It is much worse than that.  If existing tests after this step were
running destructive operations in their "..", because we have this
extra "sub" directory and such a destructive test were running
there, the damage would have been contained in $TRASH_DIRECTORY but
with this change, it will touch t/ (or the parent directory of the
$TRASH_DIRECTORY).  So, "will work properly" may not be sufficient;
we need to audit the rest of the script and make sure there is no
such funny "step outside the test enviromnent" happening before we
are sure that this is a "safe" change.

It is the "right" thing to do, though.  I do not see any reason why
we want to have an extra level "sub" directory there.




>
> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> ---
>  t/t1301-shared-repo.sh | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
> index 3ca91bf504..c4f2f72f6b 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)
>  '

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

[Jiang Xin]

On Mon, Nov 28, 2022 at 12:41 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Jiang Xin <worldhello.net@gmail.com> writes:
>
> > 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.
> >
> > Since the test case "shared=all" and all subsequent will work properly
> > in the default test repository, we don't need to create and change to
> > the "sub" directory in the test case "shared=all".
>
> It is much worse than that.  If existing tests after this step were
> running destructive operations in their "..", because we have this
> extra "sub" directory and such a destructive test were running
> there, the damage would have been contained in $TRASH_DIRECTORY but
> with this change, it will touch t/ (or the parent directory of the
> $TRASH_DIRECTORY).  So, "will work properly" may not be sufficient;
> we need to audit the rest of the script and make sure there is no
> such funny "step outside the test enviromnent" happening before we
> are sure that this is a "safe" change.

No such funny operations in the follow test cases, but will add some
notes in commit log.

Thanks.

--
Jiang Xin

>
> It is the "right" thing to do, though.  I do not see any reason why
> we want to have an extra level "sub" directory there.

[PATCH v2 0/3] t1301: various updates

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

## Changes since v1

The first version is based on the "master" branch, and this v2
revsion is based on the "maint" branch. Because only master
branch has the commit 93e02b6e1e (tests: don't assume a .git/info
for .git/info/grafts, 2022-06-03), we need add a "-p" option for
mkdir in patch 3/3.

It's very interesting. Just because the "mkdir .git/info" changes
in the above commit conflicts with our internal reference-transaction
hook which will automatically create a ".git/info" directory to
create some files inside, I found the wrong template issue in t1301
which is fixed in patch 1/3.

Other changes see the following range-diff.


## Range-diff v1...v2

1:  876709e3c8 ! 1:  bc68ffb41c t1301: fix wrong template dir for git-init
    @@ Metadata
      ## Commit message ##
         t1301: fix wrong template dir for git-init
     
    -    The template dir parepared in test case "forced modes" is not used as
    +    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
2:  515f6a5619 ! 2:  ae2b2f8afc t1301: use test_when_finished for cleanup
    @@ Metadata
      ## Commit message ##
         t1301: use test_when_finished for cleanup
     
    +    Refactor several test cases to use "test_when_finished" for cleanup.
    +
    +    1. For first of these, we used to clean-up outside the test, but instead
    +       let's use test_when_finished for that.
    +
    +    2. For the second, we used to leave "new" after we are done, but not use
    +       it at all later. Now we do clean up.
    +
    +    3. For the rest, these child.git test repositories used to follow
    +       "initialize what we are going to use to a known state before we use"
    +       pattern, which is not wrong per-se, but now we use "clean up the
    +       cruft we made after we are done" pattern, which may arguably be
    +       better simply because the test that makes cruft should know what
    +       cruft it created better than whatever comes later that may not know.
    +
    +    Helped-by: Junio C Hamano <gitster@pobox.com>
         Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
     
      ## t/t1301-shared-repo.sh ##
3:  17a318c30d < -:  ---------- t1301: wrap the statements in the for loop
4:  4738ad6f8b ! 3:  f1bf255e5a t1301: do not change $CWD in "shared=all" test case
    @@ Commit message
         test case, all subsequent test cases will have different working
         directory.
     
    -    Since the test case "shared=all" and all subsequent will work properly
    -    in the default test repository, we don't need to create and change to
    -    the "sub" directory in the test case "shared=all".
    +    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 ##
    @@ t/t1301-shared-repo.sh: do
      	git init --template= --shared=all &&
      	test 2 = $(git config core.sharedrepository)
      '
    +@@ t/t1301-shared-repo.sh: 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)" &&

--

Jiang Xin (3):
  t1301: fix wrong template dir for git-init
  t1301: use test_when_finished for cleanup
  t1301: do not change $CWD in "shared=all" test case

 t/t1301-shared-repo.sh | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

-- 
2.39.0.rc0

[PATCH v3 0/3] t1301: various updates

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

Various updates for t1301:

  * t1301: fix wrong template dir for git-init
  * t1301: use test_when_finished for cleanup
  * t1301: do not change $CWD in "shared=all" test case

## range-diff v2...v3


1:  bc68ffb41c = 1:  7a66766a10 t1301: fix wrong template dir for git-init
2:  ae2b2f8afc = 2:  97eefb1e67 t1301: use test_when_finished for cleanup
3:  f1bf255e5a ! 3:  9575f2eb1c t1301: do not change $CWD in "shared=all" test case
    @@ Commit message
         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.
    +    For the next test case, the path ".git/info" is assumed to be missing,
    +    but we no longer run the test case in the "sub" repository which is
    +    initialized from an empty template. In order for the test case to run
    +    properly, we can set "TEST_CREATE_REPO_NO_TEMPLATE=1" to initialize the
    +    default repository without a template.
     
         Helped-by: Junio C Hamano <gitster@pobox.com>
    +    Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
         Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
     
      ## t/t1301-shared-repo.sh ##
    +@@ t/t1301-shared-repo.sh: 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.
     @@ t/t1301-shared-repo.sh: do
      done
      
    @@ t/t1301-shared-repo.sh: do
      	git init --template= --shared=all &&
      	test 2 = $(git config core.sharedrepository)
      '
    -@@ t/t1301-shared-repo.sh: 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)" &&

--

Jiang Xin (3):
  t1301: fix wrong template dir for git-init
  t1301: use test_when_finished for cleanup
  t1301: do not change $CWD in "shared=all" test case

 t/t1301-shared-repo.sh | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

-- 
2.39.0.rc0

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

[Jiang Xin]

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 &&
-- 
2.39.0.rc0

[PATCH v3 2/3] t1301: use test_when_finished for cleanup

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

Refactor several test cases to use "test_when_finished" for cleanup.

1. For first of these, we used to clean-up outside the test, but instead
   let's use test_when_finished for that.

2. For the second, we used to leave "new" after we are done, but not use
   it at all later. Now we do clean up.

3. For the rest, these child.git test repositories used to follow
   "initialize what we are going to use to a known state before we use"
   pattern, which is not wrong per-se, but now we use "clean up the
   cruft we made after we are done" pattern, which may arguably be
   better simply because the test that makes cruft should know what
   cruft it created better than whatever comes later that may not know.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 t/t1301-shared-repo.sh | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index 7578e75d77..1225abbb6d 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -25,6 +25,7 @@ test_expect_success 'shared = 0400 (faulty permission u-w)' '
 for u in 002 022
 do
 	test_expect_success POSIXPERM "shared=1 does not clear bits preset by umask $u" '
+		test_when_finished "rm -rf sub" &&
 		mkdir sub && (
 			cd sub &&
 			umask $u &&
@@ -42,7 +43,6 @@ do
 			;;
 		esac
 	'
-	rm -rf sub
 done
 
 test_expect_success 'shared=all' '
@@ -132,6 +132,7 @@ test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' '
 '
 
 test_expect_success POSIXPERM 'forced modes' '
+	test_when_finished "rm -rf new" &&
 	mkdir -p templates/hooks &&
 	echo update-server-info >templates/hooks/post-update &&
 	chmod +x templates/hooks/post-update &&
@@ -174,6 +175,7 @@ test_expect_success POSIXPERM 'forced modes' '
 '
 
 test_expect_success POSIXPERM 'remote init does not use config from cwd' '
+	test_when_finished "rm -rf child.git" &&
 	git config core.sharedrepository 0666 &&
 	umask 0022 &&
 	git init --bare child.git &&
@@ -193,7 +195,7 @@ test_expect_success POSIXPERM 're-init respects core.sharedrepository (local)' '
 '
 
 test_expect_success POSIXPERM 're-init respects core.sharedrepository (remote)' '
-	rm -rf child.git &&
+	test_when_finished "rm -rf child.git" &&
 	umask 0022 &&
 	git init --bare --shared=0666 child.git &&
 	test_path_is_missing child.git/foo &&
@@ -204,7 +206,7 @@ test_expect_success POSIXPERM 're-init respects core.sharedrepository (remote)'
 '
 
 test_expect_success POSIXPERM 'template can set core.sharedrepository' '
-	rm -rf child.git &&
+	test_when_finished "rm -rf child.git" &&
 	umask 0022 &&
 	git config core.sharedrepository 0666 &&
 	cp .git/config templates/config &&
-- 
2.39.0.rc0

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

[Jiang Xin]

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, the path ".git/info" is assumed to be missing,
but we no longer run the test case in the "sub" repository which is
initialized from an empty template. In order for the test case to run
properly, we can set "TEST_CREATE_REPO_NO_TEMPLATE=1" to initialize the
default repository without a template.

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 t/t1301-shared-repo.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index 1225abbb6d..58d6da7feb 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.
@@ -46,8 +47,6 @@ do
 done
 
 test_expect_success 'shared=all' '
-	mkdir sub &&
-	cd sub &&
 	git init --template= --shared=all &&
 	test 2 = $(git config core.sharedrepository)
 '
-- 
2.39.0.rc0

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

[Jiang Xin]

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 &&
-- 
2.39.0.rc0

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

[Ævar Arnfjörð Bjarmason]

On Mon, Nov 28 2022, Jiang Xin wrote:

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

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

[Jiang Xin]

On Mon, Nov 28, 2022 at 9:28 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
>
> On Mon, Nov 28 2022, Jiang Xin wrote:
>
> > 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 &&

The "post-update" is used in this test case. A wrong template dir
leads to an empty hooks dir in "new/", that cause the test at the
end of this test case passed by accident.

        # post-update hook must be 0770
        test -z "$(sed -n -e "/post-update/{
                /^-rwxrwx---/d
                p
        }" actual)" &&

--
Jiang Xin

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

[Ævar Arnfjörð Bjarmason]

On Mon, Nov 28 2022, Jiang Xin wrote:

> On Mon, Nov 28, 2022 at 9:28 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>>
>> On Mon, Nov 28 2022, Jiang Xin wrote:
>>
>> > 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 &&
>
> The "post-update" is used in this test case. A wrong template dir
> leads to an empty hooks dir in "new/", that cause the test at the
> end of this test case passed by accident.
>
>         # post-update hook must be 0770
>         test -z "$(sed -n -e "/post-update/{
>                 /^-rwxrwx---/d
>                 p
>         }" actual)" &&

Yes exactly, which is what I was pointing out with "isn't the real
fix...?". I.e. this does seem to improve things, but as this shows this
test is really fragile already.

So I think if we're poking at this it makes sense to change that "test
-z" to use "test_modebits" or something instead, so it'll actually do
what we expect, and not just silently pass if the hook is missing...

[PATCH v2 2/3] t1301: use test_when_finished for cleanup

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

Refactor several test cases to use "test_when_finished" for cleanup.

1. For first of these, we used to clean-up outside the test, but instead
   let's use test_when_finished for that.

2. For the second, we used to leave "new" after we are done, but not use
   it at all later. Now we do clean up.

3. For the rest, these child.git test repositories used to follow
   "initialize what we are going to use to a known state before we use"
   pattern, which is not wrong per-se, but now we use "clean up the
   cruft we made after we are done" pattern, which may arguably be
   better simply because the test that makes cruft should know what
   cruft it created better than whatever comes later that may not know.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 t/t1301-shared-repo.sh | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index 7578e75d77..1225abbb6d 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -25,6 +25,7 @@ test_expect_success 'shared = 0400 (faulty permission u-w)' '
 for u in 002 022
 do
 	test_expect_success POSIXPERM "shared=1 does not clear bits preset by umask $u" '
+		test_when_finished "rm -rf sub" &&
 		mkdir sub && (
 			cd sub &&
 			umask $u &&
@@ -42,7 +43,6 @@ do
 			;;
 		esac
 	'
-	rm -rf sub
 done
 
 test_expect_success 'shared=all' '
@@ -132,6 +132,7 @@ test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' '
 '
 
 test_expect_success POSIXPERM 'forced modes' '
+	test_when_finished "rm -rf new" &&
 	mkdir -p templates/hooks &&
 	echo update-server-info >templates/hooks/post-update &&
 	chmod +x templates/hooks/post-update &&
@@ -174,6 +175,7 @@ test_expect_success POSIXPERM 'forced modes' '
 '
 
 test_expect_success POSIXPERM 'remote init does not use config from cwd' '
+	test_when_finished "rm -rf child.git" &&
 	git config core.sharedrepository 0666 &&
 	umask 0022 &&
 	git init --bare child.git &&
@@ -193,7 +195,7 @@ test_expect_success POSIXPERM 're-init respects core.sharedrepository (local)' '
 '
 
 test_expect_success POSIXPERM 're-init respects core.sharedrepository (remote)' '
-	rm -rf child.git &&
+	test_when_finished "rm -rf child.git" &&
 	umask 0022 &&
 	git init --bare --shared=0666 child.git &&
 	test_path_is_missing child.git/foo &&
@@ -204,7 +206,7 @@ test_expect_success POSIXPERM 're-init respects core.sharedrepository (remote)'
 '
 
 test_expect_success POSIXPERM 'template can set core.sharedrepository' '
-	rm -rf child.git &&
+	test_when_finished "rm -rf child.git" &&
 	umask 0022 &&
 	git config core.sharedrepository 0666 &&
 	cp .git/config templates/config &&
-- 
2.39.0.rc0

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

[Jiang Xin]

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)" &&
-- 
2.39.0.rc0

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

[Ævar Arnfjörð Bjarmason]

On Mon, Nov 28 2022, Jiang Xin wrote:

> 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?"

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

[Jiang Xin]

On Mon, Nov 28, 2022 at 9:23 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> 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.

As I said in the cover letter, it was the conflict of ".git/info" with
our internal reference transactions that drew my attention to this
test case. This is because our builtin reference-transaction hook
which will automatically create a ".git/info" directory to create some
files inside, such as ".git/info/checksum", I have to change "mkdir
.git/info" to "mkdir -p .git/info" one by one. And I found in this
test case, there is a wrong template dir.

> 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

Will use this implementation instead.

Thanks.

--
Jiang Xin

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> I think this approach goes against the effort to implicitly stop relying
> on templates.

... which I am perfectly OK with.  If you removed something that is
essential to the health of the respository from your templates, you
deserve the breakage you caused yourself and can keep both halves.