Re: [PATCH v2 3/3] t: add tests for safe.directory when running with sudo

[Carlo Arenas <carenas@gmail.com>]

On Thu, Apr 28, 2022 at 11:12 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
> > On 28/04/2022 17:55, Junio C Hamano wrote:
> >> Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:
> >>
> >>> +test_description='verify safe.directory checks while running as root'
> >>> +
> >>> +. ./test-lib.sh
> >>> +
> >>> +if [ "$IKNOWWHATIAMDOING" != "YES" ]; then
> >> Style.
> >>      if test "$IKNOWWHATIAMDOING" != "YES"
> >>      then
> >
> > Also naming - we normally prefix test environment variables with
> > GIT_TEST_. IKNOWWHATIAMDOING does not tell us what we are allowing by
> > setting the variable. Something like GIT_TEST_ALLOW_SUDO would tell us
> > what we're letting ourselves in for by setting it.
>
> If this weren't "let's reuse the same mechanism as already used in
> 1509", I would have had the same reaction.  Renaming would be better
> done outside the topic, I would think.

Since I am renaming it anyway as part of this topic with RFC v4, would
it be a good idea to require both?

I see the "IKNOWHATIAMDOING" not as a normal GIT_TEST flag, but as a
"here be dragons!" warning, and I later found that I either
misremembered it being enabled in the CI, or it was dropped with one
of those refactors we do often there.

My RFC v4 includes a new nice looking GIT_TEST variable as suggested
by Phillip which I am also enabling in the CI to hopefully make it
even more clear that this is only meant to run there, but sadly that
also means that this patch will likely have a conflict when merged
upwards.

Alternatively I could not enable the CI in this series that is aimed
at maint or at least do it in an independent patch so it could be
dropped where it is not strictly needed?

Carlo