From: Junio C Hamano Date: Tue, 16 Aug 2011 18:02:23 GMT Subject: Re: [PATCH] commit: check return value of lookup_commit() Message-ID: <7vy5ytgrps.fsf@alter.siamese.dyndns.org> In-Reply-To: Nguyen Thai Ngoc Duy writes: > 2011/8/16 Junio C Hamano : >> 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.