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

[Carlo Arenas <carenas@gmail.com>]

On Tue, May 10, 2022 at 3:10 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Carlo Marcelo Arenas Belón  <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.

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.

I have a real fix for this which will be released later as part of
that "better integration with the framework", which basically makes
sure all invocations through sudo are done through the test shell
(just like that ugly function that gets added in patch 3), but it
requires changing write_shell and therefore not something that is
worth doing now.

> +test_expect_success SUDO 'setup' '
> > +     sudo rm -rf root &&
> > +     mkdir -p root/r &&
> > +     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",
and since I know root is different and I validated in the prerequisite
that I can sudo to it, that is what is used here.

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

frankly I might had overused sudo, but it is because every extra
invocation refreshes the cache, and all tests depend on SUDO anyway,
so I wanted to make sure they were also more easily reconizable for
the real thing.

Carlo