On Fri, May 20 2022, Johannes Schindelin wrote: > Hi Ævar, > > On Fri, 20 May 2022, Ævar Arnfjörð Bjarmason wrote: > >> >> On Wed, May 18 2022, Junio C Hamano wrote: >> >> > Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: >> > >> >>> +test_expect_success '`scalar [...] <dir>` errors out when dir is missing' ' >> >>> + ! scalar run config cloned 2>err && >> >> >> >> Needs to use test_must_fail, not ! >> > >> > Good eyes and careful reading are very much appreciated, but in this >> > case, doesn't such an improvement depend on an update to teach >> > test_must_fail_acceptable about scalar being whitelisted? >> >> Yes, I think so (but haven't tested it just now), but it's a relatively >> small change to t/test-lib-functions.sh. > > Let it be noted that I fully agree with Junio that good eyes and careful > reading are very much appreciated. And that in this case, that would have > implied noticing that `test_must_fail` is reserved for Git commands. > > 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.
