Hi Junio,
On 2015-06-19 22:12, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@xxxxxx> writes:
>
>> Note that some problems are too problematic to simply ignore. For
>> example, when the header lines are mixed up, we punt after encountering
>> an incorrect line. Therefore, demoting certain warnings to errors can
>> hide other problems. Example: demoting the missingauthor error to
>> a warning would hide a problematic committer line.
>
> Is this a warning to end-users (which should be better in the doc),
> or "because some of them are too problematic to ignore" that forgot
> to add the explanation "hence we do not keep going in this code"
> (which should be in the log message if that is what is going on)?
It was intended to offer the explanation for the design decision you commented on later:
> I notice that there are many instances of
>
> if (object does not pass some test)
> return report(...);
>
> that do not do "err = report(); if (err) return;" in this function
> after applying this patch.
>
> I think that answers the above question. The answer is "because
> some are too problematic, even after this patch, we give up parsing
> the remainder of the commit object once we hit certain errors,
> leaving some other errors that appear later in the object
> undetected".
>
> I think that is a sensible design decision, but the proposed log
> message forgets to say so.
>
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@xxxxxx>
>> ---
>> fsck.c | 28 ++++++++++++++++++++--------
>> 1 file changed, 20 insertions(+), 8 deletions(-)
>>
>> diff --git a/fsck.c b/fsck.c
>> index 9faaf53..9fe9f48 100644
>> --- a/fsck.c
>> +++ b/fsck.c
>> @@ -534,12 +534,18 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer,
>>
>> if (!skip_prefix(buffer, "tree ", &buffer))
>> return report(options, &commit->object, FSCK_MSG_MISSING_TREE, "invalid format - expected 'tree' line");
>> - if (get_sha1_hex(buffer, tree_sha1) || buffer[40] != '\n')
>> - return report(options, &commit->object, FSCK_MSG_BAD_TREE_SHA1, "invalid 'tree' line format - bad sha1");
>> + if (get_sha1_hex(buffer, tree_sha1) || buffer[40] != '\n') {
>> + err = report(options, &commit->object, FSCK_MSG_BAD_TREE_SHA1, "invalid 'tree' line format - bad sha1");
>> + if (err)
>> + return err;
>> + }
>
> 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.
It was my intention not to change behavior in that way without a proper reason, and I saw none.
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.
Having said that, I agree that it could actually make sense for `git fsck` to show all errors, or at least to have an option to do so.
But that is a story for another night ;-)
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in