Re: [PATCH v4 1/3] t: regression git needs safe.directory when using sudo
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 10, 2022, 23:44 UTC
- Message-ID
- <xmqqwnet3qcv.fsf@gitster.g>
- In-Reply-To
- <CAPUEspgGZeF8LA5PZNF87=+zZLKvWLqWtpD5YUBUON4gq_TaDg@mail.gmail.com>
Carlo Arenas <carenas@gmail.com> writes:
Show 18 quoted lines
>> > +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.
Show 6 quoted lines
> 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.
Show 10 quoted lines
>> > + 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).
Show 23 quoted lines
>> > +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.