On Thu, Apr 28, 2022 at 11:12 AM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Phillip Wood <phillip.wood123@xxxxxxxxx> writes: > > > On 28/04/2022 17:55, Junio C Hamano wrote: > >> Carlo Marcelo Arenas Belón <carenas@xxxxxxxxx> 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
