Subject: Re: [PATCH v4 3/7] scalar: validate the optional enlistment argument

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Tue, 24 May 2022 20:11:57 +0200

On Tue, May 24 2022, Johannes Schindelin wrote:

> Hi Junio,
>
> On Sat, 21 May 2022, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason <[email-masked]> 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 ...