On wo, 2015-12-30 at 13:41 -0800, Junio C Hamano wrote:
> Dennis Kaarsemaker <dennis@xxxxxxxxxxxxxxx> writes:
>
> > On wo, 2015-12-30 at 13:20 -0800, Junio C Hamano wrote:
> > > Dennis Kaarsemaker <dennis@xxxxxxxxxxxxxxx> 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.
I read it, but misunderstood it. Thanks for clarifying.
> > 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.
Yeah, that makes sense, didn't think of that.
--
Dennis Kaarsemaker
www.kaarsemaker.net
--
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