Nguyễn Thái Ngọc Duy [off-list ref] writes:
If lookup_commit() returns NULL, there's usually serious error and the
command aborts anyway. However it's still nicer to have a message
telling us where it aborts, rather than segmentation fault.
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".
- 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?
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