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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 15, 2011, 17:46 UTC
Message-ID: <7vei0mlg8d.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1313422716-26432-1-git-send-email-pclouds@gmail.com>

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