Nguyen Thai Ngoc Duy <pclouds@xxxxxxxxx> writes:
> 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.
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.
--
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