Re: [PATCH v4 1/3] t: regression git needs safe.directory when using sudo

[Junio C Hamano <gitster@pobox.com>]

Carlo Arenas <carenas@gmail.com> writes:

>> > +test_lazy_prereq SUDO '
>> > +     sudo -n id -u >u &&
>> > +     id -u root >r &&
>> > +     test_cmp u r &&
>> > +     command -v git >u &&
>> > +     sudo command -v git >r &&
>> > +     test_cmp u r
>> > +'
>>
>> I vaguely recall mentions of older dash that lack "command -v" made
>> earlier, but implementations of dash I have handy seem to know it.
>> I am personally fine with this as this script has a very narrow and
>> limited audience in mind.
>
> I did check that, but think the report was mistaken.
> Debian, Ubuntu, NetBSD and OpenBSD would fail the same way here, but
> it is not because of the use of dash, as much as sudo NOT being
> configured to default to `-s` mode.

OK.

> dscho was right to point out that I should had usen type instead, but
> that wouldn't work because of the mismatch of shells and therefore the
> mismatch of outputs, so I went with command instead as an extra clever
> way to make sure both the shell inside and outside were most likely
> the same, even if some sudo somewhere decides in the name of security
> not to respect its own "-s mode" and force a "safer" shell.

In this particular case, "command -v" is the right thing to use, as
you care where the command is found on the $PATH and "type --path"
is *NOT* portable.

>> > +     sudo chown root root &&
>> > +     (
>> > +             cd root/r &&
>> > +             git init
>> > +     )
>> > +'
>>
>> So, "root/" is owned by root, "root/r" is owned by the tester.
>
> It doesn't need to be root, but needs to be different than "tester",

To make sure you do not misunderstand, I know the ownership of
root/r and root/p matter---tester and root must be different.  And
we use these two as sample repositories owned by two different
users.

What I am pointing out here is the root/ directory itself.  I do not
think its ownership does not matter anywhere in these new tests (not
just this patch, but after applying all three patches).

>> > +test_expect_failure SUDO 'sudo git status as original owner' '
>> > +     (
>> > +             cd root/r &&
>> > +             git status &&
>>
>> The tester runs "git status" in "root/r" owned by the tester and it
>> should succeed.
>>
>> > +             sudo git status
>>
>> We want the tester to be able to do the same while temporarily
>> becoming 'root' with "sudo", but we know it fails right now.
>>
>> > +     )
>> > +'
>>
>> Mental note.  We do not need root to be owned by 'root' with the
>> tests we see here.  Perhaps we would add some that requires it in
>> later patches.  We'll see.
>
> I am not good with subtle messages in a foreign language, but is this
> a way to imply that I shouldn't need to chown and instead use the
> GIT_TEST_PRETEND feature more?

No.  I just said I made a mental note of the fact that the ownership
of root/ directory (not root/r which is the other directory this
test script creates in this step) does not matter at least in patch
1/3, but I cannot say, by that observation alone, that chown we saw
earlier should be removed, because patches 2/3 and 3/3 might start
requiring root/ to be owned by 'root'.  Hence "I made a mental note
here.  We do not have anything that requires the above chown in this
patch, but we might see something that makes it a good idea to keep
the chown in later patches".

I do not think GIT_TEST_PRETEND needs to be involved at all.  It's
just root/ can be left owned by the tester, because we only care
about the owners of root/p and root/r in these three patches.

Thanks.