Should a line that doesn't parse as a review be ignored?

[Ekleog]

Raised in #6 (comment) was the issue of lines that don't even parse as a review in the review branch.

Should they be ignored, or trigger a fatal error? My current feeling is that it's better if they are ignored, because it's not possible to recover from someone having done a mistake otherwise, and it will allow a smoother upgrade path to new versions that may break the review format.

What do you think about it?

[oxij]

And what if somebody commits an invalid commit into review-branch (made by hand, but signed and all)? IMHO if anything that bad happens everything should stop until things get cleaned up and everyone agrees that the resulting review-branch state is correct. The cleanup itself is pretty simple, just force push an earlier revision, let everyone pull and accept that and then make everyone rebase reviews they added since with amended dates.

"Be strict towards data you produce, be lenient towards data you consume" produces data formats with HTML quality, IMO.

[Ekleog]

“Just force push” sounds like something that's likely to pose huge problems for things the size of nixpkgs. That'd mean (tens of?) thousands of people having to force-pull. Sure, it's not supposed to happen… but having a way to recover from compromise sounds required to me.

Actually, that's the reason why we introduced the with keyword in the first place: to recover from Bad Things™ happening in the “code” branch. I think the “review” branch shouldn't really be considered as impossible to fail, and force-pushing to it would be just as bad as force-pushing to the “code” branch.

I agree that the “strict in what you produce, lenient in what you consume” has drawbacks, but it's incredibly resilient, and it also resulted in RFC532[12]. In the absence of security issues, I'd think it's better to be resilient than to force-fail :)

[lrvick]

Most simple implementations (like bash) will read one line from a note at a time and validate them as independant objects. Forcing buffering adds complexity.

Under this, I think we parse one line at a time, but only return 0 if conditions are met. If someone appends a line of whitespace to an otherwise valid file it should not break the whole thing imo.

I favor resiliency here.

[oxij]

I see. I'm inclined to agree in case we make separate self-verification step before pushing a MUST requirement.

[Ekleog]

Sounds good to me.