Re: [PATCH] commit: check return value of lookup_commit()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 16, 2011, 18:02 UTC
- Message-ID
- <7vy5ytgrps.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <CACsJy8AusStKNWuw3j740r4Nc0FhzR+jJZJNaesxn68pr7dTqA@mail.gmail.com>
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
Show 13 quoted lines
> 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.
Show 7 quoted lines
>> - 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.