From: Junio C Hamano Date: Fri, 19 Jun 2015 21:01:16 GMT Subject: Re: [PATCH v6 08/19] fsck: Make fsck_commit() warn-friendly Message-ID: In-Reply-To: Johannes Schindelin writes: >> I do not think this "if (err) return err;" that uses the return >> value of report(), makes sense. >> >> As all the errors that use this pattern are isolated ones that does >> not break parsing of the remainder (e.g. author ident had an extra > >> in it may break "author " but that does not prevent us from checking >> "committer "). >> >> Your report() switches its return value based on the user setting; >> specifically, it returns 0 if the user tells us to ignore/skip or >> warn. Which means that the user will see all warnings, but we stop >> at the first error. >> >> Shouldn't we continue regardless of the end-user setting in order to >> show errors on other fields, too? > > I can make that happen, but please note that this is a change of > behavior: we always stopped upon the first error. Yeah, and we always died when we saw error, without giving users an option to turn it down. So? > It was my intention not to change behavior in that way without a > proper reason, and I saw none. What would be the end-user experience if you stopped at the first error? You see an error, add an "fsck. = ignore" and rerun, only to find another error and rinse and repeat? Wouldn't you rather see all of them and add the "ignore" to cover them in one go? > I actually see a really good reason to *keep* the current behavior: > one of the most prominent users of this code path is `git receive-pack > --strict`. It is used heavily by GitHub to ensure at least a certain > level of validity of pushed objects. Now, for this use case it is easy > to see that you want to stop *as soon as an error was > encountered*. And as GitHub sponsors my work on this patch series, my > main aim is to support their use case. While I understand that use case, I do not think stopping after showing three more errors in a single commit would make much difference in the bigger picture.