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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Sep 24, 2015 at 12:27 AM, Matthieu Moy
<Matthieu.Moy@xxxxxxxxxxxxxxx> wrote:
> Karthik Nayak <karthik.188@xxxxxxxxx> 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.

This looks better thanks.

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

This code is removed by the end of the series. We could use an assert()
in this patch, but I don't see the point, its removed later either ways when
we use ref-filter APIs.

-- 
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]