From: Nguyen Thai Ngoc Duy Date: Tue, 16 Aug 2011 13:22:27 GMT Subject: Re: [PATCH] commit: check return value of lookup_commit() Message-ID: In-Reply-To: <7vei0mlg8d.fsf@alter.siamese.dyndns.org> 2011/8/16 Junio C Hamano : > The change itself looks good to me but a point and a half to think about: > >  - In this if/elseif/.../else cascade, everybody except for the >   "initial_commit" case needs to make sure that head_sha1 points at a >   valid commit and get an commit object. Hoisting the scope of the >   variable "commit" one level in your patch is good, but it would make it >   easier to read and the future code modification much less error prone >   if (1) you called lookup_commit() and checked for errors before >   entering this if/elseif/... cascade, and (2) you renamed this variable >   to "head_commit". But then I would need to avoid die()ing in "initial_commit" case. So it becomes two related condition blocks (head_commit check and the if/elseif...), more error prone to me. >  - Whether we like it or not, many people have a broken reimplementations >   of git that can put a non-commit in HEAD, and they won't be fixed >   overnight. Instead of erroring out, would it be nicer of us if we just >   warned, unwrapped the tag and used the tagged commit instead? How about replacing those lookup_commit() with this? It would tolerate tag-in-branch case, but also warn users that something's gone wrong. struct commit *lookup_expect_commit(const unsigned char *sha1, const char *ref_name) { struct commit *c = lookup_commit_reference(sha1); if (!c) die(_("could not parse %s"), ref_name); if (hashcmp(sha1, c->object.sha1)) warning(_("%s %s is not a commit!"), ref_name, sha1_to_hex(sha1)); return c; } -- Duy