Re: [PATCH v6 5/8] branch: drop non-commit error reporting

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Remove the error reporting variable to make the code easier to port
>> over to using ref-filter APIs.
>>
>> This also removes the error from being displayed. As branch.c will use
>> ref-filter APIs in the following patches, the error checking becomes
>> redundant with the error reporting system found in the ref-filter
>> (ref-filter.c:1336).
>
> Hmm, do you mean these lines by the above reference?
>
>         if (filter->merge_commit || filter->with_commit) {
>                 commit = lookup_commit_reference_gently(oid->hash, 1);
>                 if (!commit)
>                         return 0;
>
> That is "silently return ignoring it if it is not a commit", i.e.  I
> do not think that deserves to be called error REPORTING system.
>
> Do you really understand what the error message you are removing is
> trying to diagnose?  A branch ref must not point at a blob or any
> non-commit object, and if we find such a branch ref, we report it as
> error.  That is what the error message is about.

>
> Now, ref-filter.c:1336 has in no position to issue that same error
> message because it does not know what it is looking at is supposed
> to be a branch ref, so it is WRONG if it gave the same error
> message.  After all, the caller may be using ref-filter to filter
> refs/tags/* with merge_commit or with_commit and found a
> light-weight tag in refs/tags/* that points at a blob.  That is not
> an event we want to get an error at all.
>
> I do not think we terribly mind if this message goes away from this
> location.  "It is a wrong codepath whose purpose is not to diagnose
> and report a repository corruption.  If we care about such a
> repository corruption, we should report it from fsck instead", would
> bea valid justification for the removal of the message.

>
> It is not a valid justification to claim that it is made redundant,
> when you actually are simply LOSING the error reporting without
> giving the same error message from another codepath to make it truly
> redundant.