Hi Junio, On Wed, 8 Dec 2021, Junio C Hamano wrote: > We ship contrib/ stuff within our primary source tree but except for > the completion scripts that are tested from our primary test suite, > their test suites are not run in the CI. > > Teach the main Makefile a "test-extra" target, which goes into each > package in contrib/ whose Makefile has its own "test" target and > runs "make test" there. Add a "test-all" target to make it easy to > drive both the primary tests and these contrib tests from CI and use > it. That sends a strong message that the stuff in contrib/ is now fully under your maintenance, i.e. first-class supported. If I were you, I wouldn't. > Junio C Hamano <gitster@xxxxxxxxx> writes: > > > That is an interesting way to demonstrate how orthogonal the issues > > are, which in turn means that it is not such a big deal to add back > > the coverage to the part that goes to contrib/scalar/. I'd rather focus, _some_ focus, on the actual Scalar idea and code. > > As the actual implementation, it is a bit too icky, though. > > So, how about doing it this way? This is based on 'master' and does > not cover contrib/scalar, but if we want to go this route, it should > be trivial to do it on top of a merge of ab/ci-updates and js/scalar > into 'master'. Good idea? Terrible idea? Not good enough? Peff mentioned a couple of times how tedious it is to address CI failures e.g. in the Windows part of Git's CI runs. So it makes only sense to avoid the same problem with contrib/scalar/ altogether, especially as long as you keep saying that you are still uncertain whether it will make it into Git as a top-level command. Which is a strong argument in favor of just leaving the CI part of contrib/scalar/ out for now, and let it remain _my_ responsibility to react to any build/test problems arising from unrelated patch series entering `seen`. Doing it that way would also have the benefit of allowing more focus on the actual code in contrib/scalar/scalar.c. Not that it needs more review, I don't think, as both Stolee and Elijah gave their thumbs-up already, and I've not received any feedback that would require further changes to `scalar.c`, at least as of _this_ patch series. Ciao, Dscho
