On 2020-05-13 09:51:56-0700, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Đoàn Trần Công Danh <congdanhqx@xxxxxxxxx> writes: > > +if test "$t1509_allowed" = YES > > +then > > + case "$jobname" in > > + osx-*) ;; > > + *) > > + chmod a+w / || sudo chmod a+w / || true > > + export IKNOWWHATIAMDOING=YES > > Eeeww ;-) This makes readers wonder where we did not enable the > test and why. Perhaps throw in a matching > > t1509_allowed=NO > > in the azure thing for completeness? I was thinking about allowing people set it via environment variable and check, but it seems too risky, now. Perhaps, always reset it to NO before the checking for $CI_TYPE, and enable it selectively for only Travis, and GitHub Actions. I didn't enable it for Azure because I can't assure it ;). > Also, do we want to give a more descriptive name than t1509 to the > variable, say, ROOT_WORK_TREE_TEST_ALLOWED? Yeah, I think all caps is better for this risky variable. I think using T1509_ROOT_WORK_TREE_TEST_ALLOWED is better, to point out which test is risky. But it require future tests with root work-tree must be written in t1509, since it's rare usecase, It'd be fine, I think. > > > diff --git a/ci/run-docker-build.sh b/ci/run-docker-build.sh > > index 8d47a5fda3..026afe275a 100755 > > --- a/ci/run-docker-build.sh > > +++ b/ci/run-docker-build.sh > > @@ -58,6 +58,8 @@ else > > test -n "$cache_dir" && chown -R $HOST_UID:$HOST_UID "$cache_dir" > > fi > > > > +chmod a+w / > > + > > # Build and test > > command $switch_cmd su -m -l $CI_USER -c " > > set -ex > > @@ -68,6 +70,7 @@ command $switch_cmd su -m -l $CI_USER -c " > > export GIT_TEST_CLONE_2GB='$GIT_TEST_CLONE_2GB' > > export MAKEFLAGS='$MAKEFLAGS' > > export cache_dir='$cache_dir' > > + export IKNOWWHATIAMDOING=YES > > cd /usr/src/git > > test -n '$cache_dir' && ln -s '$cache_dir/.prove' t/.prove > > make > > Big EWWWWWWwwww. Do we need to do this for _all_ tests, not > selectively only while running t1509? This makes me worried as a > test by mistake can easily corrupt the VM and invalidating the > tests; I know we get a fresh one every time, so there is no > permanent harm done by corrupting it, but having one fewer thing we > have to worry about is always better than having one more thing. Perhaps pass this variable all the way down from ci/lib.sh? Adding another variable into t1509 (except T1509_*) doesn't make it less risky. Or should we add T1509_ prefix to this env var? -- Danh
