Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> 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? -- 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