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

On Thu, Sep 24, 2015 at 12:27 AM, Matthieu Moy
[off-list ref] wrote:

Karthik Nayak [off-list ref] writes:

quoted

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.

quoted

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