Re: [PATCH v4 3/7] scalar: validate the optional enlistment argument
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- May 21, 2022, 09:54 UTC
- Message-ID
- <220521.86leuv199g.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <nycvar.QRO.7.76.6.2205201753300.352@tvgsbejvaqbjf.bet>
On Fri, May 20 2022, Johannes Schindelin wrote:
Show 26 quoted lines
> 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@gmail.com> 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.