| Message ID | 20221128130323.8914-2-worldhello.net@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a0883a2440903bcfcb6b0f0c9d0439168258e819 |
| Headers | show |
| Series | [v2,1/3] t1301: fix wrong template dir for git-init | expand |
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?
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
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...
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 &&