Dennis Kaarsemaker <den...@kaarsemaker.net> writes: > On wo, 2015-12-30 at 13:20 -0800, Junio C Hamano wrote: >> Dennis Kaarsemaker <den...@kaarsemaker.net> writes: >> >> > diff --git a/reflog-walk.c b/reflog-walk.c >> > index 85b8a54..b85c8e8 100644 >> > --- a/reflog-walk.c >> > +++ b/reflog-walk.c >> > @@ -236,8 +236,8 @@ void fake_reflog_parent(struct reflog_walk_info >> > *info, struct commit *commit) >> > reflog = &commit_reflog->reflogs->items[commit_reflog >> > ->recno]; >> > info->last_commit_reflog = commit_reflog; >> > commit_reflog->recno--; >> > - commit_info->commit = (struct commit *)parse_object(reflog >> > ->osha1); >> > - if (!commit_info->commit) { >> > + commit_info->commit = lookup_commit(reflog->osha1); >> > + if (!commit_info->commit || parse_commit(commit_info >> > ->commit)) { >> > commit->parents = NULL; >> > return; >> >> This looks somewhat roundabout and illogical. The original was bad >> because it blindly assumed reflgo->osha1 refers to a commit without >> making sure that assumption holds. Calling lookup_commit() blindly >> is not much better, even though you are helped that the function >> happens not to barf if the given object is not a commit. >> >> Also this changes semantics, no? Trace the original flow and think >> what happens, when we see a commit object that cannot be parsed in >> parse_commit_buffer(). parse_object() calls parse_object_buffer() >> which in turn calls parse_commit_buffer() and the entire callchain >> returns NULL. commit_info->commit will become NULL in such a case. >> >> With your code, lookup_commit() will store a non NULL in >> commit_info->commit, and parse_commit() calls parse_commit_buffer() >> and that would fail, so you clear commit->parents to NULL but fail >> to set commit_info->commit to NULL. >> >> Why not keep the parse_object() as-is and make sure we error out >> unless the result is a commit with a more explicit check, perhaps >> like this, instead? > > lookup_commit actually returns NULL (via object_as_type) for objects > that are not commits, so I don't think the above is true.
I think you did not read what you are responding to. I was talking about the error case where the object _is_ a commit (hence lookup returns it), but parse_commit_buffer() does not like its contents. > The code below also loses the diagnostic message about the object > not being a commit. Giving such a diagnostic message is a BUG. A ref can legitimately point at any type of object (only refs under refs/heads/, aka "branches", must point at commits), so you MUST NOT complain about seeing a non-commit in a reflog in general. -- 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