Re: [PATCH v6 5/8] branch: drop non-commit error reporting
From: Matthieu Moy <hidden>
Date: 2016-06-15 23:06:40
Karthik Nayak [off-list ref] 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.
quoted hunk
@@ -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. -- Matthieu Moy http://www-verimag.imag.fr/~moy/