On Thu, Nov 26 2020, Jeff King wrote: > 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. Right, it'll be hidden behind options->extra, so I could just make them ERROR. I guess I was thinking it would be confusing to stick stuff in the middle of ERROR that wasn't on by default, e.g. I've sometimes skimmed that macro definition and saw "ah, bad parent sha1 is an error", as in transfer.fsckObjects would reject it. So I'm slightly on the fence about keeping it as it is, what do you think? > 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. The problem is that since verify_headers() in fsck.c wants to allow it, there's no way for it to distinguish a fat-fingerd "didn't separate the body from the headers" v.s. actually wanting a custom header in some cases. >> 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"). *nod*, will note that. >> 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
