On Thu, Nov 26, 2020 at 02:28:49AM +0100, Ævar Arnfjörð Bjarmason wrote: > Add optional "extra" checks to fsck, these are needed to eventually > replace the custom not-quite-fsck code in mktag.c. > > The mktag checks differ from fsck_tag() in several ways, one of those > is that fsck doesn't know how to refuse an object with custom headers, > and isn't strict about header and body newline separation. > > Teach it how to optionally report these. I thought the best way to do > that given the current structure of the code was to add a new "extra" > category in addition to error/warn/info. Hmm, this new severity (and the extra options bit) feels a bit backwards. We are already passing the information on what we find to the report() callback. It seems like that is the place that should be deciding what is important and what is not. Unfortunately the defaults are somewhat backwards here. We'd have to teach the fsck callbacks to ignore these harmless entries, rather than teaching the mktag caller that they need to be respected. So probably the extra bit in options to say "do these extra tag checks" is the least-bad thing. But then why do we need to put them in their own EXTRA section? The only caller that wants them would treat them as errors. I'm slightly on the fence on whether mktag really needs to enforce the "unknown header" thing at all. Sure, we don't encourage them, but it's a plumbing tool one could use to experiment with new headers. I guess the downside is that a typo'd header would not be caught. > Under --strict the "info" > becomes a "warn" and "warn" becomes "error". Existing users of > fsck's (and others, e.g. index-pack) --strict option rely on this. Yeah, this is a weirdness I think we should eventually fix (along with re-prioritizing some of the existing checks). I'm wary of doing anything that further cements that somewhat broken world-view (keep in mind that "index-pack --strict" is not "do fsck more strictly" but "do fsck at all"). > I'm not changing fsck_commit() to validate commit objects like this > either, we could do that, but unlike in the tag case that code > wouldn't be used anywhere. If someone wants to write a "mkcommit" they > which behaves like "mktag" they can borrow or refactor this logic for > use in fsck_commit(). Seems reasonable. -Peff
