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

[Junio C Hamano <gitster@pobox.com>]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

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

That's exactly what I said.

	if (!initial)
	        /* we need to know the head_commit */
                head_commit = lookup_and_check(HEAD);

	/* depending on what kind of commit, we need different stuff */
        if (initial)
        	... going to create a parentless commit
	else if (amending)
		... use the head_commit to learn parent, reuse the message
                ... from there
	else if ...

These two are independent if/else cascades in the sense that the first is
about learning the details of head_commit, and the latter is about
learning how the commit is done, and in a subset of the latter head_commit
is used.

>>  - 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.

Yes, that is exactly what I meant.

Thakns.