Re: [PATCH v6 08/19] fsck: Make fsck_commit() warn-friendly
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 19, 2015, 21:01 UTC
- Message-ID
- <xmqqk2uz5s6b.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <ba0cb32edf26639a8a00ad9d17b49e32@www.dscho.org>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 18 quoted lines
>> 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.<msg-id> = 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?
Show 7 quoted lines
> 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.