On Thu, Sep 24, 2015 at 12:27 AM, Matthieu Moy
<matthieu....@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik....@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.
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
