Re: [PATCH] commit: check return value of lookup_commit()

2011/8/16 Junio C Hamano [off-list ref]:

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