Re: Extending whitespace checks

From: Jeff King <peff@peff.net>
Date: Nov 27, 2024, 15:04 UTC
Message-ID: <20241127150429.GD2554@coredump.intra.peff.net>
In-Reply-To: <xmqqbjy5bc6m.fsf@gitster.g>

On Sun, Nov 24, 2024 at 11:25:21AM +0900, Junio C Hamano wrote:

Show 10 quoted lines

> I am wondering what we can do to add a different kind of checks to
> help file types with fixed format by extending the same mechanism,
> or the checks I have in mind are too different from the whitespace
> checks and shoehorning it into the existing mechanism does not make
> sense.  The particular check I have an immediate need for is for a
> filetype with lines, each has exactly 4 fields separated with HT in
> between, so the check would ask "does each line have exactly 3 HT on
> it?"  It would be extended to verify CSV files with fixed number of
> fields (but the validator needs to be aware of the quoting rules for
> comma in a value in fields).

Coming from a devil's advocate position: what makes these CSV format checks any different than syntax checks we get from a compiler? Or for that matter, the result of running "make test"?

I.e., why implement a complex system for single-line verification plugins when you'd be left with the much larger problem of evaluating whole-tree states. And once you have solutions for that (like using branches to separate unverified work and then merging it once it has passed checks), then simple things like line syntax are easy to call there.

Now you could argue that the existing whitespace checks are similarly redundant. Rather than having "apply" complain about whitespace errors, you could just check them as part of "make test".

The reasons I can think of for doing something like this are:

  - catching problems earlier is almost always less work for the user

  - for things that _are_ line-oriented, looking at individual diff
    lines lets you focus on problems being added, without worrying about
    existing violations in the final state. OTOH, that's not foolproof;
    if you modify a line with an existing whitespace problem without
    fixing it, "diff --check" will still complain.

So I'm not necessarily against it. But it seems like a very deep rabbit hole to start adding in shared-library line validators, because I think it ends in "now compile this before I agree to apply the patch". And I think Git's model has mostly been the opposite: make it cheap and private to branch and make changes (including applying patches) so that you can inspect the state before deciding whether and how to publish.

-Peff