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).
>
> I would have written
>
> 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: error "branch '%s' does not point at a commit" is redundant
> with the check performed in ref_filter_handler (ref-filter.c:1336).
> Error "some refs could not be read" can only be triggered as a
> consequence of the first one hence becomes useless.

>
>> @@ -370,10 +369,8 @@ static int append_ref(const char *refname, const struct object_id *oid, int flag
>>       commit = NULL;
>>       if (ref_list->verbose || ref_list->with_commit || merge_filter != NO_FILTER) {
>>               commit = lookup_commit_reference_gently(oid->hash, 1);
>> -             if (!commit) {
>> -                     cb->ret = error(_("branch '%s' does not point at a commit"), refname);
>> +             if (!commit)
>>                       return 0;
>> -             }
>
> Am I correct that the "return 0" statement above is dead code after the
> end of the series?
>
> If so, you should add a comment explaining that it's there "just in
> case" but not supposed to happen, or replace the if statement with
> "assert(commit);" IMHO. I have a preference for assert(): I don't like
> silent failures.