Re: [PATCH] receive-pack: check object type of sha1 before using them as commits

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 2, 2008, 22:57 UTC
Message-ID: <7vprwjvns0.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <alpine.LFD.1.00.0801021418510.3010@woody.linux-foundation.org>

Linus Torvalds <torvalds@linux-foundation.org> writes:

Show 18 quoted lines

> On Wed, 2 Jan 2008, Martin Koegler wrote:
>>  
>> -		old_commit = (struct commit *)parse_object(old_sha1);
>> -		new_commit = (struct commit *)parse_object(new_sha1);
>> +		old_object = parse_object(old_sha1);
>> +		new_object = parse_object(new_sha1);
>
> I think it would be better to use
>
> 	old_object = lookup_commit_reference(old_sha1);
> 	if (!old_object)
> 		return "bad ref";
> 	new_object = lookup_commit_reference(new_sha1);
> 	if (!new_object)
> 		return "bad ref";
>
> which will write a slightly more useful error message if it's not a commit 
> (ie it will use the "check_commit()" function in commit.c)

Hmmm...  Three points.

 * lookup_commit_reference() is silent if the object pointed at
   by old/new SHA-1 is missing.

 * when storing in refs/heads, we do not want to have committish
   but we do want an actual commit object.

 * returning like that the user cannot tell which ref had the
   error.