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

[Karthik Nayak <karthik.188@gmail.com>]

On Thu, Sep 24, 2015 at 12:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

What you're saying makes sense, It doesn't reflect the fact that the error
reporting is dropped, rather seems to be substituted by ref-filter.

>
> 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.

Seems good, I'll incorporate this into the commit 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.

Yes, my bad, Will resend the patch.

-- 
Regards,
Karthik Nayak