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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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