On Tue, May 24 2022, Johannes Schindelin wrote: > Hi Junio, > > On Sat, 21 May 2022, Junio C Hamano wrote: > >> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: >> >> >> Scalar is not (yet?) a Git command. >> > >> > "test-tool" isn't "git" either, so I think this argument is a >> > non-starter. >> > >> > As the documentation for "test_must_fail" notes the distinction is >> > whether something is "system-supplied". I.e. we're not going to test >> > whether "grep" segfaults, but we should test our own code to see if it >> > segfaults. >> > >> > The scalar code is code we ship and test, so we should use the helper >> > that doesn't hide a segfault. >> > >> > I don't understand why you wouldn't think that's the obvious fix here, >> > adding "scalar" to that whitelist is a one-line fix, and clearly yields >> > a more useful end result than a test silently hiding segfaults. >> >> FWIW, I don't, either. > > Because we are still talking about code that lives as much encapsulated > inside `contrib/scalar/` as possible. > > The `! scalar` call is in `contrib/scalar/t/t9099-scalar.sh`. > > To make it work with Git's test suite, you would have to bleed an > implementation detail of something inside `contrib/` into > `t/test-lib-functions.sh`. The "scalar" command is already built by the top-level Makefile, so I don't think the distinction you're trying to maintain here even exists in practice. I.e. if we ran with this strict reasoning then surely "scalar" belongs on there just as much as "test-tool" does. Both are built by our main build process, and thus should have corresponding adjustments in our main test code, just as is already the case for both "git" and "test-tool". But even if that wasn't the case I'd still be of the view that we should add "scalar" to that list. It's just a matter of potential time sinks in the future. If we introduce a hidden segfault in the scalar code and don't notice for some time because we're using that test pattern that's going to suck, and likely to waste a lot of time. We might even ship a broken command to users. Whereas having "scalar" on that list is going to be a relatively easy matter of grepping and doing some boilerplate changes if and when we ever "git rm" it entirely, or "promote it" from contrib or whatever. I also think that just getting rid of that whitelist entirely is an acceptable solution. Perhaps it's just being overzealous in forbidding everything except "git", we should still not use it for the likes of "grep", but we could just leave that to the documentation. But I suspect Junio would disagree with that, so in lieu of that ...
