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

From: Nguyen Thai Ngoc Duy <pclouds@gmail.com>
Date: Aug 16, 2011, 13:22 UTC
Message-ID: <CACsJy8AusStKNWuw3j740r4Nc0FhzR+jJZJNaesxn68pr7dTqA@mail.gmail.com>
In-Reply-To: <7vei0mlg8d.fsf@alter.siamese.dyndns.org>

2011/8/16 Junio C Hamano <gitster@pobox.com>:

Show 10 quoted lines

> 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