2011/8/16 Junio C Hamano <gitster@xxxxxxxxx>:
> 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
--
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